Re: [PATCH] genirq/irqdomain: Clean code for __irq_domain_create()

From: Jinjie Ruan
Date: Mon May 06 2024 - 21:22:49 EST




On 2024/5/7 2:01, Thomas Gleixner wrote:
> 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.

Thanks for the good advice,subsequent patches will learn from the
experience.

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

Yes, the IRQ_DOMAIN_NAME_ALLOCATED flag is shared, the purpose of this
change is to make the code that allocates the domain name in the
__irq_domain_create() function look like it doesn't take up too much
space, so it looks cleaner.

>
> Thanks,
>
> tglx