* device_node lifetime (was: Re: [PATCH 1/7] phy: brcmstb-sata: add missing of_node_put) [not found] ` <alpine.DEB.2.02.1511172327030.2121-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org> @ 2015-11-18 19:05 ` Brian Norris [not found] ` <20151118190500.GE140057-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2015-11-19 18:44 ` Rob Herring 0 siblings, 2 replies; 5+ messages in thread From: Brian Norris @ 2015-11-18 19:05 UTC (permalink / raw) To: Julia Lawall Cc: Kishon Vijay Abraham I, kernel-janitors-u79uwXL29TY76Z2rM5mHXA, Gregory Fong, Florian Fainelli, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Russell King - ARM Linux, Thomas Petazzoni, Andrew Lunn, Bjorn Helgaas, Jason Cooper, devicetree-u79uwXL29TY76Z2rM5mHXA (changing subject, add devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org) On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote: > On Tue, 17 Nov 2015, Brian Norris wrote: > > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote: > > > Is this something that should be checked for elsewhere? > > > > I expect the same sort of problem shows up plenty of other places. I > > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these > > failures probably aren't felt by many. > > I tried the following semantic patch: > > @@ > struct device_node *e; > expression e1; > identifier fld; > @@ > > ... when != of_node_get(...) > *(<+...e1->fld...+>) = e > ... when != of_node_get(...) > return e1; > > basically, this says that a structure field is initilized to a device node > value, the structure is returned by the containing function, and the > containing function contains no of_node_get at all. Certainly this is > quite constrained, but it does produce a number of examples. > > I looked at a few of them: > > drivers/clk/ingenic/cgu.c, ingenic_cgu_new > clk/pistachio/clk.c, pistachio_clk_alloc_provider It looks like the clock core (drivers/clk/clk.c) initially grabs the clk provider node in of_clk_init(), then drops it after it's initialized, but most of these providers use of_clk_add_provider(), which seems to manage the device_node lifetime for the user. So I think these are OK. > drivers/mfd/syscon.c, of_syscon_register This one looks potentially suspect. Syscon nodes aren't usually directly managed by a single driver, and the device_node pointer is used for lookups later...so I think it should keep a kref, and it doesn't. > drivers/of/pdt.c, function of_pdt_create_node Not real sure about this one. > Any idea whether these need of_node_get? In all cases the device node > value comes in as a parameter. I'm really not an expert on this stuff. I just saw a potential problem that I happen to be looking at in other subsystems, and I wanted to know what others thought. I think this discussion should include the DT folks and the subsystems in question. For one, I'm as interested as anyone in getting this todo clarified: Documentation/devicetree/todo.txt - Document node lifecycle for CONFIG_OF_DYNAMIC Regards, Brian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20151118190500.GE140057-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>]
* Re: device_node lifetime (was: Re: [PATCH 1/7] phy: brcmstb-sata: add missing of_node_put) [not found] ` <20151118190500.GE140057-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> @ 2015-11-18 20:39 ` Julia Lawall 0 siblings, 0 replies; 5+ messages in thread From: Julia Lawall @ 2015-11-18 20:39 UTC (permalink / raw) To: Brian Norris Cc: Julia Lawall, Kishon Vijay Abraham I, kernel-janitors-u79uwXL29TY76Z2rM5mHXA, Gregory Fong, Florian Fainelli, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Russell King - ARM Linux, Thomas Petazzoni, Andrew Lunn, Bjorn Helgaas, Jason Cooper, devicetree-u79uwXL29TY76Z2rM5mHXA On Wed, 18 Nov 2015, Brian Norris wrote: > (changing subject, add devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org) > > On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote: > > On Tue, 17 Nov 2015, Brian Norris wrote: > > > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote: > > > > Is this something that should be checked for elsewhere? > > > > > > I expect the same sort of problem shows up plenty of other places. I > > > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these > > > failures probably aren't felt by many. > > > > I tried the following semantic patch: > > > > @@ > > struct device_node *e; > > expression e1; > > identifier fld; > > @@ > > > > ... when != of_node_get(...) > > *(<+...e1->fld...+>) = e > > ... when != of_node_get(...) > > return e1; > > > > basically, this says that a structure field is initilized to a device node > > value, the structure is returned by the containing function, and the > > containing function contains no of_node_get at all. Certainly this is > > quite constrained, but it does produce a number of examples. > > > > I looked at a few of them: > > > > drivers/clk/ingenic/cgu.c, ingenic_cgu_new > > clk/pistachio/clk.c, pistachio_clk_alloc_provider > > It looks like the clock core (drivers/clk/clk.c) initially grabs the clk > provider node in of_clk_init(), then drops it after it's initialized, > but most of these providers use of_clk_add_provider(), which seems to > manage the device_node lifetime for the user. So I think these are OK. > > > drivers/mfd/syscon.c, of_syscon_register > > This one looks potentially suspect. Syscon nodes aren't usually directly > managed by a single driver, and the device_node pointer is used for > lookups later...so I think it should keep a kref, and it doesn't. > > > drivers/of/pdt.c, function of_pdt_create_node > > Not real sure about this one. > > > Any idea whether these need of_node_get? In all cases the device node > > value comes in as a parameter. > > I'm really not an expert on this stuff. I just saw a potential problem > that I happen to be looking at in other subsystems, and I wanted to know > what others thought. Thanks for the analysis. I will look into them a bit more. Hopefully at least the maintainer of each file will know what should be done. julia > I think this discussion should include the DT folks > and the subsystems in question. For one, I'm as interested as anyone in > getting this todo clarified: > > Documentation/devicetree/todo.txt > - Document node lifecycle for CONFIG_OF_DYNAMIC > > Regards, > Brian > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: device_node lifetime (was: Re: [PATCH 1/7] phy: brcmstb-sata: add missing of_node_put) 2015-11-18 19:05 ` device_node lifetime (was: Re: [PATCH 1/7] phy: brcmstb-sata: add missing of_node_put) Brian Norris [not found] ` <20151118190500.GE140057-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> @ 2015-11-19 18:44 ` Rob Herring 2015-11-19 19:14 ` Russell King - ARM Linux 1 sibling, 1 reply; 5+ messages in thread From: Rob Herring @ 2015-11-19 18:44 UTC (permalink / raw) To: Brian Norris Cc: Julia Lawall, Kishon Vijay Abraham I, kernel-janitors, Gregory Fong, Florian Fainelli, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Russell King - ARM Linux, Thomas Petazzoni, Andrew Lunn, Bjorn Helgaas, Jason Cooper, devicetree@vger.kernel.org On Wed, Nov 18, 2015 at 1:05 PM, Brian Norris <computersforpeace@gmail.com> wrote: > (changing subject, add devicetree@vger.kernel.org) > > On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote: >> On Tue, 17 Nov 2015, Brian Norris wrote: >> > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote: >> > > Is this something that should be checked for elsewhere? >> > >> > I expect the same sort of problem shows up plenty of other places. I >> > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these >> > failures probably aren't felt by many. The "problem" is non-existent because either CONFIG_OF_DYNAMIC is off or where it is used is limited (memory and cpus on PSeries) and now overlays. Overlays have the potential to be problematic, but we should manage ref counting for overlays in a completely different way. What that looks like, I don't know. I'll leave that to the person that cares about removing overlays. >> basically, this says that a structure field is initilized to a device node >> value, the structure is returned by the containing function, and the >> containing function contains no of_node_get at all. Certainly this is >> quite constrained, but it does produce a number of examples. I've got no idea if this is right or not. >> drivers/of/pdt.c, function of_pdt_create_node > > Not real sure about this one. SPARC. Stay away. > >> Any idea whether these need of_node_get? In all cases the device node >> value comes in as a parameter. > > I'm really not an expert on this stuff. I just saw a potential problem > that I happen to be looking at in other subsystems, and I wanted to know > what others thought. I think this discussion should include the DT folks > and the subsystems in question. For one, I'm as interested as anyone in > getting this todo clarified: > > Documentation/devicetree/todo.txt > - Document node lifecycle for CONFIG_OF_DYNAMIC Step 2 after figuring out it can't be documented is "define a new way to handle dynamic DT refcounting aka how to get rid of of_node_get/put." Rob ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: device_node lifetime (was: Re: [PATCH 1/7] phy: brcmstb-sata: add missing of_node_put) 2015-11-19 18:44 ` Rob Herring @ 2015-11-19 19:14 ` Russell King - ARM Linux 2015-11-19 22:58 ` Rob Herring 0 siblings, 1 reply; 5+ messages in thread From: Russell King - ARM Linux @ 2015-11-19 19:14 UTC (permalink / raw) To: Rob Herring Cc: Brian Norris, Julia Lawall, Kishon Vijay Abraham I, kernel-janitors, Gregory Fong, Florian Fainelli, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Thomas Petazzoni, Andrew Lunn, Bjorn Helgaas, Jason Cooper, devicetree@vger.kernel.org On Thu, Nov 19, 2015 at 12:44:11PM -0600, Rob Herring wrote: > On Wed, Nov 18, 2015 at 1:05 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > (changing subject, add devicetree@vger.kernel.org) > > > > On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote: > >> On Tue, 17 Nov 2015, Brian Norris wrote: > >> > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote: > >> > > Is this something that should be checked for elsewhere? > >> > > >> > I expect the same sort of problem shows up plenty of other places. I > >> > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these > >> > failures probably aren't felt by many. > > The "problem" is non-existent because either CONFIG_OF_DYNAMIC is off > or where it is used is limited (memory and cpus on PSeries) and now > overlays. Overlays have the potential to be problematic, but we should > manage ref counting for overlays in a completely different way. What > that looks like, I don't know. I'll leave that to the person that > cares about removing overlays. So are you saying we should just forget about of_node_put and delete all of_node_put/of_node_get references in code outside drivers/of ? That seems pretty obtuse given that we do have the overlay code merged, and sounds to me like a very bad idea. Expecting those who want to use overlays to run around checking that the refcounting is correct in drivers is a really silly idea IMHO - the existing API is refcounted, so either people really ought to be using it correctly as it's already been designed (in other words, with correct refcounting, and we shouldn't be shovelling this problem onto other people) or the refcounting should be completely killed. The existing half-way house of "we have refcounting, but we don't care about it" is really insane. Either we have refcounting, and it's used properly, or we don't have refcounting. No middle ground IMHO. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: device_node lifetime (was: Re: [PATCH 1/7] phy: brcmstb-sata: add missing of_node_put) 2015-11-19 19:14 ` Russell King - ARM Linux @ 2015-11-19 22:58 ` Rob Herring 0 siblings, 0 replies; 5+ messages in thread From: Rob Herring @ 2015-11-19 22:58 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Brian Norris, Julia Lawall, Kishon Vijay Abraham I, kernel-janitors, Gregory Fong, Florian Fainelli, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Thomas Petazzoni, Andrew Lunn, Bjorn Helgaas, Jason Cooper, devicetree@vger.kernel.org On Thu, Nov 19, 2015 at 1:14 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Nov 19, 2015 at 12:44:11PM -0600, Rob Herring wrote: >> On Wed, Nov 18, 2015 at 1:05 PM, Brian Norris >> <computersforpeace@gmail.com> wrote: >> > (changing subject, add devicetree@vger.kernel.org) >> > >> > On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote: >> >> On Tue, 17 Nov 2015, Brian Norris wrote: >> >> > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote: >> >> > > Is this something that should be checked for elsewhere? >> >> > >> >> > I expect the same sort of problem shows up plenty of other places. I >> >> > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these >> >> > failures probably aren't felt by many. >> >> The "problem" is non-existent because either CONFIG_OF_DYNAMIC is off >> or where it is used is limited (memory and cpus on PSeries) and now >> overlays. Overlays have the potential to be problematic, but we should >> manage ref counting for overlays in a completely different way. What >> that looks like, I don't know. I'll leave that to the person that >> cares about removing overlays. > > So are you saying we should just forget about of_node_put and delete all > of_node_put/of_node_get references in code outside drivers/of ? That > seems pretty obtuse given that we do have the overlay code merged, and > sounds to me like a very bad idea. I wasn't suggesting going as far as that, but I do think we can't rely on drivers to get ref counting right. > Expecting those who want to use overlays to run around checking that > the refcounting is correct in drivers is a really silly idea IMHO - > the existing API is refcounted, so either people really ought to be > using it correctly as it's already been designed (in other words, with > correct refcounting, and we shouldn't be shovelling this problem onto > other people) or the refcounting should be completely killed. I'm only saying stop shoveling until ref This is only the tip of the iceberg. What happens when you start hot plugging GPIO controllers for example. > The existing half-way house of "we have refcounting, but we don't care > about it" is really insane. > > Either we have refcounting, and it's used properly, or we don't have > refcounting. No middle ground IMHO. > > -- > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-11-19 22:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1447673600-8881-1-git-send-email-Julia.Lawall@lip6.fr>
[not found] ` <1447673600-8881-2-git-send-email-Julia.Lawall@lip6.fr>
[not found] ` <20151117013830.GV8456@google.com>
[not found] ` <alpine.DEB.2.02.1511170711340.2045@localhost6.localdomain6>
[not found] ` <20151117174430.GA8456@google.com>
[not found] ` <alpine.DEB.2.10.1511171846120.2455@hadrien>
[not found] ` <20151117183036.GF8456@google.com>
[not found] ` <alpine.DEB.2.02.1511172327030.2121@localhost6.localdomain6>
[not found] ` <alpine.DEB.2.02.1511172327030.2121-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>
2015-11-18 19:05 ` device_node lifetime (was: Re: [PATCH 1/7] phy: brcmstb-sata: add missing of_node_put) Brian Norris
[not found] ` <20151118190500.GE140057-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2015-11-18 20:39 ` Julia Lawall
2015-11-19 18:44 ` Rob Herring
2015-11-19 19:14 ` Russell King - ARM Linux
2015-11-19 22:58 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).