Linux s390 Architecture development
 help / color / mirror / Atom feed
* [PATCH v3 0/3] s390/pci: Enable CONTEXT_ANALYSIS
@ 2026-08-05 13:36 Heiko Carstens
  2026-08-05 13:36 ` [PATCH v3 1/3] s390/pci: Rework __zpci_event_error() to remove conditional locking Heiko Carstens
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Heiko Carstens @ 2026-08-05 13:36 UTC (permalink / raw)
  To: Alexander Gordeev, Sven Schnelle, Vasily Gorbik,
	Christian Borntraeger, Niklas Schnelle, Gerd Bayer
  Cc: linux-s390, linux-kernel

v3:

- Add Reviewed-by tags from Niklas to first and third patch and address
  nits [Niklas]

- Refactor second patch again to make it hopefully more readable [Niklas]

v2:

- Address potential zdev reference leak (false positive) by adding a
  comment, and moving get_zdev_by_fid() [Sashiko, Niklas]

- Note: this reveals a pre-existing bug: zpci_remove_reserved_devices()
  calls zpci_device_reserved() without holding locks. It looks like
  this can be fixed easily by adding a mutex_lock()/mutex_unlock()
  pair in zpci_remove_reserved_devices(), but that is not within
  the scope of this series, and up to the pci maintainers

v1:
Enable CONTEXT_ANALYSYS for s390's pci code.

Static code checking for acquiring and releasing locks used to be done
with sparse. That was removed with [1] and replaced with a clang based
approach [2]. The new approach requires that each subsystem needs to be
explicitly enabled for checking.

Do that for s390's pci code. In order to avoid false positives the code
has to be slightly reworked, since conditionally acquiring and releasing
locks does not work with the checker (besides that this is sub optimal
coding style).

[1] 5b63d0ae94cc ("compiler-context-analysis: Remove Sparse support")
[2] 3269701cb256 ("compiler-context-analysis: Add infrastructure for Context Analysis with Clang")

Heiko Carstens (3):
  s390/pci: Rework __zpci_event_error() to remove conditional locking
  s390/pci: Rework __zpci_event_availability() to remove conditional locking
  s390/pci: Enable CONTEXT_ANALYSIS

 arch/s390/pci/Makefile    |   2 +
 arch/s390/pci/pci_event.c | 205 ++++++++++++++++++++------------------
 2 files changed, 111 insertions(+), 96 deletions(-)

-- 
2.53.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/3] s390/pci: Rework __zpci_event_error() to remove conditional locking
  2026-08-05 13:36 [PATCH v3 0/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
@ 2026-08-05 13:36 ` Heiko Carstens
  2026-08-05 14:09   ` sashiko-bot
  2026-08-05 13:36 ` [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() " Heiko Carstens
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Heiko Carstens @ 2026-08-05 13:36 UTC (permalink / raw)
  To: Alexander Gordeev, Sven Schnelle, Vasily Gorbik,
	Christian Borntraeger, Niklas Schnelle, Gerd Bayer
  Cc: linux-s390, linux-kernel

Clang's compiler based static context analysis does not work with
locks that are conditionally taken like in __zpci_event_error():

arch/s390/pci/pci_event.c:320:2: warning: mutex 'get_zdev_by_fid(ccdf->fid).state_lock'
 is not held on every path through here [-Wthread-safety-analysis]

Given that code which takes locks conditionally can be considered
suboptimal rework __zpci_event_error() to get rid of this.

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/pci/pci_event.c | 43 ++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index 839bd91c056e..bead4ed5d4ab 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -288,6 +288,12 @@ static void zpci_event_io_failure(struct pci_dev *pdev, pci_channel_state_t es)
 	pci_dev_unlock(pdev);
 }
 
+static void __zpci_event_print_error(struct pci_dev *pdev, struct zpci_ccdf_err *ccdf)
+{
+	pr_err("%s: Event 0x%x reports an error for PCI function 0x%x\n",
+	       pdev ? pci_name(pdev) : "n/a", ccdf->pec, ccdf->fid);
+}
+
 static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
 {
 	struct zpci_dev *zdev = get_zdev_by_fid(ccdf->fid);
@@ -301,24 +307,24 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
 	zpci_err("error CCDF:\n");
 	zpci_err_hex(ccdf, sizeof(*ccdf));
 
-	if (zdev) {
-		mutex_lock(&zdev->state_lock);
-		rc = clp_refresh_fh(zdev->fid, &fh);
-		if (rc)
-			goto no_pdev;
-		if (!fh || ccdf->fh != fh) {
-			/* Ignore events with stale handles */
-			zpci_dbg(3, "err fid:%x, fh:%x (stale %x)\n",
-				 ccdf->fid, fh, ccdf->fh);
-			goto no_pdev;
-		}
-		zpci_update_fh(zdev, ccdf->fh);
-		if (zdev->zbus->bus)
-			pdev = pci_get_slot(zdev->zbus->bus, zdev->devfn);
-	}
+	if (!zdev)
+		return __zpci_event_print_error(NULL, ccdf);
 
-	pr_err("%s: Event 0x%x reports an error for PCI function 0x%x\n",
-	       pdev ? pci_name(pdev) : "n/a", ccdf->pec, ccdf->fid);
+	mutex_lock(&zdev->state_lock);
+	rc = clp_refresh_fh(zdev->fid, &fh);
+	if (rc)
+		goto no_pdev;
+	if (!fh || ccdf->fh != fh) {
+		/* Ignore events with stale handles */
+		zpci_dbg(3, "err fid:%x, fh:%x (stale %x)\n",
+			 ccdf->fid, fh, ccdf->fh);
+		goto no_pdev;
+	}
+	zpci_update_fh(zdev, ccdf->fh);
+	if (zdev->zbus->bus)
+		pdev = pci_get_slot(zdev->zbus->bus, zdev->devfn);
+
+	__zpci_event_print_error(pdev, ccdf);
 
 	if (!pdev)
 		goto no_pdev;
@@ -340,8 +346,7 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
 	}
 	pci_dev_put(pdev);
 no_pdev:
-	if (zdev)
-		mutex_unlock(&zdev->state_lock);
+	mutex_unlock(&zdev->state_lock);
 	zpci_zdev_put(zdev);
 }
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() to remove conditional locking
  2026-08-05 13:36 [PATCH v3 0/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
  2026-08-05 13:36 ` [PATCH v3 1/3] s390/pci: Rework __zpci_event_error() to remove conditional locking Heiko Carstens
@ 2026-08-05 13:36 ` Heiko Carstens
  2026-08-05 14:01   ` Niklas Schnelle
  2026-08-05 14:23   ` sashiko-bot
  2026-08-05 13:36 ` [PATCH v3 3/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
  2026-08-07 10:48 ` [PATCH v3 0/3] " Heiko Carstens
  3 siblings, 2 replies; 9+ messages in thread
From: Heiko Carstens @ 2026-08-05 13:36 UTC (permalink / raw)
  To: Alexander Gordeev, Sven Schnelle, Vasily Gorbik,
	Christian Borntraeger, Niklas Schnelle, Gerd Bayer
  Cc: linux-s390, linux-kernel

Clang's compiler based static context analysis does not work with locks
that are conditionally taken like in __zpci_event_availability():

arch/s390/pci/pci_event.c:402:10: warning: mutex 'get_zdev_by_fid(ccdf->fid).state_lock'
  is not held on every path through here [-Wthread-safety-analysis]

Given that code which takes locks conditionally can be considered
suboptimal rework __zpci_event_availability() to get rid of this.

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/pci/pci_event.c | 162 ++++++++++++++++++++------------------
 1 file changed, 85 insertions(+), 77 deletions(-)

diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index bead4ed5d4ab..0a9eecb62bd1 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -387,98 +387,106 @@ static void zpci_event_reappear(struct zpci_dev *zdev)
 	zpci_dbg(1, "rea fid:%x, fh:%x\n", zdev->fid, zdev->fh);
 }
 
-static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
+static bool zpci_event_avail_any_device(struct zpci_ccdf_avail *ccdf)
 {
-	struct zpci_dev *zdev = get_zdev_by_fid(ccdf->fid);
-	bool existing_zdev = !!zdev;
-	enum zpci_state state;
+	/* 0x0306 - No handle or fid stored */
+	if (ccdf->pec != 0x0306)
+		return false;
+	/* 0x308 or 0x302 for multiple devices */
+	zpci_remove_reserved_devices();
+	zpci_scan_devices();
+	return true;
+}
 
-	zpci_dbg(3, "avl fid:%x, fh:%x, pec:%x\n",
-		 ccdf->fid, ccdf->fh, ccdf->pec);
-
-	if (existing_zdev)
-		mutex_lock(&zdev->state_lock);
+static void zpci_event_avail_new_device(struct zpci_ccdf_avail *ccdf)
+{
+	struct zpci_dev *zdev;
 
 	switch (ccdf->pec) {
 	case 0x0301: /* Reserved|Standby -> Configured */
-		if (!zdev) {
-			zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_CONFIGURED);
-			if (IS_ERR(zdev))
-				break;
-			if (zpci_add_device(zdev)) {
-				kfree(zdev);
-				break;
-			}
-		} else {
-			if (zdev->state == ZPCI_FN_STATE_RESERVED)
-				zpci_event_reappear(zdev);
-			/* the configuration request may be stale */
-			else if (zdev->state != ZPCI_FN_STATE_STANDBY)
-				break;
-			zdev->state = ZPCI_FN_STATE_CONFIGURED;
+		zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_CONFIGURED);
+		if (IS_ERR(zdev))
+			break;
+		if (zpci_add_device(zdev)) {
+			kfree(zdev);
+			break;
 		}
 		zpci_scan_configured_device(zdev, ccdf->fh);
 		break;
 	case 0x0302: /* Reserved -> Standby */
-		if (!zdev) {
-			zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_STANDBY);
-			if (IS_ERR(zdev))
-				break;
-			if (zpci_add_device(zdev)) {
-				kfree(zdev);
-				break;
-			}
-		} else {
-			if (zdev->state == ZPCI_FN_STATE_RESERVED)
-				zpci_event_reappear(zdev);
-			zpci_update_fh(zdev, ccdf->fh);
-		}
-		break;
-	case 0x0303: /* Deconfiguration requested */
-		if (zdev) {
-			/* The event may have been queued before we configured
-			 * the device.
-			 */
-			if (zdev->state != ZPCI_FN_STATE_CONFIGURED)
-				break;
-			zpci_update_fh(zdev, ccdf->fh);
-			zpci_deconfigure_device(zdev);
-		}
-		break;
-	case 0x0304: /* Configured -> Standby|Reserved */
-		if (zdev) {
-			/* The event may have been queued before we configured
-			 * the device.:
-			 */
-			if (zdev->state == ZPCI_FN_STATE_CONFIGURED)
-				zpci_event_hard_deconfigured(zdev, ccdf->fh);
-			/* The 0x0304 event may immediately reserve the device */
-			if (!clp_get_state(zdev->fid, &state) &&
-			    state == ZPCI_FN_STATE_RESERVED) {
-				zpci_device_reserved(zdev);
-			}
-		}
-		break;
-	case 0x0306: /* 0x308 or 0x302 for multiple devices */
-		zpci_remove_reserved_devices();
-		zpci_scan_devices();
-		break;
-	case 0x0308: /* Standby -> Reserved */
-		if (!zdev)
+		zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_STANDBY);
+		if (IS_ERR(zdev))
 			break;
-		zpci_device_reserved(zdev);
-		break;
-	default:
+		if (zpci_add_device(zdev)) {
+			kfree(zdev);
+			break;
+		}
 		break;
 	}
-	if (existing_zdev) {
-		mutex_unlock(&zdev->state_lock);
-		zpci_zdev_put(zdev);
+}
+
+static void zpci_event_avail_existing_device(struct zpci_dev *zdev, struct zpci_ccdf_avail *ccdf)
+{
+	enum zpci_state state;
+
+	switch (ccdf->pec) {
+	case 0x0301: /* Reserved|Standby -> Configured */
+		if (zdev->state == ZPCI_FN_STATE_RESERVED)
+			zpci_event_reappear(zdev);
+		/* the configuration request may be stale */
+		else if (zdev->state != ZPCI_FN_STATE_STANDBY)
+			break;
+		zdev->state = ZPCI_FN_STATE_CONFIGURED;
+		zpci_scan_configured_device(zdev, ccdf->fh);
+		break;
+	case 0x0302: /* Reserved -> Standby */
+		if (zdev->state == ZPCI_FN_STATE_RESERVED)
+			zpci_event_reappear(zdev);
+		zpci_update_fh(zdev, ccdf->fh);
+		break;
+	case 0x0303: /* Deconfiguration requested */
+		/* The event may have been queued before we configured
+		 * the device.
+		 */
+		if (zdev->state != ZPCI_FN_STATE_CONFIGURED)
+			break;
+		zpci_update_fh(zdev, ccdf->fh);
+		zpci_deconfigure_device(zdev);
+		break;
+	case 0x0304: /* Configured -> Standby|Reserved */
+		/* The event may have been queued before we configured
+		 * the device.:
+		 */
+		if (zdev->state == ZPCI_FN_STATE_CONFIGURED)
+			zpci_event_hard_deconfigured(zdev, ccdf->fh);
+		/* The 0x0304 event may immediately reserve the device */
+		if (!clp_get_state(zdev->fid, &state) &&
+		    state == ZPCI_FN_STATE_RESERVED) {
+			zpci_device_reserved(zdev);
+		}
+		break;
+	case 0x0308: /* Standby -> Reserved */
+		zpci_device_reserved(zdev);
+		break;
 	}
 }
 
 void zpci_event_availability(void *data)
 {
-	if (zpci_is_enabled())
-		__zpci_event_availability(data);
+	struct zpci_ccdf_avail *ccdf = data;
+	struct zpci_dev *zdev;
+
+	if (!zpci_is_enabled())
+		return;
+	zpci_dbg(3, "avl fid:%x, fh:%x, pec:%x\n",
+		 ccdf->fid, ccdf->fh, ccdf->pec);
+	if (zpci_event_avail_any_device(ccdf))
+		return;
+	zdev = get_zdev_by_fid(ccdf->fid);
+	if (!zdev)
+		return zpci_event_avail_new_device(ccdf);
+	mutex_lock(&zdev->state_lock);
+	zpci_event_avail_existing_device(zdev, ccdf);
+	mutex_unlock(&zdev->state_lock);
+	zpci_zdev_put(zdev);
 }
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 3/3] s390/pci: Enable CONTEXT_ANALYSIS
  2026-08-05 13:36 [PATCH v3 0/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
  2026-08-05 13:36 ` [PATCH v3 1/3] s390/pci: Rework __zpci_event_error() to remove conditional locking Heiko Carstens
  2026-08-05 13:36 ` [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() " Heiko Carstens
@ 2026-08-05 13:36 ` Heiko Carstens
  2026-08-05 14:12   ` sashiko-bot
  2026-08-07 10:48 ` [PATCH v3 0/3] " Heiko Carstens
  3 siblings, 1 reply; 9+ messages in thread
From: Heiko Carstens @ 2026-08-05 13:36 UTC (permalink / raw)
  To: Alexander Gordeev, Sven Schnelle, Vasily Gorbik,
	Christian Borntraeger, Niklas Schnelle, Gerd Bayer
  Cc: linux-s390, linux-kernel

Enable CONTEXT_ANALYSIS since s390's pci code compiles now without
warnings.

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/pci/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
index 1810e0944a4e..b8be1316491b 100644
--- a/arch/s390/pci/Makefile
+++ b/arch/s390/pci/Makefile
@@ -3,6 +3,8 @@
 # Makefile for the s390 PCI subsystem.
 #
 
+CONTEXT_ANALYSIS := y
+
 obj-$(CONFIG_PCI)	+= pci.o pci_irq.o pci_clp.o \
 			   pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
 			   pci_bus.o pci_kvm_hook.o pci_report.o pci_fixup.o
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() to remove conditional locking
  2026-08-05 13:36 ` [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() " Heiko Carstens
@ 2026-08-05 14:01   ` Niklas Schnelle
  2026-08-05 14:23   ` sashiko-bot
  1 sibling, 0 replies; 9+ messages in thread
From: Niklas Schnelle @ 2026-08-05 14:01 UTC (permalink / raw)
  To: Heiko Carstens, Alexander Gordeev, Sven Schnelle, Vasily Gorbik,
	Christian Borntraeger, Gerd Bayer
  Cc: linux-s390, linux-kernel

On Wed, 2026-08-05 at 15:36 +0200, Heiko Carstens wrote:
> Clang's compiler based static context analysis does not work with locks
> that are conditionally taken like in __zpci_event_availability():
> 
> arch/s390/pci/pci_event.c:402:10: warning: mutex 'get_zdev_by_fid(ccdf->fid).state_lock'
>   is not held on every path through here [-Wthread-safety-analysis]
> 
> Given that code which takes locks conditionally can be considered
> suboptimal rework __zpci_event_availability() to get rid of this.
> 
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/pci/pci_event.c | 162 ++++++++++++++++++++------------------
>  1 file changed, 85 insertions(+), 77 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index bead4ed5d4ab..0a9eecb62bd1 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
--- snip ---
>  void zpci_event_availability(void *data)
>  {
> -	if (zpci_is_enabled())
> -		__zpci_event_availability(data);
> +	struct zpci_ccdf_avail *ccdf = data;
> +	struct zpci_dev *zdev;
> +
> +	if (!zpci_is_enabled())
> +		return;
> +	zpci_dbg(3, "avl fid:%x, fh:%x, pec:%x\n",
> +		 ccdf->fid, ccdf->fh, ccdf->pec);
> +	if (zpci_event_avail_any_device(ccdf))
> +		return;
> +	zdev = get_zdev_by_fid(ccdf->fid);
> +	if (!zdev)
> +		return zpci_event_avail_new_device(ccdf);
> +	mutex_lock(&zdev->state_lock);
> +	zpci_event_avail_existing_device(zdev, ccdf);
> +	mutex_unlock(&zdev->state_lock);
> +	zpci_zdev_put(zdev);
>  }

Thank you, this is exactly what I had in mind! And in my opinion
definitely much cleaner than even the original code.

Feel free to add:

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>

Thanks,
Niklas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/3] s390/pci: Rework __zpci_event_error() to remove conditional locking
  2026-08-05 13:36 ` [PATCH v3 1/3] s390/pci: Rework __zpci_event_error() to remove conditional locking Heiko Carstens
@ 2026-08-05 14:09   ` sashiko-bot
  0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-05 14:09 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Vasily Gorbik, Heiko Carstens, Alexander Gordeev, linux-s390

> Clang's compiler based static context analysis does not work with
> locks that are conditionally taken like in __zpci_event_error():
> 
> arch/s390/pci/pci_event.c:320:2: warning: mutex 'get_zdev_by_fid(ccdf->fid).state_lock'
>  is not held on every path through here [-Wthread-safety-analysis]
> 
> Given that code which takes locks conditionally can be considered
> suboptimal rework __zpci_event_error() to get rid of this.
> 
> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805133627.1216607-1-hca@linux.ibm.com?part=1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] s390/pci: Enable CONTEXT_ANALYSIS
  2026-08-05 13:36 ` [PATCH v3 3/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
@ 2026-08-05 14:12   ` sashiko-bot
  0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-05 14:12 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Vasily Gorbik, Heiko Carstens, Alexander Gordeev, linux-s390

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing lock acquisition in zpci_remove_reserved_devices() leading to lockdep failure and state corruption.
- [High] Race condition in zpci_event_avail_new_device() exposing an unlocked device to concurrent state changes.
--

--- Patch [3]: [PATCH v3 3/3] s390/pci: Enable CONTEXT_ANALYSIS ---
commit f14ecb575360924ecbbf3801f2b0b0e2ff3a2eeb
Author: Heiko Carstens <hca@linux.ibm.com>

s390/pci: Enable CONTEXT_ANALYSIS

This commit enables CONTEXT_ANALYSIS in the s390 PCI Makefile since the
code now compiles without warnings.

> diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
> index 1810e0944a4ed..b8be1316491b8 100644
> --- a/arch/s390/pci/Makefile
> +++ b/arch/s390/pci/Makefile
> @@ -3,6 +3,8 @@
>  # Makefile for the s390 PCI subsystem.
>  #
>  
> +CONTEXT_ANALYSIS := y
> +

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but since context analysis is being enabled to catch locking issues,
I noticed a missing lock acquisition that it might not be catching.

In arch/s390/pci/pci.c:zpci_remove_reserved_devices():

	list_for_each_entry_safe(zdev, tmp, &remove, entry)
		zpci_device_reserved(zdev);

The function zpci_device_reserved() unconditionally asserts
lockdep_assert_held(&zdev->state_lock). Will calling it here without
holding zdev->state_lock lead to a lockdep failure or state corruption
if accessed concurrently by sysfs or event handlers?


[Severity: High]
This is also a pre-existing issue not introduced by the patch under
review, but there might be a race condition exposed here as well.

In arch/s390/pci/pci_event.c:zpci_event_avail_new_device():

	if (zpci_add_device(zdev)) {
		kfree(zdev);
		break;
	}
	zpci_scan_configured_device(zdev, ccdf->fh);

Once zpci_add_device() publishes the device globally, can concurrent
threads acquire zdev->state_lock and deconfigure the device while
zpci_scan_configured_device() is still running unlocked?

>  obj-$(CONFIG_PCI)	+= pci.o pci_irq.o pci_clp.o \
>  			   pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
>  			   pci_bus.o pci_kvm_hook.o pci_report.o pci_fixup.o

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805133627.1216607-1-hca@linux.ibm.com?part=3

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() to remove conditional locking
  2026-08-05 13:36 ` [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() " Heiko Carstens
  2026-08-05 14:01   ` Niklas Schnelle
@ 2026-08-05 14:23   ` sashiko-bot
  1 sibling, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-05 14:23 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Alexander Gordeev, Vasily Gorbik, linux-s390, Heiko Carstens

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] zpci_remove_reserved_devices() leaves zdev linked to a stack-allocated list, leading to Use-After-Scope stack memory corruption.
- [High] zpci_remove_reserved_devices() modifies device state without holding state_lock, causing data races and lockdep panics.
--

--- Patch [2]: [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() to remove conditional locking ---
commit 75e2f03936eebee8914ba9e93f94ec0e2bac2ac9
Author: Heiko Carstens <hca@linux.ibm.com>

s390/pci: Rework __zpci_event_availability() to remove conditional locking
    
This patch reworks __zpci_event_availability() to avoid taking locks
conditionally, resolving a compiler warning from static context analysis.
The event handling is split into smaller, dedicated functions.

> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index bead4ed5d4abe..0a9eecb62bd1a 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
[ ... ]
> -static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
> +static bool zpci_event_avail_any_device(struct zpci_ccdf_avail *ccdf)
>  {
> -	struct zpci_dev *zdev = get_zdev_by_fid(ccdf->fid);
> -	bool existing_zdev = !!zdev;
> -	enum zpci_state state;
> -
> -	zpci_dbg(3, "avl fid:%x, fh:%x, pec:%x\n",
> -		 ccdf->fid, ccdf->fh, ccdf->pec);
> +	/* 0x0306 - No handle or fid stored */
> +	if (ccdf->pec != 0x0306)
> +		return false;
> +	/* 0x308 or 0x302 for multiple devices */
> +	zpci_remove_reserved_devices();

[Severity: High]
This is a pre-existing issue, but does zpci_remove_reserved_devices() call
zpci_device_reserved() without holding the state_lock?

Looking at zpci_remove_reserved_devices() in arch/s390/pci/pci.c:

void zpci_remove_reserved_devices(void)
{
        ...
        list_for_each_entry_safe(zdev, tmp, &remove, entry)
                zpci_device_reserved(zdev);
}

And zpci_device_reserved() asserts this lock:

void zpci_device_reserved(struct zpci_dev *zdev)
{
        lockdep_assert_held(&zdev->state_lock);
        ...

Will this result in a lockdep warning and potential data races on the
device state during a 0x0306 PCI event?


[Severity: Critical]
This is also a pre-existing issue, but can this lead to memory corruption
if the device reference count doesn't drop to zero?

In arch/s390/pci/pci.c:zpci_remove_reserved_devices():

void zpci_remove_reserved_devices(void)
{
        ...
        LIST_HEAD(remove);

        spin_lock(&zpci_list_lock);
        list_for_each_entry_safe(zdev, tmp, &zpci_list, entry) {
                if (...)
                        list_move_tail(&zdev->entry, &remove);
        }
        ...
}

The devices are moved to a local stack-allocated list (remove).

When zpci_device_reserved(zdev) is called, it drops one reference. If
another thread currently holds a reference (for example, via
get_zdev_by_fid()), the device is not freed immediately. When
zpci_remove_reserved_devices() returns, the stack frame is destroyed, but
the device's entry remains linked to this destroyed stack memory.

Later, when the final reference is dropped, zpci_release_device() will call
list_del(&zdev->entry), writing to the invalid stack memory. Is there a
safe way to handle this list manipulation?

> +	zpci_scan_devices();
> +	return true;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805133627.1216607-1-hca@linux.ibm.com?part=2

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/3] s390/pci: Enable CONTEXT_ANALYSIS
  2026-08-05 13:36 [PATCH v3 0/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
                   ` (2 preceding siblings ...)
  2026-08-05 13:36 ` [PATCH v3 3/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
@ 2026-08-07 10:48 ` Heiko Carstens
  3 siblings, 0 replies; 9+ messages in thread
From: Heiko Carstens @ 2026-08-07 10:48 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Alexander Gordeev, Sven Schnelle, Vasily Gorbik,
	Christian Borntraeger, Niklas Schnelle, Gerd Bayer, linux-s390,
	linux-kernel

On Wed, Aug 05, 2026 at 03:36:24PM +0200, Heiko Carstens wrote:
> Enable CONTEXT_ANALYSYS for s390's pci code.
...
> Heiko Carstens (3):
>   s390/pci: Rework __zpci_event_error() to remove conditional locking
>   s390/pci: Rework __zpci_event_availability() to remove conditional locking
>   s390/pci: Enable CONTEXT_ANALYSIS
> 
>  arch/s390/pci/Makefile    |   2 +
>  arch/s390/pci/pci_event.c | 205 ++++++++++++++++++++------------------
>  2 files changed, 111 insertions(+), 96 deletions(-)

Applied.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-08-07 10:48 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-05 13:36 [PATCH v3 0/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
2026-08-05 13:36 ` [PATCH v3 1/3] s390/pci: Rework __zpci_event_error() to remove conditional locking Heiko Carstens
2026-08-05 14:09   ` sashiko-bot
2026-08-05 13:36 ` [PATCH v3 2/3] s390/pci: Rework __zpci_event_availability() " Heiko Carstens
2026-08-05 14:01   ` Niklas Schnelle
2026-08-05 14:23   ` sashiko-bot
2026-08-05 13:36 ` [PATCH v3 3/3] s390/pci: Enable CONTEXT_ANALYSIS Heiko Carstens
2026-08-05 14:12   ` sashiko-bot
2026-08-07 10:48 ` [PATCH v3 0/3] " Heiko Carstens

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox