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