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.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 kernel/irq/irqdomain.c | 75 +++++++++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 3dd1c871e091..49a983161340 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -127,6 +127,39 @@ void irq_domain_free_fwnode(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(irq_domain_free_fwnode);
 
+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;
+}
+
 static struct irq_domain *__irq_domain_create(struct fwnode_handle *fwnode,
 					      unsigned int size,
 					      irq_hw_number_t hwirq_max,
@@ -151,53 +184,31 @@ static struct irq_domain *__irq_domain_create(struct fwnode_handle *fwnode,
 
 	if (is_fwnode_irqchip(fwnode)) {
 		fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
+		domain->fwnode = fwnode;
 
 		switch (fwid->type) {
 		case IRQCHIP_FWNODE_NAMED:
 		case IRQCHIP_FWNODE_NAMED_ID:
-			domain->fwnode = fwnode;
-			domain->name = kstrdup(fwid->name, GFP_KERNEL);
-			if (!domain->name) {
-				kfree(domain);
-				return NULL;
-			}
-			domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+			if (irq_domain_alloc_name(NULL, domain, 0, fwid->name))
+				goto out_free_domain;
 			break;
 		default:
-			domain->fwnode = fwnode;
 			domain->name = fwid->name;
 			break;
 		}
 	} else if (is_of_node(fwnode) || is_acpi_device_node(fwnode) ||
 		   is_software_node(fwnode)) {
-		char *name;
-
-		/*
-		 * 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) {
-			kfree(domain);
-			return NULL;
-		}
-
-		domain->name = strreplace(name, '/', ':');
+		if (irq_domain_alloc_name(fwnode, domain, 0, NULL))
+			goto out_free_domain;
 		domain->fwnode = fwnode;
-		domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
 	}
 
 	if (!domain->name) {
 		if (fwnode)
 			pr_err("Invalid fwnode type for irqdomain\n");
-		domain->name = kasprintf(GFP_KERNEL, "unknown-%d",
-					 atomic_inc_return(&unknown_domains));
-		if (!domain->name) {
-			kfree(domain);
-			return NULL;
-		}
-		domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+		if (irq_domain_alloc_name(NULL, domain,
+		    atomic_inc_return(&unknown_domains), NULL))
+			goto out_free_domain;
 	}
 
 	fwnode_handle_get(fwnode);
@@ -228,6 +239,10 @@ static struct irq_domain *__irq_domain_create(struct fwnode_handle *fwnode,
 	irq_domain_check_hierarchy(domain);
 
 	return domain;
+
+out_free_domain:
+	kfree(domain);
+	return NULL;
 }
 
 static void __irq_domain_publish(struct irq_domain *domain)
-- 
2.34.1

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

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

On Tue, May 07 2024 at 09:22, Jinjie Ruan wrote:
> On 2024/5/7 2:01, Thomas Gleixner wrote:
>>> +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.

I'm failing to see the cleaner. All this does is shuffling code around
for no reason.

Thanks,

        tglx