Re: [PATCH v14 4/7] s390/pci: Store PCI error information for passthrough devices

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Wed, 2026-04-29 at 09:48 -0700, Farhan Ali wrote:
> On 4/29/2026 4:41 AM, Niklas Schnelle wrote:
> > On Tue, 2026-04-21 at 09:30 -0700, Farhan Ali wrote:
> > > For a passthrough device we need co-operation from user space to recover
> > > the device. This would require to bubble up any error information to user
> > > space.  Let's store this error information for passthrough devices, so it
> > > can be retrieved later.
> > > 
> > > We can now have userspace drivers (vfio-pci based) on s390x. The userspace
> > > drivers will not have any KVM fd and so no kzdev associated with them. So
> > > we need to update the logic for detecting passthrough devices to not depend
> > > on struct kvm_zdev.
> > > 
> > > Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> > > ---
> > >   arch/s390/include/asm/pci.h      |  30 ++++++++
> > >   arch/s390/pci/pci.c              |   1 +
> > >   arch/s390/pci/pci_event.c        | 117 +++++++++++++++++--------------
> > >   drivers/vfio/pci/vfio_pci_zdev.c |   9 ++-
> > >   4 files changed, 105 insertions(+), 52 deletions(-)
> > > 
> > --- snip ---
> > > +
> > > +void zpci_start_mediated_recovery(struct zpci_dev *zdev)
> > > +{
> > > +	guard(mutex)(&zdev->pending_errs_lock);
> > > +	zdev->pending_errs.mediated_recovery = true;
> > > +}
> > > +EXPORT_SYMBOL_GPL(zpci_start_mediated_recovery);
> > > +
> > > +void zpci_stop_mediated_recovery(struct zpci_dev *zdev)
> > > +{
> > > +	struct pci_dev *pdev = NULL;
> > > +
> > > +	guard(mutex)(&zdev->pending_errs_lock);
> > > +	zdev->pending_errs.mediated_recovery = false;
> > > +	pdev = pci_get_slot(zdev->zbus->bus, zdev->devfn);
> > > +	if (zdev->pending_errs.count)
> > > +		pr_info("%s: Unhandled PCI error events count=%d",
> > > +			pci_name(pdev), zdev->pending_errs.count);
> > Sashiko notes a possible ABBA locking issue here between
> > pending_errs_lock and the pci_bus_sem inside pci_get_slot(). I wonder
> > if that would also be visible with lockdep? Also Sashiko notes that
> > zdev->zbus->bus could be NULL I don't think this is possible at the
> > current callsites via vfio-pci though. Similarly I don't think
> > pci_get_slot() can really be NULL at the current call sites. This makes
> > me wonder however, would it maybe be cleaner to pass a struct pci_dev
> > here so you don't need the pci_get_slot() at all? Both
> > vfio_pci_zdev_open_device() and vfio_pci_zdev_close_device() have that
> > readily available via vdev->pdev.
> 
> The pdev here was meant for helpful error message. On second thought 
> maybe removing the pdev usage, and using the fid would be better?

Either that or maybe just return the number of left over errors and do
the print in the caller?

> 
> > 
> > 
> > > +	memset(&zdev->pending_errs, 0, sizeof(struct zpci_ccdf_pending));
> > > +	pci_dev_put(pdev);
> > > +}
> > > +EXPORT_SYMBOL_GPL(zpci_stop_mediated_recovery);
> > > +
> > >   static pci_ers_result_t zpci_event_notify_error_detected(struct pci_dev *pdev,
> > >   							 struct pci_driver *driver)
> > >   {
> > > @@ -175,7 +190,8 @@ static pci_ers_result_t zpci_event_do_reset(struct pci_dev *pdev,
> > >    * 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)
> > > +static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev,
> > > +							  struct zpci_ccdf_err *ccdf)
> > >   {
> > > 
--- snip ---
> > >   
> > > +	scoped_guard(mutex, &zdev->pending_errs_lock) {
> > > +		if (zdev->pending_errs.mediated_recovery) {
> > > +			pr_info("%s: Leaving recovery of pass-through device to user-space\n",
> > > +				pci_name(pdev));
> > > +			ers_res = PCI_ERS_RESULT_RECOVERED;
> > > +			status_str = "in progress";
> > > +			goto out_unlock;
> > > +		}
> > > +	}
> > > +
> > Sashiko notes that mixing goto unlock and lock guards within one
> > function is discouraged. Here it's not that hard to read since it is
> > just a scoped guard but I think we should still not mix it.
> > 
> > However if we also convert the device_lock() to a guard lock here the
> > goto would still make sense to go to the zpci_report_status() and that
> > is still a bit odd with guarded locks. So I think an alternative simple
> > fix, that makes this overall cleaner too, is to put the whole
> > scoped_guard() block above into a helper function then you can do the
> > goto out_unlock on that helper returning PCI_ERS_RESULT_RECVOERED in
> > line with e.g. zpci_event_notify_error_detected(). That way you don't
> > need to touch existing locks and you get to keep the guard locking.
> 
> How about changing it to mutex_lock/mutex_unlock? I think the block is 
> small enough that it shouldn't be too confusing. Moving to a function 
> opens up the possibility of using a stale value for mediated_recovery.

I'm probably missing something but I don't see how moving it to a
helper function changes whether the mediated_recovery can be stale?
You'd still use the same pending_errs_lock and passed in struct
zpci_dev * then you just return PCI_ERS_RESULT_RECOVERED if mediated
recovery is in effect? And you can do the pr_info() at the caller so
you don't need to pass in the pdev too.

> >