Received-SPF: pass (google.com: domain of linux-crypto+bounces-2849-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <dd4c655f-39c4-47c1-b5fb-4d6fc94cc430@gmail.com>
Date: Mon, 25 Mar 2024 11:40:20 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 06/11] devm-helpers: Add resource managed version of
 irq_create_mapping()
Content-Language: en-US, en-GB
To: =?UTF-8?Q?Marek_Beh=C3=BAn?= <kabel@kernel.org>,
 Arnd Bergmann <arnd@arndb.de>, Gregory CLEMENT
 <gregory.clement@bootlin.com>, soc@kernel.org, arm@kernel.org,
 Hans de Goede <hdegoede@redhat.com>, =?UTF-8?Q?Horia_Geant=C4=83?=
 <horia.geanta@nxp.com>, Pankaj Gupta <pankaj.gupta@nxp.com>,
 Gaurav Jain <gaurav.jain@nxp.com>, linux-crypto@vger.kernel.org,
 Herbert Xu <herbert@gondor.apana.org.au>
References: <20240323164359.21642-1-kabel@kernel.org>
 <20240323164359.21642-7-kabel@kernel.org>
From: Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <20240323164359.21642-7-kabel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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