[PATCH 5/5] alienware-wmi: Improves sysfs groups creation

[PATCH 5/5] alienware-wmi: Improves sysfs groups creation

[Kurt Borja]

Devices with hdmi_mux, amplifier or deepslp quirks create a sysfs group
for each available feature. To accomplish this, helper create/remove
functions were called on module init, but they had the following
problems:

 - Create helpers called remove helpers on failure, which in turn tried
   to remove the sysfs group that failed to be created
 - If group creation failed mid way, previous successfully created groups
   were not cleaned up
 - Module exit only removed hdmi_mux group

To improve this, drop all helpers in favor of two helpers that make use
of sysfs_create_groups/sysfs_remove_groups to cleanly create/remove
groups at module init/exit.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---

I have a question. Do the created sysfs groups get removed when their
kobj reference count goes to 0? I ask because I want to know if this is
a bug fix.

---
 drivers/platform/x86/dell/alienware-wmi.c | 105 ++++++++--------------
 1 file changed, 36 insertions(+), 69 deletions(-)

diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 44f1f7b57d0a..e9ed2089cba0 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -410,8 +410,10 @@ struct wmax_u32_args {
 	u8 arg3;
 };
 
+
 static struct platform_device *platform_device;
 static struct platform_zone *zone_data;
+const struct attribute_group *wmax_groups[4];
 static struct platform_profile_handler pp_handler;
 static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
 
@@ -810,22 +812,6 @@ static const struct attribute_group hdmi_attribute_group = {
 	.attrs = hdmi_attrs,
 };
 
-static void remove_hdmi(struct platform_device *dev)
-{
-	if (quirks->hdmi_mux > 0)
-		sysfs_remove_group(&dev->dev.kobj, &hdmi_attribute_group);
-}
-
-static int create_hdmi(struct platform_device *dev)
-{
-	int ret;
-
-	ret = sysfs_create_group(&dev->dev.kobj, &hdmi_attribute_group);
-	if (ret)
-		remove_hdmi(dev);
-	return ret;
-}
-
 /*
  * Alienware GFX amplifier support
  * - Currently supports reading cable status
@@ -864,22 +850,6 @@ static const struct attribute_group amplifier_attribute_group = {
 	.attrs = amplifier_attrs,
 };
 
-static void remove_amplifier(struct platform_device *dev)
-{
-	if (quirks->amplifier > 0)
-		sysfs_remove_group(&dev->dev.kobj, &amplifier_attribute_group);
-}
-
-static int create_amplifier(struct platform_device *dev)
-{
-	int ret;
-
-	ret = sysfs_create_group(&dev->dev.kobj, &amplifier_attribute_group);
-	if (ret)
-		remove_amplifier(dev);
-	return ret;
-}
-
 /*
  * Deep Sleep Control support
  * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
@@ -942,22 +912,6 @@ static const struct attribute_group deepsleep_attribute_group = {
 	.attrs = deepsleep_attrs,
 };
 
-static void remove_deepsleep(struct platform_device *dev)
-{
-	if (quirks->deepslp > 0)
-		sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
-}
-
-static int create_deepsleep(struct platform_device *dev)
-{
-	int ret;
-
-	ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
-	if (ret)
-		remove_deepsleep(dev);
-	return ret;
-}
-
 /*
  * Thermal Profile control
  *  - Provides thermal profile control through the Platform Profile API
@@ -1165,6 +1119,34 @@ static void remove_thermal_profile(void)
 		platform_profile_remove();
 }
 
+static int __init create_wmax_groups(struct platform_device *pdev)
+{
+	int no_groups = 0;
+
+	if (quirks->hdmi_mux) {
+		wmax_groups[no_groups] = &hdmi_attribute_group;
+		no_groups++;
+	}
+
+	if (quirks->amplifier) {
+		wmax_groups[no_groups] = &amplifier_attribute_group;
+		no_groups++;
+	}
+
+	if (quirks->deepslp) {
+		wmax_groups[no_groups] = &deepsleep_attribute_group;
+		no_groups++;
+	}
+
+	return no_groups > 0 ? device_add_groups(&pdev->dev, wmax_groups) : 0;
+}
+
+static void __exit remove_wmax_groups(struct platform_device *pdev)
+{
+	if (!wmax_groups[0])
+		device_remove_groups(&pdev->dev, wmax_groups);
+}
+
 static int __init alienware_wmi_init(void)
 {
 	int ret;
@@ -1203,23 +1185,9 @@ static int __init alienware_wmi_init(void)
 		goto fail_platform_device1;
 	}
 
-	if (quirks->hdmi_mux > 0) {
-		ret = create_hdmi(platform_device);
-		if (ret)
-			goto fail_prep_hdmi;
-	}
-
-	if (quirks->amplifier > 0) {
-		ret = create_amplifier(platform_device);
-		if (ret)
-			goto fail_prep_amplifier;
-	}
-
-	if (quirks->deepslp > 0) {
-		ret = create_deepsleep(platform_device);
-		if (ret)
-			goto fail_prep_deepsleep;
-	}
+	ret = create_wmax_groups(platform_device);
+	if (ret)
+		goto fail_prep_groups;
 
 	if (quirks->thermal) {
 		ret = create_thermal_profile();
@@ -1236,9 +1204,8 @@ static int __init alienware_wmi_init(void)
 fail_prep_zones:
 	remove_thermal_profile();
 fail_prep_thermal_profile:
-fail_prep_deepsleep:
-fail_prep_amplifier:
-fail_prep_hdmi:
+	remove_wmax_groups(platform_device);
+fail_prep_groups:
 	platform_device_unregister(platform_device);
 fail_platform_device1:
 	platform_driver_unregister(&platform_driver);
@@ -1251,7 +1218,7 @@ module_init(alienware_wmi_init);
 static void __exit alienware_wmi_exit(void)
 {
 	alienware_zone_exit(platform_device);
-	remove_hdmi(platform_device);
+	remove_wmax_groups(platform_device);
 	remove_thermal_profile();
 	platform_device_unregister(platform_device);
 	platform_driver_unregister(&platform_driver);
-- 
2.47.0

Re: [PATCH 5/5] alienware-wmi: Improves sysfs groups creation

[Ilpo Järvinen]

On Tue, 19 Nov 2024, Kurt Borja wrote:

> Devices with hdmi_mux, amplifier or deepslp quirks create a sysfs group
> for each available feature. To accomplish this, helper create/remove
> functions were called on module init, but they had the following
> problems:
> 
>  - Create helpers called remove helpers on failure, which in turn tried
>    to remove the sysfs group that failed to be created
>  - If group creation failed mid way, previous successfully created groups
>    were not cleaned up
>  - Module exit only removed hdmi_mux group
> 
> To improve this, drop all helpers in favor of two helpers that make use
> of sysfs_create_groups/sysfs_remove_groups to cleanly create/remove
> groups at module init/exit.
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> 
> I have a question. Do the created sysfs groups get removed when their
> kobj reference count goes to 0? I ask because I want to know if this is
> a bug fix.
> 
> ---
>  drivers/platform/x86/dell/alienware-wmi.c | 105 ++++++++--------------
>  1 file changed, 36 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 44f1f7b57d0a..e9ed2089cba0 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -410,8 +410,10 @@ struct wmax_u32_args {
>  	u8 arg3;
>  };
>  
> +
>  static struct platform_device *platform_device;
>  static struct platform_zone *zone_data;
> +const struct attribute_group *wmax_groups[4];
>  static struct platform_profile_handler pp_handler;
>  static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
>  
> @@ -810,22 +812,6 @@ static const struct attribute_group hdmi_attribute_group = {
>  	.attrs = hdmi_attrs,
>  };
>  
> -static void remove_hdmi(struct platform_device *dev)
> -{
> -	if (quirks->hdmi_mux > 0)
> -		sysfs_remove_group(&dev->dev.kobj, &hdmi_attribute_group);
> -}
> -
> -static int create_hdmi(struct platform_device *dev)
> -{
> -	int ret;
> -
> -	ret = sysfs_create_group(&dev->dev.kobj, &hdmi_attribute_group);
> -	if (ret)
> -		remove_hdmi(dev);
> -	return ret;
> -}
> -
>  /*
>   * Alienware GFX amplifier support
>   * - Currently supports reading cable status
> @@ -864,22 +850,6 @@ static const struct attribute_group amplifier_attribute_group = {
>  	.attrs = amplifier_attrs,
>  };
>  
> -static void remove_amplifier(struct platform_device *dev)
> -{
> -	if (quirks->amplifier > 0)
> -		sysfs_remove_group(&dev->dev.kobj, &amplifier_attribute_group);
> -}
> -
> -static int create_amplifier(struct platform_device *dev)
> -{
> -	int ret;
> -
> -	ret = sysfs_create_group(&dev->dev.kobj, &amplifier_attribute_group);
> -	if (ret)
> -		remove_amplifier(dev);
> -	return ret;
> -}
> -
>  /*
>   * Deep Sleep Control support
>   * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
> @@ -942,22 +912,6 @@ static const struct attribute_group deepsleep_attribute_group = {
>  	.attrs = deepsleep_attrs,
>  };
>  
> -static void remove_deepsleep(struct platform_device *dev)
> -{
> -	if (quirks->deepslp > 0)
> -		sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
> -}
> -
> -static int create_deepsleep(struct platform_device *dev)
> -{
> -	int ret;
> -
> -	ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
> -	if (ret)
> -		remove_deepsleep(dev);
> -	return ret;
> -}
> -
>  /*
>   * Thermal Profile control
>   *  - Provides thermal profile control through the Platform Profile API
> @@ -1165,6 +1119,34 @@ static void remove_thermal_profile(void)
>  		platform_profile_remove();
>  }
>  
> +static int __init create_wmax_groups(struct platform_device *pdev)
> +{
> +	int no_groups = 0;
> +
> +	if (quirks->hdmi_mux) {
> +		wmax_groups[no_groups] = &hdmi_attribute_group;
> +		no_groups++;
> +	}
> +
> +	if (quirks->amplifier) {
> +		wmax_groups[no_groups] = &amplifier_attribute_group;
> +		no_groups++;
> +	}
> +
> +	if (quirks->deepslp) {
> +		wmax_groups[no_groups] = &deepsleep_attribute_group;
> +		no_groups++;
> +	}
> +
> +	return no_groups > 0 ? device_add_groups(&pdev->dev, wmax_groups) : 0;

Couldn't these use .dev_groups and .is_visible?

-- 
 i.


> +}
> +
> +static void __exit remove_wmax_groups(struct platform_device *pdev)
> +{
> +	if (!wmax_groups[0])
> +		device_remove_groups(&pdev->dev, wmax_groups);
> +}
> +
>  static int __init alienware_wmi_init(void)
>  {
>  	int ret;
> @@ -1203,23 +1185,9 @@ static int __init alienware_wmi_init(void)
>  		goto fail_platform_device1;
>  	}
>  
> -	if (quirks->hdmi_mux > 0) {
> -		ret = create_hdmi(platform_device);
> -		if (ret)
> -			goto fail_prep_hdmi;
> -	}
> -
> -	if (quirks->amplifier > 0) {
> -		ret = create_amplifier(platform_device);
> -		if (ret)
> -			goto fail_prep_amplifier;
> -	}
> -
> -	if (quirks->deepslp > 0) {
> -		ret = create_deepsleep(platform_device);
> -		if (ret)
> -			goto fail_prep_deepsleep;
> -	}
> +	ret = create_wmax_groups(platform_device);
> +	if (ret)
> +		goto fail_prep_groups;
>  
>  	if (quirks->thermal) {
>  		ret = create_thermal_profile();
> @@ -1236,9 +1204,8 @@ static int __init alienware_wmi_init(void)
>  fail_prep_zones:
>  	remove_thermal_profile();
>  fail_prep_thermal_profile:
> -fail_prep_deepsleep:
> -fail_prep_amplifier:
> -fail_prep_hdmi:
> +	remove_wmax_groups(platform_device);
> +fail_prep_groups:
>  	platform_device_unregister(platform_device);
>  fail_platform_device1:
>  	platform_driver_unregister(&platform_driver);
> @@ -1251,7 +1218,7 @@ module_init(alienware_wmi_init);
>  static void __exit alienware_wmi_exit(void)
>  {
>  	alienware_zone_exit(platform_device);
> -	remove_hdmi(platform_device);
> +	remove_wmax_groups(platform_device);
>  	remove_thermal_profile();
>  	platform_device_unregister(platform_device);
>  	platform_driver_unregister(&platform_driver);
> 

Re: [PATCH 5/5] alienware-wmi: Improves sysfs groups creation

[Kurt Borja]

On Tue, Nov 19, 2024 at 10:40:50AM +0200, Ilpo Järvinen wrote:
> On Tue, 19 Nov 2024, Kurt Borja wrote:
> 
> > Devices with hdmi_mux, amplifier or deepslp quirks create a sysfs group
> > for each available feature. To accomplish this, helper create/remove
> > functions were called on module init, but they had the following
> > problems:
> > 
> >  - Create helpers called remove helpers on failure, which in turn tried
> >    to remove the sysfs group that failed to be created
> >  - If group creation failed mid way, previous successfully created groups
> >    were not cleaned up
> >  - Module exit only removed hdmi_mux group
> > 
> > To improve this, drop all helpers in favor of two helpers that make use
> > of sysfs_create_groups/sysfs_remove_groups to cleanly create/remove
> > groups at module init/exit.
> > 
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > ---
> > 
> > I have a question. Do the created sysfs groups get removed when their
> > kobj reference count goes to 0? I ask because I want to know if this is
> > a bug fix.
> > 
> > ---
> >  drivers/platform/x86/dell/alienware-wmi.c | 105 ++++++++--------------
> >  1 file changed, 36 insertions(+), 69 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > index 44f1f7b57d0a..e9ed2089cba0 100644
> > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > @@ -410,8 +410,10 @@ struct wmax_u32_args {
> >  	u8 arg3;
> >  };
> >  
> > +
> >  static struct platform_device *platform_device;
> >  static struct platform_zone *zone_data;
> > +const struct attribute_group *wmax_groups[4];
> >  static struct platform_profile_handler pp_handler;
> >  static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
> >  
> > @@ -810,22 +812,6 @@ static const struct attribute_group hdmi_attribute_group = {
> >  	.attrs = hdmi_attrs,
> >  };
> >  
> > -static void remove_hdmi(struct platform_device *dev)
> > -{
> > -	if (quirks->hdmi_mux > 0)
> > -		sysfs_remove_group(&dev->dev.kobj, &hdmi_attribute_group);
> > -}
> > -
> > -static int create_hdmi(struct platform_device *dev)
> > -{
> > -	int ret;
> > -
> > -	ret = sysfs_create_group(&dev->dev.kobj, &hdmi_attribute_group);
> > -	if (ret)
> > -		remove_hdmi(dev);
> > -	return ret;
> > -}
> > -
> >  /*
> >   * Alienware GFX amplifier support
> >   * - Currently supports reading cable status
> > @@ -864,22 +850,6 @@ static const struct attribute_group amplifier_attribute_group = {
> >  	.attrs = amplifier_attrs,
> >  };
> >  
> > -static void remove_amplifier(struct platform_device *dev)
> > -{
> > -	if (quirks->amplifier > 0)
> > -		sysfs_remove_group(&dev->dev.kobj, &amplifier_attribute_group);
> > -}
> > -
> > -static int create_amplifier(struct platform_device *dev)
> > -{
> > -	int ret;
> > -
> > -	ret = sysfs_create_group(&dev->dev.kobj, &amplifier_attribute_group);
> > -	if (ret)
> > -		remove_amplifier(dev);
> > -	return ret;
> > -}
> > -
> >  /*
> >   * Deep Sleep Control support
> >   * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
> > @@ -942,22 +912,6 @@ static const struct attribute_group deepsleep_attribute_group = {
> >  	.attrs = deepsleep_attrs,
> >  };
> >  
> > -static void remove_deepsleep(struct platform_device *dev)
> > -{
> > -	if (quirks->deepslp > 0)
> > -		sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
> > -}
> > -
> > -static int create_deepsleep(struct platform_device *dev)
> > -{
> > -	int ret;
> > -
> > -	ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
> > -	if (ret)
> > -		remove_deepsleep(dev);
> > -	return ret;
> > -}
> > -
> >  /*
> >   * Thermal Profile control
> >   *  - Provides thermal profile control through the Platform Profile API
> > @@ -1165,6 +1119,34 @@ static void remove_thermal_profile(void)
> >  		platform_profile_remove();
> >  }
> >  
> > +static int __init create_wmax_groups(struct platform_device *pdev)
> > +{
> > +	int no_groups = 0;
> > +
> > +	if (quirks->hdmi_mux) {
> > +		wmax_groups[no_groups] = &hdmi_attribute_group;
> > +		no_groups++;
> > +	}
> > +
> > +	if (quirks->amplifier) {
> > +		wmax_groups[no_groups] = &amplifier_attribute_group;
> > +		no_groups++;
> > +	}
> > +
> > +	if (quirks->deepslp) {
> > +		wmax_groups[no_groups] = &deepsleep_attribute_group;
> > +		no_groups++;
> > +	}
> > +
> > +	return no_groups > 0 ? device_add_groups(&pdev->dev, wmax_groups) : 0;
> 
> Couldn't these use .dev_groups and .is_visible?

You're right, I will use this instead!

> 
> -- 
>  i.
> 
> 
> > +}
> > +
> > +static void __exit remove_wmax_groups(struct platform_device *pdev)
> > +{
> > +	if (!wmax_groups[0])
> > +		device_remove_groups(&pdev->dev, wmax_groups);
> > +}
> > +
> >  static int __init alienware_wmi_init(void)
> >  {
> >  	int ret;
> > @@ -1203,23 +1185,9 @@ static int __init alienware_wmi_init(void)
> >  		goto fail_platform_device1;
> >  	}
> >  
> > -	if (quirks->hdmi_mux > 0) {
> > -		ret = create_hdmi(platform_device);
> > -		if (ret)
> > -			goto fail_prep_hdmi;
> > -	}
> > -
> > -	if (quirks->amplifier > 0) {
> > -		ret = create_amplifier(platform_device);
> > -		if (ret)
> > -			goto fail_prep_amplifier;
> > -	}
> > -
> > -	if (quirks->deepslp > 0) {
> > -		ret = create_deepsleep(platform_device);
> > -		if (ret)
> > -			goto fail_prep_deepsleep;
> > -	}
> > +	ret = create_wmax_groups(platform_device);
> > +	if (ret)
> > +		goto fail_prep_groups;
> >  
> >  	if (quirks->thermal) {
> >  		ret = create_thermal_profile();
> > @@ -1236,9 +1204,8 @@ static int __init alienware_wmi_init(void)
> >  fail_prep_zones:
> >  	remove_thermal_profile();
> >  fail_prep_thermal_profile:
> > -fail_prep_deepsleep:
> > -fail_prep_amplifier:
> > -fail_prep_hdmi:
> > +	remove_wmax_groups(platform_device);
> > +fail_prep_groups:
> >  	platform_device_unregister(platform_device);
> >  fail_platform_device1:
> >  	platform_driver_unregister(&platform_driver);
> > @@ -1251,7 +1218,7 @@ module_init(alienware_wmi_init);
> >  static void __exit alienware_wmi_exit(void)
> >  {
> >  	alienware_zone_exit(platform_device);
> > -	remove_hdmi(platform_device);
> > +	remove_wmax_groups(platform_device);
> >  	remove_thermal_profile();
> >  	platform_device_unregister(platform_device);
> >  	platform_driver_unregister(&platform_driver);
> > 
>