| Message ID | 20210509015708.112766-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events | expand |
Hi Mark, On 5/9/21 3:57 AM, Mark Pearson wrote: > This will be used by the Dell and Lenovo WMI management drivers to > prevent both drivers being active. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > --- > Changes in v2: > This is a new file requested as part of the review of the proposed > think_lmi.c driver. Labeling as V2 to keep series consistent (hope > that's correct). Yes that is correct; and thank you for taking care of making the code for registering the firmware_attribute class shared. > > drivers/platform/x86/Kconfig | 6 +++ > drivers/platform/x86/Makefile | 1 + > .../platform/x86/firmware_attributes_class.c | 51 +++++++++++++++++++ > .../platform/x86/firmware_attributes_class.h | 13 +++++ > 4 files changed, 71 insertions(+) > create mode 100644 drivers/platform/x86/firmware_attributes_class.c > create mode 100644 drivers/platform/x86/firmware_attributes_class.h > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 2714f7c38..b0e1e5f65 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -1076,6 +1076,12 @@ config TOUCHSCREEN_DMI > the OS-image for the device. This option supplies the missing info. > Enable this for x86 tablets with Silead or Chipone touchscreens. > > +config FW_ATTR_CLASS > + tristate "Firmware attributes class helper module"> + help > + This option should be enabled by any modules using the firmware > + attributes class. My idea here was to have this be a kernel-library which drivers which need it select. In this case it should not be visible to end-users and does not need a help text, so this should look like this: config FW_ATTR_CLASS tristate default n > config INTEL_IMR > bool "Intel Isolated Memory Region support" > depends on X86_INTEL_QUARK && IOSF_MBI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index dcc8cdb95..147573f69 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -115,6 +115,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o > obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o > +obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o > > # Intel uncore drivers > obj-$(CONFIG_INTEL_IPS) += intel_ips.o > diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c > new file mode 100644 > index 000000000..4ed959d6c > --- /dev/null > +++ b/drivers/platform/x86/firmware_attributes_class.c > @@ -0,0 +1,51 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +/* Firmware attributes class helper module */ > + > +#include <linux/mutex.h> > +#include <linux/device/class.h> > +#include <linux/module.h> > + > +static DEFINE_MUTEX(fw_attr_lock); > +bool fw_attr_inuse; > + > +static struct class firmware_attributes_class = { > + .name = "firmware-attributes", > +}; > + > +int fw_attributes_class_register(struct class **fw_attr_class) > +{ > + int err; > + > + mutex_lock(&fw_attr_lock); > + /* We can only have one active FW attribute class */ > + if (fw_attr_inuse) { > + mutex_unlock(&fw_attr_lock); > + return -EEXIST; > + } I think it should be allowed to have multiple drivers using the firmware_attributes class, e.g. besides the main system BIOS an Ethermet or FiberChannel; card with an option ROM which supports booting from iSCSI/FCoE or FiberChannel SCSI disks / LUNs could expose settings to configure which disk/LUN to boot from this way. And there is nothing wrong with multiple drivers calling device_create with the same class. So I suggest renaming fw_attributes_class_register to fw_attributes_class_get (and unregister to put) and to make fw_attr_inuse a counter and create the class when the counter goes from 0 -> 1 and free it when it goes from 1 to 0 again. Otherwise this looks good. I like that you already thought about races and have protected fw_attr_inuse with a mutex. Regards, Hans > + > + err = class_register(&firmware_attributes_class); > + if (err) { > + mutex_unlock(&fw_attr_lock); > + return err; > + } > + fw_attr_inuse = true; > + *fw_attr_class = &firmware_attributes_class; > + mutex_unlock(&fw_attr_lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(fw_attributes_class_register); > + > +void fw_attributes_class_remove(void) > +{ > + mutex_lock(&fw_attr_lock); > + fw_attr_inuse = false; > + class_unregister(&firmware_attributes_class); > + mutex_unlock(&fw_attr_lock); > +} > +EXPORT_SYMBOL_GPL(fw_attributes_class_remove); > + > +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); > +MODULE_LICENSE("GPL"); > + > + > diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h > new file mode 100644 > index 000000000..e479a5720 > --- /dev/null > +++ b/drivers/platform/x86/firmware_attributes_class.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +/* Firmware attributes class helper module */ > + > +#ifndef FW_ATTR_CLASS_H > +#define FW_ATTR_CLASS_H > + > +int fw_attributes_class_register(struct class **fw_attr_class); > +void fw_attributes_class_remove(void); > + > +#endif /* FW_ATTR_CLASS_H */ > + > + >
p.s. I'm getting bounces for: mario.limonciello@dell.com (Mario has a new (also Linux related) job elsewhere), for the next version of the series please replace the Cc to Mario with a Cc to Dell.Client.Kernel@dell.com. On 5/9/21 3:57 AM, Mark Pearson wrote: > This will be used by the Dell and Lenovo WMI management drivers to > prevent both drivers being active. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > --- > Changes in v2: > This is a new file requested as part of the review of the proposed > think_lmi.c driver. Labeling as V2 to keep series consistent (hope > that's correct). > > drivers/platform/x86/Kconfig | 6 +++ > drivers/platform/x86/Makefile | 1 + > .../platform/x86/firmware_attributes_class.c | 51 +++++++++++++++++++ > .../platform/x86/firmware_attributes_class.h | 13 +++++ > 4 files changed, 71 insertions(+) > create mode 100644 drivers/platform/x86/firmware_attributes_class.c > create mode 100644 drivers/platform/x86/firmware_attributes_class.h > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 2714f7c38..b0e1e5f65 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -1076,6 +1076,12 @@ config TOUCHSCREEN_DMI > the OS-image for the device. This option supplies the missing info. > Enable this for x86 tablets with Silead or Chipone touchscreens. > > +config FW_ATTR_CLASS > + tristate "Firmware attributes class helper module" > + help > + This option should be enabled by any modules using the firmware > + attributes class. > + > config INTEL_IMR > bool "Intel Isolated Memory Region support" > depends on X86_INTEL_QUARK && IOSF_MBI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index dcc8cdb95..147573f69 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -115,6 +115,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o > obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o > +obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o > > # Intel uncore drivers > obj-$(CONFIG_INTEL_IPS) += intel_ips.o > diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c > new file mode 100644 > index 000000000..4ed959d6c > --- /dev/null > +++ b/drivers/platform/x86/firmware_attributes_class.c > @@ -0,0 +1,51 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +/* Firmware attributes class helper module */ > + > +#include <linux/mutex.h> > +#include <linux/device/class.h> > +#include <linux/module.h> > + > +static DEFINE_MUTEX(fw_attr_lock); > +bool fw_attr_inuse; > + > +static struct class firmware_attributes_class = { > + .name = "firmware-attributes", > +}; > + > +int fw_attributes_class_register(struct class **fw_attr_class) > +{ > + int err; > + > + mutex_lock(&fw_attr_lock); > + /* We can only have one active FW attribute class */ > + if (fw_attr_inuse) { > + mutex_unlock(&fw_attr_lock); > + return -EEXIST; > + } > + > + err = class_register(&firmware_attributes_class); > + if (err) { > + mutex_unlock(&fw_attr_lock); > + return err; > + } > + fw_attr_inuse = true; > + *fw_attr_class = &firmware_attributes_class; > + mutex_unlock(&fw_attr_lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(fw_attributes_class_register); > + > +void fw_attributes_class_remove(void) > +{ > + mutex_lock(&fw_attr_lock); > + fw_attr_inuse = false; > + class_unregister(&firmware_attributes_class); > + mutex_unlock(&fw_attr_lock); > +} > +EXPORT_SYMBOL_GPL(fw_attributes_class_remove); > + > +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); > +MODULE_LICENSE("GPL"); > + > + > diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h > new file mode 100644 > index 000000000..e479a5720 > --- /dev/null > +++ b/drivers/platform/x86/firmware_attributes_class.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +/* Firmware attributes class helper module */ > + > +#ifndef FW_ATTR_CLASS_H > +#define FW_ATTR_CLASS_H > + > +int fw_attributes_class_register(struct class **fw_attr_class); > +void fw_attributes_class_remove(void); > + > +#endif /* FW_ATTR_CLASS_H */ > + > + >
Thanks Hans On 2021-05-19 12:15 p.m., Hans de Goede wrote: > Hi Mark, > > On 5/9/21 3:57 AM, Mark Pearson wrote: >> This will be used by the Dell and Lenovo WMI management drivers to >> prevent both drivers being active. >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >> --- >> Changes in v2: >> This is a new file requested as part of the review of the proposed >> think_lmi.c driver. Labeling as V2 to keep series consistent (hope >> that's correct). > > Yes that is correct; and thank you for taking care of making > the code for registering the firmware_attribute class shared. > >> >> drivers/platform/x86/Kconfig | 6 +++ >> drivers/platform/x86/Makefile | 1 + >> .../platform/x86/firmware_attributes_class.c | 51 +++++++++++++++++++ >> .../platform/x86/firmware_attributes_class.h | 13 +++++ >> 4 files changed, 71 insertions(+) >> create mode 100644 drivers/platform/x86/firmware_attributes_class.c >> create mode 100644 drivers/platform/x86/firmware_attributes_class.h >> >> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >> index 2714f7c38..b0e1e5f65 100644 >> --- a/drivers/platform/x86/Kconfig >> +++ b/drivers/platform/x86/Kconfig >> @@ -1076,6 +1076,12 @@ config TOUCHSCREEN_DMI >> the OS-image for the device. This option supplies the missing info. >> Enable this for x86 tablets with Silead or Chipone touchscreens. >> >> +config FW_ATTR_CLASS >> + tristate "Firmware attributes class helper module"> + help >> + This option should be enabled by any modules using the firmware >> + attributes class. > > My idea here was to have this be a kernel-library which drivers which need it > select. In this case it should not be visible to end-users and does not need a > help text, so this should look like this: > > config FW_ATTR_CLASS > tristate > default n > Got it - I'll update >> config INTEL_IMR >> bool "Intel Isolated Memory Region support" >> depends on X86_INTEL_QUARK && IOSF_MBI >> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile >> index dcc8cdb95..147573f69 100644 >> --- a/drivers/platform/x86/Makefile >> +++ b/drivers/platform/x86/Makefile >> @@ -115,6 +115,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o >> obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o >> obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o >> obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o >> +obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o >> >> # Intel uncore drivers >> obj-$(CONFIG_INTEL_IPS) += intel_ips.o >> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c >> new file mode 100644 >> index 000000000..4ed959d6c >> --- /dev/null >> +++ b/drivers/platform/x86/firmware_attributes_class.c >> @@ -0,0 +1,51 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> + >> +/* Firmware attributes class helper module */ >> + >> +#include <linux/mutex.h> >> +#include <linux/device/class.h> >> +#include <linux/module.h> >> + >> +static DEFINE_MUTEX(fw_attr_lock); >> +bool fw_attr_inuse; >> + >> +static struct class firmware_attributes_class = { >> + .name = "firmware-attributes", >> +}; >> + >> +int fw_attributes_class_register(struct class **fw_attr_class) >> +{ >> + int err; >> + >> + mutex_lock(&fw_attr_lock); >> + /* We can only have one active FW attribute class */ >> + if (fw_attr_inuse) { >> + mutex_unlock(&fw_attr_lock); >> + return -EEXIST; >> + } > > I think it should be allowed to have multiple drivers > using the firmware_attributes class, e.g. besides the main system > BIOS an Ethermet or FiberChannel; card with an option ROM which supports > booting from iSCSI/FCoE or FiberChannel SCSI disks / LUNs could expose > settings to configure which disk/LUN to boot from this way. > > And there is nothing wrong with multiple drivers calling device_create > with the same class. > > So I suggest renaming fw_attributes_class_register to > fw_attributes_class_get (and unregister to put) and to make > fw_attr_inuse a counter and create the class when the counter > goes from 0 -> 1 and free it when it goes from 1 to 0 again. > > Otherwise this looks good. I like that you already thought about > races and have protected fw_attr_inuse with a mutex. > Ah - I hadn't considered it as being used with devices other than the BIOS. That all makes sense and I'll update. Thanks for the review! Mark
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 2714f7c38..b0e1e5f65 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1076,6 +1076,12 @@ config TOUCHSCREEN_DMI the OS-image for the device. This option supplies the missing info. Enable this for x86 tablets with Silead or Chipone touchscreens. +config FW_ATTR_CLASS + tristate "Firmware attributes class helper module" + help + This option should be enabled by any modules using the firmware + attributes class. + config INTEL_IMR bool "Intel Isolated Memory Region support" depends on X86_INTEL_QUARK && IOSF_MBI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index dcc8cdb95..147573f69 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -115,6 +115,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o +obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o # Intel uncore drivers obj-$(CONFIG_INTEL_IPS) += intel_ips.o diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c new file mode 100644 index 000000000..4ed959d6c --- /dev/null +++ b/drivers/platform/x86/firmware_attributes_class.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* Firmware attributes class helper module */ + +#include <linux/mutex.h> +#include <linux/device/class.h> +#include <linux/module.h> + +static DEFINE_MUTEX(fw_attr_lock); +bool fw_attr_inuse; + +static struct class firmware_attributes_class = { + .name = "firmware-attributes", +}; + +int fw_attributes_class_register(struct class **fw_attr_class) +{ + int err; + + mutex_lock(&fw_attr_lock); + /* We can only have one active FW attribute class */ + if (fw_attr_inuse) { + mutex_unlock(&fw_attr_lock); + return -EEXIST; + } + + err = class_register(&firmware_attributes_class); + if (err) { + mutex_unlock(&fw_attr_lock); + return err; + } + fw_attr_inuse = true; + *fw_attr_class = &firmware_attributes_class; + mutex_unlock(&fw_attr_lock); + return 0; +} +EXPORT_SYMBOL_GPL(fw_attributes_class_register); + +void fw_attributes_class_remove(void) +{ + mutex_lock(&fw_attr_lock); + fw_attr_inuse = false; + class_unregister(&firmware_attributes_class); + mutex_unlock(&fw_attr_lock); +} +EXPORT_SYMBOL_GPL(fw_attributes_class_remove); + +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); +MODULE_LICENSE("GPL"); + + diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h new file mode 100644 index 000000000..e479a5720 --- /dev/null +++ b/drivers/platform/x86/firmware_attributes_class.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* Firmware attributes class helper module */ + +#ifndef FW_ATTR_CLASS_H +#define FW_ATTR_CLASS_H + +int fw_attributes_class_register(struct class **fw_attr_class); +void fw_attributes_class_remove(void); + +#endif /* FW_ATTR_CLASS_H */ + +
This will be used by the Dell and Lenovo WMI management drivers to prevent both drivers being active. Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- Changes in v2: This is a new file requested as part of the review of the proposed think_lmi.c driver. Labeling as V2 to keep series consistent (hope that's correct). drivers/platform/x86/Kconfig | 6 +++ drivers/platform/x86/Makefile | 1 + .../platform/x86/firmware_attributes_class.c | 51 +++++++++++++++++++ .../platform/x86/firmware_attributes_class.h | 13 +++++ 4 files changed, 71 insertions(+) create mode 100644 drivers/platform/x86/firmware_attributes_class.c create mode 100644 drivers/platform/x86/firmware_attributes_class.h