[PATCH v2 0/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[PATCH v2 0/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[Niklas Schnelle]

Hi All,

This series fixes two issues found in zpci_report_status() by
LLM review. A missing pci_dev_put() which could prevent proper removal
on hot unplug and a too short device lock critical section.

Thanks,
Niklas

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Changes in v2:
- Added a second fix to address Sashiko's finding of having
  to hold the device lock during the call to zpci_report_status()
- Add R-bs from Matthew and Farhan
- Link to v1: https://lore.kernel.org/r/20260818-fix_zpci_report_status_pdev_leak-v1-1-576520f4d068@linux.ibm.com

---
Niklas Schnelle (2):
      s390/pci: Fix missing pci_dev_put() in zpci_report_status()
      s390/pci: Extend device lock to cover zpci_report_status()

 arch/s390/pci/pci_event.c  | 2 +-
 arch/s390/pci/pci_report.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260817-fix_zpci_report_status_pdev_leak-1b101c1fe021

Best regards,
-- 
Niklas Schnelle

[PATCH v2 1/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[Niklas Schnelle]

In zpci_report_status() a reference to the struct pci_dev associated
with the struct zpci_dev being reported about is acquired using
pci_get_slot(). This reference needs to be dropped with pci_dev_put()
but this call is missing thus leaking the reference. On subsequent hot
unplug this will cause the struct pci_dev to not be released leaking
memory and potentially prevent reattach.

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Cc: stable@vger.kernel.org
Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/s390/pci/pci_report.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/pci/pci_report.c b/arch/s390/pci/pci_report.c
index 7030f7052926..76b8afc146fe 100644
--- a/arch/s390/pci/pci_report.c
+++ b/arch/s390/pci/pci_report.c
@@ -151,6 +151,7 @@ int zpci_report_status(struct zpci_dev *zdev, const char *operation, const char
 	else
 		pr_info("Reported PCI device status\n");
 
+	pci_dev_put(pdev);
 	free_page((unsigned long)report);
 
 	return ret;

-- 
2.53.0

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

[Niklas Schnelle]

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: stable@vger.kernel.org
Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 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;
 }

-- 
2.53.0

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

[Matthew Rosato]

On 8/19/26 5: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: stable@vger.kernel.org
> Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

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

[Farhan Ali]

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: stable@vger.kernel.org
> Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>   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 
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().

Either way
Reviewed-by: Farhan Ali<alifm@linux.ibm.com>

Thanks

Farhan

Re: [PATCH v2 1/2] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[Benjamin Block]

On Wed, Aug 19, 2026 at 11:58:30AM +0200, Niklas Schnelle wrote:
> In zpci_report_status() a reference to the struct pci_dev associated
> with the struct zpci_dev being reported about is acquired using
> pci_get_slot(). This reference needs to be dropped with pci_dev_put()
> but this call is missing thus leaking the reference. On subsequent hot
> unplug this will cause the struct pci_dev to not be released leaking
> memory and potentially prevent reattach.
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Cc: stable@vger.kernel.org
> Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
> Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  arch/s390/pci/pci_report.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/pci/pci_report.c b/arch/s390/pci/pci_report.c
> index 7030f7052926..76b8afc146fe 100644
> --- a/arch/s390/pci/pci_report.c
> +++ b/arch/s390/pci/pci_report.c
> @@ -151,6 +151,7 @@ int zpci_report_status(struct zpci_dev *zdev, const char *operation, const char
>  	else
>  		pr_info("Reported PCI device status\n");
>  
> +	pci_dev_put(pdev);
>  	free_page((unsigned long)report);
>  
>  	return ret;

Looks good to me!


Reviewed-by: Benjamin Block <bblock@linux.ibm.com>

-- 
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

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

[Niklas Schnelle]

On Wed, 2026-08-19 at 14:30 -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: stable@vger.kernel.org
> > Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >   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 
> 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().
> 
> Either way
> Reviewed-by: Farhan Ali<alifm@linux.ibm.com>

Was pondering that as well. I was on the edge so left it out but since
you're mentioning it too I'll add it and send a v3 with that plus all
the R-bs.

Thanks,
Niklas

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

[Benjamin Block]

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: stable@vger.kernel.org
> > Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >   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().

-- 
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

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

[Niklas Schnelle]

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: stable@vger.kernel.org
> > > Fixes: 4ec6054e7321 ("s390/pci: Report PCI error recovery results via SCLP")
> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > ---
> > >   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?

Thanks,
Niklas