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 <[email protected]>
---
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