| Message ID | 20221202224744.1447448-3-allenwebb@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add sysfs match-id modalias attribute for USB modules | expand |
Le 02/12/2022 à 23:47, Allen Webb a écrit : > When the modalias attribute is read, invoke a subsystem-specific > callback for each driver registered by the specific module. > > The intent of the new modalias attribute is to expose the > match-id-based modaliases to userspace for builtin and loaded kernel > modules. > > Signed-off-by: Allen Webb <allenwebb@google.com> > --- > include/linux/device/bus.h | 7 +++++ > kernel/module/sysfs.c | 57 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h > index 82a5583437099..cce0bedec63d9 100644 > --- a/include/linux/device/bus.h > +++ b/include/linux/device/bus.h > @@ -61,6 +61,10 @@ struct fwnode_handle; > * this bus. > * @dma_cleanup: Called to cleanup DMA configuration on a device on > * this bus. > + * @drv_to_modalias: Called to convert the matching IDs in a > + * struct device_driver to their corresponding modaliases. > + * Note that the struct device_driver is expected to belong > + * to this bus. > * @pm: Power management operations of this bus, callback the specific > * device driver's pm-ops. > * @iommu_ops: IOMMU specific operations for this bus, used to attach IOMMU > @@ -107,6 +111,9 @@ struct bus_type { > int (*dma_configure)(struct device *dev); > void (*dma_cleanup)(struct device *dev); > > + ssize_t (*drv_to_modalias)(struct device_driver *drv, char *buf, > + size_t count); > + It doesn't fit on a single line ? Up to 100 chars are tolerated as it would increase readability. > const struct dev_pm_ops *pm; > > const struct iommu_ops *iommu_ops; > diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c > index 8dafec7455fbe..651c677c4ab96 100644 > --- a/kernel/module/sysfs.c > +++ b/kernel/module/sysfs.c > @@ -5,6 +5,8 @@ > * Copyright (C) 2008 Rusty Russell > */ > > +#include <linux/device/bus.h> > +#include <linux/device/driver.h> > #include <linux/module.h> > #include <linux/kernel.h> > #include <linux/fs.h> > @@ -240,11 +242,64 @@ static inline void add_notes_attrs(struct module *mod, const struct load_info *i > static inline void remove_notes_attrs(struct module *mod) { } > #endif /* CONFIG_KALLSYMS */ > > +/* Track of the buffer and module identity in callbacks when walking the list of > + * drivers for each bus. > + */ Comments style. > +struct modalias_bus_print_state { > + struct module_kobject *mk; > + char *buf; > + size_t count; > + ssize_t len; > +}; > + > +static int print_modalias_for_drv(struct device_driver *drv, void *p) > +{ > + struct modalias_bus_print_state *s = p; > + struct module_kobject *mk = s->mk; > + ssize_t len; > + /* Skip drivers that do not match this module. */ > + if (mk->mod) { > + if (mk->mod != drv->owner) > + return 0; > + } else if (!mk->kobj.name || !drv->mod_name || > + strcmp(mk->kobj.name, drv->mod_name)) > + return 0; if/else style, see https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces > + > + if (drv->bus && drv->bus->drv_to_modalias) { > + len = drv->bus->drv_to_modalias(drv, s->buf + s->len, > + s->count - s->len); > + if (len < 0) > + return len; > + s->len += len; > + } > + return 0; > +} > + > +static int print_modalias_for_bus(struct bus_type *type, void *p) > +{ > + return bus_for_each_drv(type, NULL, p, print_modalias_for_drv); > +} > + > static ssize_t module_modalias_read(struct file *filp, struct kobject *kobj, > struct bin_attribute *bin_attr, > char *buf, loff_t pos, size_t count) > { > - return 0; > + struct module_kobject *mk = container_of(kobj, struct module_kobject, > + kobj); Doesn't it fit on one 100 chars line ? > + struct modalias_bus_print_state state = {mk, buf, count, 0}; > + int error = 0; Don't initialise vars when it's not needed. > + > + if (pos != 0) > + return -EINVAL; > + > + error = bus_for_each(&state, print_modalias_for_bus); > + if (error) > + return error; > + > + /* > + * The caller checked the pos and count against our size. > + */ > + return state.len; > } > > /* Used in kernel/params.c for builtin modules.
On Fri, Dec 02, 2022 at 04:47:42PM -0600, Allen Webb wrote: > When the modalias attribute is read, invoke a subsystem-specific > callback for each driver registered by the specific module. > > The intent of the new modalias attribute is to expose the > match-id-based modaliases to userspace for builtin and loaded kernel > modules. > > Signed-off-by: Allen Webb <allenwebb@google.com> > --- > include/linux/device/bus.h | 7 +++++ > kernel/module/sysfs.c | 57 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h > index 82a5583437099..cce0bedec63d9 100644 > --- a/include/linux/device/bus.h > +++ b/include/linux/device/bus.h > @@ -61,6 +61,10 @@ struct fwnode_handle; > * this bus. > * @dma_cleanup: Called to cleanup DMA configuration on a device on > * this bus. > + * @drv_to_modalias: Called to convert the matching IDs in a > + * struct device_driver to their corresponding modaliases. > + * Note that the struct device_driver is expected to belong > + * to this bus. If the driver was not part of this bus, that just wouldn't work at all so I don't think you need to say this. And what is the format here? New lines? structures? > * @pm: Power management operations of this bus, callback the specific > * device driver's pm-ops. > * @iommu_ops: IOMMU specific operations for this bus, used to attach IOMMU > @@ -107,6 +111,9 @@ struct bus_type { > int (*dma_configure)(struct device *dev); > void (*dma_cleanup)(struct device *dev); > > + ssize_t (*drv_to_modalias)(struct device_driver *drv, char *buf, > + size_t count); > + > const struct dev_pm_ops *pm; > > const struct iommu_ops *iommu_ops; > diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c > index 8dafec7455fbe..651c677c4ab96 100644 > --- a/kernel/module/sysfs.c > +++ b/kernel/module/sysfs.c > @@ -5,6 +5,8 @@ > * Copyright (C) 2008 Rusty Russell > */ > > +#include <linux/device/bus.h> > +#include <linux/device/driver.h> That feels wrong, modules shouldn't care about busses or drivers. Why can't this all be in the driver core instead? > #include <linux/module.h> > #include <linux/kernel.h> > #include <linux/fs.h> > @@ -240,11 +242,64 @@ static inline void add_notes_attrs(struct module *mod, const struct load_info *i > static inline void remove_notes_attrs(struct module *mod) { } > #endif /* CONFIG_KALLSYMS */ > > +/* Track of the buffer and module identity in callbacks when walking the list of > + * drivers for each bus. > + */ > +struct modalias_bus_print_state { > + struct module_kobject *mk; > + char *buf; > + size_t count; > + ssize_t len; > +}; > + > +static int print_modalias_for_drv(struct device_driver *drv, void *p) > +{ > + struct modalias_bus_print_state *s = p; > + struct module_kobject *mk = s->mk; > + ssize_t len; > + /* Skip drivers that do not match this module. */ Always use checkpatch.pl on your changes before sening them out :( > + if (mk->mod) { > + if (mk->mod != drv->owner) > + return 0; > + } else if (!mk->kobj.name || !drv->mod_name || > + strcmp(mk->kobj.name, drv->mod_name)) > + return 0; > + > + if (drv->bus && drv->bus->drv_to_modalias) { > + len = drv->bus->drv_to_modalias(drv, s->buf + s->len, > + s->count - s->len); > + if (len < 0) > + return len; > + s->len += len; > + } > + return 0; > +} > + > +static int print_modalias_for_bus(struct bus_type *type, void *p) > +{ > + return bus_for_each_drv(type, NULL, p, print_modalias_for_drv); > +} > + > static ssize_t module_modalias_read(struct file *filp, struct kobject *kobj, > struct bin_attribute *bin_attr, > char *buf, loff_t pos, size_t count) > { > - return 0; > + struct module_kobject *mk = container_of(kobj, struct module_kobject, > + kobj); > + struct modalias_bus_print_state state = {mk, buf, count, 0}; You allocate this on the stack? > + int error = 0; No need to initialize this. > + > + if (pos != 0) > + return -EINVAL; Why? > + > + error = bus_for_each(&state, print_modalias_for_bus); So for every module attribute, we walk all busses in the system? Why not the bus that this module has a driver for instead? thanks, greg k-h
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h index 82a5583437099..cce0bedec63d9 100644 --- a/include/linux/device/bus.h +++ b/include/linux/device/bus.h @@ -61,6 +61,10 @@ struct fwnode_handle; * this bus. * @dma_cleanup: Called to cleanup DMA configuration on a device on * this bus. + * @drv_to_modalias: Called to convert the matching IDs in a + * struct device_driver to their corresponding modaliases. + * Note that the struct device_driver is expected to belong + * to this bus. * @pm: Power management operations of this bus, callback the specific * device driver's pm-ops. * @iommu_ops: IOMMU specific operations for this bus, used to attach IOMMU @@ -107,6 +111,9 @@ struct bus_type { int (*dma_configure)(struct device *dev); void (*dma_cleanup)(struct device *dev); + ssize_t (*drv_to_modalias)(struct device_driver *drv, char *buf, + size_t count); + const struct dev_pm_ops *pm; const struct iommu_ops *iommu_ops; diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c index 8dafec7455fbe..651c677c4ab96 100644 --- a/kernel/module/sysfs.c +++ b/kernel/module/sysfs.c @@ -5,6 +5,8 @@ * Copyright (C) 2008 Rusty Russell */ +#include <linux/device/bus.h> +#include <linux/device/driver.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/fs.h> @@ -240,11 +242,64 @@ static inline void add_notes_attrs(struct module *mod, const struct load_info *i static inline void remove_notes_attrs(struct module *mod) { } #endif /* CONFIG_KALLSYMS */ +/* Track of the buffer and module identity in callbacks when walking the list of + * drivers for each bus. + */ +struct modalias_bus_print_state { + struct module_kobject *mk; + char *buf; + size_t count; + ssize_t len; +}; + +static int print_modalias_for_drv(struct device_driver *drv, void *p) +{ + struct modalias_bus_print_state *s = p; + struct module_kobject *mk = s->mk; + ssize_t len; + /* Skip drivers that do not match this module. */ + if (mk->mod) { + if (mk->mod != drv->owner) + return 0; + } else if (!mk->kobj.name || !drv->mod_name || + strcmp(mk->kobj.name, drv->mod_name)) + return 0; + + if (drv->bus && drv->bus->drv_to_modalias) { + len = drv->bus->drv_to_modalias(drv, s->buf + s->len, + s->count - s->len); + if (len < 0) + return len; + s->len += len; + } + return 0; +} + +static int print_modalias_for_bus(struct bus_type *type, void *p) +{ + return bus_for_each_drv(type, NULL, p, print_modalias_for_drv); +} + static ssize_t module_modalias_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t pos, size_t count) { - return 0; + struct module_kobject *mk = container_of(kobj, struct module_kobject, + kobj); + struct modalias_bus_print_state state = {mk, buf, count, 0}; + int error = 0; + + if (pos != 0) + return -EINVAL; + + error = bus_for_each(&state, print_modalias_for_bus); + if (error) + return error; + + /* + * The caller checked the pos and count against our size. + */ + return state.len; } /* Used in kernel/params.c for builtin modules.
When the modalias attribute is read, invoke a subsystem-specific callback for each driver registered by the specific module. The intent of the new modalias attribute is to expose the match-id-based modaliases to userspace for builtin and loaded kernel modules. Signed-off-by: Allen Webb <allenwebb@google.com> --- include/linux/device/bus.h | 7 +++++ kernel/module/sysfs.c | 57 +++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-)