Re: [PATCH v2 1/3] s390/pci: Rework __zpci_event_error() to remove conditional locking

On Mon, 2026-08-03 at 16:29 +0200, Heiko Carstens wrote:
> Clang's compiler based static context analysis does not work with
> locks that are conditionally taken like in __zpci_event_error():
>=20
> arch/s390/pci/pci_event.c:320:2: warning: mutex 'get_zdev_by_fid(ccdf->fi=
d).state_lock'
>  is not held on every path through here [-Wthread-safety-analysis]
>=20
> Given that code which takes locks conditionally can be considered
> suboptimal rework __zpci_event_error() to get rid of this.
>=20
> Signed-off-by: Heiko Carstens <[email protected]>
> ---
>  arch/s390/pci/pci_event.c | 43 ++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
>=20
> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index 839bd91c056e..48fa26dcbee1 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 *pd=
ev, pci_channel_state_t es)
>  	pci_dev_unlock(pdev);
>  }
> =20
> +static void __zpci_event_print_error(struct pci_dev *pdev, struct zpci_c=
cdf_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 =3D 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));
> =20
> -	if (zdev) {
> -		mutex_lock(&zdev->state_lock);
> -		rc =3D clp_refresh_fh(zdev->fid, &fh);
> -		if (rc)
> -			goto no_pdev;
> -		if (!fh || ccdf->fh !=3D 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 =3D pci_get_slot(zdev->zbus->bus, zdev->devfn);
> -	}
> +	if (!zdev)
> +		return __zpci_event_print_error(pdev, ccdf);

Nit: Here pdev is always NULL because the assignment moved below.
Functionally this is fine but it would be more clear this is the case
if you passed NULL for pdev explicitly.

> =20
> -	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 =3D clp_refresh_fh(zdev->fid, &fh);
> +	if (rc)
> +		goto no_pdev;
> +	if (!fh || ccdf->fh !=3D 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 =3D pci_get_slot(zdev->zbus->bus, zdev->devfn);
> +
> +	__zpci_event_print_error(pdev, ccdf);
> =20
>  	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);
>  }
> =20

Thanks for cleaning this up. Even on its own this would be a net
improvement. So with or without my nit resolved feel free to add:

Reviewed-by: Niklas Schnelle <[email protected]>

Thanks,
Niklas