Re: [patch 02/10] genirq/msi: Use lock guards for MSI descriptor locking

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Sun,  9 Mar 2025 09:41:44 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> Provide a lock guard for MSI descriptor locking and update the core code
> accordingly.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
One case in here made me go back to the helpful docs Dan wrote
after early confusion on how to use this stuff.  We might
want to consider updating as you've found a case that I at least
didn't consider when reviewing those.  +CC Dan.

Anyhow, this seems fine to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


>  
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -443,7 +443,6 @@ EXPORT_SYMBOL_GPL(msi_next_desc);

>  
> @@ -1037,25 +1032,23 @@ bool msi_create_device_irq_domain(struct
>  	if (msi_setup_device_data(dev))

Hmm. We might want to make the docs in cleanup.h more nuanced.
They specifically say to not mix goto and auto cleanup, but 
in the case of scoped_guard() unlikely almost any other case
it should be fine.

>  		goto free_fwnode;
>  
> -	msi_lock_descs(dev);
> -
> -	if (WARN_ON_ONCE(msi_get_device_domain(dev, domid)))
> -		goto fail;
> -
> -	if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info))
> -		goto fail;
> -
> -	domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE, parent);
> -	if (!domain)
> -		goto fail;
> -
> -	domain->dev = dev;
> -	dev->msi.data->__domains[domid].domain = domain;
> -	msi_unlock_descs(dev);
> -	return true;
> +	scoped_guard(msi_descs_lock, dev) {
> +		if (WARN_ON_ONCE(msi_get_device_domain(dev, domid)))
> +			goto free_fwnode;
> +
> +		if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info))
> +			goto free_fwnode;
> +
> +		domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE,
> +						 parent);
> +		if (!domain)
> +			goto free_fwnode;
> +
> +		domain->dev = dev;
> +		dev->msi.data->__domains[domid].domain = domain;
> +		return true;
> +	}
>  
> -fail:
> -	msi_unlock_descs(dev);
>  free_fwnode:
>  	irq_domain_free_fwnode(fwnalloced);
>  free_bundle: