| Message ID | 20210530223111.25929-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v5] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events | expand |
Hi, On 5/31/21 12:31 AM, Mark Pearson wrote: > This will be used by the Dell and Lenovo WMI management drivers to > prevent both drivers being active. > > Reported-by: kernel test robot <lkp@intel.com> > 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 > > Changes in v3: > - Set default in Kconfig, and removed help text > - Allow multiple modules to register with module. Change API names to > better reflect this. > > Changes in v4: > - version bump for consistency in series > > Changes in v5: > - Fix issue reported by kernel test robot. Add header file to includes Thanks Mark, Unfortunately you squashed the Kconfig and Makefile changes which I made to v4 when fixing it up during merging into 3/3 instead of having them in v5 of this patch. No worries, since this was the only problem which I could see I've fixed this up in my review-hans branch while merging v5 of this series there (replacing v4). I did notice a bit of dead code while reviewing the changes which you made to 3/3 in response to Andy's review. I'll send a follow-up patch fixing that. I'll leave this sit in my review-hans branch for a bit to give Andy a chance to give his Reviewed-by and then I'll push this to for-next. Regards, Hans > > drivers/platform/x86/Kconfig | 4 ++ > drivers/platform/x86/Makefile | 1 + > .../platform/x86/firmware_attributes_class.c | 54 +++++++++++++++++++ > .../platform/x86/firmware_attributes_class.h | 13 +++++ > 4 files changed, 72 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..57da8352d 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -1076,6 +1076,10 @@ 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" > + 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..b407880f0 > --- /dev/null > +++ b/drivers/platform/x86/firmware_attributes_class.c > @@ -0,0 +1,54 @@ > +// 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> > +#include "firmware_attributes_class.h" > + > +static DEFINE_MUTEX(fw_attr_lock); > +int fw_attr_inuse; > + > +static struct class firmware_attributes_class = { > + .name = "firmware-attributes", > +}; > + > +int fw_attributes_class_get(struct class **fw_attr_class) > +{ > + int err; > + > + mutex_lock(&fw_attr_lock); > + if (!fw_attr_inuse) { /*first time class is being used*/ > + err = class_register(&firmware_attributes_class); > + if (err) { > + mutex_unlock(&fw_attr_lock); > + return err; > + } > + } > + fw_attr_inuse++; > + *fw_attr_class = &firmware_attributes_class; > + mutex_unlock(&fw_attr_lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(fw_attributes_class_get); > + > +int fw_attributes_class_put(void) > +{ > + mutex_lock(&fw_attr_lock); > + if (!fw_attr_inuse) { > + mutex_unlock(&fw_attr_lock); > + return -EINVAL; > + } > + fw_attr_inuse--; > + if (!fw_attr_inuse) /* No more consumers */ > + class_unregister(&firmware_attributes_class); > + mutex_unlock(&fw_attr_lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(fw_attributes_class_put); > + > +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..802f12b45 > --- /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_get(struct class **fw_attr_class); > +int fw_attributes_class_put(void); > + > +#endif /* FW_ATTR_CLASS_H */ > + > + >
On 2021-05-31 9:56 a.m., Hans de Goede wrote: > Hi, > > On 5/31/21 12:31 AM, Mark Pearson wrote: >> This will be used by the Dell and Lenovo WMI management drivers to >> prevent both drivers being active. >> >> Reported-by: kernel test robot <lkp@intel.com> >> 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 >> >> Changes in v3: >> - Set default in Kconfig, and removed help text >> - Allow multiple modules to register with module. Change API names to >> better reflect this. >> >> Changes in v4: >> - version bump for consistency in series >> >> Changes in v5: >> - Fix issue reported by kernel test robot. Add header file to includes > > Thanks Mark, > > Unfortunately you squashed the Kconfig and Makefile changes which I made > to v4 when fixing it up during merging into 3/3 instead of having them in > v5 of this patch. Oh - apologies; I tried to be careful and make sure I picked up the fixes you'd made as well. I must have missed them here somehow :( > > No worries, since this was the only problem which I could see I've fixed > this up in my review-hans branch while merging v5 of this series there > (replacing v4). > > I did notice a bit of dead code while reviewing the changes which you > made to 3/3 in response to Andy's review. I'll send a follow-up patch > fixing that. Sounds good. I'm intrigued what I've missed. > > I'll leave this sit in my review-hans branch for a bit to give Andy > a chance to give his Reviewed-by and then I'll push this to for-next. Sounds good. Thank you! Mark
On Mon, May 31, 2021 at 03:56:41PM +0200, Hans de Goede wrote: > Hi, > > On 5/31/21 12:31 AM, Mark Pearson wrote: > > This will be used by the Dell and Lenovo WMI management drivers to > > prevent both drivers being active. > > > > Reported-by: kernel test robot <lkp@intel.com> > > 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 > > > > Changes in v3: > > - Set default in Kconfig, and removed help text > > - Allow multiple modules to register with module. Change API names to > > better reflect this. > > > > Changes in v4: > > - version bump for consistency in series > > > > Changes in v5: > > - Fix issue reported by kernel test robot. Add header file to includes > > Thanks Mark, > > Unfortunately you squashed the Kconfig and Makefile changes which I made > to v4 when fixing it up during merging into 3/3 instead of having them in > v5 of this patch. > > No worries, since this was the only problem which I could see I've fixed > this up in my review-hans branch while merging v5 of this series there > (replacing v4). > > I did notice a bit of dead code while reviewing the changes which you > made to 3/3 in response to Andy's review. I'll send a follow-up patch > fixing that. > > I'll leave this sit in my review-hans branch for a bit to give Andy > a chance to give his Reviewed-by and then I'll push this to for-next. > > Regards, > > Hans It looks like this series causes allyesconfig to break on linux-next: https://github.com/ClangBuiltLinux/continuous-integration2/runs/2773158286?check_suite_focus=true $ make -skj"$(nproc)" allyesconfig all ld: drivers/platform/x86/think-lmi.o:(.bss+0x0): multiple definition of `fw_attr_class'; drivers/platform/x86/dell/dell-wmi-sysman/sysman.o:(.bss+0x0): first defined here Cheers, Nathan
Hi, On 6/8/21 7:48 PM, Nathan Chancellor wrote: > On Mon, May 31, 2021 at 03:56:41PM +0200, Hans de Goede wrote: >> Hi, >> >> On 5/31/21 12:31 AM, Mark Pearson wrote: >>> This will be used by the Dell and Lenovo WMI management drivers to >>> prevent both drivers being active. >>> >>> Reported-by: kernel test robot <lkp@intel.com> >>> 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 >>> >>> Changes in v3: >>> - Set default in Kconfig, and removed help text >>> - Allow multiple modules to register with module. Change API names to >>> better reflect this. >>> >>> Changes in v4: >>> - version bump for consistency in series >>> >>> Changes in v5: >>> - Fix issue reported by kernel test robot. Add header file to includes >> >> Thanks Mark, >> >> Unfortunately you squashed the Kconfig and Makefile changes which I made >> to v4 when fixing it up during merging into 3/3 instead of having them in >> v5 of this patch. >> >> No worries, since this was the only problem which I could see I've fixed >> this up in my review-hans branch while merging v5 of this series there >> (replacing v4). >> >> I did notice a bit of dead code while reviewing the changes which you >> made to 3/3 in response to Andy's review. I'll send a follow-up patch >> fixing that. >> >> I'll leave this sit in my review-hans branch for a bit to give Andy >> a chance to give his Reviewed-by and then I'll push this to for-next. >> >> Regards, >> >> Hans > > It looks like this series causes allyesconfig to break on linux-next: > > https://github.com/ClangBuiltLinux/continuous-integration2/runs/2773158286?check_suite_focus=true > > $ make -skj"$(nproc)" allyesconfig all > ld: drivers/platform/x86/think-lmi.o:(.bss+0x0): multiple definition of `fw_attr_class'; drivers/platform/x86/dell/dell-wmi-sysman/sysman.o:(.bss+0x0): first defined here Thank you for reporting this. This is caused by both these files: drivers/platform/x86/dell/dell-wmi-sysman/sysman.c drivers/platform/x86/think-lmi.c having a global struct class *fw_attr_class variable, which should be static in both files. I'll send a patch fixing this (and merge the patch into the pdx86/for-next branch). Regards, Hans
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 2714f7c38..57da8352d 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1076,6 +1076,10 @@ 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" + 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..b407880f0 --- /dev/null +++ b/drivers/platform/x86/firmware_attributes_class.c @@ -0,0 +1,54 @@ +// 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> +#include "firmware_attributes_class.h" + +static DEFINE_MUTEX(fw_attr_lock); +int fw_attr_inuse; + +static struct class firmware_attributes_class = { + .name = "firmware-attributes", +}; + +int fw_attributes_class_get(struct class **fw_attr_class) +{ + int err; + + mutex_lock(&fw_attr_lock); + if (!fw_attr_inuse) { /*first time class is being used*/ + err = class_register(&firmware_attributes_class); + if (err) { + mutex_unlock(&fw_attr_lock); + return err; + } + } + fw_attr_inuse++; + *fw_attr_class = &firmware_attributes_class; + mutex_unlock(&fw_attr_lock); + return 0; +} +EXPORT_SYMBOL_GPL(fw_attributes_class_get); + +int fw_attributes_class_put(void) +{ + mutex_lock(&fw_attr_lock); + if (!fw_attr_inuse) { + mutex_unlock(&fw_attr_lock); + return -EINVAL; + } + fw_attr_inuse--; + if (!fw_attr_inuse) /* No more consumers */ + class_unregister(&firmware_attributes_class); + mutex_unlock(&fw_attr_lock); + return 0; +} +EXPORT_SYMBOL_GPL(fw_attributes_class_put); + +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..802f12b45 --- /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_get(struct class **fw_attr_class); +int fw_attributes_class_put(void); + +#endif /* FW_ATTR_CLASS_H */ + +
This will be used by the Dell and Lenovo WMI management drivers to prevent both drivers being active. Reported-by: kernel test robot <lkp@intel.com> 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 Changes in v3: - Set default in Kconfig, and removed help text - Allow multiple modules to register with module. Change API names to better reflect this. Changes in v4: - version bump for consistency in series Changes in v5: - Fix issue reported by kernel test robot. Add header file to includes drivers/platform/x86/Kconfig | 4 ++ drivers/platform/x86/Makefile | 1 + .../platform/x86/firmware_attributes_class.c | 54 +++++++++++++++++++ .../platform/x86/firmware_attributes_class.h | 13 +++++ 4 files changed, 72 insertions(+) create mode 100644 drivers/platform/x86/firmware_attributes_class.c create mode 100644 drivers/platform/x86/firmware_attributes_class.h