[PATCH 4/5] alienware-wmi: Fix module init error handling

[PATCH 4/5] alienware-wmi: Fix module init error handling

[Kurt Borja]

Propagate led_classdev_register return value in case of error.
Call led_classdev_unregister in case sysfs_create_group fails.

If alienware_zone_init fails, alienware_zone_exit should not be called
because the latter unregisters/removes the led class and the sysfs
group, which may not be registered/created if the former failed
prematurely.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/dell/alienware-wmi.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 980ffc545093..44f1f7b57d0a 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -628,6 +628,7 @@ static int alienware_zone_init(struct platform_device *pdev)
 	char *name;
 	struct device_attribute *zone_dev_attrs;
 	struct attribute **zone_attrs;
+	int ret;
 
 	if (interface == WMAX) {
 		lighting_control_state = WMAX_RUNNING;
@@ -675,9 +676,19 @@ static int alienware_zone_init(struct platform_device *pdev)
 	zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr;
 	zone_attribute_group.attrs = zone_attrs;
 
-	led_classdev_register(&pdev->dev, &global_led);
+	ret = led_classdev_register(&pdev->dev, &global_led);
+	if (ret < 0)
+		return ret;
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &zone_attribute_group);
+	if (ret < 0)
+		goto fail_prep_zone_group;
+
+	return 0;
 
-	return sysfs_create_group(&pdev->dev.kobj, &zone_attribute_group);
+fail_prep_zone_group:
+	led_classdev_unregister(&global_led);
+	return ret;
 }
 
 static void alienware_zone_exit(struct platform_device *dev)
@@ -1223,7 +1234,6 @@ static int __init alienware_wmi_init(void)
 	return 0;
 
 fail_prep_zones:
-	alienware_zone_exit(platform_device);
 	remove_thermal_profile();
 fail_prep_thermal_profile:
 fail_prep_deepsleep:
-- 
2.47.0

Re: [PATCH 4/5] alienware-wmi: Fix module init error handling

[Markus Elfring]

> Propagate led_classdev_register return value in case of error.
> Call led_classdev_unregister in case sysfs_create_group fails.
>
> If alienware_zone_init fails, alienware_zone_exit should not be called
> because the latter unregisters/removes the led class and the sysfs
> group, which may not be registered/created if the former failed
> prematurely.

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n145

Regards,
Markus

Re: [PATCH 4/5] alienware-wmi: Fix module init error handling

[Kurt Borja]

On Tue, Nov 19, 2024 at 03:55:08PM +0100, Markus Elfring wrote:
> > Propagate led_classdev_register return value in case of error.
> > Call led_classdev_unregister in case sysfs_create_group fails.
> >
> > If alienware_zone_init fails, alienware_zone_exit should not be called
> > because the latter unregisters/removes the led class and the sysfs
> > group, which may not be registered/created if the former failed
> > prematurely.
> 
> How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n145

Sure! I will on v2.

This might be kind of obvious but, if I add the Fixes tag, do I have to
make the patch over that commit, over stable trees or leave it as is?

Regards,
Kurt

> 
> Regards,
> Markus

Re: [PATCH 4/5] alienware-wmi: Fix module init error handling

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

On Tue, 19 Nov 2024, Kurt Borja wrote:

> On Tue, Nov 19, 2024 at 03:55:08PM +0100, Markus Elfring wrote:
> > > Propagate led_classdev_register return value in case of error.
> > > Call led_classdev_unregister in case sysfs_create_group fails.
> > >
> > > If alienware_zone_init fails, alienware_zone_exit should not be called
> > > because the latter unregisters/removes the led class and the sysfs
> > > group, which may not be registered/created if the former failed
> > > prematurely.
> > 
> > How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n145
> 
> Sure! I will on v2.
> 
> This might be kind of obvious but, if I add the Fixes tag, do I have to
> make the patch over that commit, over stable trees or leave it as is?

Hi Kurt,

You do it normally on top of pdx86 fixes branch. In this case because of 
the on-going merge window, you'll have for-next patch to enter there 
before your fix will get merged after -rc1.

Don't worry about stable at this point too much, other than try to have
the fixes patches as early as possible in your series to avoid conflicts 
with the other patches within your own patch series.

-- 
 i.

Re: [PATCH 4/5] alienware-wmi: Fix module init error handling

[Kurt Borja]

On Tue, Nov 19, 2024 at 06:22:34PM +0200, Ilpo Järvinen wrote:
> On Tue, 19 Nov 2024, Kurt Borja wrote:
> 
> > On Tue, Nov 19, 2024 at 03:55:08PM +0100, Markus Elfring wrote:
> > > > Propagate led_classdev_register return value in case of error.
> > > > Call led_classdev_unregister in case sysfs_create_group fails.
> > > >
> > > > If alienware_zone_init fails, alienware_zone_exit should not be called
> > > > because the latter unregisters/removes the led class and the sysfs
> > > > group, which may not be registered/created if the former failed
> > > > prematurely.
> > > 
> > > How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n145
> > 
> > Sure! I will on v2.
> > 
> > This might be kind of obvious but, if I add the Fixes tag, do I have to
> > make the patch over that commit, over stable trees or leave it as is?
> 
> Hi Kurt,
> 
> You do it normally on top of pdx86 fixes branch. In this case because of 
> the on-going merge window, you'll have for-next patch to enter there 
> before your fix will get merged after -rc1.
> 
> Don't worry about stable at this point too much, other than try to have
> the fixes patches as early as possible in your series to avoid conflicts 
> with the other patches within your own patch series.

That's good to know, thank you very much :).

> 
> -- 
>  i.

Re: [PATCH 4/5] alienware-wmi: Fix module init error handling

[Mario Limonciello]

On 11/18/2024 22:35, Kurt Borja wrote:
> Propagate led_classdev_register return value in case of error.
> Call led_classdev_unregister in case sysfs_create_group fails.
> 
> If alienware_zone_init fails, alienware_zone_exit should not be called
> because the latter unregisters/removes the led class and the sysfs
> group, which may not be registered/created if the former failed
> prematurely.
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>   drivers/platform/x86/dell/alienware-wmi.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 980ffc545093..44f1f7b57d0a 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -628,6 +628,7 @@ static int alienware_zone_init(struct platform_device *pdev)
>   	char *name;
>   	struct device_attribute *zone_dev_attrs;
>   	struct attribute **zone_attrs;
> +	int ret;
>   
>   	if (interface == WMAX) {
>   		lighting_control_state = WMAX_RUNNING;
> @@ -675,9 +676,19 @@ static int alienware_zone_init(struct platform_device *pdev)
>   	zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr;
>   	zone_attribute_group.attrs = zone_attrs;
>   
> -	led_classdev_register(&pdev->dev, &global_led);
> +	ret = led_classdev_register(&pdev->dev, &global_led);

How about switching to the devm version of led_classdev_register instead 
so you can keep a simpler cleanup?

> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &zone_attribute_group);
> +	if (ret < 0)
> +		goto fail_prep_zone_group;
> +
> +	return 0;
>   
> -	return sysfs_create_group(&pdev->dev.kobj, &zone_attribute_group);
> +fail_prep_zone_group:
> +	led_classdev_unregister(&global_led);
> +	return ret;
>   }
>   
>   static void alienware_zone_exit(struct platform_device *dev)
> @@ -1223,7 +1234,6 @@ static int __init alienware_wmi_init(void)
>   	return 0;
>   
>   fail_prep_zones:
> -	alienware_zone_exit(platform_device);
>   	remove_thermal_profile();
>   fail_prep_thermal_profile:
>   fail_prep_deepsleep:

Re: [PATCH 4/5] alienware-wmi: Fix module init error handling

[Kurt Borja]

On Tue, Nov 19, 2024 at 10:06:48AM -0600, Mario Limonciello wrote:
> On 11/18/2024 22:35, Kurt Borja wrote:
> > Propagate led_classdev_register return value in case of error.
> > Call led_classdev_unregister in case sysfs_create_group fails.
> > 
> > If alienware_zone_init fails, alienware_zone_exit should not be called
> > because the latter unregisters/removes the led class and the sysfs
> > group, which may not be registered/created if the former failed
> > prematurely.
> > 
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > ---
> >   drivers/platform/x86/dell/alienware-wmi.c | 16 +++++++++++++---
> >   1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > index 980ffc545093..44f1f7b57d0a 100644
> > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > @@ -628,6 +628,7 @@ static int alienware_zone_init(struct platform_device *pdev)
> >   	char *name;
> >   	struct device_attribute *zone_dev_attrs;
> >   	struct attribute **zone_attrs;
> > +	int ret;
> >   	if (interface == WMAX) {
> >   		lighting_control_state = WMAX_RUNNING;
> > @@ -675,9 +676,19 @@ static int alienware_zone_init(struct platform_device *pdev)
> >   	zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr;
> >   	zone_attribute_group.attrs = zone_attrs;
> > -	led_classdev_register(&pdev->dev, &global_led);
> > +	ret = led_classdev_register(&pdev->dev, &global_led);
> 
> How about switching to the devm version of led_classdev_register instead so
> you can keep a simpler cleanup?

Sure! I missed that function.

> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sysfs_create_group(&pdev->dev.kobj, &zone_attribute_group);
> > +	if (ret < 0)
> > +		goto fail_prep_zone_group;
> > +
> > +	return 0;
> > -	return sysfs_create_group(&pdev->dev.kobj, &zone_attribute_group);
> > +fail_prep_zone_group:
> > +	led_classdev_unregister(&global_led);
> > +	return ret;
> >   }
> >   static void alienware_zone_exit(struct platform_device *dev)
> > @@ -1223,7 +1234,6 @@ static int __init alienware_wmi_init(void)
> >   	return 0;
> >   fail_prep_zones:
> > -	alienware_zone_exit(platform_device);
> >   	remove_thermal_profile();
> >   fail_prep_thermal_profile:
> >   fail_prep_deepsleep:
>