On Thu Feb 20, 2025 at 5:19 PM CET, Greg Kroah-Hartman wrote:
On Thu, Feb 20, 2025 at 04:46:59PM +0100, Théo Lebrun wrote:
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.
Many people got this wrong when we designed busses, it's not unique. But we should learn from our mistakes where we can :)
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.
I don't know which quote, sorry.
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.
The fragility of attempting to match a driver to a device purely by a name is a very week part of using platform devices.
I never said the opposite, and I agree. However the mechanism exists and I was focused on not breaking it.
Why would a driver change the device name? It's been given to the driver to "bind to" not to change its name. That shouldn't be ok, fix those drivers.
I do get the argument that devices shouldn't change device names. I'll take the devil's advocate and give at least one argument FOR allowing changing names: prettier names, especially as device names leak into userspace through pseudo filesystems.
If we agree that device names shouldn't be changed one a device is matched with a driver, then (1) we can remove the pdev->name field and (2) `dev_set_name()` should warn when used too late.
Turn the implicit explicit.
diff --git a/drivers/base/core.c b/drivers/base/core.c index 5a1f05198114..3532b068e32d 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3462,10 +3462,13 @@ static void device_remove_class_symlinks(struct device *dev) int dev_set_name(struct device *dev, const char *fmt, ...) { va_list vargs; int err;
+ if (dev_WARN_ONCE(dev, dev->driver, "device name is static once matched")) + return -EPERM; + va_start(vargs, fmt); err = kobject_set_name_vargs(&dev->kobj, fmt, vargs); va_end(vargs); return err; }
(Unsure about the exact error code to return.)
[...]
Do we have examples today of platform drivers that like to rename devices? I did a quick search and couldn't find any in-tree, but I might have missed some.
The cover letter expands on the quest for those drivers:
On Tue Feb 18, 2025 at 12:00 PM CET, Théo Lebrun wrote:
Out of the 37 drivers that deal with platform devices and do a dev_set_name() call, only one might be affected. That driver is loongson-i2s-plat [0]. All other dev_set_name() calls are on children devices created on the spot. The issue was found on downstream kernels and we don't have what it takes to test loongson-i2s-plat.
[...]
⟩ # Finding potential trouble-makers: ⟩ git grep -l 'struct platform_device' | xargs grep -l dev_set_name
[...]
[...]
Or if this really is an issue, let's fix OF to not use the platform bus and have it's own bus for stuff like this.
That used to exist! I cannot see how it could be a good idea to reintroduce the distinction though.
commit eca3930163ba8884060ce9d9ff5ef0d9b7c7b00f Author: Grant Likely grant.likely@secretlab.ca Date: Tue Jun 8 07:48:21 2010 -0600
of: Merge of_platform_bus_type with platform_bus_type
Thanks,
-- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com