Re: [PATCH 1/3] driver core: generalize driver_override in struct device

[Armin Wolf <W_Armin@gmx.de>]

Am 02.03.26 um 01:25 schrieb Danilo Krummrich:

> Currently, there are 12 busses (including platform and PCI) that
> duplicate the driver_override logic for their individual devices.
>
> All of them seem to be prone to the bug described in [1].
>
> While this could be solved for every bus individually using a separate
> lock, solving this in the driver-core generically results in less (and
> cleaner) changes overall.
>
> Thus, move driver_override to struct device, provide corresponding
> accessors for busses and handle locking with a separate lock internally.
>
> In particular, add device_set_driver_override(),
> device_has_driver_override(), device_match_driver_override() and a
> helper, DEVICE_ATTR_DRIVER_OVERRIDE(), to declare the corresponding
> sysfs store() and show() callbacks.

Nice patch series, centralizing the handling of driver_override in the
device core removes a lot of duplicated code.

I wonder if we can also move DEVICE_ATTR_DRIVER_OVERRIDE() into the device
core by registering the associated sysfs attribute inside bus_add_device().

Bus types that honor driver_override could for example set a boolean flag
inside struct bus_type to tell the device core that driver_override is
supported.

However this is just a suggestion, the current patch series seems fine to me.

Thanks,
Armin Wolf

> Until all busses have migrated, keep driver_set_override() in place.
>
> Note that we can't use the device lock for the reasons described in [2].
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=220789 [1]
> Link: https://lore.kernel.org/driver-core/DGRGTIRHA62X.3RY09D9SOK77P@kernel.org/ [2]
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>   drivers/base/core.c    |  2 ++
>   drivers/base/dd.c      | 60 +++++++++++++++++++++++++++++++
>   include/linux/device.h | 81 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 143 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 791f9e444df8..a8cb90577d10 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2566,6 +2566,7 @@ static void device_release(struct kobject *kobj)
>   	else
>   		WARN(1, KERN_ERR "Device '%s' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
>   			dev_name(dev));
> +	kfree(dev->driver_override.name);
>   	kfree(p);
>   }
>   
> @@ -3159,6 +3160,7 @@ void device_initialize(struct device *dev)
>   	kobject_init(&dev->kobj, &device_ktype);
>   	INIT_LIST_HEAD(&dev->dma_pools);
>   	mutex_init(&dev->mutex);
> +	spin_lock_init(&dev->driver_override.lock);
>   	lockdep_set_novalidate_class(&dev->mutex);
>   	spin_lock_init(&dev->devres_lock);
>   	INIT_LIST_HEAD(&dev->devres_head);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0354f209529c..697e36e63cab 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -381,6 +381,66 @@ static void __exit deferred_probe_exit(void)
>   }
>   __exitcall(deferred_probe_exit);
>   
> +int __device_set_driver_override(struct device *dev, const char *s, size_t len)
> +{
> +	const char *new, *old;
> +	char *cp;
> +
> +	if (!s)
> +		return -EINVAL;
> +
> +	/*
> +	 * The stored value will be used in sysfs show callback (sysfs_emit()),
> +	 * which has a length limit of PAGE_SIZE and adds a trailing newline.
> +	 * Thus we can store one character less to avoid truncation during sysfs
> +	 * show.
> +	 */
> +	if (len >= (PAGE_SIZE - 1))
> +		return -EINVAL;
> +
> +	/*
> +	 * Compute the real length of the string in case userspace sends us a
> +	 * bunch of \0 characters like python likes to do.
> +	 */
> +	len = strlen(s);
> +
> +	if (!len) {
> +		/* Empty string passed - clear override */
> +		spin_lock(&dev->driver_override.lock);
> +		old = dev->driver_override.name;
> +		dev->driver_override.name = NULL;
> +		spin_unlock(&dev->driver_override.lock);
> +		kfree(old);
> +
> +		return 0;
> +	}
> +
> +	cp = strnchr(s, len, '\n');
> +	if (cp)
> +		len = cp - s;
> +
> +	new = kstrndup(s, len, GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
> +
> +	spin_lock(&dev->driver_override.lock);
> +	old = dev->driver_override.name;
> +	if (cp != s) {
> +		dev->driver_override.name = new;
> +		spin_unlock(&dev->driver_override.lock);
> +	} else {
> +		/* "\n" passed - clear override */
> +		dev->driver_override.name = NULL;
> +		spin_unlock(&dev->driver_override.lock);
> +
> +		kfree(new);
> +	}
> +	kfree(old);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(__device_set_driver_override);
> +
>   /**
>    * device_is_bound() - Check if device is bound to a driver
>    * @dev: device to check
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 0be95294b6e6..4599156d5cbd 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -266,6 +266,33 @@ ssize_t device_show_string(struct device *dev, struct device_attribute *attr,
>   	struct dev_ext_attribute dev_attr_##_name = \
>   		{ __ATTR(_name, (_mode) & ~0222, device_show_string, NULL), (_var) }
>   
> +/**
> + * DEVICE_ATTR_DRIVER_OVERRIDE - Define sysfs driver_override attribute callbacks
> + *
> + * Generates the standard driver_override_show() and driver_override_store()
> + * sysfs callbacks and the static DEVICE_ATTR_RW(driver_override) declaration.
> + */
> +#define DEVICE_ATTR_DRIVER_OVERRIDE()						\
> +static ssize_t driver_override_store(struct device *dev,			\
> +				     struct device_attribute *attr,		\
> +				     const char *buf, size_t count)		\
> +{										\
> +	int ret;								\
> +										\
> +	ret = __device_set_driver_override(dev, buf, count);			\
> +	if (ret)								\
> +		return ret;							\
> +										\
> +	return count;								\
> +}										\
> +static ssize_t driver_override_show(struct device *dev,				\
> +				    struct device_attribute *attr, char *buf)	\
> +{										\
> +	guard(spinlock)(&dev->driver_override.lock);				\
> +	return sysfs_emit(buf, "%s\n", dev->driver_override.name);		\
> +}										\
> +static DEVICE_ATTR_RW(driver_override)
> +
>   #define DEVICE_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
>   	struct device_attribute dev_attr_##_name =		\
>   		__ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> @@ -483,6 +510,8 @@ struct device_physical_location {
>    * 		on.  This shrinks the "Board Support Packages" (BSPs) and
>    * 		minimizes board-specific #ifdefs in drivers.
>    * @driver_data: Private pointer for driver specific info.
> + * @driver_override: Driver name to force a match.  Do not touch directly; use
> + *		     device_set_driver_override() instead.
>    * @links:	Links to suppliers and consumers of this device.
>    * @power:	For device power management.
>    *		See Documentation/driver-api/pm/devices.rst for details.
> @@ -576,6 +605,10 @@ struct device {
>   					   core doesn't touch it */
>   	void		*driver_data;	/* Driver data, set and get with
>   					   dev_set_drvdata/dev_get_drvdata */
> +	struct {
> +		const char	*name;
> +		spinlock_t	lock;
> +	} driver_override;
>   	struct mutex		mutex;	/* mutex to synchronize calls to
>   					 * its driver.
>   					 */
> @@ -701,6 +734,54 @@ struct device_link {
>   
>   #define kobj_to_dev(__kobj)	container_of_const(__kobj, struct device, kobj)
>   
> +int __device_set_driver_override(struct device *dev, const char *s, size_t len);
> +
> +/**
> + * device_set_driver_override() - Helper to set or clear driver override.
> + * @dev: Device to change
> + * @s: NUL-terminated string, new driver name to force a match, pass empty
> + *     string to clear it ("" or "\n", where the latter is only for sysfs
> + *     interface).
> + *
> + * Helper to set or clear driver override of a device.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +static inline int device_set_driver_override(struct device *dev, const char *s)
> +{
> +	return __device_set_driver_override(dev, s, strlen(s));
> +}
> +
> +/**
> + * device_has_driver_override() - Check if a driver override has been set.
> + * @dev: device to check
> + *
> + * Returns true if a driver override has been set for this device.
> + */
> +static inline bool device_has_driver_override(struct device *dev)
> +{
> +	guard(spinlock)(&dev->driver_override.lock);
> +	return !!dev->driver_override.name;
> +}
> +
> +/**
> + * device_match_driver_override() - Match a driver against the device's driver_override.
> + * @dev: device to check
> + * @drv: driver to match against
> + *
> + * Returns > 0 if a driver override is set and matches the given driver, 0 if a
> + * driver override is set but does not match, or < 0 if a driver override is not
> + * set at all.
> + */
> +static inline int device_match_driver_override(struct device *dev,
> +					       const struct device_driver *drv)
> +{
> +	guard(spinlock)(&dev->driver_override.lock);
> +	if (dev->driver_override.name)
> +		return !strcmp(dev->driver_override.name, drv->name);
> +	return -1;
> +}
> +
>   /**
>    * device_iommu_mapped - Returns true when the device DMA is translated
>    *			 by an IOMMU