| Message ID | 20250104-firmware-attributes-simplify-v1-1-949f9709e405@weissschuh.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: firmware_attributes_class: Simplify API | expand |
Am 04.01.25 um 00:05 schrieb Thomas Weißschuh: > The header firmware_attributes_class.h uses 'struct class'. It should > also include the necessary dependency header. Hi, i like this patch series, but i would prefer that we do not expose the raw class through the header. Looking at the callers of fw_attributes_class_get(), everywhere the class value is used only to call: device_create(fw_attr_class, NULL, MKDEV(0, 0), NULL, "%s", <driver name>); I suggest that we introduce two new functions for that: struct device *firmware_attributes_device_register(struct device *parent, const char *name); void firmware_attributes_device_unregister(struct device *dev); This would have three major benefits: - the raw class can be made internal - reduced code size - drivers would stop copying the flawed use of device_destroy() Regarding the use of device_destroy(): this is actually an error since device_destroy() cannot be reliably used when devt is not unique. Since all those drivers are setting devt to MKDEV(0, 0) this could result in a kernel panic should multiple firmware-attribute class devices exist at the same time. What do you think? Thanks, Armin Wolf > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > drivers/platform/x86/firmware_attributes_class.c | 1 - > drivers/platform/x86/firmware_attributes_class.h | 2 ++ > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c > index 182a07d8ae3dfa8925bb5b71a43d0219c3cf0fa0..cbc56e5db59283ba99ac0b915ac5fb2432afbdc9 100644 > --- a/drivers/platform/x86/firmware_attributes_class.c > +++ b/drivers/platform/x86/firmware_attributes_class.c > @@ -3,7 +3,6 @@ > /* Firmware attributes class helper module */ > > #include <linux/mutex.h> > -#include <linux/device/class.h> > #include <linux/module.h> > #include "firmware_attributes_class.h" > > diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h > index 363c75f1ac1b89df879a8689b070e6b11d3bb7fd..8e0f47cfdf92eb4dc8722b7d8371916af0d84efa 100644 > --- a/drivers/platform/x86/firmware_attributes_class.h > +++ b/drivers/platform/x86/firmware_attributes_class.h > @@ -5,6 +5,8 @@ > #ifndef FW_ATTR_CLASS_H > #define FW_ATTR_CLASS_H > > +#include <linux/device/class.h> > + > int fw_attributes_class_get(const struct class **fw_attr_class); > int fw_attributes_class_put(void); > >
Hi, On 2025-01-04 07:55:15+0100, Armin Wolf wrote: > Am 04.01.25 um 00:05 schrieb Thomas Weißschuh: > > > The header firmware_attributes_class.h uses 'struct class'. It should > > also include the necessary dependency header. > i like this patch series, but i would prefer that we do not expose the raw class through the header. > > Looking at the callers of fw_attributes_class_get(), everywhere the class value is used only to call: > > device_create(fw_attr_class, NULL, MKDEV(0, 0), NULL, "%s", <driver name>); > > I suggest that we introduce two new functions for that: > > struct device *firmware_attributes_device_register(struct device *parent, const char *name); > > void firmware_attributes_device_unregister(struct device *dev); > > This would have three major benefits: > > - the raw class can be made internal > - reduced code size > - drivers would stop copying the flawed use of device_destroy() > > Regarding the use of device_destroy(): this is actually an error since device_destroy() cannot be > reliably used when devt is not unique. Since all those drivers are setting devt to MKDEV(0, 0) this > could result in a kernel panic should multiple firmware-attribute class devices exist at the same time. > > What do you think? Completely agree. This is exactly what the "further improvements" mentioned in the cover letter do. And also add devm_firmware_attributes_device_register() and a custom sysfs attribute type that makes the driver code much simplerr. But this will be some more work. Also the conversions of the drivers will be harder and take longer, so we can't drop the raw exposed class as easily and have to keep the "legacy" interface for a bit. > Thanks, > Armin Wolf > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > --- > > drivers/platform/x86/firmware_attributes_class.c | 1 - > > drivers/platform/x86/firmware_attributes_class.h | 2 ++ > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c > > index 182a07d8ae3dfa8925bb5b71a43d0219c3cf0fa0..cbc56e5db59283ba99ac0b915ac5fb2432afbdc9 100644 > > --- a/drivers/platform/x86/firmware_attributes_class.c > > +++ b/drivers/platform/x86/firmware_attributes_class.c > > @@ -3,7 +3,6 @@ > > /* Firmware attributes class helper module */ > > > > #include <linux/mutex.h> > > -#include <linux/device/class.h> > > #include <linux/module.h> > > #include "firmware_attributes_class.h" > > > > diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h > > index 363c75f1ac1b89df879a8689b070e6b11d3bb7fd..8e0f47cfdf92eb4dc8722b7d8371916af0d84efa 100644 > > --- a/drivers/platform/x86/firmware_attributes_class.h > > +++ b/drivers/platform/x86/firmware_attributes_class.h > > @@ -5,6 +5,8 @@ > > #ifndef FW_ATTR_CLASS_H > > #define FW_ATTR_CLASS_H > > > > +#include <linux/device/class.h> > > + > > int fw_attributes_class_get(const struct class **fw_attr_class); > > int fw_attributes_class_put(void); > > > >
Am 04.01.25 um 08:06 schrieb Thomas Weißschuh: > Hi, > > On 2025-01-04 07:55:15+0100, Armin Wolf wrote: >> Am 04.01.25 um 00:05 schrieb Thomas Weißschuh: >> >>> The header firmware_attributes_class.h uses 'struct class'. It should >>> also include the necessary dependency header. >> i like this patch series, but i would prefer that we do not expose the raw class through the header. >> >> Looking at the callers of fw_attributes_class_get(), everywhere the class value is used only to call: >> >> device_create(fw_attr_class, NULL, MKDEV(0, 0), NULL, "%s", <driver name>); >> >> I suggest that we introduce two new functions for that: >> >> struct device *firmware_attributes_device_register(struct device *parent, const char *name); >> >> void firmware_attributes_device_unregister(struct device *dev); >> >> This would have three major benefits: >> >> - the raw class can be made internal >> - reduced code size >> - drivers would stop copying the flawed use of device_destroy() >> >> Regarding the use of device_destroy(): this is actually an error since device_destroy() cannot be >> reliably used when devt is not unique. Since all those drivers are setting devt to MKDEV(0, 0) this >> could result in a kernel panic should multiple firmware-attribute class devices exist at the same time. >> >> What do you think? > Completely agree. This is exactly what the "further improvements" > mentioned in the cover letter do. > And also add devm_firmware_attributes_device_register() and a custom > sysfs attribute type that makes the driver code much simplerr. > > But this will be some more work. Also the conversions of the drivers > will be harder and take longer, so we can't drop the raw exposed class > as easily and have to keep the "legacy" interface for a bit. Fair point. In this case the current approach should be fine. >> Thanks, >> Armin Wolf >> >>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> >>> --- >>> drivers/platform/x86/firmware_attributes_class.c | 1 - >>> drivers/platform/x86/firmware_attributes_class.h | 2 ++ >>> 2 files changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c >>> index 182a07d8ae3dfa8925bb5b71a43d0219c3cf0fa0..cbc56e5db59283ba99ac0b915ac5fb2432afbdc9 100644 >>> --- a/drivers/platform/x86/firmware_attributes_class.c >>> +++ b/drivers/platform/x86/firmware_attributes_class.c >>> @@ -3,7 +3,6 @@ >>> /* Firmware attributes class helper module */ >>> >>> #include <linux/mutex.h> >>> -#include <linux/device/class.h> >>> #include <linux/module.h> >>> #include "firmware_attributes_class.h" >>> >>> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h >>> index 363c75f1ac1b89df879a8689b070e6b11d3bb7fd..8e0f47cfdf92eb4dc8722b7d8371916af0d84efa 100644 >>> --- a/drivers/platform/x86/firmware_attributes_class.h >>> +++ b/drivers/platform/x86/firmware_attributes_class.h >>> @@ -5,6 +5,8 @@ >>> #ifndef FW_ATTR_CLASS_H >>> #define FW_ATTR_CLASS_H >>> >>> +#include <linux/device/class.h> I think it would make more sense to not include the complete class header and instead only define "struct class;" inside firmware_attributes_class.h. Thanks, Armin Wolf >>> + >>> int fw_attributes_class_get(const struct class **fw_attr_class); >>> int fw_attributes_class_put(void); >>> >>>
Am 04.01.25 um 08:20 schrieb Armin Wolf: > Am 04.01.25 um 08:06 schrieb Thomas Weißschuh: > >> Hi, >> >> On 2025-01-04 07:55:15+0100, Armin Wolf wrote: >>> Am 04.01.25 um 00:05 schrieb Thomas Weißschuh: >>> >>>> The header firmware_attributes_class.h uses 'struct class'. It should >>>> also include the necessary dependency header. >>> i like this patch series, but i would prefer that we do not expose >>> the raw class through the header. >>> >>> Looking at the callers of fw_attributes_class_get(), everywhere the >>> class value is used only to call: >>> >>> device_create(fw_attr_class, NULL, MKDEV(0, 0), NULL, "%s", >>> <driver name>); >>> >>> I suggest that we introduce two new functions for that: >>> >>> struct device *firmware_attributes_device_register(struct device >>> *parent, const char *name); >>> >>> void firmware_attributes_device_unregister(struct device *dev); >>> >>> This would have three major benefits: >>> >>> - the raw class can be made internal >>> - reduced code size >>> - drivers would stop copying the flawed use of device_destroy() >>> >>> Regarding the use of device_destroy(): this is actually an error >>> since device_destroy() cannot be >>> reliably used when devt is not unique. Since all those drivers are >>> setting devt to MKDEV(0, 0) this >>> could result in a kernel panic should multiple firmware-attribute >>> class devices exist at the same time. >>> >>> What do you think? >> Completely agree. This is exactly what the "further improvements" >> mentioned in the cover letter do. >> And also add devm_firmware_attributes_device_register() and a custom >> sysfs attribute type that makes the driver code much simplerr. >> >> But this will be some more work. Also the conversions of the drivers >> will be harder and take longer, so we can't drop the raw exposed class >> as easily and have to keep the "legacy" interface for a bit. > > Fair point. In this case the current approach should be fine. > >>> Thanks, >>> Armin Wolf >>> >>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> >>>> --- >>>> drivers/platform/x86/firmware_attributes_class.c | 1 - >>>> drivers/platform/x86/firmware_attributes_class.h | 2 ++ >>>> 2 files changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/platform/x86/firmware_attributes_class.c >>>> b/drivers/platform/x86/firmware_attributes_class.c >>>> index >>>> 182a07d8ae3dfa8925bb5b71a43d0219c3cf0fa0..cbc56e5db59283ba99ac0b915ac5fb2432afbdc9 >>>> 100644 >>>> --- a/drivers/platform/x86/firmware_attributes_class.c >>>> +++ b/drivers/platform/x86/firmware_attributes_class.c >>>> @@ -3,7 +3,6 @@ >>>> /* Firmware attributes class helper module */ >>>> >>>> #include <linux/mutex.h> >>>> -#include <linux/device/class.h> >>>> #include <linux/module.h> >>>> #include "firmware_attributes_class.h" >>>> >>>> diff --git a/drivers/platform/x86/firmware_attributes_class.h >>>> b/drivers/platform/x86/firmware_attributes_class.h >>>> index >>>> 363c75f1ac1b89df879a8689b070e6b11d3bb7fd..8e0f47cfdf92eb4dc8722b7d8371916af0d84efa >>>> 100644 >>>> --- a/drivers/platform/x86/firmware_attributes_class.h >>>> +++ b/drivers/platform/x86/firmware_attributes_class.h >>>> @@ -5,6 +5,8 @@ >>>> #ifndef FW_ATTR_CLASS_H >>>> #define FW_ATTR_CLASS_H >>>> >>>> +#include <linux/device/class.h> > > I think it would make more sense to not include the complete class > header and instead only > define "struct class;" inside firmware_attributes_class.h. > > Thanks, > Armin Wolf Forget what i just said, we still need the header once we expose the class. For the whole series: Reviewed-by: Armin Wolf <W_Armin@gmx.de> > >>>> + >>>> int fw_attributes_class_get(const struct class **fw_attr_class); >>>> int fw_attributes_class_put(void); >>>> >>>> >
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c index 182a07d8ae3dfa8925bb5b71a43d0219c3cf0fa0..cbc56e5db59283ba99ac0b915ac5fb2432afbdc9 100644 --- a/drivers/platform/x86/firmware_attributes_class.c +++ b/drivers/platform/x86/firmware_attributes_class.c @@ -3,7 +3,6 @@ /* Firmware attributes class helper module */ #include <linux/mutex.h> -#include <linux/device/class.h> #include <linux/module.h> #include "firmware_attributes_class.h" diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h index 363c75f1ac1b89df879a8689b070e6b11d3bb7fd..8e0f47cfdf92eb4dc8722b7d8371916af0d84efa 100644 --- a/drivers/platform/x86/firmware_attributes_class.h +++ b/drivers/platform/x86/firmware_attributes_class.h @@ -5,6 +5,8 @@ #ifndef FW_ATTR_CLASS_H #define FW_ATTR_CLASS_H +#include <linux/device/class.h> + int fw_attributes_class_get(const struct class **fw_attr_class); int fw_attributes_class_put(void);
The header firmware_attributes_class.h uses 'struct class'. It should also include the necessary dependency header. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- drivers/platform/x86/firmware_attributes_class.c | 1 - drivers/platform/x86/firmware_attributes_class.h | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-)