Re: [PATCH v2 2/2] s390/pci: Extend device lock to cover zpci_report_status()

On Fri, Aug 21, 2026 at 03:14:32PM +0200, Niklas Schnelle wrote:
> On Fri, 2026-08-21 at 14:59 +0200, Benjamin Block wrote:
> > On Wed, Aug 19, 2026 at 02:30:18PM -0700, Farhan Ali wrote:
> > > 
> > > On 8/19/2026 2:58 AM, Niklas Schnelle wrote:
> > > > As zpci_report_status() accesses the device's driver the device lock
> > > > needs to be held. Extend the critical section to cover the
> > > > zpci_report_status() call.
> > > > 
> > > > Cc: [email protected]
> > > > Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
> > > > Signed-off-by: Niklas Schnelle <[email protected]>
> > > > ---
> > > >   arch/s390/pci/pci_event.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> > > > index 839bd91c056e..48c150d6ba81 100644
> > > > --- a/arch/s390/pci/pci_event.c
> > > > +++ b/arch/s390/pci/pci_event.c
> > > > @@ -256,8 +256,8 @@ static pci_ers_result_t zpci_event_attempt_error_recovery(struct pci_dev *pdev)
> > > >   		driver->err_handler->resume(pdev);
> > > >   	pci_uevent_ers(pdev, PCI_ERS_RESULT_RECOVERED);
> > > >   out_unlock:
> > > > -	device_unlock(&pdev->dev);
> > > >   	zpci_report_status(zdev, "recovery", status_str);
> > > > +	device_unlock(&pdev->dev);
> > > >   
> > > >   	return ers_res;
> > > >   }
> > > 
> > > AFAICT this change is correct, but should we also add a lockdep_assert 
> > 
> > Andy maybe the context one `__must_hold()` or something as function attribute.
> > 
> > > in zpci_report_status()? Since this is the only placed its called, it 
> > > maybe fine as is. I just fear that we could miss on getting the lock if 
> > > we were to re-use zpci_report_status().
> > 
> > Yeah, I agree. It's not obvious.
> > 
> > I guess the critical point here why we need the lock is this line:
> > 
> > 		driver = to_pci_driver(pdev->dev.driver);
> > 
> > right? Because we assume the relation between driver and device stays intact
> > after we get that assignment.
> > 
> > Because otherwise I don't see why we must get the lock, if we get a valid
> > reference in zpci_report_status().
> 
> Yes exactly, we need to make sure the driver isn't unbound or worse
> unloaded in the middle of it. It's quite subtle which is why I forgot
> it in the first place I guess.
> 
> Also note that the function may be called, and does handle, both an
> unbound device (driver == NULL) and/or a zdev prior to probing the
> associated pdev (pdev == NULL). This means the device_lock_assert()
> needs to be inside the !pdev block. I'd think this also means we can't
> really use __must_hold(), right?

Hmm, true. Yeah, that makes it awkward. Doesn't seem like we can use
`__must_hold()` here then. Always found that nice since it is right next to
the function name; can hardly miss it when one uses the function.

-- 
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Wolfgang Wendt         /        Geschäftsführung: David Faller
Sitz der Ges.: Ehningen     /     Registergericht: AmtsG Stuttgart, HRB 243294