Re: [PATCH 4/4] s390/pci: Use lock guard for pci_rescan_remove_lock

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 6 Mar 2026, Benjamin Block wrote:

> There are quite a few places in the s390 architecture code for the PCI
> subsystem where the kernel needs to lock `pci_rescan_remove_lock` now;
> which is done by calling pci_lock_rescan_remove() to lock, and
> pci_unlock_rescan_remove() to unlock the mutex.
> 
> Instead of always manually calling both functions, which induces a
> certain amount of visual clutter, and in some cases of errors, cleanup,
> and jumplabels more complexity, use either guard() or scoped_guard()
> depending on the context.
> 
> Convert all users in the s390 architecture code for PCI.
> 
> Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
> ---
>  arch/s390/pci/pci.c       |  8 +++----
>  arch/s390/pci/pci_bus.c   |  3 +--
>  arch/s390/pci/pci_event.c | 45 +++++++++++++++++----------------------
>  arch/s390/pci/pci_iov.c   |  3 +--
>  arch/s390/pci/pci_sysfs.c |  9 +++-----
>  5 files changed, 27 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index fd16e6ad21c1..86ef1e516857 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -632,10 +632,9 @@ void pcibios_release_device(struct pci_dev *pdev)
>  {
>  	struct zpci_dev *zdev = to_zpci(pdev);
>  
> -	pci_lock_rescan_remove();
> +	guard(pci_rescan_remove)();
>  	zpci_unmap_resources(pdev);
>  	zpci_zdev_put(zdev);
> -	pci_unlock_rescan_remove();
>  }
>  
>  int pcibios_enable_device(struct pci_dev *pdev, int mask)
> @@ -1213,9 +1212,8 @@ static int __init pci_base_init(void)
>  	if (rc)
>  		goto out_irq;
>  
> -	pci_lock_rescan_remove();
> -	rc = zpci_scan_devices();
> -	pci_unlock_rescan_remove();
> +	scoped_guard(pci_rescan_remove)
> +		rc = zpci_scan_devices();
>  	if (rc)
>  		goto out_find;
>  
> diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
> index 2b598222c621..c1b48b572e86 100644
> --- a/arch/s390/pci/pci_bus.c
> +++ b/arch/s390/pci/pci_bus.c
> @@ -82,9 +82,8 @@ int zpci_bus_scan_device(struct zpci_dev *zdev)
>  	if (!pdev)
>  		return -ENODEV;
>  
> -	pci_lock_rescan_remove();
> +	guard(pci_rescan_remove)();
>  	pci_bus_add_device(pdev);
> -	pci_unlock_rescan_remove();
>  
>  	return 0;
>  }
> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index edfaeed737ac..98253706b591 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
> @@ -342,9 +342,8 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
>  no_pdev:
>  	if (zdev)
>  		mutex_unlock(&zdev->state_lock);
> -	pci_lock_rescan_remove();
> +	guard(pci_rescan_remove)();
>  	zpci_zdev_put(zdev);
> -	pci_unlock_rescan_remove();
>  }
>  
>  void zpci_event_error(void *data)
> @@ -389,7 +388,6 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
>  	struct zpci_dev *zdev = get_zdev_by_fid(ccdf->fid);
>  	bool existing_zdev = !!zdev;
>  	enum zpci_state state;
> -	int rc;
>  
>  	zpci_dbg(3, "avl fid:%x, fh:%x, pec:%x\n",
>  		 ccdf->fid, ccdf->fh, ccdf->pec);
> @@ -403,12 +401,11 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
>  			zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_CONFIGURED);
>  			if (IS_ERR(zdev))
>  				break;
> -			pci_lock_rescan_remove();
> -			rc = zpci_add_device(zdev);
> -			pci_unlock_rescan_remove();
> -			if (rc) {
> -				kfree(zdev);
> -				break;
> +			scoped_guard(pci_rescan_remove) {
> +				if (zpci_add_device(zdev)) {
> +					kfree(zdev);
> +					break;
> +				}
>  			}
>  		} else {
>  			if (zdev->state == ZPCI_FN_STATE_RESERVED)
> @@ -425,12 +422,11 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
>  			zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_STANDBY);
>  			if (IS_ERR(zdev))
>  				break;
> -			pci_lock_rescan_remove();
> -			rc = zpci_add_device(zdev);
> -			pci_unlock_rescan_remove();
> -			if (rc) {
> -				kfree(zdev);
> -				break;
> +			scoped_guard(pci_rescan_remove) {
> +				if (zpci_add_device(zdev)) {
> +					kfree(zdev);
> +					break;
> +				}
>  			}
>  		} else {
>  			if (zdev->state == ZPCI_FN_STATE_RESERVED)
> @@ -459,33 +455,30 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
>  			/* The 0x0304 event may immediately reserve the device */
>  			if (!clp_get_state(zdev->fid, &state) &&
>  			    state == ZPCI_FN_STATE_RESERVED) {
> -				pci_lock_rescan_remove();
> +				guard(pci_rescan_remove)();
>  				zpci_device_reserved(zdev);
> -				pci_unlock_rescan_remove();
>  			}
>  		}
>  		break;
>  	case 0x0306: /* 0x308 or 0x302 for multiple devices */
> -		pci_lock_rescan_remove();
> -		zpci_remove_reserved_devices();
> -		zpci_scan_devices();
> -		pci_unlock_rescan_remove();
> +		scoped_guard(pci_rescan_remove) {
> +			zpci_remove_reserved_devices();
> +			zpci_scan_devices();
> +		}
>  		break;
>  	case 0x0308: /* Standby -> Reserved */
>  		if (!zdev)
>  			break;
> -		pci_lock_rescan_remove();
> -		zpci_device_reserved(zdev);
> -		pci_unlock_rescan_remove();
> +		scoped_guard(pci_rescan_remove)
> +			zpci_device_reserved(zdev);

Order in this series is weird. Why not introduce *guard() support before 
the fix (reorder patches 2 and 3) and then use guard direct here so you 
don't have to immediately change the code again to "convert" it to use 
*guard() in patch 4?

-- 
 i.

>  		break;
>  	default:
>  		break;
>  	}
>  	if (existing_zdev) {
>  		mutex_unlock(&zdev->state_lock);
> -		pci_lock_rescan_remove();
> +		guard(pci_rescan_remove)();
>  		zpci_zdev_put(zdev);
> -		pci_unlock_rescan_remove();
>  	}
>  }
>  
> diff --git a/arch/s390/pci/pci_iov.c b/arch/s390/pci/pci_iov.c
> index 13050ce5c3e9..1f7e4dd018e7 100644
> --- a/arch/s390/pci/pci_iov.c
> +++ b/arch/s390/pci/pci_iov.c
> @@ -38,10 +38,9 @@ void zpci_iov_map_resources(struct pci_dev *pdev)
>  
>  void zpci_iov_remove_virtfn(struct pci_dev *pdev, int vfn)
>  {
> -	pci_lock_rescan_remove();
> +	guard(pci_rescan_remove)();
>  	/* Linux' vfid's start at 0 vfn at 1 */
>  	pci_iov_remove_virtfn(pdev->physfn, vfn - 1);
> -	pci_unlock_rescan_remove();
>  }
>  
>  static int zpci_iov_link_virtfn(struct pci_dev *pdev, struct pci_dev *virtfn, int vfid)
> diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c
> index c2444a23e26c..f5027aa95928 100644
> --- a/arch/s390/pci/pci_sysfs.c
> +++ b/arch/s390/pci/pci_sysfs.c
> @@ -98,9 +98,9 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
>  	WARN_ON_ONCE(!kn);
>  
>  	/* Device needs to be configured and state must not change */
> -	mutex_lock(&zdev->state_lock);
> +	guard(mutex)(&zdev->state_lock);
>  	if (zdev->state != ZPCI_FN_STATE_CONFIGURED)
> -		goto out;
> +		return count;
>  
>  	/* device_remove_file() serializes concurrent calls ignoring all but
>  	 * the first
> @@ -112,15 +112,12 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
>  	 * Once it unblocks from pci_lock_rescan_remove() the original pdev
>  	 * will already be removed.
>  	 */
> -	pci_lock_rescan_remove();
> +	guard(pci_rescan_remove)();
>  	if (pci_dev_is_added(pdev)) {
>  		ret = _do_recover(pdev, zdev);
>  	}
>  	pci_rescan_bus(zdev->zbus->bus);
> -	pci_unlock_rescan_remove();
>  
> -out:
> -	mutex_unlock(&zdev->state_lock);
>  	if (kn)
>  		sysfs_unbreak_active_protection(kn);
>  	return ret ? ret : count;
>