| Message ID | 20170808122259.6299-7-rrichter@cavium.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+Lorenzo On 08/08/17 13:22, Robert Richter wrote: > Use an initcall to initialize its. This allows us to use the device > framework during initialization that is up at this point. We use > subsys_initcall() here since we need the arch to be initialized > first. It is before pci and platform device probe where devices are > bound to msi interrupts. > > Signed-off-by: Robert Richter <rrichter@cavium.com> > --- > drivers/irqchip/irq-gic-v3-its.c | 3 ++- > drivers/irqchip/irq-gic-v3.c | 5 ----- > include/linux/irqchip/arm-gic-v3.h | 1 - > 3 files changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index 5e2d4f2876d8..488f811d5978 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -1994,7 +1994,7 @@ int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists, > return 0; > } > > -int __init its_init(void) > +static int __init its_init(void) > { > struct its_node *its, *tmp; > int err = 0, err2; > @@ -2036,3 +2036,4 @@ int __init its_init(void) > > return 0; > } > +subsys_initcall(its_init); *ding*! I'm sorry, but that's a total NAK. We're trading hard to maintain hardcoded dependencies for even more difficult to deal with, link-order driven dependencies. That's exactly what Lorenzo and I have been fighting against in the XGene ACPI case, and I'm not going to introduce more of this. We need to break the dependency between the HW and the associated MSI domains, not making it tighter. Let the HW be probed at some time, and the MSI domains lazily created as needed once the end-points are probed. I'm also pretty worried that (as mentioned in my reply to patch #2) that this "make it happen later" games with the initcalls are breaking stuff (see how platform devices are instantiated on the back of an arch_initcall_sync). As far as I can tell, non-PCI MSI is dead after patch #2. I think there is a number of things to fix in the core code before we can start playing that kind of tricks. The major issue I see is that MSI domains are expected to be available way too early, at device discovery time rather than at driver probe time. This has a cascading effect which we should solve first. I'll try to prototype something... Thanks, M.
Marc, thanks for your review. On 21.08.17 09:30:24, Marc Zyngier wrote: > +Lorenzo > > On 08/08/17 13:22, Robert Richter wrote: > > Use an initcall to initialize its. This allows us to use the device > > framework during initialization that is up at this point. We use > > subsys_initcall() here since we need the arch to be initialized > > first. It is before pci and platform device probe where devices are > > bound to msi interrupts. > > > > Signed-off-by: Robert Richter <rrichter@cavium.com> > > --- > > drivers/irqchip/irq-gic-v3-its.c | 3 ++- > > drivers/irqchip/irq-gic-v3.c | 5 ----- > > include/linux/irqchip/arm-gic-v3.h | 1 - > > 3 files changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > > index 5e2d4f2876d8..488f811d5978 100644 > > --- a/drivers/irqchip/irq-gic-v3-its.c > > +++ b/drivers/irqchip/irq-gic-v3-its.c > > @@ -1994,7 +1994,7 @@ int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists, > > return 0; > > } > > > > -int __init its_init(void) > > +static int __init its_init(void) > > { > > struct its_node *its, *tmp; > > int err = 0, err2; > > @@ -2036,3 +2036,4 @@ int __init its_init(void) > > > > return 0; > > } > > +subsys_initcall(its_init); > > *ding*! > > I'm sorry, but that's a total NAK. We're trading hard to maintain > hardcoded dependencies for even more difficult to deal with, link-order > driven dependencies. That's exactly what Lorenzo and I have been > fighting against in the XGene ACPI case, and I'm not going to introduce > more of this. > > We need to break the dependency between the HW and the associated MSI > domains, not making it tighter. Let the HW be probed at some time, and > the MSI domains lazily created as needed once the end-points are probed. I see your point here. the ordering of initialization functions using initcalls is fragile. > I'm also pretty worried that (as mentioned in my reply to patch #2) that > this "make it happen later" games with the initcalls are breaking stuff > (see how platform devices are instantiated on the back of an > arch_initcall_sync). As far as I can tell, non-PCI MSI is dead after > patch #2. Yes, I missed that as you pointed out in #2. Even if we change it to a subsys_initcall (not sure if that would generally work for this), there still would be link order dependencies. I have tested it and even pci msi has order dependencies that are not guaranteed: ffff000008c14e60 t __initcall_its_init4 ffff000008c14e68 t __initcall_its_pci_msi_init4 ffff000008c14e70 t __initcall_its_pmsi_init4 With a different order the intialization would fail here. > I think there is a number of things to fix in the core code before we > can start playing that kind of tricks. The major issue I see is that MSI > domains are expected to be available way too early, at device discovery > time rather than at driver probe time. This has a cascading effect which > we should solve first. > > I'll try to prototype something... I will look into this too. Thanks, -Robert
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 5e2d4f2876d8..488f811d5978 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -1994,7 +1994,7 @@ int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists, return 0; } -int __init its_init(void) +static int __init its_init(void) { struct its_node *its, *tmp; int err = 0, err2; @@ -2036,3 +2036,4 @@ int __init its_init(void) return 0; } +subsys_initcall(its_init); diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 1476ce12b3b8..32bdfd656ea7 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -1167,9 +1167,6 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare gic_populate_ppi_partitions(node); gic_of_setup_kvm_info(node); - - its_init(); - return 0; out_unmap_rdist: @@ -1459,8 +1456,6 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end) acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle); gic_acpi_setup_kvm_info(); - its_init(); - return 0; out_fwhandle_free: diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index 15cbedbd82f1..3c375c593800 100644 --- a/include/linux/irqchip/arm-gic-v3.h +++ b/include/linux/irqchip/arm-gic-v3.h @@ -493,7 +493,6 @@ struct irq_domain; struct fwnode_handle; int its_probe(struct fwnode_handle *handle, struct rdists *rdists, struct irq_domain *domain); -int its_init(void); static inline bool gic_enable_sre(void) {
Use an initcall to initialize its. This allows us to use the device framework during initialization that is up at this point. We use subsys_initcall() here since we need the arch to be initialized first. It is before pci and platform device probe where devices are bound to msi interrupts. Signed-off-by: Robert Richter <rrichter@cavium.com> --- drivers/irqchip/irq-gic-v3-its.c | 3 ++- drivers/irqchip/irq-gic-v3.c | 5 ----- include/linux/irqchip/arm-gic-v3.h | 1 - 3 files changed, 2 insertions(+), 7 deletions(-)