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

[Alex Williamson]

On Tue, 7 Apr 2026 11:00:05 -0700
Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
> On 4/7/2026 8:38 AM, Alex Williamson wrote:
> > On Mon, 30 Mar 2026 10:40:08 -0700
> > Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
> >> @@ -194,13 +214,6 @@ static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev)
> >>   	}
> >>   	pdev->error_state = pci_channel_io_frozen;
> >>   
> >> -	if (is_passed_through(pdev)) {
> >> -		pr_info("%s: Cannot be recovered in the host because it is a pass-through device\n",
> >> -			pci_name(pdev));
> >> -		status_str = "failed (pass-through)";
> >> -		goto out_unlock;
> >> -	}
> >> -
> >>   	driver = to_pci_driver(pdev->dev.driver);
> >>   	if (!is_driver_supported(driver)) {
> >>   		if (!driver) {
> >> @@ -216,12 +229,23 @@ static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev)
> >>   		goto out_unlock;
> >>   	}
> >>   
> >> +	if (needs_mediated_recovery(pdev))  
> > The test result is invalid here.  Why not call zpci_store_pci_error()
> > unconditionally here and wrap both in the same guard?
> > needs_mediated_recovery() should have a lockdep_assert.  
> 
> I think storing the error via zpci_store_pci_error() only made sense if 
> the error needed to be handled outside the kernel. Thinking more now I 
> think it makes sense to have separate locks, as the mediated_recovery 
> flag really provides information on if the vfio device is still opened 
> or closed.

I don't think separate locks help the situation, the might conflate it
further.  I think the problem here is that we're using the lock as if
it protects two separate things, the mediated_recovery flag vs the
pending_errs structure, but in fact they're related.  We only record
errors to the structure when we're in mediated_recovery and we clear
the structure when exiting mediated recovery.  One lock makes sense to
me, but it needs to protect both the state of the flag and the use of
the structure together, not independently.  Thanks,

Alex