Re: [PATCH 1/3] driver core: generalize driver_override in struct device
Geert Uytterhoeven <[email protected]>
| Newsgroups | gmane.linux.ports.arm.msm,gmane.linux.kernel,gmane.linux.kernel.hwmon,gmane.linux.sound,gmane.linux.ports.sh.devel |
|---|---|
| Message-ID | <CAMuHMdUi3uNoDJ67WkNSKn=BD1F7D1Ot=gz4TwFPvaaYmKaNcg@mail.gmail.com> |
Hi Danilo, On Mon, 2 Mar 2026 at 01:28, Danilo Krummrich <[email protected]> wrote: > 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. > > 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/[email protected]/ [2] > Signed-off-by: Danilo Krummrich <[email protected]> Thanks for your patch! > --- 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); > + The newline case below is is basically the same case as the empty string. Hence if you would move the newline check here: if (len) { cp = strnchr(s, len, '\n'); if (cp) len = cp - s; } then the "cp != s" check below is no longer needed. > + 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; > + } Also, this block can be eliminated completely... > + > + cp = strnchr(s, len, '\n'); > + if (cp) > + len = cp - s; > + > + new = kstrndup(s, len, GFP_KERNEL); > + if (!new) > + return -ENOMEM; ... by pre-initializing new to NULL, and making the allocation of new conditional on len being non-zero. > + > + 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 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds