All of lore.kernel.org
 help / color / mirror / Atom feed
From: pawel.moll@arm.com (Pawel Moll)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] of: Keep track of populated platform devices
Date: Thu, 15 May 2014 16:08:53 +0100	[thread overview]
Message-ID: <1400166533.3672.3.camel@hornet> (raw)
In-Reply-To: <20140514105657.CF966C4153D@trevor.secretlab.ca>

On Wed, 2014-05-14 at 11:56 +0100, Grant Likely wrote:
> > Agreed, but, unless I'm missing some fundamental issue, I believe we can
> > solve the flag clearing problem in a generic way:
> > 
> > --8<-----------
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 3cf61a1..0f489fb 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/amba/bus.h>
> >  #include <linux/sizes.h>
> > +#include <linux/of_device.h>
> >  
> >  #include <asm/irq.h>
> >  
> > @@ -268,6 +269,7 @@ static void amba_device_release(struct device *dev)
> >  {
> >  	struct amba_device *d = to_amba_device(dev);
> >  
> > +	of_device_node_put(dev);
> >  	if (d->res.parent)
> >  		release_resource(&d->res);
> >  	kfree(d);
> > diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> > index ef37021..fd14d46 100644
> > --- a/include/linux/of_device.h
> > +++ b/include/linux/of_device.h
> > @@ -41,6 +41,8 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env
> >  
> >  static inline void of_device_node_put(struct device *dev)
> >  {
> > +	if (dev->of_node)
> > +		of_node_clear_flag(dev->of_node, OF_POPULATED);
> >  	of_node_put(dev->of_node);
> >  }
> > --8<-----------
> > 
> > This will work even if one manually unregisters a DT-originating device,
> > because of_device_node_put() would be called in both amba and platform
> > device (kobj) release path.
> 
> Doing it that way will also catch platform_devices that were created
> apart from of_platform_populate() and manually attached to an of_node,
> which is done some times. It will also break whenever multiple devices
> reference the same node if they call of_device_node_put().
> 
> For example, a platform device providing an i2c bus will have a child
> device that represents the i2c bus, and that i2c device will point to
> the same node.

Yes, can I see it doing this. Without 

> The flag clear really does need to be kept in the
> platform_device_unpopulate path because it is only to be used internally
> by of_platform_{populate,depopulate}

Ok, will move it there.
 
> > By the way, the fact that today amba_device_release() doesn't do
> > of_device_node_put() seems like a bug to me? (node's reference counter
> > won't be decremented)
> 
> Yes, that is a bug. I want to get some unittests added to the DT code to
> verify that reference counts are being tracked correctly. Right now it
> is an utter mess.

Ok. If I move the flag, this bit is not longer an integral part of the
patch so will split it into a separate one, but merge the _depopulate()
one instead.

Thanks!

Pawe?

WARNING: multiple messages have this Message-ID (diff)
From: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
To: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	"linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
Subject: Re: [PATCH v2] of: Keep track of populated platform devices
Date: Thu, 15 May 2014 16:08:53 +0100	[thread overview]
Message-ID: <1400166533.3672.3.camel@hornet> (raw)
In-Reply-To: <20140514105657.CF966C4153D-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>

On Wed, 2014-05-14 at 11:56 +0100, Grant Likely wrote:
> > Agreed, but, unless I'm missing some fundamental issue, I believe we can
> > solve the flag clearing problem in a generic way:
> > 
> > --8<-----------
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 3cf61a1..0f489fb 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/amba/bus.h>
> >  #include <linux/sizes.h>
> > +#include <linux/of_device.h>
> >  
> >  #include <asm/irq.h>
> >  
> > @@ -268,6 +269,7 @@ static void amba_device_release(struct device *dev)
> >  {
> >  	struct amba_device *d = to_amba_device(dev);
> >  
> > +	of_device_node_put(dev);
> >  	if (d->res.parent)
> >  		release_resource(&d->res);
> >  	kfree(d);
> > diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> > index ef37021..fd14d46 100644
> > --- a/include/linux/of_device.h
> > +++ b/include/linux/of_device.h
> > @@ -41,6 +41,8 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env
> >  
> >  static inline void of_device_node_put(struct device *dev)
> >  {
> > +	if (dev->of_node)
> > +		of_node_clear_flag(dev->of_node, OF_POPULATED);
> >  	of_node_put(dev->of_node);
> >  }
> > --8<-----------
> > 
> > This will work even if one manually unregisters a DT-originating device,
> > because of_device_node_put() would be called in both amba and platform
> > device (kobj) release path.
> 
> Doing it that way will also catch platform_devices that were created
> apart from of_platform_populate() and manually attached to an of_node,
> which is done some times. It will also break whenever multiple devices
> reference the same node if they call of_device_node_put().
> 
> For example, a platform device providing an i2c bus will have a child
> device that represents the i2c bus, and that i2c device will point to
> the same node.

Yes, can I see it doing this. Without 

> The flag clear really does need to be kept in the
> platform_device_unpopulate path because it is only to be used internally
> by of_platform_{populate,depopulate}

Ok, will move it there.
 
> > By the way, the fact that today amba_device_release() doesn't do
> > of_device_node_put() seems like a bug to me? (node's reference counter
> > won't be decremented)
> 
> Yes, that is a bug. I want to get some unittests added to the DT code to
> verify that reference counts are being tracked correctly. Right now it
> is an utter mess.

Ok. If I move the flag, this bit is not longer an integral part of the
patch so will split it into a separate one, but merge the _depopulate()
one instead.

Thanks!

Paweł

--
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

  reply	other threads:[~2014-05-15 15:08 UTC|newest]

Thread overview: 96+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-28 17:57 [PATCH 00/10] Versatile Express changes for 3.16 Pawel Moll
2014-04-28 17:57 ` Pawel Moll
2014-04-28 17:57 ` [PATCH 01/10] of: Keep track of populated platform devices Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 18:02   ` Rob Herring
2014-04-28 18:02     ` Rob Herring
2014-04-28 18:02     ` Rob Herring
2014-04-29 12:56     ` Grant Likely
2014-04-29 12:56       ` Grant Likely
2014-04-29 12:56       ` Grant Likely
2014-04-30 11:48       ` Pawel Moll
2014-04-30 11:48         ` Pawel Moll
2014-04-30 11:48         ` Pawel Moll
2014-04-30 14:05         ` [PATCH v2] " Pawel Moll
2014-04-30 14:05           ` Pawel Moll
2014-04-30 15:22           ` Rob Herring
2014-04-30 15:22             ` Rob Herring
2014-05-01  9:26             ` Grant Likely
2014-05-01  9:26               ` Grant Likely
2014-05-01  9:43               ` Grant Likely
2014-05-01  9:43                 ` Grant Likely
2014-05-07 14:37                 ` Pawel Moll
2014-05-07 14:37                   ` Pawel Moll
2014-05-14 10:56                   ` Grant Likely
2014-05-14 10:56                     ` Grant Likely
2014-05-15 15:08                     ` Pawel Moll [this message]
2014-05-15 15:08                       ` Pawel Moll
2014-04-28 17:57 ` [lm-sensors] [PATCH 02/10] mfd: vexpress: Convert custom func API to regmap Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-29 22:21   ` [lm-sensors] " Mark Brown
2014-04-29 22:21     ` Mark Brown
2014-04-29 22:21     ` Mark Brown
2014-04-30 13:58   ` [lm-sensors] " Lee Jones
2014-04-30 13:58     ` Lee Jones
2014-04-30 13:58     ` Lee Jones
2014-04-30 14:13     ` [lm-sensors] " Pawel Moll
2014-04-30 14:13       ` Pawel Moll
2014-04-30 14:13       ` Pawel Moll
2014-04-30 14:29       ` [lm-sensors] " Lee Jones
2014-04-30 14:29         ` Lee Jones
2014-04-30 14:29         ` Lee Jones
2014-04-30 14:38         ` [lm-sensors] " Pawel Moll
2014-04-30 14:38           ` Pawel Moll
2014-04-30 14:38           ` Pawel Moll
2014-04-30 16:01   ` [lm-sensors] [PATCH v2] " Pawel Moll
2014-04-30 16:01     ` Pawel Moll
2014-04-30 16:01     ` Pawel Moll
2014-04-30 17:05     ` [lm-sensors] " Guenter Roeck
2014-04-30 17:05       ` Guenter Roeck
2014-04-30 17:05       ` Guenter Roeck
2014-05-01 18:58     ` [lm-sensors] " Mike Turquette
2014-05-01 18:58       ` Mike Turquette
2014-05-01 18:58       ` Mike Turquette
2014-04-28 17:57 ` [PATCH 03/10] mfd: syscon: Add platform data with a regmap config name Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 17:57 ` [PATCH 04/10] mfd: vexpress: Define the device as MFD cells Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-05-09 11:24   ` Lee Jones
2014-05-09 11:24     ` Lee Jones
2014-04-28 17:57 ` [PATCH 05/10] clk: versatile: Split config options for sp810 and vexpress_osc Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-05-01 18:55   ` Mike Turquette
2014-05-01 18:55     ` Mike Turquette
2014-04-28 17:57 ` [PATCH 06/10] clocksource: Sched clock source for Versatile Express Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 17:57 ` [PATCH 07/10] ARM: vexpress: remove redundant vexpress_dt_cpus_num to get cpu count Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 17:57 ` [PATCH 08/10] ARM: vexpress: Simplify SMP operations for DT-powered system Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 17:57 ` [PATCH 09/10] ARM: vexpress: move HBI check to sysreg driver Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-30 14:02   ` Lee Jones
2014-04-30 14:02     ` Lee Jones
2014-04-28 17:57 ` [lm-sensors] [PATCH 10/10] hwmon: vexpress: Use devm helper for hwmon device registration Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 17:57   ` Pawel Moll
2014-04-28 22:59   ` [lm-sensors] " Guenter Roeck
2014-04-28 22:59     ` Guenter Roeck
2014-04-28 22:59     ` Guenter Roeck
2014-04-30 15:16     ` [lm-sensors] " Pawel Moll
2014-04-30 15:16       ` Pawel Moll
2014-04-30 15:16       ` Pawel Moll
2014-04-30 15:27       ` [lm-sensors] " Guenter Roeck
2014-04-30 15:27         ` Guenter Roeck
2014-04-30 15:27         ` Guenter Roeck
2014-04-30 15:33         ` [lm-sensors] " Pawel Moll
2014-04-30 15:33           ` Pawel Moll
2014-04-30 15:33           ` Pawel Moll
     [not found] <Message-ID: <1399473437.3706.25.camel@hornet>
2014-05-13 11:48 ` [PATCH v3] of: Keep track of populated platform devices Pawel Moll
2014-05-13 11:48   ` Pawel Moll
2014-05-13 12:24   ` Rob Herring
2014-05-13 12:24     ` Rob Herring
2014-05-14 11:05     ` Grant Likely
2014-05-14 11:05       ` Grant Likely

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1400166533.3672.3.camel@hornet \
    --to=pawel.moll@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.