diff mbox series

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

Message ID 20240323164359.21642-7-kabel@kernel.org (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series Turris Omnia MCU driver | expand

Commit Message

Marek Behún March 23, 2024, 4:43 p.m. UTC
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(-)

Comments

Matti Vaittinen March 25, 2024, 9:40 a.m. UTC | #1
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
Marek Behún March 25, 2024, 9:57 a.m. UTC | #2
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
Dan Carpenter March 26, 2024, 9 a.m. UTC | #3
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.

regards,
dan carpenter

> +
> +	/*
> +	 * 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
> -- 
> 2.43.2
>
Marek Behún March 27, 2024, 9:34 a.m. UTC | #4
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
Dan Carpenter March 27, 2024, 11:39 a.m. UTC | #5
On Wed, Mar 27, 2024 at 10:34:19AM +0100, Marek Behún wrote:
> 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.

Yeah.  But imagine how much easier it would be if devm_irq_create_mapping()
returned -ENXIO directly.

	irq = devm_irq_create_mapping();
	if (irq < 0)
		return dev_err_probe(dev, irq,
				     "Cannot map MESSAGE_SIGNED IRQ\n");

> 
> 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?

-ENXIO is fine.

regards,
dan carpenter
diff mbox series

Patch

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>
 
 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