linux-kernel - Re: [PATCH] genirq/irqdomain: Clean code for __irq_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]

Date: Mon, 06 May 2024 20:01:26 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Jinjie Ruan <ruanjinjie@...wei.com>, linux-kernel@...r.kernel.org
Cc: ruanjinjie@...wei.com
Subject: Re: [PATCH] genirq/irqdomain: Clean code for __irq_domain_create()

On Tue, Apr 30 2024 at 17:33, Jinjie Ruan wrote:
> Introduce irq_domain_alloc_name() function to handle name allocation for
> the irq domain, add "out_free_domain" label to free the irq domain, and
> when "is_fwnode_irqchip(fwnode)" is true, "domain->fwnode = fwnode" is
> the common action, so do it outside the switch, which can make the code
> more clearer.

First of all changelogs should describe the reason for the change and
not enumerate a list of things which the patch does. The latter can be
seen from the patch itself.

> +static int irq_domain_alloc_name(struct fwnode_handle *fwnode,
> +				 struct irq_domain *domain,
> +				 int unknown_domains, char *in_name)
> +{
> +	char *name;
> +
> +	if (fwnode == NULL) {
> +		if (unknown_domains)
> +			domain->name = kasprintf(GFP_KERNEL, "unknown-%d",
> +						 unknown_domains);
> +		else
> +			domain->name = kstrdup(in_name, GFP_KERNEL);
> +		if (!domain->name)
> +			return -ENOMEM;
> +		goto out;
> +	}
> +
> +	/*
> +	 * fwnode paths contain '/', which debugfs is legitimately
> +	 * unhappy about. Replace them with ':', which does
> +	 * the trick and is not as offensive as '\'...
> +	 */
> +	name = kasprintf(GFP_KERNEL, "%pfw", fwnode);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	domain->name = strreplace(name, '/', ':');
> +
> +out:
> +	domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
> +	return 0;

This function is horrible. The only shared thing here is the
domain->flags manipulation. I'm not seeing how this is an improvement.

Thanks,

        tglx