linux-kernel - Re: [PATCH v2a 2/3] irqdomain: Allow giving name suffix for domain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3f77a70a-2bcb-4cb2-ac4f-a5d785f6f7ff@gmail.com>
Date: Fri, 9 Aug 2024 08:03:38 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>,
 Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: Mark Brown <broonie@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2a 2/3] irqdomain: Allow giving name suffix for domain

On 8/8/24 23:23, Thomas Gleixner wrote:
> 
> From: Matti Vaittinen <mazziesaccount@...il.com>
> 
> Devices can provide multiple interrupt lines. One reason for this is that
> a device has multiple subfunctions, each providing its own interrupt line.
> Another reason is that a device can be designed to be used (also) on a
> system where some of the interrupts can be routed to another processor.
> 
> A line often further acts as a demultiplex for specific interrupts
> and has it's respective set of interrupt (status, mask, ack, ...)
> registers.
> 
> Regmap supports the handling of these registers and demultiplexing
> interrupts, but the interrupt domain code ends up assigning the same name
> for the per interrupt line domains. This causes a naming collision in the
> debugFS code and leads to confusion, as /proc/interrupts shows two separate
> interrupts with the same domain name and hardware interrupt number.
> 
> Instead of adding a workaround in regmap or driver code, allow giving a
> name suffix for the domain name when the domain is created.
> 
> Add a name_suffix field in the irq_domain_info structure and make
> irq_domain_instantiate() use this suffix if it is given when a domain is
> created.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> Revision history:
> v2 => v2a:
>     Update to name allocation cleanup patch. Fix the invalid NULL return.
> v1 => v2:
>   - typofix in comment. 'collison' to 'collision'.
> ---
>   include/linux/irqdomain.h |    3 +++
>   kernel/irq/irqdomain.c    |   32 +++++++++++++++++++++++---------
>   2 files changed, 26 insertions(+), 9 deletions(-)
> 
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -295,6 +295,8 @@ struct irq_domain_chip_generic_info;
>    * @virq_base:		The first Linux interrupt number for legacy domains to
>    *			immediately associate the interrupts after domain creation
>    * @bus_token:		Domain bus token
> + * @name_suffix:	Optional name suffix to avoid collisions when multiple
> + *			domains are added using same fwnode
>    * @ops:		Domain operation callbacks
>    * @host_data:		Controller private data pointer
>    * @dgc_info:		Geneneric chip information structure pointer used to
> @@ -313,6 +315,7 @@ struct irq_domain_info {
>   	unsigned int				hwirq_base;
>   	unsigned int				virq_base;
>   	enum irq_domain_bus_token		bus_token;
> +	const char				*name_suffix;
>   	const struct irq_domain_ops		*ops;
>   	void					*host_data;
>   #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -140,11 +140,14 @@ static int alloc_name(struct irq_domain
>   }
>   
>   static int alloc_fwnode_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
> -			     enum irq_domain_bus_token bus_token)
> +			     enum irq_domain_bus_token bus_token, const char *suffix)
>   {
> -	char *name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
> -				 kasprintf(GFP_KERNEL, "%pfw", fwnode);
> +	const char *sep = suffix ? "-" : "";
> +	const char *suf = suffix ? : "";
> +	char *name;
>   
> +	name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%s%s%d", fwnode, suf, sep, bus_token) :
> +			   kasprintf(GFP_KERNEL, "%pfw-%s", fwnode, suf);
>   	if (!name)
>   		return -ENOMEM;
>   

Thanks Thomas!
This looks much, much cleaner to me compared to my original version :)

> @@ -172,13 +175,24 @@ static int alloc_unknown_name(struct irq
>   	return 0;
>   }
>

Yours,
	-- Matti

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

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