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

[Farhan Ali <alifm@linux.ibm.com>]

On 4/30/2026 12:48 AM, Niklas Schnelle wrote:
> 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.

I think Alex raised a concern about a similar approach in a previous 
version[1]. IIUC there could be a device open/close when we check the 
mediated_recovery and return PCI_ERS_RESULT_RECOVERED to when execute 
the goto. So i think its safer to do this with the pending_errs lock held.

[1]https://lore.kernel.org/all/20260407093814.3d95263a@shazbot.org/

Thanks

Farhan