of/platform: Propagate firmware node by calling device_set_node()
PATCH 1/1 of/platform: Propagate firmware node by calling device_set_node()
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.
--- drivers/of/platform.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/of/platform.c +3 -4
@@ -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)
Signature · 1 line
2.40.0.1.gaa8946217a0b
Quoted 3 lines · Andy Shevchenko
Insulate
of_device_alloc()andof_amba_device_create()from possible changes tofwnode_handleimplementation by usingdevice_set_node()instead of open-codingdev->dev.fwnodeassignments.
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 8 lines · Andy Shevchenko
Quoted 3 lines · Andy Shevchenko
Insulate
of_device_alloc()andof_amba_device_create()from possible changes tofwnode_handleimplementation by usingdevice_set_node()instead of open-codingdev->dev.fwnodeassignments.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.
Quoted 7 lines
in the code which seems to me problematic in two ways:
1) (minor) thedev_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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · Andy Shevchenko
Quoted 3 lines · Andy Shevchenko
Insulate
of_device_alloc()andof_amba_device_create()from possible changes tofwnode_handleimplementation by usingdevice_set_node()instead of open-codingdev->dev.fwnodeassignments.Side note. When I preparing this change I have noticed a lot of
dev_set_name(... dev_name())
Quoted 7 lines
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.
Quoted 7 lines
Quoted 7 lines
in the code which seems to me problematic in two ways:
1) (minor) thedev_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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Rob Herring
maintainerQuoted 13 lines · Andy Shevchenko
Quoted 12 lines · Andy Shevchenko
Quoted 8 lines · Andy Shevchenko
Quoted 3 lines · Andy Shevchenko
Insulate
of_device_alloc()andof_amba_device_create()from possible changes tofwnode_handleimplementation by usingdevice_set_node()instead of open-codingdev->dev.fwnodeassignments.Side note. When I preparing this change I have noticed a lot of
dev_set_name(... dev_name())Plus
Quoted 11 lines
Quoted 5 lines
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.Quoted 2 lines
Quoted 2 lines
in the code which seems to me problematic in two ways:
1) (minor) thedev_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
Quoted 4 lines · Rob Herring
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Andy Shevchenko
On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote:
...
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
in the code which seems to me problematic in two ways:
1) (minor) thedev_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?
Quoted 2 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Rob Herring
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Andy Shevchenko
On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote:
...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
in the code which seems to me problematic in two ways:
1) (minor) thedev_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;
}Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 9 lines · Andy Shevchenko
Insulate
of_device_alloc()andof_amba_device_create()from possible changes tofwnode_handleimplementation by usingdevice_set_node()instead of open-codingdev->dev.fwnodeassignments.Signed-off-by Andy Shevchenko <[email protected]>--- drivers/of/platform.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Applied, thanks!