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@xxxxxxxxxxxxxx> > > --- > > 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); > > > >
