| Message ID | 20131123015034.GP10023@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 22, 2013 at 7:50 PM, Tony Lindgren <tony@atomide.com> wrote: > * Tony Lindgren <tony@atomide.com> [131122 17:16]: >> * Tony Lindgren <tony@atomide.com> [131122 17:09]: >> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [131122 16:56]: >> > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote: >> > > > + /* See of_device_resource_notify for populating interrupts */ >> > > > + for (i = 0; i < num_irq; i++, res++) { >> > > > + res->flags = IORESOURCE_IRQ; >> > > > + res->start = -EPROBE_DEFER; >> > > > + res->end = -EPROBE_DEFER; >> > > >> > > NAK. Definitely a bad idea to start introducing magic values other into >> > > resources. Please don't do this. >> > >> > Do you have any better ideas on how to sort out this issue then? >> >> I guess we could allocate all the resources lazily here, I'll take a look >> at that. > > Here's a version that allocates the resources lazily with the notifier. > Seems to boot, need to play with it a bit more though to make sure we're > not overwriting resources for any legacy devices. Have you seen Thierry's series[1]? While your approach is certainly more concise, it seems like a work-around for the problem. I don't think a notifier is the right long term solution. Rob [1] http://www.spinics.net/lists/arm-kernel/msg274110.html
* Rob Herring <robherring2@gmail.com> [131123 07:43]: > On Fri, Nov 22, 2013 at 7:50 PM, Tony Lindgren <tony@atomide.com> wrote: > > * Tony Lindgren <tony@atomide.com> [131122 17:16]: > >> * Tony Lindgren <tony@atomide.com> [131122 17:09]: > >> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [131122 16:56]: > >> > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote: > >> > > > + /* See of_device_resource_notify for populating interrupts */ > >> > > > + for (i = 0; i < num_irq; i++, res++) { > >> > > > + res->flags = IORESOURCE_IRQ; > >> > > > + res->start = -EPROBE_DEFER; > >> > > > + res->end = -EPROBE_DEFER; > >> > > > >> > > NAK. Definitely a bad idea to start introducing magic values other into > >> > > resources. Please don't do this. > >> > > >> > Do you have any better ideas on how to sort out this issue then? > >> > >> I guess we could allocate all the resources lazily here, I'll take a look > >> at that. > > > > Here's a version that allocates the resources lazily with the notifier. > > Seems to boot, need to play with it a bit more though to make sure we're > > not overwriting resources for any legacy devices. > > Have you seen Thierry's series[1]? While your approach is certainly > more concise, it seems like a work-around for the problem. I don't > think a notifier is the right long term solution. OK cool. I think we can fix the $Subject bug first without making all those changes, then do the rest of the reorg for v3.14. The bug is that we try to populate IRQ resources at a wrong time when they may not exist. Based on a quick look it seems we could combine Thierry's addition of the new function of_platform_probe(struct platform_device *pdev) and use that to allocate all resources at driver probe time like my patch is doing. And then there's no need for the notifier. Regards, Tony > [1] http://www.spinics.net/lists/arm-kernel/msg274110.html
On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren <tony@atomide.com> wrote: > * Tony Lindgren <tony@atomide.com> [131122 17:16]: > > * Tony Lindgren <tony@atomide.com> [131122 17:09]: > > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [131122 16:56]: > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote: > > > > > + /* See of_device_resource_notify for populating interrupts */ > > > > > + for (i = 0; i < num_irq; i++, res++) { > > > > > + res->flags = IORESOURCE_IRQ; > > > > > + res->start = -EPROBE_DEFER; > > > > > + res->end = -EPROBE_DEFER; > > > > > > > > NAK. Definitely a bad idea to start introducing magic values other into > > > > resources. Please don't do this. > > > > > > Do you have any better ideas on how to sort out this issue then? > > > > I guess we could allocate all the resources lazily here, I'll take a look > > at that. > > Here's a version that allocates the resources lazily with the notifier. > Seems to boot, need to play with it a bit more though to make sure we're > not overwriting resources for any legacy devices. Blurg. Using a notifier really feels like we don't have a good handle on a reasonable solution yet. Basically it means we're hooking into the driver core without /looking/ like we're hooking into the driver core. I don't think this is any better, but I don't have a better suggestion at the moment. :-( g.
On Sat, Nov 23, 2013 at 08:32:40AM -0800, Tony Lindgren wrote: > * Rob Herring <robherring2@gmail.com> [131123 07:43]: > > On Fri, Nov 22, 2013 at 7:50 PM, Tony Lindgren <tony@atomide.com> wrote: > > > * Tony Lindgren <tony@atomide.com> [131122 17:16]: > > >> * Tony Lindgren <tony@atomide.com> [131122 17:09]: > > >> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [131122 16:56]: > > >> > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote: > > >> > > > + /* See of_device_resource_notify for populating interrupts */ > > >> > > > + for (i = 0; i < num_irq; i++, res++) { > > >> > > > + res->flags = IORESOURCE_IRQ; > > >> > > > + res->start = -EPROBE_DEFER; > > >> > > > + res->end = -EPROBE_DEFER; > > >> > > > > >> > > NAK. Definitely a bad idea to start introducing magic values other into > > >> > > resources. Please don't do this. > > >> > > > >> > Do you have any better ideas on how to sort out this issue then? > > >> > > >> I guess we could allocate all the resources lazily here, I'll take a look > > >> at that. > > > > > > Here's a version that allocates the resources lazily with the notifier. > > > Seems to boot, need to play with it a bit more though to make sure we're > > > not overwriting resources for any legacy devices. > > > > Have you seen Thierry's series[1]? While your approach is certainly > > more concise, it seems like a work-around for the problem. I don't > > think a notifier is the right long term solution. > > OK cool. I think we can fix the $Subject bug first without making all > those changes, then do the rest of the reorg for v3.14. > > The bug is that we try to populate IRQ resources at a wrong time > when they may not exist. > > Based on a quick look it seems we could combine Thierry's addition > of the new function of_platform_probe(struct platform_device *pdev) > and use that to allocate all resources at driver probe time like my > patch is doing. And then there's no need for the notifier. My series already does the allocation at probe time as well. That was the whole point. The reason why I added of_platform_probe() is because I think we'll be needing this for other types of resources in the future as well, so it could serve as a central place to do all of that. There was also a proposal[0] by Arnd a few weeks ago that solved this in a more generic way. I've said it before, and I'll say again that the idea scares me somewhat, but it does solve some interesting aspects and has the potential to get rid of a whole lot of boilerplate code. While the original purpose was to handle all things devm_*(), I suspect that same mechanism could be used to resolve DT references at probe time. Thierry [0]: http://www.spinics.net/lists/devicetree/msg10684.html
Hi Grant, Rob, On Sun, 24 Nov 2013, Grant Likely wrote: > On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren <tony@atomide.com> wrote: > > * Tony Lindgren <tony@atomide.com> [131122 17:16]: > > > * Tony Lindgren <tony@atomide.com> [131122 17:09]: > > > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [131122 16:56]: > > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote: > > > > > > + /* See of_device_resource_notify for populating interrupts */ > > > > > > + for (i = 0; i < num_irq; i++, res++) { > > > > > > + res->flags = IORESOURCE_IRQ; > > > > > > + res->start = -EPROBE_DEFER; > > > > > > + res->end = -EPROBE_DEFER; > > > > > > > > > > NAK. Definitely a bad idea to start introducing magic values other into > > > > > resources. Please don't do this. > > > > > > > > Do you have any better ideas on how to sort out this issue then? > > > > > > I guess we could allocate all the resources lazily here, I'll take a look > > > at that. > > > > Here's a version that allocates the resources lazily with the notifier. > > Seems to boot, need to play with it a bit more though to make sure we're > > not overwriting resources for any legacy devices. > > Blurg. Using a notifier really feels like we don't have a good handle on > a reasonable solution yet. Basically it means we're hooking into the > driver core without /looking/ like we're hooking into the driver core. I > don't think this is any better, but I don't have a better suggestion at > the moment. :-( Unfortunately this patch, or something that accomplishes the same results, is somewhat high-priority for us on OMAP. OMAP37xx got prematurely converted to DT booting, and now dynamic power management is broken: http://marc.info/?l=linux-arm-kernel&m=138658294830408&w=2 Tony writes that this patch is one of the two patches needed to get things working again: http://marc.info/?l=linux-arm-kernel&m=138660942506846&w=2 Is it possible to get this patch, or something similar, merged for v3.13-rc? Once something like PM is broken, it's pretty easy for other broken patches to make it into the tree, since it becomes very difficult to test without turning into a maintainer denial-of-service attack. - Paul
Hi Grant, Rob, On Tue, 10 Dec 2013, Paul Walmsley wrote: > On Sun, 24 Nov 2013, Grant Likely wrote: > > > On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren <tony@atomide.com> wrote: > > > * Tony Lindgren <tony@atomide.com> [131122 17:16]: > > > > * Tony Lindgren <tony@atomide.com> [131122 17:09]: > > > > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [131122 16:56]: > > > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote: > > > > > > > + /* See of_device_resource_notify for populating interrupts */ > > > > > > > + for (i = 0; i < num_irq; i++, res++) { > > > > > > > + res->flags = IORESOURCE_IRQ; > > > > > > > + res->start = -EPROBE_DEFER; > > > > > > > + res->end = -EPROBE_DEFER; > > > > > > > > > > > > NAK. Definitely a bad idea to start introducing magic values other into > > > > > > resources. Please don't do this. > > > > > > > > > > Do you have any better ideas on how to sort out this issue then? > > > > > > > > I guess we could allocate all the resources lazily here, I'll take a look > > > > at that. > > > > > > Here's a version that allocates the resources lazily with the notifier. > > > Seems to boot, need to play with it a bit more though to make sure we're > > > not overwriting resources for any legacy devices. > > > > Blurg. Using a notifier really feels like we don't have a good handle on > > a reasonable solution yet. Basically it means we're hooking into the > > driver core without /looking/ like we're hooking into the driver core. I > > don't think this is any better, but I don't have a better suggestion at > > the moment. :-( > > Unfortunately this patch, or something that accomplishes the same results, > is somewhat high-priority for us on OMAP. OMAP37xx got prematurely > converted to DT booting, and now dynamic power management is broken: > > http://marc.info/?l=linux-arm-kernel&m=138658294830408&w=2 > > Tony writes that this patch is one of the two patches needed to get things > working again: > > http://marc.info/?l=linux-arm-kernel&m=138660942506846&w=2 > > Is it possible to get this patch, or something similar, merged for > v3.13-rc? > > Once something like PM is broken, it's pretty easy for other broken > patches to make it into the tree, since it becomes very difficult to test > without turning into a maintainer denial-of-service attack. Ping. Could you please provide some guidance here about what you'd need to get this solved for v3.13-rc? - Paul
On Mon, Dec 30, 2013 at 4:10 PM, Paul Walmsley <paul@pwsan.com> wrote: > Hi Grant, Rob, > > On Tue, 10 Dec 2013, Paul Walmsley wrote: > >> On Sun, 24 Nov 2013, Grant Likely wrote: >> >> > On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren <tony@atomide.com> wrote: >> > > * Tony Lindgren <tony@atomide.com> [131122 17:16]: >> > > > * Tony Lindgren <tony@atomide.com> [131122 17:09]: >> > > > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [131122 16:56]: >> > > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote: >> > > > > > > + /* See of_device_resource_notify for populating interrupts */ >> > > > > > > + for (i = 0; i < num_irq; i++, res++) { >> > > > > > > + res->flags = IORESOURCE_IRQ; >> > > > > > > + res->start = -EPROBE_DEFER; >> > > > > > > + res->end = -EPROBE_DEFER; >> > > > > > >> > > > > > NAK. Definitely a bad idea to start introducing magic values other into >> > > > > > resources. Please don't do this. >> > > > > >> > > > > Do you have any better ideas on how to sort out this issue then? >> > > > >> > > > I guess we could allocate all the resources lazily here, I'll take a look >> > > > at that. >> > > >> > > Here's a version that allocates the resources lazily with the notifier. >> > > Seems to boot, need to play with it a bit more though to make sure we're >> > > not overwriting resources for any legacy devices. >> > >> > Blurg. Using a notifier really feels like we don't have a good handle on >> > a reasonable solution yet. Basically it means we're hooking into the >> > driver core without /looking/ like we're hooking into the driver core. I >> > don't think this is any better, but I don't have a better suggestion at >> > the moment. :-( >> >> Unfortunately this patch, or something that accomplishes the same results, >> is somewhat high-priority for us on OMAP. OMAP37xx got prematurely >> converted to DT booting, and now dynamic power management is broken: >> >> http://marc.info/?l=linux-arm-kernel&m=138658294830408&w=2 >> >> Tony writes that this patch is one of the two patches needed to get things >> working again: >> >> http://marc.info/?l=linux-arm-kernel&m=138660942506846&w=2 >> >> Is it possible to get this patch, or something similar, merged for >> v3.13-rc? >> >> Once something like PM is broken, it's pretty easy for other broken >> patches to make it into the tree, since it becomes very difficult to test >> without turning into a maintainer denial-of-service attack. > > Ping. Could you please provide some guidance here about what you'd need > to get this solved for v3.13-rc? I agree with doing the platform_get_irq modification and feel free to add my ack. I'm going to be offline for the next week. Rob
Hi Tony, Rob, On Tue, 31 Dec 2013, Rob Herring wrote: > On Mon, Dec 30, 2013 at 4:10 PM, Paul Walmsley <paul@pwsan.com> wrote: > > Hi Grant, Rob, > > > > On Tue, 10 Dec 2013, Paul Walmsley wrote: > > > >> On Sun, 24 Nov 2013, Grant Likely wrote: > >> > >> > On Fri, 22 Nov 2013 17:50:35 -0800, Tony Lindgren <tony@atomide.com> wrote: > >> > > * Tony Lindgren <tony@atomide.com> [131122 17:16]: > >> > > > * Tony Lindgren <tony@atomide.com> [131122 17:09]: > >> > > > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [131122 16:56]: > >> > > > > > On Fri, Nov 22, 2013 at 04:43:35PM -0800, Tony Lindgren wrote: > >> > > > > > > + /* See of_device_resource_notify for populating interrupts */ > >> > > > > > > + for (i = 0; i < num_irq; i++, res++) { > >> > > > > > > + res->flags = IORESOURCE_IRQ; > >> > > > > > > + res->start = -EPROBE_DEFER; > >> > > > > > > + res->end = -EPROBE_DEFER; > >> > > > > > > >> > > > > > NAK. Definitely a bad idea to start introducing magic values other into > >> > > > > > resources. Please don't do this. > >> > > > > > >> > > > > Do you have any better ideas on how to sort out this issue then? > >> > > > > >> > > > I guess we could allocate all the resources lazily here, I'll take a look > >> > > > at that. > >> > > > >> > > Here's a version that allocates the resources lazily with the notifier. > >> > > Seems to boot, need to play with it a bit more though to make sure we're > >> > > not overwriting resources for any legacy devices. > >> > > >> > Blurg. Using a notifier really feels like we don't have a good handle on > >> > a reasonable solution yet. Basically it means we're hooking into the > >> > driver core without /looking/ like we're hooking into the driver core. I > >> > don't think this is any better, but I don't have a better suggestion at > >> > the moment. :-( > >> > >> Unfortunately this patch, or something that accomplishes the same results, > >> is somewhat high-priority for us on OMAP. OMAP37xx got prematurely > >> converted to DT booting, and now dynamic power management is broken: > >> > >> http://marc.info/?l=linux-arm-kernel&m=138658294830408&w=2 > >> > >> Tony writes that this patch is one of the two patches needed to get things > >> working again: > >> > >> http://marc.info/?l=linux-arm-kernel&m=138660942506846&w=2 > >> > >> Is it possible to get this patch, or something similar, merged for > >> v3.13-rc? > >> > >> Once something like PM is broken, it's pretty easy for other broken > >> patches to make it into the tree, since it becomes very difficult to test > >> without turning into a maintainer denial-of-service attack. > > > > Ping. Could you please provide some guidance here about what you'd need > > to get this solved for v3.13-rc? > > I agree with doing the platform_get_irq modification and feel free to > add my ack. I'm going to be offline for the next week. Thanks Rob! Tony, would you consider sending this patch upstream for v3.13-rc along with your 37xxevm fix patches? - Paul
* Paul Walmsley <paul@pwsan.com> [140106 15:43]: > On Tue, 31 Dec 2013, Rob Herring wrote: > > On Mon, Dec 30, 2013 at 4:10 PM, Paul Walmsley <paul@pwsan.com> wrote: > > > On Tue, 10 Dec 2013, Paul Walmsley wrote: > > >> Is it possible to get this patch, or something similar, merged for > > >> v3.13-rc? > > >> > > >> Once something like PM is broken, it's pretty easy for other broken > > >> patches to make it into the tree, since it becomes very difficult to test > > >> without turning into a maintainer denial-of-service attack. > > > > > > Ping. Could you please provide some guidance here about what you'd need > > > to get this solved for v3.13-rc? > > > > I agree with doing the platform_get_irq modification and feel free to > > add my ack. I'm going to be offline for the next week. > > Thanks Rob! > > Tony, would you consider sending this patch upstream for v3.13-rc along > with your 37xxevm fix patches? Hmm I'd like to see a proper patch posted for platform_get_irq() by Thierry as that one is really his patch. And it also had a comment from Grant. After that I can queue it if nobody else is picking it as I have a dependency to it. Regards, Tony
--- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -141,13 +141,47 @@ struct platform_device *of_device_alloc(struct device_node *np, struct device *parent) { struct platform_device *dev; - int rc, i, num_reg = 0, num_irq; - struct resource *res, temp_res; dev = platform_device_alloc("", -1); if (!dev) return NULL; + dev->dev.of_node = of_node_get(np); +#if defined(CONFIG_MICROBLAZE) + dev->dev.dma_mask = &dev->archdata.dma_mask; +#endif + dev->dev.parent = parent; + + if (bus_id) + dev_set_name(&dev->dev, "%s", bus_id); + else + of_device_make_bus_id(&dev->dev); + + /* See of_device_resource_notify for populating the resources */ + + return dev; +} +EXPORT_SYMBOL(of_device_alloc); + +/* + * The device interrupts are not necessarily available for all + * irqdomains initially so we need to populate them using a + * notifier. + */ +static int of_device_resource_notify(struct notifier_block *nb, + unsigned long event, void *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct device_node *np = pdev->dev.of_node; + int rc, i, num_reg = 0, num_irq; + struct resource *res, temp_res; + + if (event != BUS_NOTIFY_BIND_DRIVER) + return 0; + + if (!np) + goto out; + /* count the io and irq resources */ if (of_can_translate_address(np)) while (of_address_to_resource(np, num_reg, &temp_res) == 0) @@ -158,12 +192,12 @@ struct platform_device *of_device_alloc(struct device_node *np, if (num_irq || num_reg) { res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL); if (!res) { - platform_device_put(dev); - return NULL; + platform_device_put(pdev); + goto out; } - dev->num_resources = num_reg + num_irq; - dev->resource = res; + pdev->num_resources = num_reg + num_irq; + pdev->resource = res; for (i = 0; i < num_reg; i++, res++) { rc = of_address_to_resource(np, i, res); WARN_ON(rc); @@ -171,20 +205,9 @@ struct platform_device *of_device_alloc(struct device_node *np, WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq); } - dev->dev.of_node = of_node_get(np); -#if defined(CONFIG_MICROBLAZE) - dev->dev.dma_mask = &dev->archdata.dma_mask; -#endif - dev->dev.parent = parent; - - if (bus_id) - dev_set_name(&dev->dev, "%s", bus_id); - else - of_device_make_bus_id(&dev->dev); - - return dev; +out: + return NOTIFY_DONE; } -EXPORT_SYMBOL(of_device_alloc); /** * of_platform_device_create_pdata - Alloc, initialize and register an of_device @@ -447,6 +470,8 @@ int of_platform_bus_probe(struct device_node *root, } EXPORT_SYMBOL(of_platform_bus_probe); +static struct notifier_block resource_nb; + /** * of_platform_populate() - Populate platform_devices from device tree data * @root: parent of the first level to probe or NULL for the root of the tree @@ -478,6 +503,11 @@ int of_platform_populate(struct device_node *root, if (!root) return -EINVAL; + if (!resource_nb.notifier_call) { + resource_nb.notifier_call = of_device_resource_notify, + bus_register_notifier(&platform_bus_type, &resource_nb); + } + for_each_child_of_node(root, child) { rc = of_platform_bus_create(child, matches, lookup, parent, true); if (rc)