Patchwork [v2,01/22,HACK] of: dev_node has struct device pointer

login
register
mail settings
Submitter Hiroshi Doyu
Date July 5, 2013, 10:44 a.m.
Message ID <1373021097-32420-2-git-send-email-hdoyu@nvidia.com>
Download mbox | patch
Permalink /patch/257080/
State Superseded, archived
Headers show

Comments

Hiroshi Doyu - July 5, 2013, 10:44 a.m.
To prevent of_platform_populate() from trying to populate duplicate
devices if a device has been already populated.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
Need to take care of early_platform_devices, or alternative solution.
---
 drivers/of/base.c     |   23 +++++++++++++++++++++++
 drivers/of/platform.c |    8 ++++++++
 include/linux/of.h    |   16 ++++++++++++++++
 3 files changed, 47 insertions(+)
Stephen Warren - July 16, 2013, 10:57 p.m.
On 07/05/2013 04:44 AM, Hiroshi Doyu wrote:
> To prevent of_platform_populate() from trying to populate duplicate
> devices if a device has been already populated.

You need to send drivers/of patches to the DT maintainer and
devicetree-discuss mailing list.

> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
> Need to take care of early_platform_devices, or alternative solution.

I assume that's a TODO item... Are you planning on fleshing out this
patch to address that issue?

> diff --git a/drivers/of/base.c b/drivers/of/base.c

> +struct device *of_get_device(const struct device_node *node)
> +{
> +	struct device *dev;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	dev = node->dev;
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);

The read (and write in of_set_device()) aren't atomic already? I guess
perhaps they aren't necessarily. It sure seems like some higher-level
locking is needed in of_platform_create_pdata() though, since there's
quite a window there between the get() and set() calls. Or, is there
already some locking in place above that, in which case, I 'm not sure
why there's a need for this level of locking here...

> diff --git a/drivers/of/platform.c b/drivers/of/platform.c

> +	tmp = of_get_device(np);
> +	if (tmp) {
> +		dev_info(tmp, "Already populated\n");
> +		return to_platform_device(tmp);
> +	}

Did you check all callers of this function to make sure that they also
don't do any kind of double-processing?


> diff --git a/include/linux/of.h b/include/linux/of.h

> @@ -60,6 +60,9 @@ struct device_node {
>  	struct	kref kref;
>  	unsigned long _flags;
>  	void	*data;
> +
> +	struct device *dev;		/* Set only after populated */
> +
>  #if defined(CONFIG_SPARC)

Are the extra blank lines needed?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding - July 16, 2013, 11:16 p.m.
On Tue, Jul 16, 2013 at 04:57:03PM -0600, Stephen Warren wrote:
> On 07/05/2013 04:44 AM, Hiroshi Doyu wrote:
> > To prevent of_platform_populate() from trying to populate duplicate
> > devices if a device has been already populated.
> 
> You need to send drivers/of patches to the DT maintainer and
> devicetree-discuss mailing list.
> 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> > Need to take care of early_platform_devices, or alternative solution.
> 
> I assume that's a TODO item... Are you planning on fleshing out this
> patch to address that issue?

There was some more discussion about it, which can be found in the
following thread:

	https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036934.html

That's one of the last messages and it should have most of the
background. If not you may have to walk up the thread for more context.
In a nutshell I raised the question as to why we can't simply call
of_platform_populate() earlier and side-step all the workarounds that
have found their way into the kernel to side-step the issue of their not
being a struct device associated with the struct device_node.

If of_platform_populate() can be made to run early, then we can restore
a whole lot of consistency in how "early" drivers on an OF kernel work.

Thierry
Hiroshi Doyu - July 29, 2013, 10:13 a.m.
Thierry Reding <thierry.reding@gmail.com> wrote @ Wed, 17 Jul 2013 01:16:30 +0200:

> * PGP Signed by an unknown key
> 
> On Tue, Jul 16, 2013 at 04:57:03PM -0600, Stephen Warren wrote:
> > On 07/05/2013 04:44 AM, Hiroshi Doyu wrote:
> > > To prevent of_platform_populate() from trying to populate duplicate
> > > devices if a device has been already populated.
> > 
> > You need to send drivers/of patches to the DT maintainer and
> > devicetree-discuss mailing list.
> > 
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > ---
> > > Need to take care of early_platform_devices, or alternative solution.
> > 
> > I assume that's a TODO item... Are you planning on fleshing out this
> > patch to address that issue?
>
> There was some more discussion about it, which can be found in the
> following thread:
> 
> 	https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036934.html
> 
> That's one of the last messages and it should have most of the
> background. If not you may have to walk up the thread for more context.
> In a nutshell I raised the question as to why we can't simply call
> of_platform_populate() earlier and side-step all the workarounds that
> have found their way into the kernel to side-step the issue of their not
> being a struct device associated with the struct device_node.

This is the thread which Lorenzo started originally. He may have some
plan to implement/proceed with this, Lorenzo?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi - July 29, 2013, 1:13 p.m.
Hi Hiroshi,

On Mon, Jul 29, 2013 at 11:13:44AM +0100, Hiroshi Doyu wrote:
> Thierry Reding <thierry.reding@gmail.com> wrote @ Wed, 17 Jul 2013 01:16:30 +0200:
> 
> > * PGP Signed by an unknown key
> > 
> > On Tue, Jul 16, 2013 at 04:57:03PM -0600, Stephen Warren wrote:
> > > On 07/05/2013 04:44 AM, Hiroshi Doyu wrote:
> > > > To prevent of_platform_populate() from trying to populate duplicate
> > > > devices if a device has been already populated.
> > > 
> > > You need to send drivers/of patches to the DT maintainer and
> > > devicetree-discuss mailing list.
> > > 
> > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > > ---
> > > > Need to take care of early_platform_devices, or alternative solution.
> > > 
> > > I assume that's a TODO item... Are you planning on fleshing out this
> > > patch to address that issue?
> >
> > There was some more discussion about it, which can be found in the
> > following thread:
> > 
> > 	https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036934.html
> > 
> > That's one of the last messages and it should have most of the
> > background. If not you may have to walk up the thread for more context.
> > In a nutshell I raised the question as to why we can't simply call
> > of_platform_populate() earlier and side-step all the workarounds that
> > have found their way into the kernel to side-step the issue of their not
> > being a struct device associated with the struct device_node.
> 
> This is the thread which Lorenzo started originally. He may have some
> plan to implement/proceed with this, Lorenzo?

I was trying to raise the issue and apparently I succeeded. For now,
I put in place yet-another-temporary-solution to the problem in one
of the drivers I need for power management, but certainly the problem is
not solved, and I can't allocate time to do it myself unfortunately.

I am more than happy to review patches though.

Thanks,
Lorenzo

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5c54279..99062dd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -230,6 +230,29 @@  const void *of_get_property(const struct device_node *np, const char *name,
 }
 EXPORT_SYMBOL(of_get_property);
 
+struct device *of_get_device(const struct device_node *node)
+{
+	struct device *dev;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	dev = node->dev;
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	return dev;
+}
+EXPORT_SYMBOL(of_get_device);
+
+void of_set_device(struct device_node *node, struct device *dev)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	node->dev = dev;
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+}
+EXPORT_SYMBOL(of_set_device);
+
 /** Checks if the given "compat" string matches one of the strings in
  * the device's "compatible" property
  */
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index e0a6514..a8f6b09 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -203,10 +203,17 @@  struct platform_device *of_platform_device_create_pdata(
 					struct device *parent)
 {
 	struct platform_device *dev;
+	struct device *tmp;
 
 	if (!of_device_is_available(np))
 		return NULL;
 
+	tmp = of_get_device(np);
+	if (tmp) {
+		dev_info(tmp, "Already populated\n");
+		return to_platform_device(tmp);
+	}
+
 	dev = of_device_alloc(np, bus_id, parent);
 	if (!dev)
 		return NULL;
@@ -228,6 +235,7 @@  struct platform_device *of_platform_device_create_pdata(
 		return NULL;
 	}
 
+	of_set_device(np, &dev->dev);
 	return dev;
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 1fd08ca..b548522 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -60,6 +60,9 @@  struct device_node {
 	struct	kref kref;
 	unsigned long _flags;
 	void	*data;
+
+	struct device *dev;		/* Set only after populated */
+
 #if defined(CONFIG_SPARC)
 	const char *path_component_name;
 	unsigned int unique_id;
@@ -268,6 +271,8 @@  extern const void *of_get_property(const struct device_node *node,
 				int *lenp);
 #define for_each_property_of_node(dn, pp) \
 	for (pp = dn->properties; pp != NULL; pp = pp->next)
+extern struct device *of_get_device(const struct device_node *node);
+extern void of_set_device(struct device_node *node, struct device *dev);
 
 extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
@@ -459,6 +464,17 @@  static inline const void *of_get_property(const struct device_node *node,
 	return NULL;
 }
 
+static inline struct device *of_get_device(const struct device_node *node)
+{
+	return NULL;
+}
+
+static inline void of_set_device(const struct device_node *node,
+				 struct device *dev);
+{
+	return -ENOSYS;
+}
+
 static inline int of_property_read_u64(const struct device_node *np,
 				       const char *propname, u64 *out_value)
 {