| Message ID | 20250318124706.94156-2-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/mlx5: Introduce shared devlink instance for PFs on same chip | expand |
On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote: > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -744,7 +744,7 @@ static inline void __module_get(struct module *module) > /* This is a #define so the string doesn't get put in every .o file */ > #define module_name(mod) \ > ({ \ > - struct module *__mod = (mod); \ > + const struct module *__mod = (mod); \ > __mod ? __mod->name : "kernel"; \ > }) This feels like it should be a separate change, right? Doesn't have to do with this patch. greg k-h
Tue, Mar 18, 2025 at 02:46:56PM +0100, gregkh@linuxfoundation.org wrote: >On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote: >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >> @@ -744,7 +744,7 @@ static inline void __module_get(struct module *module) >> /* This is a #define so the string doesn't get put in every .o file */ >> #define module_name(mod) \ >> ({ \ >> - struct module *__mod = (mod); \ >> + const struct module *__mod = (mod); \ >> __mod ? __mod->name : "kernel"; \ >> }) > >This feels like it should be a separate change, right? Doesn't have to >do with this patch. True. Will split. > >greg k-h
On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > It is hard for the faux user to avoid potential name conflicts, as it is > only in control of faux devices it creates. Therefore extend the faux > device creation function by module parameter, embed the module name into > the device name in format "modulename_permodulename" and allow module to > control it's namespace. Do you have an example of how this will change the current names we have in the system to this new way? What is going to break if those names change? I say this as the perf devices seem to have "issues" with their names and locations in sysfs as userspace tools use them today, and in a straight port to faux it is ok, but if the device name changes, that is going to have problems. Why can't you handle this "namespace" issue yourself in the caller to the api? Why must the faux code handle it for you? We don't do this for platform devices, why is this any different? thanks, greg k-h
Tue, Mar 18, 2025 at 03:36:34PM +0100, gregkh@linuxfoundation.org wrote: >On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> >> It is hard for the faux user to avoid potential name conflicts, as it is >> only in control of faux devices it creates. Therefore extend the faux >> device creation function by module parameter, embed the module name into >> the device name in format "modulename_permodulename" and allow module to >> control it's namespace. > >Do you have an example of how this will change the current names we have >in the system to this new way? What is going to break if those names >change? I was under impression, that since there are no in-tree users of faux yet (at least I don't see them in net-next tree), there is no breakage. > >I say this as the perf devices seem to have "issues" with their names >and locations in sysfs as userspace tools use them today, and in a >straight port to faux it is ok, but if the device name changes, that is >going to have problems. Got it. I didn't consider that. > >Why can't you handle this "namespace" issue yourself in the caller to >the api? Why must the faux code handle it for you? We don't do this >for platform devices, why is this any different? Well, I wanted to avoid alloc&printf names in driver, since dev_set_name() accepts vararg and faux_device_create()/faux_device_create_with_groups() don't. Perhaps "const char *name" could be formatted as well for faux_device_create()/faux_device_create_with_groups(). My laziness wanted to avoid that :) Would that make sense to you? > >thanks, > >greg k-h
On Tue, Mar 18, 2025 at 04:26:05PM +0100, Jiri Pirko wrote: > Tue, Mar 18, 2025 at 03:36:34PM +0100, gregkh@linuxfoundation.org wrote: > >On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote: > >> From: Jiri Pirko <jiri@nvidia.com> > >> > >> It is hard for the faux user to avoid potential name conflicts, as it is > >> only in control of faux devices it creates. Therefore extend the faux > >> device creation function by module parameter, embed the module name into > >> the device name in format "modulename_permodulename" and allow module to > >> control it's namespace. > > > >Do you have an example of how this will change the current names we have > >in the system to this new way? What is going to break if those names > >change? > > I was under impression, that since there are no in-tree users of faux > yet (at least I don't see them in net-next tree), there is no breakage. Look at linux-next please. > >Why can't you handle this "namespace" issue yourself in the caller to > >the api? Why must the faux code handle it for you? We don't do this > >for platform devices, why is this any different? > > Well, I wanted to avoid alloc&printf names in driver, since > dev_set_name() accepts vararg and faux_device_create()/faux_device_create_with_groups() > don't. If you want to do something complex, do it in your driver :) > Perhaps "const char *name" could be formatted as well for > faux_device_create()/faux_device_create_with_groups(). My laziness > wanted to avoid that :) Would that make sense to you? I wouldn't object to that, making it a vararg? How would the rust binding handle that? thanks, greg k-h
Tue, Mar 18, 2025 at 05:04:37PM +0100, gregkh@linuxfoundation.org wrote: >On Tue, Mar 18, 2025 at 04:26:05PM +0100, Jiri Pirko wrote: >> Tue, Mar 18, 2025 at 03:36:34PM +0100, gregkh@linuxfoundation.org wrote: >> >On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote: >> >> From: Jiri Pirko <jiri@nvidia.com> >> >> >> >> It is hard for the faux user to avoid potential name conflicts, as it is >> >> only in control of faux devices it creates. Therefore extend the faux >> >> device creation function by module parameter, embed the module name into >> >> the device name in format "modulename_permodulename" and allow module to >> >> control it's namespace. >> > >> >Do you have an example of how this will change the current names we have >> >in the system to this new way? What is going to break if those names >> >change? >> >> I was under impression, that since there are no in-tree users of faux >> yet (at least I don't see them in net-next tree), there is no breakage. > >Look at linux-next please. Sure, but it's still next. Next might break (uapi) as long it's next, right? > >> >Why can't you handle this "namespace" issue yourself in the caller to >> >the api? Why must the faux code handle it for you? We don't do this >> >for platform devices, why is this any different? >> >> Well, I wanted to avoid alloc&printf names in driver, since >> dev_set_name() accepts vararg and faux_device_create()/faux_device_create_with_groups() >> don't. > >If you want to do something complex, do it in your driver :) Yeah, I don't really want to do anything complex, that's why I wanted to take leverage of dev_set_name() > >> Perhaps "const char *name" could be formatted as well for >> faux_device_create()/faux_device_create_with_groups(). My laziness >> wanted to avoid that :) Would that make sense to you? > >I wouldn't object to that, making it a vararg? How would the rust >binding handle that? Why should I care about rust? I got the impression the deal is that rust bindings are taken care of by rust people. Did that change and we need to keep rust in mind for all internal API? That sounds scarry to me :( > >thanks, > >greg k-h
diff --git a/drivers/base/faux.c b/drivers/base/faux.c index 531e9d789ee0..b1fcac6b0946 100644 --- a/drivers/base/faux.c +++ b/drivers/base/faux.c @@ -85,8 +85,9 @@ static void faux_device_release(struct device *dev) * faux_device_create_with_groups - Create and register with the driver * core a faux device and populate the device with an initial * set of sysfs attributes. - * @name: The name of the device we are adding, must be unique for - * all faux devices. + * @module: Pointer to module this device is associated. + * @name: The name of the device we are adding, must be unique for all + * faux devices within a single module. * @parent: Pointer to a potential parent struct device. If set to * NULL, the device will be created in the "root" of the faux * device tree in sysfs. @@ -108,7 +109,8 @@ static void faux_device_release(struct device *dev) * * NULL if an error happened with creating the device * * pointer to a valid struct faux_device that is registered with sysfs */ -struct faux_device *faux_device_create_with_groups(const char *name, +struct faux_device *faux_device_create_with_groups(const struct module *module, + const char *name, struct device *parent, const struct faux_device_ops *faux_ops, const struct attribute_group **groups) @@ -137,12 +139,12 @@ struct faux_device *faux_device_create_with_groups(const char *name, dev->parent = &faux_bus_root; dev->bus = &faux_bus_type; dev->groups = groups; - dev_set_name(dev, "%s", name); + dev_set_name(dev, "%s_%s", module_name(module), name); ret = device_add(dev); if (ret) { pr_err("%s: device_add for faux device '%s' failed with %d\n", - __func__, name, ret); + __func__, dev_name(dev), ret); put_device(dev); return NULL; } @@ -153,8 +155,9 @@ EXPORT_SYMBOL_GPL(faux_device_create_with_groups); /** * faux_device_create - create and register with the driver core a faux device + * @module: Pointer to module this device is associated. * @name: The name of the device we are adding, must be unique for all - * faux devices. + * faux devices within a single module. * @parent: Pointer to a potential parent struct device. If set to * NULL, the device will be created in the "root" of the faux * device tree in sysfs. @@ -174,11 +177,12 @@ EXPORT_SYMBOL_GPL(faux_device_create_with_groups); * * NULL if an error happened with creating the device * * pointer to a valid struct faux_device that is registered with sysfs */ -struct faux_device *faux_device_create(const char *name, +struct faux_device *faux_device_create(const struct module *module, + const char *name, struct device *parent, const struct faux_device_ops *faux_ops) { - return faux_device_create_with_groups(name, parent, faux_ops, NULL); + return faux_device_create_with_groups(module, name, parent, faux_ops, NULL); } EXPORT_SYMBOL_GPL(faux_device_create); diff --git a/include/linux/device/faux.h b/include/linux/device/faux.h index 9f43c0e46aa4..b1393a34b4f9 100644 --- a/include/linux/device/faux.h +++ b/include/linux/device/faux.h @@ -47,10 +47,12 @@ struct faux_device_ops { void (*remove)(struct faux_device *faux_dev); }; -struct faux_device *faux_device_create(const char *name, +struct faux_device *faux_device_create(const struct module *module, + const char *name, struct device *parent, const struct faux_device_ops *faux_ops); -struct faux_device *faux_device_create_with_groups(const char *name, +struct faux_device *faux_device_create_with_groups(const struct module *module, + const char *name, struct device *parent, const struct faux_device_ops *faux_ops, const struct attribute_group **groups); diff --git a/include/linux/module.h b/include/linux/module.h index 30e5b19bafa9..8d1a7e65d2e4 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -744,7 +744,7 @@ static inline void __module_get(struct module *module) /* This is a #define so the string doesn't get put in every .o file */ #define module_name(mod) \ ({ \ - struct module *__mod = (mod); \ + const struct module *__mod = (mod); \ __mod ? __mod->name : "kernel"; \ })