[v5,06/11] devm-helpers: Add resource managed version of irq_create_mapping()

Add resource managed version of irq_create_mapping(), to help drivers
automatically dispose a linux irq mapping when driver is detached.

The new function devm_irq_create_mapping() is not yet used, but the
action function can be used in the FSL CAAM driver.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/crypto/caam/jr.c     |  8 ++----
 include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 6 deletions(-)

On 3/23/24 18:43, Marek Behún wrote:
> Add resource managed version of irq_create_mapping(), to help drivers
> automatically dispose a linux irq mapping when driver is detached.
> 
> The new function devm_irq_create_mapping() is not yet used, but the
> action function can be used in the FSL CAAM driver.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>   drivers/crypto/caam/jr.c     |  8 ++----
>   include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 56 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index 26eba7de3fb0..ad0295b055f8 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -7,6 +7,7 @@
>    * Copyright 2019, 2023 NXP
>    */
>   
> +#include <linux/devm-helpers.h>
>   #include <linux/of_irq.h>
>   #include <linux/of_address.h>
>   #include <linux/platform_device.h>
> @@ -576,11 +577,6 @@ static int caam_jr_init(struct device *dev)
>   	return error;
>   }
>   
> -static void caam_jr_irq_dispose_mapping(void *data)
> -{
> -	irq_dispose_mapping((unsigned long)data);
> -}
> -
>   /*
>    * Probe routine for each detected JobR subsystem.
>    */
> @@ -656,7 +652,7 @@ static int caam_jr_probe(struct platform_device *pdev)
>   		return -EINVAL;
>   	}
>   
> -	error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
> +	error = devm_add_action_or_reset(jrdev, devm_irq_mapping_drop,
>   					 (void *)(unsigned long)jrpriv->irq);
>   	if (error)
>   		return error;
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..3805551fd433 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,8 @@
>    */
>   
>   #include <linux/device.h>
> +#include <linux/kconfig.h>
> +#include <linux/irqdomain.h>
>   #include <linux/workqueue.h>

My confidence level is not terribly high today, so I am likely to accept 
just about any counter arguments :) But ... More I think of this whole 
header, less convinced I am that this (the header) is a great idea. I 
wonder who has authored a concept like this... :rolleyes:

Pulling punch of unrelated APIs (or, unrelated except the devm-usage) in 
one header has potential to be including a lot of unneeded stuff to the 
users. I am under impression this can be bad for example for the build 
times.

I think that ideally the devm-APIs should live close to their non-devm 
counterparts, and this header should be just used as a last resort, when 
all the other options fail :) May I assume all other options have failed 
for the IRQ stuff?

Well, I will leave the big picture to the bigger minds. When just 
looking at the important things like the function names and coding style 
- this change looks Ok to me ;)

>   static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +78,56 @@ static inline int devm_work_autocancel(struct device *dev,
>   	return devm_add_action(dev, devm_work_drop, w);
>   }
>   
> +/**
> + * devm_irq_mapping_drop - devm action for disposing an irq mapping
> + * @res:	linux irq number cast to the void * type
> + *
> + * devm_irq_mapping_drop() can be used as an action parameter for the
> + * devm_add_action_or_reset() function in order to automatically dispose
> + * a linux irq mapping when a device driver is detached.
> + */
> +static inline void devm_irq_mapping_drop(void *res)
> +{
> +	irq_dispose_mapping((unsigned int)(unsigned long)res);
> +}
> +
> +/**
> + * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
> + * @dev:	Device which lifetime the mapping is bound to
> + * @domain:	domain owning this hardware interrupt or NULL for default domain
> + * @hwirq:	hardware irq number in that domain space
> + *
> + * Create an irq mapping to linux irq space which is automatically disposed when
> + * the driver is detached.
> + * devm_irq_create_mapping() can be used to omit the explicit
> + * irq_dispose_mapping() call when driver is detached.
> + *
> + * Returns a linux irq number on success, 0 if mapping could not be created, or
> + * a negative error number if devm action could not be added.
> + */
> +static inline int devm_irq_create_mapping(struct device *dev,
> +					  struct irq_domain *domain,
> +					  irq_hw_number_t hwirq)
> +{
> +	unsigned int virq = irq_create_mapping(domain, hwirq);
> +
> +	if (!virq)
> +		return 0;
> +
> +	/*
> +	 * irq_dispose_mapping() is an empty function if CONFIG_IRQ_DOMAIN is
> +	 * disabled. No need to register an action in that case.
> +	 */
> +	if (IS_ENABLED(CONFIG_IRQ_DOMAIN)) {
> +		int err;
> +
> +		err = devm_add_action_or_reset(dev, devm_irq_mapping_drop,
> +					       (void *)(unsigned long)virq);
> +		if (err)
> +			return err;
> +	}
> +
> +	return virq;
> +}
> +
>   #endif

On Mon, 25 Mar 2024 11:40:20 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 3/23/24 18:43, Marek Behún wrote:
> > Add resource managed version of irq_create_mapping(), to help drivers
> > automatically dispose a linux irq mapping when driver is detached.
> > 
> > The new function devm_irq_create_mapping() is not yet used, but the
> > action function can be used in the FSL CAAM driver.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >   drivers/crypto/caam/jr.c     |  8 ++----
> >   include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
> >   2 files changed, 56 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> > index 26eba7de3fb0..ad0295b055f8 100644
> > --- a/drivers/crypto/caam/jr.c
> > +++ b/drivers/crypto/caam/jr.c
> > @@ -7,6 +7,7 @@
> >    * Copyright 2019, 2023 NXP
> >    */
> >   
> > +#include <linux/devm-helpers.h>
> >   #include <linux/of_irq.h>
> >   #include <linux/of_address.h>
> >   #include <linux/platform_device.h>
> > @@ -576,11 +577,6 @@ static int caam_jr_init(struct device *dev)
> >   	return error;
> >   }
> >   
> > -static void caam_jr_irq_dispose_mapping(void *data)
> > -{
> > -	irq_dispose_mapping((unsigned long)data);
> > -}
> > -
> >   /*
> >    * Probe routine for each detected JobR subsystem.
> >    */
> > @@ -656,7 +652,7 @@ static int caam_jr_probe(struct platform_device *pdev)
> >   		return -EINVAL;
> >   	}
> >   
> > -	error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
> > +	error = devm_add_action_or_reset(jrdev, devm_irq_mapping_drop,
> >   					 (void *)(unsigned long)jrpriv->irq);
> >   	if (error)
> >   		return error;
> > diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> > index 74891802200d..3805551fd433 100644
> > --- a/include/linux/devm-helpers.h
> > +++ b/include/linux/devm-helpers.h
> > @@ -24,6 +24,8 @@
> >    */
> >   
> >   #include <linux/device.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/irqdomain.h>
> >   #include <linux/workqueue.h>  
> 
> My confidence level is not terribly high today, so I am likely to accept 
> just about any counter arguments :) But ... More I think of this whole 
> header, less convinced I am that this (the header) is a great idea. I 
> wonder who has authored a concept like this... :rolleyes:
> 
> Pulling punch of unrelated APIs (or, unrelated except the devm-usage) in 
> one header has potential to be including a lot of unneeded stuff to the 
> users. I am under impression this can be bad for example for the build 
> times.
> 
> I think that ideally the devm-APIs should live close to their non-devm 
> counterparts, and this header should be just used as a last resort, when 
> all the other options fail :) May I assume all other options have failed 
> for the IRQ stuff?
> 
> Well, I will leave the big picture to the bigger minds. When just 
> looking at the important things like the function names and coding style 
> - this change looks Ok to me ;)

If the authors of devm-helpers or someone else decide it should not
exist (due for example of long build times), I am OK with that.

But currently this seems to me to be the proper place to put this into.

Marek

On Tue, 26 Mar 2024 12:00:25 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Sat, Mar 23, 2024 at 05:43:54PM +0100, Marek Behún wrote:
> > +/**
> > + * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
> > + * @dev:	Device which lifetime the mapping is bound to
> > + * @domain:	domain owning this hardware interrupt or NULL for default domain
> > + * @hwirq:	hardware irq number in that domain space
> > + *
> > + * Create an irq mapping to linux irq space which is automatically disposed when
> > + * the driver is detached.
> > + * devm_irq_create_mapping() can be used to omit the explicit
> > + * irq_dispose_mapping() call when driver is detached.
> > + *
> > + * Returns a linux irq number on success, 0 if mapping could not be created, or  
>                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> > + * a negative error number if devm action could not be added.
> > + */
> > +static inline int devm_irq_create_mapping(struct device *dev,
> > +					  struct irq_domain *domain,
> > +					  irq_hw_number_t hwirq)
> > +{
> > +	unsigned int virq = irq_create_mapping(domain, hwirq);
> > +
> > +	if (!virq)
> > +		return 0;  
> 
> What is the point of returning zero instead of an error code?  Neither
> of the callers that are introduced later in the patchset use this.
> 
> I understand that it matches some of the other legacy irq function
> behaviors, but I think we are trying to move away from that because it
> just leads to bugs.
> 
> Since we don't need the zero now, let's wait until we have a user before
> introducing this behavior.  Then we can add a new function that returns
> zero, but we'll still encourage people to use the standard error code
> function where possible.  And at the same time, when we do introduce the
> zero is an error code, function you should contact
> kernel-janitors@vger.kernel.org so someone an write a static checker
> rule to detect the bugs that result from it.

Hi Dan,

the first user of this function is the very next patch of this series,
and it does this:

+	irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
+	if (irq <= 0)
+		return dev_err_probe(dev, irq ?: -ENXIO,
+				     "Cannot map MESSAGE_SIGNED IRQ\n");

So it handles !irq as -ENXIO.

I looked into several users who do
  virq = irq_create_mapping()
and then reutrn errno if !virq:

  git grep -A 3 'virq = irq_create_mapping'

Some return -ENOMEM, some -ENXIO, some -EINVAL.

What do you think?

Or should I send this driver without introducing this helper for now?

Marek