Currently the fwnode API and things that rely on it like fw_devlink will
not reliably work for devices created from DT since each subsystem that
creates devices must individually set dev->fwnode in addition to setting
dev->of_node, currently a number of subsystems don't do so. Ensure that
this can't get missed by setting fwnode from of_node if it's not
previously been set by the subsystem.
Reported-by: Saravana Kannan <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
*Very* minimally tested.
drivers/base/core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index d661ada1518f..658626bafd76 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2864,6 +2864,10 @@ int device_add(struct device *dev)
if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
set_dev_node(dev, dev_to_node(parent));
+ /* ensure that fwnode is set up */
+ if (IS_ENABLED(CONFIG_OF) && dev->of_node && !dev->fwnode)
+ dev->fwnode = of_fwnode_handle(dev->of_node);
+
/* first, register with generic layer. */
/* we require the name to be set before, and pass NULL */
error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
--
2.20.1
+Rob and Frank
On Fri, Nov 6, 2020 at 7:09 AM Mark Brown <[email protected]> wrote:
>
> Currently the fwnode API and things that rely on it like fw_devlink will
> not reliably work for devices created from DT since each subsystem that
> creates devices must individually set dev->fwnode in addition to setting
> dev->of_node, currently a number of subsystems don't do so. Ensure that
> this can't get missed by setting fwnode from of_node if it's not
> previously been set by the subsystem.
>
> Reported-by: Saravana Kannan <[email protected]>
> Signed-off-by: Mark Brown <[email protected]>
> ---
>
> *Very* minimally tested.
>
> drivers/base/core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d661ada1518f..658626bafd76 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2864,6 +2864,10 @@ int device_add(struct device *dev)
> if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
> set_dev_node(dev, dev_to_node(parent));
>
> + /* ensure that fwnode is set up */
> + if (IS_ENABLED(CONFIG_OF) && dev->of_node && !dev->fwnode)
> + dev->fwnode = of_fwnode_handle(dev->of_node);
> +
I don't think we should add more CONFIG_OF specific code in drivers/base/
If you want to do this in "one common place", then I think the way to
do this is have include/linux/of.h provide something like:
void of_set_device_of_node(dev, of_node)
{
dev->of_node = of_node;
dev->fw_node = &of_node->fwnode;
/* bunch of other housekeeping like setting OF_POPULATED and doing
proper of_node_get() */
// Passing NULL for of_node could undo all the above for dev->of_node.
}
And all the subsystems that create their own device from an of_node
should use of_set_device_of_node() to set the device's of_node. That
way, all this is done in a consistent manner across subsystems and
avoid all of the of_get/put() and OF_POPULATED set/clear strewn all
over the subsystems.
For example a bunch of subsystems don't do of_get()/of_put() correctly
when they det a device's of_node (I sent patches as I found them).
-Saravana
On Fri, Nov 06, 2020 at 11:09:17AM -0800, Saravana Kannan wrote:
> If you want to do this in "one common place", then I think the way to
> do this is have include/linux/of.h provide something like:
> void of_set_device_of_node(dev, of_node)
> {
> dev->of_node = of_node;
> dev->fw_node = &of_node->fwnode;
> /* bunch of other housekeeping like setting OF_POPULATED and doing
> proper of_node_get() */
> // Passing NULL for of_node could undo all the above for dev->of_node.
> }
That also sounds good, particularly if we have a coccinelle spatch or
some other mechanism that enforced the usage of the function where
appropriate, my main concern is making sure that we do something which
ensures that the boilerplate stuff is handled.
On Fri, Nov 6, 2020 at 11:23 AM Mark Brown <[email protected]> wrote:
>
> On Fri, Nov 06, 2020 at 11:09:17AM -0800, Saravana Kannan wrote:
>
> > If you want to do this in "one common place", then I think the way to
> > do this is have include/linux/of.h provide something like:
>
> > void of_set_device_of_node(dev, of_node)
> > {
> > dev->of_node = of_node;
> > dev->fw_node = &of_node->fwnode;
> > /* bunch of other housekeeping like setting OF_POPULATED and doing
> > proper of_node_get() */
> > // Passing NULL for of_node could undo all the above for dev->of_node.
> > }
>
> That also sounds good, particularly if we have a coccinelle spatch
I've never used coccinelle. But I can fix 5-10 easily findable ones
like i2c, platform, spi, slimbus, spmi, etc.
> or
> some other mechanism that enforced the usage of the function where
> appropriate, my main concern is making sure that we do something which
> ensures that the boilerplate stuff is handled.
Rob/Frank,
I spent an hour or more looking at this and there are many ways of
doing this. Wanted to know how much you wanted to do inside these
boilerplate functions.
This is the minimum we should do in these helper functions.
+/**
+ * of_unset_dev_of_node - Unset a device's of_node
+ * @dev - device to unset the of_node for
+ *
+ * Use this when you delete a device on which you had called
+ * of_set_dev_of_node() before.
+ */
+static inline void of_unset_dev_of_node(struct device *dev)
+{
+ struct device_node *node = dev->of_node
+
+ if (!node)
+ return;
+
+ dev->fwnode = NULL;
+ dev->of_node = NULL;
+ of_node_put(node);
+}
+
+/**
+ * of_set_dev_of_node - Set a device's of_node
+ * @dev - device to set the of_node for
+ * @node - the device_node that the device was constructed from
+ *
+ * Use this when you create a new device from a device_node. It takes care some
+ * of the housekeeping work that's necessary when you set a device's of_node.
+ *
+ * Use of_unset_dev_of_node() before you delete the device.
+ *
+ * Returns an error if the device already has its of_node set.
+ */
+static inline int of_set_dev_of_node(struct device *dev, struct
device_node *node)
+{
+ if (!node)
+ return 0;
+
+ if (WARN_ON(dev->of_node))
+ return -EBUSY;
+
+ of_node_get(node);
+ dev->of_node = node;
+ dev->fwnode = of_fwnode_handle(node);
+
+ return 0;
+}
But I also had another version that set/cleared OF_POPULATED. But that
meant of_device_alloc() will allocate the device before checking if
the node has already been populated (because I'd delete the check
that's already there and use the one rolled into these helper
functions). I think that inefficiency is okay because I don't think
"trying to populate an already populated node" would be a common
occurrence. But I wasn't sure how you'd feel about it.
Any preferences? Thoughts?
Additional context:
https://lore.kernel.org/lkml/[email protected]/
-Saravana
On Fri, Nov 06, 2020 at 05:55:10PM -0800, Saravana Kannan wrote:
> On Fri, Nov 6, 2020 at 11:23 AM Mark Brown <[email protected]> wrote:
> > That also sounds good, particularly if we have a coccinelle spatch
> I've never used coccinelle. But I can fix 5-10 easily findable ones
> like i2c, platform, spi, slimbus, spmi, etc.
The thought with coccinelle (or like I say some other mechanism) is that
we would have something around to catch any new users that should be
using the helper but aren't rather than issues with making the changes.
On Fri, Nov 6, 2020 at 1:09 PM Saravana Kannan <[email protected]> wrote:
>
> +Rob and Frank
>
> On Fri, Nov 6, 2020 at 7:09 AM Mark Brown <[email protected]> wrote:
> >
> > Currently the fwnode API and things that rely on it like fw_devlink will
> > not reliably work for devices created from DT since each subsystem that
> > creates devices must individually set dev->fwnode in addition to setting
> > dev->of_node, currently a number of subsystems don't do so. Ensure that
> > this can't get missed by setting fwnode from of_node if it's not
> > previously been set by the subsystem.
> >
> > Reported-by: Saravana Kannan <[email protected]>
> > Signed-off-by: Mark Brown <[email protected]>
> > ---
> >
> > *Very* minimally tested.
> >
> > drivers/base/core.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d661ada1518f..658626bafd76 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2864,6 +2864,10 @@ int device_add(struct device *dev)
> > if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
> > set_dev_node(dev, dev_to_node(parent));
> >
> > + /* ensure that fwnode is set up */
> > + if (IS_ENABLED(CONFIG_OF) && dev->of_node && !dev->fwnode)
> > + dev->fwnode = of_fwnode_handle(dev->of_node);
> > +
>
> I don't think we should add more CONFIG_OF specific code in drivers/base/
It's fwnode specific code because it's fwnode that needs it...
> If you want to do this in "one common place", then I think the way to
> do this is have include/linux/of.h provide something like:
> void of_set_device_of_node(dev, of_node)
> {
> dev->of_node = of_node;
> dev->fw_node = &of_node->fwnode;
> /* bunch of other housekeeping like setting OF_POPULATED and doing
> proper of_node_get() */
> // Passing NULL for of_node could undo all the above for dev->of_node.
> }
>
> And all the subsystems that create their own device from an of_node
> should use of_set_device_of_node() to set the device's of_node. That
> way, all this is done in a consistent manner across subsystems and
> avoid all of the of_get/put() and OF_POPULATED set/clear strewn all
> over the subsystems.
Perhaps a fwnode call in device_add instead that implements the above
and anything else needed for each type of fwnode. It might even be
possible with that to get rid of most of
of_platform_device_create_pdata() and of_device_add(). IIRC, those
were pretty much copies of the core code.
That would also be less fragile than having a coccinelle script.
Rob
On Mon, Nov 09, 2020 at 11:25:39AM -0600, Rob Herring wrote:
> On Fri, Nov 6, 2020 at 1:09 PM Saravana Kannan <[email protected]> wrote:
> > And all the subsystems that create their own device from an of_node
> > should use of_set_device_of_node() to set the device's of_node. That
> > way, all this is done in a consistent manner across subsystems and
> > avoid all of the of_get/put() and OF_POPULATED set/clear strewn all
> > over the subsystems.
> Perhaps a fwnode call in device_add instead that implements the above
> and anything else needed for each type of fwnode. It might even be
> possible with that to get rid of most of
I'd suggested doing it with a callout as well in the original thread
that sparked my proposal - my patch was just the most minimal
implementation to fix the current problem.
> of_platform_device_create_pdata() and of_device_add(). IIRC, those
> were pretty much copies of the core code.
> That would also be less fragile than having a coccinelle script.