[PATCH 0/6] platform/x86: firmware_attributes_class: Simplify API

[PATCH 0/6] platform/x86: firmware_attributes_class: Simplify API

[Thomas Weißschuh]

Looking at the users of firmware_attributes_class makes my head hurt.
Simplify the subsystem and its users.

This will break the currently developed samsung-galaxybook driver,
resolving the breakage should be trivial.

Only compile-tested.

I have some further improvements in the pipeline building on these
changes, but those will need some more time.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (6):
      platform/x86: firmware_attributes_class: Move include linux/device/class.h
      platform/x86: firmware_attributes_class: Simplify API
      platform/x86: think-lmi: Directly use firmware_attributes_class
      platform/x86: hp-bioscfg: Directly use firmware_attributes_class
      platform/x86: dell-sysman: Directly use firmware_attributes_class
      platform/x86: firmware_attributes_class: Drop lifecycle functions

 drivers/platform/x86/dell/dell-wmi-sysman/sysman.c | 17 +++------
 drivers/platform/x86/firmware_attributes_class.c   | 42 +++++-----------------
 drivers/platform/x86/firmware_attributes_class.h   |  5 +--
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c       | 14 ++------
 drivers/platform/x86/think-lmi.c                   | 13 ++-----
 5 files changed, 21 insertions(+), 70 deletions(-)
---
base-commit: 0bc21e701a6ffacfdde7f04f87d664d82e8a13bf
change-id: 20250103-firmware-attributes-simplify-9ae561459260

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/6] platform/x86: firmware_attributes_class: Move include linux/device/class.h

[Thomas Weißschuh]

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(-)

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

-- 
2.47.1

[PATCH 2/6] platform/x86: firmware_attributes_class: Simplify API

[Thomas Weißschuh]

The module core already guarantees that a module can only be unloaded
after all other modules using its symbols have been unloaded.
As it's already the responsibility of the drivers using
firmware_attributes_class to clean up their devices before unloading,
the lifetime of the firmware_attributes_class can be bound to the
lifetime of the module.
This enables the direct usage of firmware_attributes_class from the
drivers, without having to go through the lifecycle functions,
leading to simplifications for both the subsystem and its users.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/firmware_attributes_class.c | 40 +++++++++---------------
 drivers/platform/x86/firmware_attributes_class.h |  1 +
 2 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index cbc56e5db59283ba99ac0b915ac5fb2432afbdc9..87672c49e86ae3ef5b99aa99be532c1d84805adc 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -2,47 +2,35 @@
 
 /* Firmware attributes class helper module */
 
-#include <linux/mutex.h>
 #include <linux/module.h>
 #include "firmware_attributes_class.h"
 
-static DEFINE_MUTEX(fw_attr_lock);
-static int fw_attr_inuse;
-
-static const struct class firmware_attributes_class = {
+const struct class firmware_attributes_class = {
 	.name = "firmware-attributes",
 };
+EXPORT_SYMBOL_GPL(firmware_attributes_class);
+
+static __init int fw_attributes_class_init(void)
+{
+	return class_register(&firmware_attributes_class);
+}
+module_init(fw_attributes_class_init);
+
+static __exit void fw_attributes_class_exit(void)
+{
+	class_unregister(&firmware_attributes_class);
+}
+module_exit(fw_attributes_class_exit);
 
 int fw_attributes_class_get(const 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);
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index 8e0f47cfdf92eb4dc8722b7d8371916af0d84efa..ef6c3764a83497ad7e75b0102154c92ce476e5ae 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -7,6 +7,7 @@
 
 #include <linux/device/class.h>
 
+extern const struct class firmware_attributes_class;
 int fw_attributes_class_get(const struct class **fw_attr_class);
 int fw_attributes_class_put(void);
 

-- 
2.47.1

[PATCH 3/6] platform/x86: think-lmi: Directly use firmware_attributes_class

[Thomas Weißschuh]

The usage of the lifecycle functions is not necessary anymore.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/think-lmi.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 38de0cb20d7785d4e3b490e07edaf4d02f8f3370..323316ac6783aa343c4c6430040a4af648786880 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -194,7 +194,6 @@ static const char * const level_options[] = {
 	[TLMI_LEVEL_MASTER] = "master",
 };
 static struct think_lmi tlmi_priv;
-static const struct class *fw_attr_class;
 static DEFINE_MUTEX(tlmi_mutex);
 
 static inline struct tlmi_pwd_setting *to_tlmi_pwd_setting(struct kobject *kobj)
@@ -1446,11 +1445,7 @@ static int tlmi_sysfs_init(void)
 {
 	int i, ret;
 
-	ret = fw_attributes_class_get(&fw_attr_class);
-	if (ret)
-		return ret;
-
-	tlmi_priv.class_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0),
+	tlmi_priv.class_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
 			NULL, "%s", "thinklmi");
 	if (IS_ERR(tlmi_priv.class_dev)) {
 		ret = PTR_ERR(tlmi_priv.class_dev);
@@ -1563,9 +1558,8 @@ static int tlmi_sysfs_init(void)
 fail_create_attr:
 	tlmi_release_attr();
 fail_device_created:
-	device_destroy(fw_attr_class, MKDEV(0, 0));
+	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
 fail_class_created:
-	fw_attributes_class_put();
 	return ret;
 }
 
@@ -1788,8 +1782,7 @@ static int tlmi_analyze(void)
 static void tlmi_remove(struct wmi_device *wdev)
 {
 	tlmi_release_attr();
-	device_destroy(fw_attr_class, MKDEV(0, 0));
-	fw_attributes_class_put();
+	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
 }
 
 static int tlmi_probe(struct wmi_device *wdev, const void *context)

-- 
2.47.1

[PATCH 4/6] platform/x86: hp-bioscfg: Directly use firmware_attributes_class

[Thomas Weißschuh]

The usage of the lifecycle functions is not necessary anymore.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 2dc50152158a3dd0401d9d9e1de288c0d76ef937..0b277b7e37dd6b598e4fe5de6dc0ac47300a8e3b 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -24,8 +24,6 @@ struct bioscfg_priv bioscfg_drv = {
 	.mutex = __MUTEX_INITIALIZER(bioscfg_drv.mutex),
 };
 
-static const struct class *fw_attr_class;
-
 ssize_t display_name_language_code_show(struct kobject *kobj,
 					struct kobj_attribute *attr,
 					char *buf)
@@ -972,11 +970,7 @@ static int __init hp_init(void)
 	if (ret)
 		return ret;
 
-	ret = fw_attributes_class_get(&fw_attr_class);
-	if (ret)
-		goto err_unregister_class;
-
-	bioscfg_drv.class_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0),
+	bioscfg_drv.class_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
 					      NULL, "%s", DRIVER_NAME);
 	if (IS_ERR(bioscfg_drv.class_dev)) {
 		ret = PTR_ERR(bioscfg_drv.class_dev);
@@ -1043,10 +1037,9 @@ static int __init hp_init(void)
 	release_attributes_data();
 
 err_destroy_classdev:
-	device_destroy(fw_attr_class, MKDEV(0, 0));
+	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
 
 err_unregister_class:
-	fw_attributes_class_put();
 	hp_exit_attr_set_interface();
 
 	return ret;
@@ -1055,9 +1048,8 @@ static int __init hp_init(void)
 static void __exit hp_exit(void)
 {
 	release_attributes_data();
-	device_destroy(fw_attr_class, MKDEV(0, 0));
+	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
 
-	fw_attributes_class_put();
 	hp_exit_attr_set_interface();
 }
 

-- 
2.47.1

[PATCH 5/6] platform/x86: dell-sysman: Directly use firmware_attributes_class

[Thomas Weißschuh]

The usage of the lifecycle functions is not necessary anymore.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/dell/dell-wmi-sysman/sysman.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
index 40ddc6eb75624e713dc4d2f7e92bc5f63fa4fde8..d00389b860e4ea0655c740c78bc3751f323b6370 100644
--- a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
+++ b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
@@ -25,7 +25,6 @@ struct wmi_sysman_priv wmi_priv = {
 /* reset bios to defaults */
 static const char * const reset_types[] = {"builtinsafe", "lastknowngood", "factory", "custom"};
 static int reset_option = -1;
-static const struct class *fw_attr_class;
 
 
 /**
@@ -541,15 +540,11 @@ static int __init sysman_init(void)
 		goto err_exit_bios_attr_pass_interface;
 	}
 
-	ret = fw_attributes_class_get(&fw_attr_class);
-	if (ret)
-		goto err_exit_bios_attr_pass_interface;
-
-	wmi_priv.class_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0),
+	wmi_priv.class_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
 				  NULL, "%s", DRIVER_NAME);
 	if (IS_ERR(wmi_priv.class_dev)) {
 		ret = PTR_ERR(wmi_priv.class_dev);
-		goto err_unregister_class;
+		goto err_exit_bios_attr_pass_interface;
 	}
 
 	wmi_priv.main_dir_kset = kset_create_and_add("attributes", NULL,
@@ -602,10 +597,7 @@ static int __init sysman_init(void)
 	release_attributes_data();
 
 err_destroy_classdev:
-	device_destroy(fw_attr_class, MKDEV(0, 0));
-
-err_unregister_class:
-	fw_attributes_class_put();
+	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
 
 err_exit_bios_attr_pass_interface:
 	exit_bios_attr_pass_interface();
@@ -619,8 +611,7 @@ static int __init sysman_init(void)
 static void __exit sysman_exit(void)
 {
 	release_attributes_data();
-	device_destroy(fw_attr_class, MKDEV(0, 0));
-	fw_attributes_class_put();
+	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
 	exit_bios_attr_set_interface();
 	exit_bios_attr_pass_interface();
 }

-- 
2.47.1

[PATCH 6/6] platform/x86: firmware_attributes_class: Drop lifecycle functions

[Thomas Weißschuh]

There are no users left.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/firmware_attributes_class.c | 13 -------------
 drivers/platform/x86/firmware_attributes_class.h |  2 --
 2 files changed, 15 deletions(-)

diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 87672c49e86ae3ef5b99aa99be532c1d84805adc..736e96c186d9dc6d945517f090e9af903e93bbf4 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -22,19 +22,6 @@ static __exit void fw_attributes_class_exit(void)
 }
 module_exit(fw_attributes_class_exit);
 
-int fw_attributes_class_get(const struct class **fw_attr_class)
-{
-	*fw_attr_class = &firmware_attributes_class;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(fw_attributes_class_get);
-
-int fw_attributes_class_put(void)
-{
-	return 0;
-}
-EXPORT_SYMBOL_GPL(fw_attributes_class_put);
-
 MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
 MODULE_DESCRIPTION("Firmware attributes class helper module");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index ef6c3764a83497ad7e75b0102154c92ce476e5ae..d27abe54fcf9812a2f0868eec5426bbc8e7eb21c 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -8,7 +8,5 @@
 #include <linux/device/class.h>
 
 extern const struct class firmware_attributes_class;
-int fw_attributes_class_get(const struct class **fw_attr_class);
-int fw_attributes_class_put(void);
 
 #endif /* FW_ATTR_CLASS_H */

-- 
2.47.1

Re: [PATCH 0/6] platform/x86: firmware_attributes_class: Simplify API

[Mario Limonciello]

On 1/3/2025 17:05, Thomas Weißschuh wrote:
> Looking at the users of firmware_attributes_class makes my head hurt.
> Simplify the subsystem and its users.
> 
> This will break the currently developed samsung-galaxybook driver,
> resolving the breakage should be trivial.

It will also break the lenovo legion wmi drivers that are in review 
right now too, but I agree it should be trivial to fix that too.

> 
> Only compile-tested.
> 
> I have some further improvements in the pipeline building on these
> changes, but those will need some more time.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
> Thomas Weißschuh (6):
>        platform/x86: firmware_attributes_class: Move include linux/device/class.h
>        platform/x86: firmware_attributes_class: Simplify API
>        platform/x86: think-lmi: Directly use firmware_attributes_class
>        platform/x86: hp-bioscfg: Directly use firmware_attributes_class
>        platform/x86: dell-sysman: Directly use firmware_attributes_class
>        platform/x86: firmware_attributes_class: Drop lifecycle functions
> 
>   drivers/platform/x86/dell/dell-wmi-sysman/sysman.c | 17 +++------
>   drivers/platform/x86/firmware_attributes_class.c   | 42 +++++-----------------
>   drivers/platform/x86/firmware_attributes_class.h   |  5 +--
>   drivers/platform/x86/hp/hp-bioscfg/bioscfg.c       | 14 ++------
>   drivers/platform/x86/think-lmi.c                   | 13 ++-----
>   5 files changed, 21 insertions(+), 70 deletions(-)
> ---
> base-commit: 0bc21e701a6ffacfdde7f04f87d664d82e8a13bf
> change-id: 20250103-firmware-attributes-simplify-9ae561459260
> 
> Best regards,

Re: [PATCH 1/6] platform/x86: firmware_attributes_class: Move include linux/device/class.h

[Armin Wolf]

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

Re: [PATCH 1/6] platform/x86: firmware_attributes_class: Move include linux/device/class.h

[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.

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

Re: [PATCH 1/6] platform/x86: firmware_attributes_class: Move include linux/device/class.h

[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

>>> +
>>>    int fw_attributes_class_get(const struct class **fw_attr_class);
>>>    int fw_attributes_class_put(void);
>>>
>>>

Re: [PATCH 1/6] platform/x86: firmware_attributes_class: Move include linux/device/class.h

[Armin Wolf]

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

Re: [PATCH 0/6] platform/x86: firmware_attributes_class: Simplify API

[Mark Pearson]

Hi Thomas,

On Fri, Jan 3, 2025, at 6:05 PM, Thomas Weißschuh wrote:
> Looking at the users of firmware_attributes_class makes my head hurt.
> Simplify the subsystem and its users.
>
> This will break the currently developed samsung-galaxybook driver,
> resolving the breakage should be trivial.
>
> Only compile-tested.
>
> I have some further improvements in the pipeline building on these
> changes, but those will need some more time.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Thomas Weißschuh (6):
>       platform/x86: firmware_attributes_class: Move include linux/device/class.h
>       platform/x86: firmware_attributes_class: Simplify API
>       platform/x86: think-lmi: Directly use firmware_attributes_class
>       platform/x86: hp-bioscfg: Directly use firmware_attributes_class
>       platform/x86: dell-sysman: Directly use firmware_attributes_class
>       platform/x86: firmware_attributes_class: Drop lifecycle functions
>
>  drivers/platform/x86/dell/dell-wmi-sysman/sysman.c | 17 +++------
>  drivers/platform/x86/firmware_attributes_class.c   | 42 +++++-----------------
>  drivers/platform/x86/firmware_attributes_class.h   |  5 +--
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c       | 14 ++------
>  drivers/platform/x86/think-lmi.c                   | 13 ++-----
>  5 files changed, 21 insertions(+), 70 deletions(-)
> ---
> base-commit: 0bc21e701a6ffacfdde7f04f87d664d82e8a13bf
> change-id: 20250103-firmware-attributes-simplify-9ae561459260
>
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>

For the series - looks good to me. Tested on X1 Carbon G12 and confirmed think-lmi is working as expected.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

Re: [PATCH 0/6] platform/x86: firmware_attributes_class: Simplify API

[Ilpo Järvinen]

On Sat, 04 Jan 2025 00:05:08 +0100, Thomas Weißschuh wrote:

> Looking at the users of firmware_attributes_class makes my head hurt.
> Simplify the subsystem and its users.
> 
> This will break the currently developed samsung-galaxybook driver,
> resolving the breakage should be trivial.
> 
> Only compile-tested.
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo-next branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-next branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/6] platform/x86: firmware_attributes_class: Move include linux/device/class.h
      commit: 34a5894672f73ebce2a7efec2a8a7d785c76adab
[2/6] platform/x86: firmware_attributes_class: Simplify API
      commit: ab12105347d50857aece92b65a3edbe93c0d3060
[3/6] platform/x86: think-lmi: Directly use firmware_attributes_class
      commit: 20d23c51a0d69c38f4f2b02ae969b9cb33cf1555
[4/6] platform/x86: hp-bioscfg: Directly use firmware_attributes_class
      commit: 2765aa5e28fdab6590e9b4633a2f7ad26f3b7392
[5/6] platform/x86: dell-sysman: Directly use firmware_attributes_class
      commit: e8866e8892753dcdad48d6aa9192d91dcd32d94c
[6/6] platform/x86: firmware_attributes_class: Drop lifecycle functions
      commit: c200f9b6c2a461fbbdae3a04fee9a045ec215297

--
 i.