| Message ID | 20241119043523.25650-1-kuurtb@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | alienware-wmi: Fixes and improvements | expand |
> 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
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
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:
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: >
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.
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.
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:
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(-)