| Message ID | 1373021097-32420-2-git-send-email-hdoyu@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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?
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
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) {
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(+)