On Thu Feb 20, 2025 at 3:06 PM CET, Greg Kroah-Hartman wrote:
On Thu, Feb 20, 2025 at 02:31:29PM +0100, Théo Lebrun wrote:
On Thu Feb 20, 2025 at 1:41 PM CET, Greg Kroah-Hartman wrote:
On Tue, Feb 18, 2025 at 12:00:11PM +0100, Théo Lebrun wrote:
The solution proposed is to add a flag to platform_device that tells if it is responsible for freeing its name. We can then duplicate the device name inside of_device_add() instead of copying the pointer.
Ick.
What is done elsewhere?
- Platform bus code does a copy of the argument name that is stored alongside the struct platform_device; see platform_device_alloc()[1].
- Other busses duplicate the device name; either through a dynamic allocation [2] or through an array embedded inside devices [3].
- Some busses don't have a separate name; when they want a name they take it from the device [4].
Really ick.
Let's do the right thing here and just get rid of the name pointer entirely in struct platform_device please. Isn't that the correct thing that way the driver core logic will work properly for all of this.
I would agree, if it wasn't for this consideration that is found in the commit message [0]:
What, that the of code is broken? Then it should be fixed, why does it need a pointer to a name at all anyway? It shouldn't be needed there either.
I cannot guess why it originally has a separate pdev->name field. All I can tell you is a good reason to have one, as quoted below.
It is important to duplicate! pdev->name must not change to make sure the platform_match() return value is stable over time. If we updated pdev->name alongside dev->name, once a device probes and changes its name then the platform_match() return value would change.
I'd be fine sending a V2 that removes the field *and the fallback* [1], but I don't have the full scope in mind to know what would become broken.
The fallback will not need to be removed, properly point to the name of the device and it should work correctly.
No, it will not work correctly, as the above quote indicates.
Let's assume we remove the field, this situation would be broken: - OF allocates platform devices and gives them names. - A device matches with a driver, which gets probed. - During the probe, driver does a dev_set_name(). - Afterwards, the upcoming platform_match() against other drivers are called with another device name.
We should be safe as there are guardraids to not probe twice a device, see __driver_probe_device() that checks dev->driver is NULL. But it isn't a situation we should be in.
Another broken situation: - OF allocates platform devices and gives them names. - A device matches with a driver, which gets probed based on its name. - During the probe, driver does a dev_set_name(). - Module is removed. - Module is re-added, the (driver, device) pair don't end up matching again because the device name changed.
I might be missing other edge-cases.
Conclusion: we need a constant name for platform devices as we want the return value of platform_match() to stay stable across time.
Regards,
-- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com