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

Matti Vaittinen
2024-03-25T09:40:25Z

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

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~