Hello,
This series implements automatic error recovery for PCI devices on s390
following the scheme outlined at Documentation/PCI/pci-error-recovery.rst
it applies on top of currenct v5.15-rc1.
The patches are almost completely s390 specific except for one patch
exporting existing functionality for use by arch/s390/pci/ code. Nevertheless
I would appreciate any feedback. This is down from two common code changes in
the first version of this series.
The outline of the patches is as follows:
Patch 1 and 2 add s390 specific code implementing a reset mechanism that
takes the PCI function out of the platform specific error state.
Patch 3 "PCI: Export pci_dev_lock()" is basically an extension to commit
e3a9b1212b9d ("PCI: Export pci_dev_trylock() and pci_dev_unlock()") which
already exported pci_dev_trylock(). In the final patch we make use of
pci_dev_lock() to wait for any other exclusive uses of the pdev to be finished
before starting recovery.
Finally Patch 4 implements the recovery flow as part of the existing s390
specific PCI availability and error event mechanism. Previously the error event
handler only set pdev->error_state and required manual intervention to make the
device usable again. Now we handle the case where firmware has already reset
a PCI function after an error was encountered informing the OS that it should
be ready to be used again. In that case if the driver supports error recovery
we use it to transparently reset the device or simply take it out of the error
state and then if possible let the driver resume operations.
Note that the same event is also issued by the hypervisor if the function was
previously taken into a service mode for example for firmware upgrade via the
hypervisor and is now ready to be used again.
Changes since v1:
- Dropped the patch moving pci_dev_is_added(), we can rely on pdev->driver
being unset for a device that has already been removed or not yet
initialized. While I believe pci_dev_is_added() would still be a cleaner
check we need to check for a bound driver anyway and that is sufficient.
- Adapted the hotplug_slot_ops::reset_slot() signature to current upstream
taking a bool instead of an int
- Added a missing parameter documentation and reworded some comments
- Reworded some debug/info messages
Thanks,
Niklas Schnelle
Niklas Schnelle (4):
s390/pci: refresh function handle in iomap
s390/pci: implement reset_slot for hotplug slot
PCI: Export pci_dev_lock()
s390/pci: implement minimal PCI error recovery
arch/s390/include/asm/pci.h | 6 +-
arch/s390/pci/pci.c | 143 +++++++++++++++++++++-
arch/s390/pci/pci_event.c | 188 ++++++++++++++++++++++++++++-
arch/s390/pci/pci_insn.c | 4 +-
arch/s390/pci/pci_irq.c | 9 ++
drivers/pci/hotplug/s390_pci_hpc.c | 24 ++++
drivers/pci/pci.c | 3 +-
include/linux/pci.h | 1 +
8 files changed, 366 insertions(+), 12 deletions(-)
--
2.25.1
When the platform detects an error on a PCI function or a service action
has been performed it is put in the error state and an error event
notification is provided to the OS.
Currently we treat all error event notifications the same and simply set
pdev->error_state = pci_channel_io_perm_failure requiring user
intervention such as use of the recover attribute to get the device
usable again. Despite requiring a manual step this also has the
disadvantage that the device is completely torn down and recreated
resulting in higher level devices such as a block or network device
being recreated. In case of a block device this also means that it may
need to be removed and added to a software raid even if that could
otherwise survive with a temporary degradation.
This is of course not ideal more so since an error notification with PEC
0x3A indicates that the platform already performed error recovery
successfully or that the error state was caused by a service action that
is now finished.
At least in this case we can assume that the error state can be reset
and the function made usable again. So as not to have the disadvantage
of a full tear down and recreation we need to coordinate this recovery
with the driver. Thankfully there is already a well defined recovery
flow for this described in Documentation/PCI/pci-error-recovery.rst.
The implementation of this is somewhat straight forward and simplified
by the fact that our recovery flow is defined per PCI function. As
a reset we use the newly introduced zpci_hot_reset_device() which also
takes the PCI function out of the error state.
Signed-off-by: Niklas Schnelle <[email protected]>
---
v1 -> v2:
- Dropped use of pci_dev_is_added(), pdev->driver check is enough
- Improved some comments and messages
arch/s390/include/asm/pci.h | 4 +-
arch/s390/pci/pci.c | 49 ++++++++++
arch/s390/pci/pci_event.c | 182 +++++++++++++++++++++++++++++++++++-
3 files changed, 233 insertions(+), 2 deletions(-)
diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 2a2ed165a270..558877aff2e5 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -294,8 +294,10 @@ void zpci_debug_exit(void);
void zpci_debug_init_device(struct zpci_dev *, const char *);
void zpci_debug_exit_device(struct zpci_dev *);
-/* Error reporting */
+/* Error handling */
int zpci_report_error(struct pci_dev *, struct zpci_report_error_header *);
+int zpci_clear_error_state(struct zpci_dev *zdev);
+int zpci_reset_load_store_blocked(struct zpci_dev *zdev);
#ifdef CONFIG_NUMA
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index dce60f16e94a..b987c9d76510 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -954,6 +954,55 @@ int zpci_report_error(struct pci_dev *pdev,
}
EXPORT_SYMBOL(zpci_report_error);
+/**
+ * zpci_clear_error_state() - Clears the zPCI error state of the device
+ * @zdev: The zdev for which the zPCI error state should be reset
+ *
+ * Clear the zPCI error state of the device. If clearing the zPCI error state
+ * fails the device is left in the error state. In this case it may make sense
+ * to call zpci_io_perm_failure() on the associated pdev if it exists.
+ *
+ * Returns: 0 on success, -EIO otherwise
+ */
+int zpci_clear_error_state(struct zpci_dev *zdev)
+{
+ u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_ERROR);
+ struct zpci_fib fib = {0};
+ u8 status;
+ int rc;
+
+ rc = zpci_mod_fc(req, &fib, &status);
+ if (rc)
+ return -EIO;
+
+ return 0;
+}
+
+/**
+ * zpci_reset_load_store_blocked() - Re-enables L/S from error state
+ * @zdev: The zdev for which to unblock load/store access
+ *
+ * R-eenables load/store access for a PCI function in the error state while
+ * keeping DMA blocked. In this state drivers can poke MMIO space to determine
+ * if error recovery is possible while catching any rogue DMA access from the
+ * device.
+ *
+ * Returns: 0 on success, -EIO otherwise
+ */
+int zpci_reset_load_store_blocked(struct zpci_dev *zdev)
+{
+ u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_BLOCK);
+ struct zpci_fib fib = {0};
+ u8 status;
+ int rc;
+
+ rc = zpci_mod_fc(req, &fib, &status);
+ if (rc)
+ return -EIO;
+
+ return 0;
+}
+
static int zpci_mem_init(void)
{
BUILD_BUG_ON(!is_power_of_2(__alignof__(struct zpci_fmb)) ||
diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index e868d996ec5b..73389e161872 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -47,15 +47,182 @@ struct zpci_ccdf_avail {
u16 pec; /* PCI event code */
} __packed;
+static inline bool ers_result_indicates_abort(pci_ers_result_t ers_res)
+{
+ switch (ers_res) {
+ case PCI_ERS_RESULT_CAN_RECOVER:
+ case PCI_ERS_RESULT_RECOVERED:
+ case PCI_ERS_RESULT_NEED_RESET:
+ return false;
+ default:
+ return true;
+ }
+}
+
+static pci_ers_result_t zpci_event_notify_error_detected(struct pci_dev *pdev)
+{
+ pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
+ struct pci_driver *driver = pdev->driver;
+
+ pr_debug("%s: asking driver to determine recoverability\n", pci_name(pdev));
+ ers_res = driver->err_handler->error_detected(pdev, pdev->error_state);
+ if (ers_result_indicates_abort(ers_res))
+ pr_info("%s: driver can't recover\n", pci_name(pdev));
+ else if (ers_res == PCI_ERS_RESULT_NEED_RESET)
+ pr_debug("%s: driver needs reset to recover\n", pci_name(pdev));
+
+ return ers_res;
+}
+
+static pci_ers_result_t zpci_event_do_error_state_clear(struct pci_dev *pdev)
+{
+ pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
+ struct pci_driver *driver = pdev->driver;
+ struct zpci_dev *zdev = to_zpci(pdev);
+ int rc;
+
+ pr_debug("%s: reset load/store blocked\n", pci_name(pdev));
+ rc = zpci_reset_load_store_blocked(zdev);
+ if (rc) {
+ pr_err("%s: reset load/store blocked failed\n", pci_name(pdev));
+ /* Let's try a full reset instead */
+ return PCI_ERS_RESULT_NEED_RESET;
+ }
+
+ if (driver->err_handler->mmio_enabled) {
+ pr_debug("%s: calling mmio_enabled() callback\n", pci_name(pdev));
+ ers_res = driver->err_handler->mmio_enabled(pdev);
+ if (ers_result_indicates_abort(ers_res)) {
+ pr_info("%s: driver can't recover after enabling MMIO\n", pci_name(pdev));
+ return ers_res;
+ } else if (ers_res == PCI_ERS_RESULT_NEED_RESET) {
+ pr_debug("%s: driver needs reset to recover\n", pci_name(pdev));
+ return ers_res;
+ }
+ }
+
+ pr_debug("%s: clearing error state\n", pci_name(pdev));
+ rc = zpci_clear_error_state(zdev);
+ if (!rc) {
+ pdev->error_state = pci_channel_io_normal;
+ } else {
+ pr_err("%s: resetting error state failed\n", pci_name(pdev));
+ /* Let's try a full reset instead */
+ return PCI_ERS_RESULT_NEED_RESET;
+ }
+
+ return ers_res;
+}
+
+static pci_ers_result_t zpci_event_do_reset(struct pci_dev *pdev)
+{
+ pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
+ struct pci_driver *driver = pdev->driver;
+
+ pr_info("%s: initiating reset\n", pci_name(pdev));
+ if (zpci_hot_reset_device(to_zpci(pdev))) {
+ pr_err("%s: resetting function failed\n", pci_name(pdev));
+ return ers_res;
+ }
+ pdev->error_state = pci_channel_io_normal;
+ if (driver->err_handler->slot_reset) {
+ ers_res = driver->err_handler->slot_reset(pdev);
+ if (ers_result_indicates_abort(ers_res)) {
+ pr_info("%s: driver can't recover after slot reset\n", pci_name(pdev));
+ return ers_res;
+ }
+ }
+
+ return ers_res;
+}
+
+/* zpci_event_attempt_error_recovery - Try to recover the given PCI function
+ * @pdev: PCI function to recover currently in the error state
+ *
+ * We follow the scheme outlined in Documentation/PCI/pci-error-recovery.rst.
+ * With the simplification that recovery always happens per function
+ * and the platform determines which functions are affected for
+ * multi-function devices.
+ */
+static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev)
+{
+ pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
+ struct pci_driver *driver;
+
+ /*
+ * Ensure that the PCI function is not removed concurrently, no driver
+ * is unbound or probed and that userspace can't access its
+ * configuration space while we perform recovery.
+ */
+ pci_dev_lock(pdev);
+ if (pdev->error_state == pci_channel_io_perm_failure) {
+ ers_res = PCI_ERS_RESULT_DISCONNECT;
+ goto out_unlock;
+ }
+ pdev->error_state = pci_channel_io_frozen;
+
+ driver = pdev->driver;
+ if (!driver || !driver->err_handler || !driver->err_handler->error_detected) {
+ pr_info("%s: driver does not support error recovery\n", pci_name(pdev));
+ goto out_unlock;
+ }
+
+ ers_res = zpci_event_notify_error_detected(pdev);
+ if (ers_result_indicates_abort(ers_res))
+ goto out_unlock;
+
+ if (ers_res == PCI_ERS_RESULT_CAN_RECOVER) {
+ ers_res = zpci_event_do_error_state_clear(pdev);
+ if (ers_result_indicates_abort(ers_res))
+ goto out_unlock;
+ }
+
+ if (ers_res == PCI_ERS_RESULT_NEED_RESET)
+ ers_res = zpci_event_do_reset(pdev);
+
+ if (ers_res != PCI_ERS_RESULT_RECOVERED) {
+ pr_err("%s: failed to recover\n", pci_name(pdev));
+ goto out_unlock;
+ }
+
+ pr_info("%s: driver may resume operations\n", pci_name(pdev));
+ if (driver->err_handler->resume)
+ driver->err_handler->resume(pdev);
+out_unlock:
+ pci_dev_unlock(pdev);
+
+ return ers_res;
+}
+
+/* zpci_event_io_failure - Report PCI channel failure state to driver
+ * @pdev: PCI function for which to report
+ * @es: PCI channel failure state to report
+ */
+static void zpci_event_io_failure(struct pci_dev *pdev, pci_channel_state_t es)
+{
+ struct pci_driver *driver;
+
+ pci_dev_lock(pdev);
+ pdev->error_state = es;
+ driver = pdev->driver;
+ if (driver && driver->err_handler && driver->err_handler->error_detected)
+ driver->err_handler->error_detected(pdev, pdev->error_state);
+ pci_dev_unlock(pdev);
+}
+
static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
{
struct zpci_dev *zdev = get_zdev_by_fid(ccdf->fid);
struct pci_dev *pdev = NULL;
+ pci_ers_result_t ers_res;
zpci_err("error CCDF:\n");
zpci_err_hex(ccdf, sizeof(*ccdf));
if (zdev)
+ zpci_update_fh(zdev, ccdf->fh);
+
+ if (zdev->zbus->bus)
pdev = pci_get_slot(zdev->zbus->bus, zdev->devfn);
pr_err("%s: Event 0x%x reports an error for PCI function 0x%x\n",
@@ -64,7 +231,20 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
if (!pdev)
return;
- pdev->error_state = pci_channel_io_perm_failure;
+ switch (ccdf->pec) {
+ case 0x003a: /* Service Action or Error Recovery Successful */
+ ers_res = zpci_event_attempt_error_recovery(pdev);
+ if (ers_res != PCI_ERS_RESULT_RECOVERED)
+ zpci_event_io_failure(pdev, pci_channel_io_perm_failure);
+ break;
+ default:
+ /*
+ * Mark as frozen not permanently failed because the device
+ * could be subsequently recovered by the platform.
+ */
+ zpci_event_io_failure(pdev, pci_channel_io_frozen);
+ break;
+ }
pci_dev_put(pdev);
}
--
2.25.1
The function handle of a PCI function is updated when disabling or
enabling it as well as when the function's availability changes or it
enters the error state.
Until now this only occurred either while there is no struct pci_dev
associated with the function yet or the function became unavailable.
This meant that leaving a stale function handle in the iomap either
didn't happen because there was no iomap yet or it lead to errors on PCI
access but so would the correct disabled function handle.
In the future a CLP Set PCI Function Disable/Enable cycle during PCI
device recovery may be done while the device is bound to a driver. In
this case we must update the iomap associated with the now-stale
function handle to ensure that the resulting zPCI instruction references
an accurate function handle.
Since the function handle is accessed by the PCI accessor helpers
without locking use READ_ONCE()/WRITE_ONCE() to mark this access and
prevent compiler optimizations that would move the load/store.
With that infrastructure in place let's also properly update the
function handle in the existing cases. This makes sure that in the
future debugging of a zPCI function access through the handle will
show an up to date handle reducing the chance of confusion. Also it
makes sure we have one single place where a zPCI function handle is
updated after initialization.
Signed-off-by: Niklas Schnelle <[email protected]>
---
arch/s390/include/asm/pci.h | 1 +
arch/s390/pci/pci.c | 36 ++++++++++++++++++++++++++++++++----
arch/s390/pci/pci_event.c | 6 +++---
arch/s390/pci/pci_insn.c | 4 ++--
4 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index e4803ec51110..5e6cba22a801 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -211,6 +211,7 @@ int zpci_deconfigure_device(struct zpci_dev *zdev);
int zpci_register_ioat(struct zpci_dev *, u8, u64, u64, u64);
int zpci_unregister_ioat(struct zpci_dev *, u8);
void zpci_remove_reserved_devices(void);
+void zpci_update_fh(struct zpci_dev *zdev, u32 fh);
/* CLP */
int clp_setup_writeback_mio(void);
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index e7e6788d75a8..af22778551c1 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -481,6 +481,34 @@ static void zpci_free_iomap(struct zpci_dev *zdev, int entry)
spin_unlock(&zpci_iomap_lock);
}
+static void zpci_do_update_iomap_fh(struct zpci_dev *zdev, u32 fh)
+{
+ int bar, idx;
+
+ spin_lock(&zpci_iomap_lock);
+ for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+ if (!zdev->bars[bar].size)
+ continue;
+ idx = zdev->bars[bar].map_idx;
+ if (!zpci_iomap_start[idx].count)
+ continue;
+ WRITE_ONCE(zpci_iomap_start[idx].fh, zdev->fh);
+ }
+ spin_unlock(&zpci_iomap_lock);
+}
+
+void zpci_update_fh(struct zpci_dev *zdev, u32 fh)
+{
+ if (!fh || zdev->fh == fh)
+ return;
+
+ zdev->fh = fh;
+ if (zpci_use_mio(zdev))
+ return;
+ if (zdev->has_resources && zdev_enabled(zdev))
+ zpci_do_update_iomap_fh(zdev, fh);
+}
+
static struct resource *__alloc_res(struct zpci_dev *zdev, unsigned long start,
unsigned long size, unsigned long flags)
{
@@ -668,7 +696,7 @@ int zpci_enable_device(struct zpci_dev *zdev)
if (clp_enable_fh(zdev, &fh, ZPCI_NR_DMA_SPACES))
rc = -EIO;
else
- zdev->fh = fh;
+ zpci_update_fh(zdev, fh);
return rc;
}
@@ -679,14 +707,14 @@ int zpci_disable_device(struct zpci_dev *zdev)
cc = clp_disable_fh(zdev, &fh);
if (!cc) {
- zdev->fh = fh;
+ zpci_update_fh(zdev, fh);
} else if (cc == CLP_RC_SETPCIFN_ALRDY) {
pr_info("Disabling PCI function %08x had no effect as it was already disabled\n",
zdev->fid);
/* Function is already disabled - update handle */
rc = clp_refresh_fh(zdev->fid, &fh);
if (!rc) {
- zdev->fh = fh;
+ zpci_update_fh(zdev, fh);
rc = -EINVAL;
}
} else {
@@ -768,7 +796,7 @@ int zpci_scan_configured_device(struct zpci_dev *zdev, u32 fh)
{
int rc;
- zdev->fh = fh;
+ zpci_update_fh(zdev, fh);
/* the PCI function will be scanned once function 0 appears */
if (!zdev->zbus->bus)
return 0;
diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index c856f80cb21b..e868d996ec5b 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -76,7 +76,7 @@ void zpci_event_error(void *data)
static void zpci_event_hard_deconfigured(struct zpci_dev *zdev, u32 fh)
{
- zdev->fh = fh;
+ zpci_update_fh(zdev, fh);
/* Give the driver a hint that the function is
* already unusable.
*/
@@ -117,7 +117,7 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
if (!zdev)
zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_STANDBY);
else
- zdev->fh = ccdf->fh;
+ zpci_update_fh(zdev, ccdf->fh);
break;
case 0x0303: /* Deconfiguration requested */
if (zdev) {
@@ -126,7 +126,7 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
*/
if (zdev->state != ZPCI_FN_STATE_CONFIGURED)
break;
- zdev->fh = ccdf->fh;
+ zpci_update_fh(zdev, ccdf->fh);
zpci_deconfigure_device(zdev);
}
break;
diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
index 2e43996159f0..28d863aaafea 100644
--- a/arch/s390/pci/pci_insn.c
+++ b/arch/s390/pci/pci_insn.c
@@ -163,7 +163,7 @@ static inline int zpci_load_fh(u64 *data, const volatile void __iomem *addr,
unsigned long len)
{
struct zpci_iomap_entry *entry = &zpci_iomap_start[ZPCI_IDX(addr)];
- u64 req = ZPCI_CREATE_REQ(entry->fh, entry->bar, len);
+ u64 req = ZPCI_CREATE_REQ(READ_ONCE(entry->fh), entry->bar, len);
return __zpci_load(data, req, ZPCI_OFFSET(addr));
}
@@ -244,7 +244,7 @@ static inline int zpci_store_fh(const volatile void __iomem *addr, u64 data,
unsigned long len)
{
struct zpci_iomap_entry *entry = &zpci_iomap_start[ZPCI_IDX(addr)];
- u64 req = ZPCI_CREATE_REQ(entry->fh, entry->bar, len);
+ u64 req = ZPCI_CREATE_REQ(READ_ONCE(entry->fh), entry->bar, len);
return __zpci_store(data, req, ZPCI_OFFSET(addr));
}
--
2.25.1
On Thu, Sep 16, 2021 at 11:33:36AM +0200, Niklas Schnelle wrote:
> When the platform detects an error on a PCI function or a service action
> has been performed it is put in the error state and an error event
> notification is provided to the OS.
>
> Currently we treat all error event notifications the same and simply set
> pdev->error_state = pci_channel_io_perm_failure requiring user
> intervention such as use of the recover attribute to get the device
> usable again. Despite requiring a manual step this also has the
> disadvantage that the device is completely torn down and recreated
> resulting in higher level devices such as a block or network device
> being recreated. In case of a block device this also means that it may
> need to be removed and added to a software raid even if that could
> otherwise survive with a temporary degradation.
>
> This is of course not ideal more so since an error notification with PEC
> 0x3A indicates that the platform already performed error recovery
> successfully or that the error state was caused by a service action that
> is now finished.
>
> At least in this case we can assume that the error state can be reset
> and the function made usable again. So as not to have the disadvantage
> of a full tear down and recreation we need to coordinate this recovery
> with the driver. Thankfully there is already a well defined recovery
> flow for this described in Documentation/PCI/pci-error-recovery.rst.
>
> The implementation of this is somewhat straight forward and simplified
> by the fact that our recovery flow is defined per PCI function. As
> a reset we use the newly introduced zpci_hot_reset_device() which also
> takes the PCI function out of the error state.
>
> Signed-off-by: Niklas Schnelle <[email protected]>
> ---
> v1 -> v2:
> - Dropped use of pci_dev_is_added(), pdev->driver check is enough
> - Improved some comments and messages
>
> arch/s390/include/asm/pci.h | 4 +-
> arch/s390/pci/pci.c | 49 ++++++++++
> arch/s390/pci/pci_event.c | 182 +++++++++++++++++++++++++++++++++++-
> 3 files changed, 233 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 2a2ed165a270..558877aff2e5 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -294,8 +294,10 @@ void zpci_debug_exit(void);
> void zpci_debug_init_device(struct zpci_dev *, const char *);
> void zpci_debug_exit_device(struct zpci_dev *);
>
> -/* Error reporting */
> +/* Error handling */
> int zpci_report_error(struct pci_dev *, struct zpci_report_error_header *);
> +int zpci_clear_error_state(struct zpci_dev *zdev);
> +int zpci_reset_load_store_blocked(struct zpci_dev *zdev);
>
> #ifdef CONFIG_NUMA
>
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index dce60f16e94a..b987c9d76510 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -954,6 +954,55 @@ int zpci_report_error(struct pci_dev *pdev,
> }
> EXPORT_SYMBOL(zpci_report_error);
>
> +/**
> + * zpci_clear_error_state() - Clears the zPCI error state of the device
> + * @zdev: The zdev for which the zPCI error state should be reset
> + *
> + * Clear the zPCI error state of the device. If clearing the zPCI error state
> + * fails the device is left in the error state. In this case it may make sense
> + * to call zpci_io_perm_failure() on the associated pdev if it exists.
> + *
> + * Returns: 0 on success, -EIO otherwise
> + */
> +int zpci_clear_error_state(struct zpci_dev *zdev)
> +{
> + u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_ERROR);
> + struct zpci_fib fib = {0};
> + u8 status;
> + int rc;
> +
> + rc = zpci_mod_fc(req, &fib, &status);
> + if (rc)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +/**
> + * zpci_reset_load_store_blocked() - Re-enables L/S from error state
> + * @zdev: The zdev for which to unblock load/store access
> + *
> + * R-eenables load/store access for a PCI function in the error state while
> + * keeping DMA blocked. In this state drivers can poke MMIO space to determine
> + * if error recovery is possible while catching any rogue DMA access from the
> + * device.
> + *
> + * Returns: 0 on success, -EIO otherwise
> + */
> +int zpci_reset_load_store_blocked(struct zpci_dev *zdev)
> +{
> + u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_BLOCK);
> + struct zpci_fib fib = {0};
> + u8 status;
> + int rc;
> +
> + rc = zpci_mod_fc(req, &fib, &status);
> + if (rc)
> + return -EIO;
> +
> + return 0;
> +}
> +
> static int zpci_mem_init(void)
> {
> BUILD_BUG_ON(!is_power_of_2(__alignof__(struct zpci_fmb)) ||
> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index e868d996ec5b..73389e161872 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
> @@ -47,15 +47,182 @@ struct zpci_ccdf_avail {
> u16 pec; /* PCI event code */
> } __packed;
>
> +static inline bool ers_result_indicates_abort(pci_ers_result_t ers_res)
> +{
> + switch (ers_res) {
> + case PCI_ERS_RESULT_CAN_RECOVER:
> + case PCI_ERS_RESULT_RECOVERED:
> + case PCI_ERS_RESULT_NEED_RESET:
> + return false;
> + default:
> + return true;
> + }
> +}
> +
> +static pci_ers_result_t zpci_event_notify_error_detected(struct pci_dev *pdev)
> +{
> + pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
> + struct pci_driver *driver = pdev->driver;
> +
> + pr_debug("%s: asking driver to determine recoverability\n", pci_name(pdev));
> + ers_res = driver->err_handler->error_detected(pdev, pdev->error_state);
> + if (ers_result_indicates_abort(ers_res))
> + pr_info("%s: driver can't recover\n", pci_name(pdev));
> + else if (ers_res == PCI_ERS_RESULT_NEED_RESET)
> + pr_debug("%s: driver needs reset to recover\n", pci_name(pdev));
Are you following a convention of using pr_info(), etc here? I try to
use the pci_info() family (wrappers around dev_info()) whenever I can.
On Tue, 2021-09-28 at 13:11 -0500, Bjorn Helgaas wrote:
> On Thu, Sep 16, 2021 at 11:33:36AM +0200, Niklas Schnelle wrote:
> > When the platform detects an error on a PCI function or a service action
> > has been performed it is put in the error state and an error event
> > notification is provided to the OS.
> >
> > Currently we treat all error event notifications the same and simply set
> > pdev->error_state = pci_channel_io_perm_failure requiring user
> > intervention such as use of the recover attribute to get the device
> > usable again. Despite requiring a manual step this also has the
> > disadvantage that the device is completely torn down and recreated
> > resulting in higher level devices such as a block or network device
> > being recreated. In case of a block device this also means that it may
> > need to be removed and added to a software raid even if that could
> > otherwise survive with a temporary degradation.
> >
> > This is of course not ideal more so since an error notification with PEC
> > 0x3A indicates that the platform already performed error recovery
> > successfully or that the error state was caused by a service action that
> > is now finished.
> >
> > At least in this case we can assume that the error state can be reset
> > and the function made usable again. So as not to have the disadvantage
> > of a full tear down and recreation we need to coordinate this recovery
> > with the driver. Thankfully there is already a well defined recovery
> > flow for this described in Documentation/PCI/pci-error-recovery.rst.
> >
> > The implementation of this is somewhat straight forward and simplified
> > by the fact that our recovery flow is defined per PCI function. As
> > a reset we use the newly introduced zpci_hot_reset_device() which also
> > takes the PCI function out of the error state.
> >
> > Signed-off-by: Niklas Schnelle <[email protected]>
> > ---
> > v1 -> v2:
> > - Dropped use of pci_dev_is_added(), pdev->driver check is enough
> > - Improved some comments and messages
> >
> > arch/s390/include/asm/pci.h | 4 +-
> > arch/s390/pci/pci.c | 49 ++++++++++
> > arch/s390/pci/pci_event.c | 182 +++++++++++++++++++++++++++++++++++-
> > 3 files changed, 233 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > index 2a2ed165a270..558877aff2e5 100644
> > --- a/arch/s390/include/asm/pci.h
> > +++ b/arch/s390/include/asm/pci.h
> > @@ -294,8 +294,10 @@ void zpci_debug_exit(void);
> > void zpci_debug_init_device(struct zpci_dev *, const char *);
> > void zpci_debug_exit_device(struct zpci_dev *);
> >
> > -/* Error reporting */
> > +/* Error handling */
> > int zpci_report_error(struct pci_dev *, struct zpci_report_error_header *);
> > +int zpci_clear_error_state(struct zpci_dev *zdev);
> > +int zpci_reset_load_store_blocked(struct zpci_dev *zdev);
> >
> > #ifdef CONFIG_NUMA
> >
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index dce60f16e94a..b987c9d76510 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -954,6 +954,55 @@ int zpci_report_error(struct pci_dev *pdev,
> > }
> > EXPORT_SYMBOL(zpci_report_error);
> >
> > +/**
> > + * zpci_clear_error_state() - Clears the zPCI error state of the device
> > + * @zdev: The zdev for which the zPCI error state should be reset
> > + *
> > + * Clear the zPCI error state of the device. If clearing the zPCI error state
> > + * fails the device is left in the error state. In this case it may make sense
> > + * to call zpci_io_perm_failure() on the associated pdev if it exists.
> > + *
> > + * Returns: 0 on success, -EIO otherwise
> > + */
> > +int zpci_clear_error_state(struct zpci_dev *zdev)
> > +{
> > + u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_ERROR);
> > + struct zpci_fib fib = {0};
> > + u8 status;
> > + int rc;
> > +
> > + rc = zpci_mod_fc(req, &fib, &status);
> > + if (rc)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * zpci_reset_load_store_blocked() - Re-enables L/S from error state
> > + * @zdev: The zdev for which to unblock load/store access
> > + *
> > + * R-eenables load/store access for a PCI function in the error state while
> > + * keeping DMA blocked. In this state drivers can poke MMIO space to determine
> > + * if error recovery is possible while catching any rogue DMA access from the
> > + * device.
> > + *
> > + * Returns: 0 on success, -EIO otherwise
> > + */
> > +int zpci_reset_load_store_blocked(struct zpci_dev *zdev)
> > +{
> > + u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_BLOCK);
> > + struct zpci_fib fib = {0};
> > + u8 status;
> > + int rc;
> > +
> > + rc = zpci_mod_fc(req, &fib, &status);
> > + if (rc)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > static int zpci_mem_init(void)
> > {
> > BUILD_BUG_ON(!is_power_of_2(__alignof__(struct zpci_fmb)) ||
> > diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> > index e868d996ec5b..73389e161872 100644
> > --- a/arch/s390/pci/pci_event.c
> > +++ b/arch/s390/pci/pci_event.c
> > @@ -47,15 +47,182 @@ struct zpci_ccdf_avail {
> > u16 pec; /* PCI event code */
> > } __packed;
> >
> > +static inline bool ers_result_indicates_abort(pci_ers_result_t ers_res)
> > +{
> > + switch (ers_res) {
> > + case PCI_ERS_RESULT_CAN_RECOVER:
> > + case PCI_ERS_RESULT_RECOVERED:
> > + case PCI_ERS_RESULT_NEED_RESET:
> > + return false;
> > + default:
> > + return true;
> > + }
> > +}
> > +
> > +static pci_ers_result_t zpci_event_notify_error_detected(struct pci_dev *pdev)
> > +{
> > + pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
> > + struct pci_driver *driver = pdev->driver;
> > +
> > + pr_debug("%s: asking driver to determine recoverability\n", pci_name(pdev));
> > + ers_res = driver->err_handler->error_detected(pdev, pdev->error_state);
> > + if (ers_result_indicates_abort(ers_res))
> > + pr_info("%s: driver can't recover\n", pci_name(pdev));
> > + else if (ers_res == PCI_ERS_RESULT_NEED_RESET)
> > + pr_debug("%s: driver needs reset to recover\n", pci_name(pdev));
>
> Are you following a convention of using pr_info(), etc here? I try to
> use the pci_info() family (wrappers around dev_info()) whenever I can.
A convention in the sense that so far all code under arch/s390/pci/
uses pr_info() which comes out as "zpci: ..". It seems that similar
pr_info() constructs are also common in other arch/s390/ and
drivers/s390 code.
On the other hand we already agreed that some of the s390 specific PCI
code is more like a PCI controller. So I'm open to suggestions.
On Tue, Sep 28, 2021 at 08:30:44PM +0200, Niklas Schnelle wrote:
> On Tue, 2021-09-28 at 13:11 -0500, Bjorn Helgaas wrote:
> > On Thu, Sep 16, 2021 at 11:33:36AM +0200, Niklas Schnelle wrote:
> > > When the platform detects an error on a PCI function or a service action
> > > has been performed it is put in the error state and an error event
> > > notification is provided to the OS.
> > >
> > > Currently we treat all error event notifications the same and simply set
> > > pdev->error_state = pci_channel_io_perm_failure requiring user
> > > intervention such as use of the recover attribute to get the device
> > > usable again. Despite requiring a manual step this also has the
> > > disadvantage that the device is completely torn down and recreated
> > > resulting in higher level devices such as a block or network device
> > > being recreated. In case of a block device this also means that it may
> > > need to be removed and added to a software raid even if that could
> > > otherwise survive with a temporary degradation.
> > >
> > > This is of course not ideal more so since an error notification with PEC
> > > 0x3A indicates that the platform already performed error recovery
> > > successfully or that the error state was caused by a service action that
> > > is now finished.
> > >
> > > At least in this case we can assume that the error state can be reset
> > > and the function made usable again. So as not to have the disadvantage
> > > of a full tear down and recreation we need to coordinate this recovery
> > > with the driver. Thankfully there is already a well defined recovery
> > > flow for this described in Documentation/PCI/pci-error-recovery.rst.
> > >
> > > The implementation of this is somewhat straight forward and simplified
> > > by the fact that our recovery flow is defined per PCI function. As
> > > a reset we use the newly introduced zpci_hot_reset_device() which also
> > > takes the PCI function out of the error state.
> > >
> > > Signed-off-by: Niklas Schnelle <[email protected]>
> > > ---
> > > v1 -> v2:
> > > - Dropped use of pci_dev_is_added(), pdev->driver check is enough
> > > - Improved some comments and messages
> > >
> > > arch/s390/include/asm/pci.h | 4 +-
> > > arch/s390/pci/pci.c | 49 ++++++++++
> > > arch/s390/pci/pci_event.c | 182 +++++++++++++++++++++++++++++++++++-
> > > 3 files changed, 233 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > > index 2a2ed165a270..558877aff2e5 100644
> > > --- a/arch/s390/include/asm/pci.h
> > > +++ b/arch/s390/include/asm/pci.h
> > > @@ -294,8 +294,10 @@ void zpci_debug_exit(void);
> > > void zpci_debug_init_device(struct zpci_dev *, const char *);
> > > void zpci_debug_exit_device(struct zpci_dev *);
> > >
> > > -/* Error reporting */
> > > +/* Error handling */
> > > int zpci_report_error(struct pci_dev *, struct zpci_report_error_header *);
> > > +int zpci_clear_error_state(struct zpci_dev *zdev);
> > > +int zpci_reset_load_store_blocked(struct zpci_dev *zdev);
> > >
> > > #ifdef CONFIG_NUMA
> > >
> > > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > > index dce60f16e94a..b987c9d76510 100644
> > > --- a/arch/s390/pci/pci.c
> > > +++ b/arch/s390/pci/pci.c
> > > @@ -954,6 +954,55 @@ int zpci_report_error(struct pci_dev *pdev,
> > > }
> > > EXPORT_SYMBOL(zpci_report_error);
> > >
> > > +/**
> > > + * zpci_clear_error_state() - Clears the zPCI error state of the device
> > > + * @zdev: The zdev for which the zPCI error state should be reset
> > > + *
> > > + * Clear the zPCI error state of the device. If clearing the zPCI error state
> > > + * fails the device is left in the error state. In this case it may make sense
> > > + * to call zpci_io_perm_failure() on the associated pdev if it exists.
> > > + *
> > > + * Returns: 0 on success, -EIO otherwise
> > > + */
> > > +int zpci_clear_error_state(struct zpci_dev *zdev)
> > > +{
> > > + u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_ERROR);
> > > + struct zpci_fib fib = {0};
> > > + u8 status;
> > > + int rc;
> > > +
> > > + rc = zpci_mod_fc(req, &fib, &status);
> > > + if (rc)
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * zpci_reset_load_store_blocked() - Re-enables L/S from error state
> > > + * @zdev: The zdev for which to unblock load/store access
> > > + *
> > > + * R-eenables load/store access for a PCI function in the error state while
> > > + * keeping DMA blocked. In this state drivers can poke MMIO space to determine
> > > + * if error recovery is possible while catching any rogue DMA access from the
> > > + * device.
> > > + *
> > > + * Returns: 0 on success, -EIO otherwise
> > > + */
> > > +int zpci_reset_load_store_blocked(struct zpci_dev *zdev)
> > > +{
> > > + u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_BLOCK);
> > > + struct zpci_fib fib = {0};
> > > + u8 status;
> > > + int rc;
> > > +
> > > + rc = zpci_mod_fc(req, &fib, &status);
> > > + if (rc)
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int zpci_mem_init(void)
> > > {
> > > BUILD_BUG_ON(!is_power_of_2(__alignof__(struct zpci_fmb)) ||
> > > diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> > > index e868d996ec5b..73389e161872 100644
> > > --- a/arch/s390/pci/pci_event.c
> > > +++ b/arch/s390/pci/pci_event.c
> > > @@ -47,15 +47,182 @@ struct zpci_ccdf_avail {
> > > u16 pec; /* PCI event code */
> > > } __packed;
> > >
> > > +static inline bool ers_result_indicates_abort(pci_ers_result_t ers_res)
> > > +{
> > > + switch (ers_res) {
> > > + case PCI_ERS_RESULT_CAN_RECOVER:
> > > + case PCI_ERS_RESULT_RECOVERED:
> > > + case PCI_ERS_RESULT_NEED_RESET:
> > > + return false;
> > > + default:
> > > + return true;
> > > + }
> > > +}
> > > +
> > > +static pci_ers_result_t zpci_event_notify_error_detected(struct pci_dev *pdev)
> > > +{
> > > + pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
> > > + struct pci_driver *driver = pdev->driver;
> > > +
> > > + pr_debug("%s: asking driver to determine recoverability\n", pci_name(pdev));
> > > + ers_res = driver->err_handler->error_detected(pdev, pdev->error_state);
> > > + if (ers_result_indicates_abort(ers_res))
> > > + pr_info("%s: driver can't recover\n", pci_name(pdev));
> > > + else if (ers_res == PCI_ERS_RESULT_NEED_RESET)
> > > + pr_debug("%s: driver needs reset to recover\n", pci_name(pdev));
> >
> > Are you following a convention of using pr_info(), etc here? I try to
> > use the pci_info() family (wrappers around dev_info()) whenever I can.
>
> A convention in the sense that so far all code under arch/s390/pci/
> uses pr_info() which comes out as "zpci: ..". It seems that similar
> pr_info() constructs are also common in other arch/s390/ and
> drivers/s390 code.
That sounds like a convention to me. As long as it exists, I would
follow it. Maybe somebody will decide someday to convert them all at
once, but that would be a separate project.
> On the other hand we already agreed that some of the s390 specific PCI
> code is more like a PCI controller. So I'm open to suggestions.
>
On Tue, 2021-09-28 at 13:39 -0500, Bjorn Helgaas wrote:
> On Tue, Sep 28, 2021 at 08:30:44PM +0200, Niklas Schnelle wrote:
> > On Tue, 2021-09-28 at 13:11 -0500, Bjorn Helgaas wrote:
> > > On Thu, Sep 16, 2021 at 11:33:36AM +0200, Niklas Schnelle wrote:
> > > > When the platform detects an error on a PCI function or a service action
> > > > has been performed it is put in the error state and an error event
> > > > notification is provided to the OS.
> > > >
> > > > Currently we treat all error event notifications the same and simply set
> > > > pdev->error_state = pci_channel_io_perm_failure requiring user
> > > > intervention such as use of the recover attribute to get the device
> > > > usable again. Despite requiring a manual step this also has the
> > > > disadvantage that the device is completely torn down and recreated
> > > > resulting in higher level devices such as a block or network device
> > > > being recreated. In case of a block device this also means that it may
> > > > need to be removed and added to a software raid even if that could
> > > > otherwise survive with a temporary degradation.
> > > >
> > > > This is of course not ideal more so since an error notification with PEC
> > > > 0x3A indicates that the platform already performed error recovery
> > > > successfully or that the error state was caused by a service action that
> > > > is now finished.
> > > >
> > > > At least in this case we can assume that the error state can be reset
> > > > and the function made usable again. So as not to have the disadvantage
> > > > of a full tear down and recreation we need to coordinate this recovery
> > > > with the driver. Thankfully there is already a well defined recovery
> > > > flow for this described in Documentation/PCI/pci-error-recovery.rst.
> > > >
> > > > The implementation of this is somewhat straight forward and simplified
> > > > by the fact that our recovery flow is defined per PCI function. As
> > > > a reset we use the newly introduced zpci_hot_reset_device() which also
> > > > takes the PCI function out of the error state.
> > > >
> > > > Signed-off-by: Niklas Schnelle <[email protected]>
> > > > ---
> > > > v1 -> v2:
> > > > - Dropped use of pci_dev_is_added(), pdev->driver check is enough
> > > > - Improved some comments and messages
> > > >
> > > > arch/s390/include/asm/pci.h | 4 +-
> > > > arch/s390/pci/pci.c | 49 ++++++++++
> > > > arch/s390/pci/pci_event.c | 182 +++++++++++++++++++++++++++++++++++-
> > > > 3 files changed, 233 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > > > index 2a2ed165a270..558877aff2e5 100644
> > > > --- a/arch/s390/include/asm/pci.h
> > > > +++ b/arch/s390/include/asm/pci.h
> > > > @@ -294,8 +294,10 @@ void zpci_debug_exit(void);
> > > > void zpci_debug_init_device(struct zpci_dev *, const char *);
> > > > void zpci_debug_exit_device(struct zpci_dev *);
> > > >
> > > > -/* Error reporting */
> > > > +/* Error handling */
> > > > int zpci_report_error(struct pci_dev *, struct zpci_report_error_header *);
> > > > +int zpci_clear_error_state(struct zpci_dev *zdev);
> > > > +int zpci_reset_load_store_blocked(struct zpci_dev *zdev);
> > > >
> > > > #ifdef CONFIG_NUMA
> > > >
> > > > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > > > index dce60f16e94a..b987c9d76510 100644
> > > > --- a/arch/s390/pci/pci.c
> > > > +++ b/arch/s390/pci/pci.c
> > > > @@ -954,6 +954,55 @@ int zpci_report_error(struct pci_dev *pdev,
> > > > }
> > > > EXPORT_SYMBOL(zpci_report_error);
> > > >
> > > > +/**
> > > > + * zpci_clear_error_state() - Clears the zPCI error state of the device
> > > > + * @zdev: The zdev for which the zPCI error state should be reset
> > > > + *
> > > > + * Clear the zPCI error state of the device. If clearing the zPCI error state
> > > > + * fails the device is left in the error state. In this case it may make sense
> > > > + * to call zpci_io_perm_failure() on the associated pdev if it exists.
> > > > + *
> > > > + * Returns: 0 on success, -EIO otherwise
> > > > + */
> > > > +int zpci_clear_error_state(struct zpci_dev *zdev)
> > > > +{
> > > > + u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_ERROR);
> > > > + struct zpci_fib fib = {0};
> > > > + u8 status;
> > > > + int rc;
> > > > +
> > > > + rc = zpci_mod_fc(req, &fib, &status);
> > > > + if (rc)
> > > > + return -EIO;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * zpci_reset_load_store_blocked() - Re-enables L/S from error state
> > > > + * @zdev: The zdev for which to unblock load/store access
> > > > + *
> > > > + * R-eenables load/store access for a PCI function in the error state while
> > > > + * keeping DMA blocked. In this state drivers can poke MMIO space to determine
> > > > + * if error recovery is possible while catching any rogue DMA access from the
> > > > + * device.
> > > > + *
> > > > + * Returns: 0 on success, -EIO otherwise
> > > > + */
> > > > +int zpci_reset_load_store_blocked(struct zpci_dev *zdev)
> > > > +{
> > > > + u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_RESET_BLOCK);
> > > > + struct zpci_fib fib = {0};
> > > > + u8 status;
> > > > + int rc;
> > > > +
> > > > + rc = zpci_mod_fc(req, &fib, &status);
> > > > + if (rc)
> > > > + return -EIO;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int zpci_mem_init(void)
> > > > {
> > > > BUILD_BUG_ON(!is_power_of_2(__alignof__(struct zpci_fmb)) ||
> > > > diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> > > > index e868d996ec5b..73389e161872 100644
> > > > --- a/arch/s390/pci/pci_event.c
> > > > +++ b/arch/s390/pci/pci_event.c
> > > > @@ -47,15 +47,182 @@ struct zpci_ccdf_avail {
> > > > u16 pec; /* PCI event code */
> > > > } __packed;
> > > >
> > > > +static inline bool ers_result_indicates_abort(pci_ers_result_t ers_res)
> > > > +{
> > > > + switch (ers_res) {
> > > > + case PCI_ERS_RESULT_CAN_RECOVER:
> > > > + case PCI_ERS_RESULT_RECOVERED:
> > > > + case PCI_ERS_RESULT_NEED_RESET:
> > > > + return false;
> > > > + default:
> > > > + return true;
> > > > + }
> > > > +}
> > > > +
> > > > +static pci_ers_result_t zpci_event_notify_error_detected(struct pci_dev *pdev)
> > > > +{
> > > > + pci_ers_result_t ers_res = PCI_ERS_RESULT_DISCONNECT;
> > > > + struct pci_driver *driver = pdev->driver;
> > > > +
> > > > + pr_debug("%s: asking driver to determine recoverability\n", pci_name(pdev));
> > > > + ers_res = driver->err_handler->error_detected(pdev, pdev->error_state);
> > > > + if (ers_result_indicates_abort(ers_res))
> > > > + pr_info("%s: driver can't recover\n", pci_name(pdev));
> > > > + else if (ers_res == PCI_ERS_RESULT_NEED_RESET)
> > > > + pr_debug("%s: driver needs reset to recover\n", pci_name(pdev));
> > >
> > > Are you following a convention of using pr_info(), etc here? I try to
> > > use the pci_info() family (wrappers around dev_info()) whenever I can.
> >
> > A convention in the sense that so far all code under arch/s390/pci/
> > uses pr_info() which comes out as "zpci: ..". It seems that similar
> > pr_info() constructs are also common in other arch/s390/ and
> > drivers/s390 code.
>
> That sounds like a convention to me. As long as it exists, I would
> follow it. Maybe somebody will decide someday to convert them all at
> once, but that would be a separate project.
I agree. I think this might make sense if/when we split the PCI
controller like parts off and move them to drivers/pci/. Thanks anyway
for the suggestion it's definitely something to keep in mind.