| Message ID | 20230803175916.3174453-12-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISC-V: ACPI: Add external interrupt controller support | expand |
On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote: > From: Anup Patel <apatel@ventanamicro.com> > > swnode framework can be used to create fwnode for interrupt > controllers. Why? What is this for? Can you elaborate? This commit message is poorly written... And why firmware node is not enough for ACPI case? I assume the fwnode in DT case is already provided by OF. > This helps in keeping the drivers same for both > DT and ACPI. To enable this, enhance the swnode framework so > that it can be created early during boot without dependency > on sysfs. ... > - swnode->kobj.kset = swnode_kset; > + swnode->kobj.kset = (!early) ? swnode_kset : NULL; Too many unneeded characters. Why parentheses? Why negative check? ... > + if (early) { > + ret = 0; > + kobject_init(&swnode->kobj, &software_node_type_early); > + swnode->kobj.parent = parent ? &parent->kobj : NULL; > + if (node->name) > + ret = kobject_set_name(&swnode->kobj, > + "%s", node->name); > + else > + ret = kobject_set_name(&swnode->kobj, > + "node%d", swnode->id); > + if (!ret) { > + spin_lock(&swnode_early_lock); > + list_add_tail(&swnode->early, &swnode_early_list); > + spin_unlock(&swnode_early_lock); > + } > + } else { > + if (node->name) > + ret = kobject_init_and_add(&swnode->kobj, &software_node_type, > + parent ? &parent->kobj : NULL, This looks like have a duplication. > + "%s", node->name); > + else > + ret = kobject_init_and_add(&swnode->kobj, &software_node_type, > + parent ? &parent->kobj : NULL, > + "node%d", swnode->id); > + } Maybe it's possible to refactor this piece to be more compact? ... > - return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0)); > + return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0, 0)); In one case you use boolean, here is unsigned int for early flag, why is the inconsistency added? ... > -struct fwnode_handle * > -fwnode_create_software_node(const struct property_entry *properties, > - const struct fwnode_handle *parent) > +static struct fwnode_handle * > +fwnode_create_software_node_common(const struct property_entry *properties, > + const struct fwnode_handle *parent, > + bool early) Why would you need this API in early stages?
Hi Andy, On Fri, Aug 04, 2023 at 09:09:16AM +0300, Andy Shevchenko wrote: > On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote: > > From: Anup Patel <apatel@ventanamicro.com> > > > > swnode framework can be used to create fwnode for interrupt > > controllers. > > Why? What is this for? > Can you elaborate? This commit message is poorly written... > > And why firmware node is not enough for ACPI case? > I assume the fwnode in DT case is already provided by OF. > Thanks a lot for the review!. You are right, OF provides the fwnode for irqchip drivers. However, for ACPI case, it is typically created using irq_domain_alloc_named_fwnode or irq_domain_alloc_fwnode since these are not ACPI devices in the namespace but from MADT. The fwnode created using irq_domain_alloc_fwnode() is a simple one which doesn't support properties similar to the one created by OF framework or software node framework. Hence, lot of data from the MADT structures need to be cached as separate structures in the drivers and also would need several ifdefs to check for ACPI and some amount of code duplication is also required due to the way DT driver gets the information vs ACPI. The beauty of software node framework is, it supports adding properties and also is a supported fwnode type in __irq_domain_create(). So, if we can create the fwnode for these irqchip using software node, we can attach the same properties and the actual irqchip driver which uses the fwnode doesn't need to have any ACPI vs DT checks. Same driver will work seamlessly on both DT and ACPI platforms. But the challenge is, currently swnode expects to be created with sysfs which won't be available during early boot when irqchip drivers need to be probed. So, adding support to create without dependency on sysfs help us to reuse the same framework for irqchip use case also. Apologies for not descriptive in the commit message. Please let us know your feedback on this approach. > > This helps in keeping the drivers same for both > > DT and ACPI. To enable this, enhance the swnode framework so > > that it can be created early during boot without dependency > > on sysfs. > > ... > > > - swnode->kobj.kset = swnode_kset; > > + swnode->kobj.kset = (!early) ? swnode_kset : NULL; > > Too many unneeded characters. Why parentheses? Why negative check? > Sure, will update in next version. > ... > > > + if (early) { > > + ret = 0; > > + kobject_init(&swnode->kobj, &software_node_type_early); > > + swnode->kobj.parent = parent ? &parent->kobj : NULL; > > + if (node->name) > > + ret = kobject_set_name(&swnode->kobj, > > + "%s", node->name); > > + else > > + ret = kobject_set_name(&swnode->kobj, > > + "node%d", swnode->id); > > + if (!ret) { > > + spin_lock(&swnode_early_lock); > > + list_add_tail(&swnode->early, &swnode_early_list); > > + spin_unlock(&swnode_early_lock); > > + } > > + } else { > > + if (node->name) > > + ret = kobject_init_and_add(&swnode->kobj, &software_node_type, > > + parent ? &parent->kobj : NULL, > > This looks like have a duplication. > > > + "%s", node->name); > > + else > > + ret = kobject_init_and_add(&swnode->kobj, &software_node_type, > > + parent ? &parent->kobj : NULL, > > + "node%d", swnode->id); > > + } > > Maybe it's possible to refactor this piece to be more compact? > The issue is, kobject_init_and_add() expects sysfs. Let me try to compact this in next version. Thanks! > ... > > > - return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0)); > > + return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0, 0)); > > In one case you use boolean, here is unsigned int for early flag, why is the > inconsistency added? > Yeah, it should be bool. Let me fix it in next version. > ... > > > -struct fwnode_handle * > > -fwnode_create_software_node(const struct property_entry *properties, > > - const struct fwnode_handle *parent) > > +static struct fwnode_handle * > > +fwnode_create_software_node_common(const struct property_entry *properties, > > + const struct fwnode_handle *parent, > > + bool early) > > Why would you need this API in early stages? > Hope I answered the question above. Thanks! Sunil
On Thu, 03 Aug 2023 18:59:06 +0100, Sunil V L <sunilvl@ventanamicro.com> wrote: > > From: Anup Patel <apatel@ventanamicro.com> > > swnode framework can be used to create fwnode for interrupt > controllers. This helps in keeping the drivers same for both > DT and ACPI. To enable this, enhance the swnode framework so > that it can be created early during boot without dependency > on sysfs. Where is this coming from? We have had common abstractions for irqchips for a very long time. We can create fwnodes any odd way we want, but why oh why should we invent another method for that? We already have multiple architectures such as arm64 and loongarch already having both DT and ACPI. Do they need another level of "abstraction"? No. Why is risc-v so special? M.
On Fri, 04 Aug 2023 09:11:05 +0100, Sunil V L <sunilvl@ventanamicro.com> wrote: > > Hi Andy, > > On Fri, Aug 04, 2023 at 09:09:16AM +0300, Andy Shevchenko wrote: > > On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote: > > > From: Anup Patel <apatel@ventanamicro.com> > > > > > > swnode framework can be used to create fwnode for interrupt > > > controllers. > > > > Why? What is this for? > > Can you elaborate? This commit message is poorly written... > > > > And why firmware node is not enough for ACPI case? > > I assume the fwnode in DT case is already provided by OF. > > > Thanks a lot for the review!. > > You are right, OF provides the fwnode for irqchip drivers. However, for > ACPI case, it is typically created using irq_domain_alloc_named_fwnode > or irq_domain_alloc_fwnode since these are not ACPI devices in the > namespace but from MADT. The fwnode created using > irq_domain_alloc_fwnode() is a simple one which doesn't support properties > similar to the one created by OF framework or software node framework. > Hence, lot of data from the MADT structures need to be cached as > separate structures in the drivers and also would need several ifdefs to > check for ACPI and some amount of code duplication is also required due > to the way DT driver gets the information vs ACPI. > > The beauty of software node framework is, it supports adding properties > and also is a supported fwnode type in __irq_domain_create(). There is no beauty here. Only some extra bloat that we do not need. DT and ACPI exposes very different attributes. One describe the HW, the other one describe an OS abstraction. Pretending that you can summon both into the same infrastructure is a fallacy. You'll just end up with the cross product of both infrastructure, and pollute the rest of the kernel with pointless cruft. > So, if we > can create the fwnode for these irqchip using software node, we can > attach the same properties and the actual irqchip driver which uses the > fwnode doesn't need to have any ACPI vs DT checks. Same driver will work > seamlessly on both DT and ACPI platforms. But the challenge is, > currently swnode expects to be created with sysfs which won't be > available during early boot when irqchip drivers need to be probed. So, > adding support to create without dependency on sysfs help us to reuse > the same framework for irqchip use case also. That's another fallacy. Most irqchips *DO NOT* need to be probed early. Only the root irqchip. Given that this series is about *secondary* interrupt controllers, they absolutely don't need to be probed early. To be clear: I do not intend to merge anything that: - invents yet another way to "abstract" firmware interfaces - adds more "early probe" hacks for non-primary interrupt controllers I have already said that in response to Anup's AIA series, and this equally applies to this series. M.
On Tue, Aug 08, 2023 at 02:17:22PM +0100, Marc Zyngier wrote: > On Fri, 04 Aug 2023 09:11:05 +0100, > Sunil V L <sunilvl@ventanamicro.com> wrote: > > > > Hi Andy, > > > > On Fri, Aug 04, 2023 at 09:09:16AM +0300, Andy Shevchenko wrote: > > > On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote: > > > > From: Anup Patel <apatel@ventanamicro.com> > > > > > > > > swnode framework can be used to create fwnode for interrupt > > > > controllers. > > > > > > Why? What is this for? > > > Can you elaborate? This commit message is poorly written... > > > > > > And why firmware node is not enough for ACPI case? > > > I assume the fwnode in DT case is already provided by OF. > > > > > Thanks a lot for the review!. > > > > You are right, OF provides the fwnode for irqchip drivers. However, for > > ACPI case, it is typically created using irq_domain_alloc_named_fwnode > > or irq_domain_alloc_fwnode since these are not ACPI devices in the > > namespace but from MADT. The fwnode created using > > irq_domain_alloc_fwnode() is a simple one which doesn't support properties > > similar to the one created by OF framework or software node framework. > > Hence, lot of data from the MADT structures need to be cached as > > separate structures in the drivers and also would need several ifdefs to > > check for ACPI and some amount of code duplication is also required due > > to the way DT driver gets the information vs ACPI. > > > > The beauty of software node framework is, it supports adding properties > > and also is a supported fwnode type in __irq_domain_create(). > > There is no beauty here. Only some extra bloat that we do not need. > > DT and ACPI exposes very different attributes. One describe the HW, > the other one describe an OS abstraction. Pretending that you can > summon both into the same infrastructure is a fallacy. You'll just end > up with the cross product of both infrastructure, and pollute the rest > of the kernel with pointless cruft. > Hi Marc, Thank you very much for the feedback!. Sure, let me revert this approach and do as you recommended in next version. > > So, if we > > can create the fwnode for these irqchip using software node, we can > > attach the same properties and the actual irqchip driver which uses the > > fwnode doesn't need to have any ACPI vs DT checks. Same driver will work > > seamlessly on both DT and ACPI platforms. But the challenge is, > > currently swnode expects to be created with sysfs which won't be > > available during early boot when irqchip drivers need to be probed. So, > > adding support to create without dependency on sysfs help us to reuse > > the same framework for irqchip use case also. > > That's another fallacy. > > Most irqchips *DO NOT* need to be probed early. Only the root > irqchip. Given that this series is about *secondary* interrupt > controllers, they absolutely don't need to be probed early. > Since we created swnode for root irqchip also in this approach, we had to support early creation. With your feedback, this is no longer required. > To be clear: I do not intend to merge anything that: > > - invents yet another way to "abstract" firmware interfaces > > - adds more "early probe" hacks for non-primary interrupt controllers > > I have already said that in response to Anup's AIA series, and this > equally applies to this series. > In Anup's AIA v7 series, he has made non-primary controller drivers as platform drivers which are not probed early. Thanks, Sunil
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 1886995a0b3a..43f191a38980 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -10,6 +10,7 @@ #include <linux/kernel.h> #include <linux/property.h> #include <linux/slab.h> +#include <linux/spinlock.h> #include "base.h" @@ -21,6 +22,7 @@ struct swnode { /* hierarchy */ struct ida child_ids; + struct list_head early; struct list_head entry; struct list_head children; struct swnode *parent; @@ -32,6 +34,9 @@ struct swnode { static DEFINE_IDA(swnode_root_ids); static struct kset *swnode_kset; +static DEFINE_SPINLOCK(swnode_early_lock); +static LIST_HEAD(swnode_early_list); + #define kobj_to_swnode(_kobj_) container_of(_kobj_, struct swnode, kobj) static const struct fwnode_operations software_node_ops; @@ -73,6 +78,17 @@ software_node_to_swnode(const struct software_node *node) if (!node) return NULL; + spin_lock(&swnode_early_lock); + + list_for_each_entry(swnode, &swnode_early_list, early) { + if (swnode->node == node) { + spin_unlock(&swnode_early_lock); + return swnode; + } + } + + spin_unlock(&swnode_early_lock); + spin_lock(&swnode_kset->list_lock); list_for_each_entry(k, &swnode_kset->list, entry) { @@ -698,6 +714,19 @@ software_node_find_by_name(const struct software_node *parent, const char *name) if (!name) return NULL; + spin_lock(&swnode_early_lock); + + list_for_each_entry(swnode, &swnode_early_list, early) { + if (parent == swnode->node->parent && swnode->node->name && + !strcmp(name, swnode->node->name)) { + kobject_get(&swnode->kobj); + spin_unlock(&swnode_early_lock); + return swnode->node; + } + } + + spin_unlock(&swnode_early_lock); + spin_lock(&swnode_kset->list_lock); list_for_each_entry(k, &swnode_kset->list, entry) { @@ -742,10 +771,16 @@ static void software_node_free(const struct software_node *node) kfree(node); } -static void software_node_release(struct kobject *kobj) +static void software_node_release_common(struct kobject *kobj, bool early) { struct swnode *swnode = kobj_to_swnode(kobj); + if (early) { + spin_lock(&swnode_early_lock); + list_del(&swnode->early); + spin_unlock(&swnode_early_lock); + } + if (swnode->parent) { ida_simple_remove(&swnode->parent->child_ids, swnode->id); list_del(&swnode->entry); @@ -760,6 +795,20 @@ static void software_node_release(struct kobject *kobj) kfree(swnode); } +static void software_node_release(struct kobject *kobj) +{ + software_node_release_common(kobj, false); +} + +static void software_node_release_early(struct kobject *kobj) +{ + software_node_release_common(kobj, true); +} + +static const struct kobj_type software_node_type_early = { + .release = software_node_release_early +}; + static const struct kobj_type software_node_type = { .release = software_node_release, .sysfs_ops = &kobj_sysfs_ops, @@ -767,7 +816,7 @@ static const struct kobj_type software_node_type = { static struct fwnode_handle * swnode_register(const struct software_node *node, struct swnode *parent, - unsigned int allocated) + unsigned int allocated, unsigned int early) { struct swnode *swnode; int ret; @@ -786,21 +835,39 @@ swnode_register(const struct software_node *node, struct swnode *parent, swnode->id = ret; swnode->node = node; swnode->parent = parent; - swnode->kobj.kset = swnode_kset; + swnode->kobj.kset = (!early) ? swnode_kset : NULL; fwnode_init(&swnode->fwnode, &software_node_ops); ida_init(&swnode->child_ids); + INIT_LIST_HEAD(&swnode->early); INIT_LIST_HEAD(&swnode->entry); INIT_LIST_HEAD(&swnode->children); - if (node->name) - ret = kobject_init_and_add(&swnode->kobj, &software_node_type, - parent ? &parent->kobj : NULL, - "%s", node->name); - else - ret = kobject_init_and_add(&swnode->kobj, &software_node_type, - parent ? &parent->kobj : NULL, - "node%d", swnode->id); + if (early) { + ret = 0; + kobject_init(&swnode->kobj, &software_node_type_early); + swnode->kobj.parent = parent ? &parent->kobj : NULL; + if (node->name) + ret = kobject_set_name(&swnode->kobj, + "%s", node->name); + else + ret = kobject_set_name(&swnode->kobj, + "node%d", swnode->id); + if (!ret) { + spin_lock(&swnode_early_lock); + list_add_tail(&swnode->early, &swnode_early_list); + spin_unlock(&swnode_early_lock); + } + } else { + if (node->name) + ret = kobject_init_and_add(&swnode->kobj, &software_node_type, + parent ? &parent->kobj : NULL, + "%s", node->name); + else + ret = kobject_init_and_add(&swnode->kobj, &software_node_type, + parent ? &parent->kobj : NULL, + "node%d", swnode->id); + } if (ret) { kobject_put(&swnode->kobj); return ERR_PTR(ret); @@ -815,7 +882,8 @@ swnode_register(const struct software_node *node, struct swnode *parent, if (parent) list_add_tail(&swnode->entry, &parent->children); - kobject_uevent(&swnode->kobj, KOBJ_ADD); + if (!early) + kobject_uevent(&swnode->kobj, KOBJ_ADD); return &swnode->fwnode; } @@ -892,7 +960,7 @@ int software_node_register(const struct software_node *node) if (node->parent && !parent) return -EINVAL; - return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0)); + return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0, 0)); } EXPORT_SYMBOL_GPL(software_node_register); @@ -910,9 +978,10 @@ void software_node_unregister(const struct software_node *node) } EXPORT_SYMBOL_GPL(software_node_unregister); -struct fwnode_handle * -fwnode_create_software_node(const struct property_entry *properties, - const struct fwnode_handle *parent) +static struct fwnode_handle * +fwnode_create_software_node_common(const struct property_entry *properties, + const struct fwnode_handle *parent, + bool early) { struct fwnode_handle *fwnode; struct software_node *node; @@ -931,12 +1000,26 @@ fwnode_create_software_node(const struct property_entry *properties, node->parent = p ? p->node : NULL; - fwnode = swnode_register(node, p, 1); + fwnode = swnode_register(node, p, 1, early); if (IS_ERR(fwnode)) software_node_free(node); return fwnode; } + +struct fwnode_handle * +fwnode_create_software_node_early(const struct property_entry *properties, + const struct fwnode_handle *parent) +{ + return fwnode_create_software_node_common(properties, parent, true); +} + +struct fwnode_handle * +fwnode_create_software_node(const struct property_entry *properties, + const struct fwnode_handle *parent) +{ + return fwnode_create_software_node_common(properties, parent, false); +} EXPORT_SYMBOL_GPL(fwnode_create_software_node); void fwnode_remove_software_node(struct fwnode_handle *fwnode) diff --git a/include/linux/property.h b/include/linux/property.h index 8c3c6685a2ae..7137338bfabb 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -503,6 +503,9 @@ void software_node_unregister_node_group(const struct software_node **node_group int software_node_register(const struct software_node *node); void software_node_unregister(const struct software_node *node); +struct fwnode_handle * +fwnode_create_software_node_early(const struct property_entry *properties, + const struct fwnode_handle *parent); struct fwnode_handle * fwnode_create_software_node(const struct property_entry *properties, const struct fwnode_handle *parent);