Received-SPF: pass (google.com: domain of linux-kernel+bounces-170542-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <887d424c-c6b5-e5c7-21d3-9b920272fbc3@huawei.com>
Date: Tue, 7 May 2024 09:22:35 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Subject: Re: [PATCH] genirq/irqdomain: Clean code for __irq_domain_create()
Content-Language: en-US
To: Thomas Gleixner <tglx@linutronix.de>, <linux-kernel@vger.kernel.org>
References: <20240430093353.1919862-1-ruanjinjie@huawei.com>
 <87pltyrf9l.ffs@tglx>
From: Jinjie Ruan <ruanjinjie@huawei.com>
In-Reply-To: <87pltyrf9l.ffs@tglx>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit


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