Re: [PATCH v2 2/3] s390/pci: Rework__zpci_event_availability() to remove conditional locking

[Niklas Schnelle]

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_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@xxxxxxxxxxxxx>
> ---
>  arch/s390/pci/pci_event.c | 108 +++++++++++++++++++-------------------
>  1 file changed, 53 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index 48fa26dcbee1..f96ee87405f9 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
> @@ -389,19 +389,25 @@ static void zpci_event_reappear(struct zpci_dev *zdev)
>  
>  static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
>  {
> -	struct zpci_dev *zdev = get_zdev_by_fid(ccdf->fid);
> -	bool existing_zdev = !!zdev;
> +	struct zpci_dev *zdev;
>  	enum zpci_state state;
>  
>  	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);
> +	/* 0x0306 - No handle or fid stored */
> +	if (ccdf->pec == 0x0306) {
> +		/* 0x308 or 0x302 for multiple devices */
> +		zpci_remove_reserved_devices();
> +		zpci_scan_devices();
> +		return;
> +	}
>  
> -	switch (ccdf->pec) {
> -	case 0x0301: /* Reserved|Standby -> Configured */
> -		if (!zdev) {
> +	zdev = get_zdev_by_fid(ccdf->fid);
> +

Nit: Stray empty line

> +	if (!zdev) {
> +		switch (ccdf->pec) {
> +		case 0x0301: /* Reserved|Standby -> Configured */
>  			zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_CONFIGURED);
>  			if (IS_ERR(zdev))
>  				break;
--- snip ---
> +			break;
>  		}
> +		return;
> +	}
--- snip ---
> +
> +	mutex_lock(&zdev->state_lock);
> +	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);
--- snip ---
>  	}
> +	mutex_unlock(&zdev->state_lock);
> +	zpci_zdev_put(zdev);
>  }
>  
>  void zpci_event_availability(void *data)

Personally I think I'd put the 0x0306 and each of the two switches in
helper functions to improve readability as this is getting awfully
long. Maybe something like zpci_event_avail_any_device() (0x0306),
zpci_event_avail_new_device() and zpci_event_avail_existing_device().

If you prefer and since I'm doing a follow up for the missing locking
in zpci_reserved_devices() I can also do that in a separate patch. Also
just to clarify Sashiko is right in that the missing locking is a pre-
existing issue as the state_lock was already not taken in that case,
it's just more obvious now.

Either way, functionality looks good to me so feel free to add:

Reviewed-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>

Thanks,
Niklas