[PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[Andy Shevchenko]

Insulate of_device_alloc() and of_amba_device_create() from possible
changes to fwnode_handle implementation by using device_set_node()
instead of open-coding dev->dev.fwnode assignments.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/of/platform.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 78ae84187449..051e29b7ad2b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -140,8 +140,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
 		}
 	}
 
-	dev->dev.of_node = of_node_get(np);
-	dev->dev.fwnode = &np->fwnode;
+	/* setup generic device info */
+	device_set_node(&dev->dev, of_fwnode_handle(np));
 	dev->dev.parent = parent ? : &platform_bus;
 
 	if (bus_id)
@@ -239,8 +239,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
 	dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
 
 	/* setup generic device info */
-	dev->dev.of_node = of_node_get(node);
-	dev->dev.fwnode = &node->fwnode;
+	device_set_node(&dev->dev, of_fwnode_handle(node));
 	dev->dev.parent = parent ? : &platform_bus;
 	dev->dev.platform_data = platform_data;
 	if (bus_id)
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote:
> Insulate of_device_alloc() and of_amba_device_create() from possible
> changes to fwnode_handle implementation by using device_set_node()
> instead of open-coding dev->dev.fwnode assignments.

Side note. When I preparing this change I have noticed a lot of

	dev_set_name(... dev_name())

in the code which seems to me problematic in two ways:
1) (minor) the dev_set_name() may fail, no checks are there;
2) (major?) the above construction leaks memory.

Is it on purpose (esp. second point)? If no, can it be fixed?
Note, I'm not familiar with OF platform code, so I would help
reviewing the change, but that's it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote:
> > Insulate of_device_alloc() and of_amba_device_create() from possible
> > changes to fwnode_handle implementation by using device_set_node()
> > instead of open-coding dev->dev.fwnode assignments.
> 
> Side note. When I preparing this change I have noticed a lot of
> 
> 	dev_set_name(... dev_name())

Plus

	dev_set_name(dev, ...)
	...
	dev_set_name(dev, ...)

on the same device will also give a memory leak.

> in the code which seems to me problematic in two ways:
> 1) (minor) the dev_set_name() may fail, no checks are there;
> 2) (major?) the above construction leaks memory.
> 
> Is it on purpose (esp. second point)? If no, can it be fixed?
> Note, I'm not familiar with OF platform code, so I would help
> reviewing the change, but that's it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 06:01:17PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote:
> > > Insulate of_device_alloc() and of_amba_device_create() from possible
> > > changes to fwnode_handle implementation by using device_set_node()
> > > instead of open-coding dev->dev.fwnode assignments.
> > 
> > Side note. When I preparing this change I have noticed a lot of
> > 
> > 	dev_set_name(... dev_name())
> 
> Plus
> 
> 	dev_set_name(dev, ...)
> 	...
> 	dev_set_name(dev, ...)
> 
> on the same device will also give a memory leak.

Ah, seems false alarm, the kobject_set_name_vargs() frees the old one.
Sorry for the noise for second point. But the first one still applies.

> > in the code which seems to me problematic in two ways:
> > 1) (minor) the dev_set_name() may fail, no checks are there;
> > 2) (major?) the above construction leaks memory.
> > 
> > Is it on purpose (esp. second point)? If no, can it be fixed?
> > Note, I'm not familiar with OF platform code, so I would help
> > reviewing the change, but that's it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[Rob Herring]

On Thu, Jun 15, 2023 at 06:03:52PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 15, 2023 at 06:01:17PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote:
> > > > Insulate of_device_alloc() and of_amba_device_create() from possible
> > > > changes to fwnode_handle implementation by using device_set_node()
> > > > instead of open-coding dev->dev.fwnode assignments.
> > > 
> > > Side note. When I preparing this change I have noticed a lot of
> > > 
> > > 	dev_set_name(... dev_name())
> > 
> > Plus
> > 
> > 	dev_set_name(dev, ...)
> > 	...
> > 	dev_set_name(dev, ...)
> > 
> > on the same device will also give a memory leak.
> 
> Ah, seems false alarm, the kobject_set_name_vargs() frees the old one.
> Sorry for the noise for second point. But the first one still applies.
> 
> > > in the code which seems to me problematic in two ways:
> > > 1) (minor) the dev_set_name() may fail, no checks are there;

Is there anything besides a memory alloc failure? What will print a 
message already. Wouldn't we fail a bit later on when adding the 
device anyways?

In a rough count, 92 out of 500 cases check the return of 
dev_set_name().

Rob

Re: [PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[Rob Herring]

On Thu, 15 Jun 2023 17:52:43 +0300, Andy Shevchenko wrote:
> Insulate of_device_alloc() and of_amba_device_create() from possible
> changes to fwnode_handle implementation by using device_set_node()
> instead of open-coding dev->dev.fwnode assignments.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/of/platform.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 

Applied, thanks!

Re: [PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 10:44:52AM -0600, Rob Herring wrote:
> On Thu, Jun 15, 2023 at 06:03:52PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 15, 2023 at 06:01:17PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote:

...

> > > > in the code which seems to me problematic in two ways:
> > > > 1) (minor) the dev_set_name() may fail, no checks are there;
> 
> Is there anything besides a memory alloc failure? What will print a
> message already. Wouldn't we fail a bit later on when adding the
> device anyways?

I don't see how we fail. Any pointers?

> In a rough count, 92 out of 500 cases check the return of
> dev_set_name().

Yeah, because it was new finding about that. Some static analyzers complain
nowadays about this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] of/platform: Propagate firmware node by calling device_set_node()

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 08:13:52PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 15, 2023 at 10:44:52AM -0600, Rob Herring wrote:
> > On Thu, Jun 15, 2023 at 06:03:52PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 15, 2023 at 06:01:17PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote:

...

> > > > > in the code which seems to me problematic in two ways:
> > > > > 1) (minor) the dev_set_name() may fail, no checks are there;
> > 
> > Is there anything besides a memory alloc failure? What will print a
> > message already. Wouldn't we fail a bit later on when adding the
> > device anyways?
> 
> I don't see how we fail. Any pointers?

Okay, code in question:

        /* subsystems can specify simple device enumeration */
        if (!dev_name(dev) && dev->bus && dev->bus->dev_name)
                dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id);

        if (!dev_name(dev)) {
                error = -EINVAL;
                goto name_error;
        }

-- 
With Best Regards,
Andy Shevchenko