| Message ID | 20170407130713.8417-6-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Fri, Apr 07, 2017 at 03:07:12PM +0200, Michał Kępień wrote: > If acpi_fujitsu_laptop_leds_register() returns an error, the latter will > become the return value of acpi_fujitsu_laptop_add(), which in turn will > be reported by driver core. Simplify code by replacing pr_err() calls > with return statements. Return 0 instead of result when no errors occur > in order to make the code easier to read. Hi Michał, Jonathan's comment regarding the information loss of removing the pr_err statements seems valid to me. Based on the outer if block, I take it each registration only fails in true error scenarios and not because some laptop might have one but not another LED in the list. If so, then the pr_err messages would only appear when there was a legitimate problem. I think they're worth This seems to introduce a behavior change as well. Previously only the last LED registered would determine the result - which is wrong of course and I believe you noted a related bug in an early patch. Previously, however, if LOGOLAMP_POWERON failed, for example, the KEYBOARD_LAMPS would still be attempted. So the question really comes down to this: Is there a legitimate situation in which one LEDs registration fails and another succeeds? If so, then this would constitute a regression for such systems. > > Signed-off-by: Michał Kępień <kernel@kempniu.pl> > --- > drivers/platform/x86/fujitsu-laptop.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c > index ce658789e748..177b9b57ac2f 100644 > --- a/drivers/platform/x86/fujitsu-laptop.c > +++ b/drivers/platform/x86/fujitsu-laptop.c > @@ -739,22 +739,20 @@ static struct led_classdev eco_led = { > > static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) > { > - int result = 0; > + int result; > > if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) { > result = devm_led_classdev_register(&device->dev, > &logolamp_led); > if (result) > - pr_err("Could not register LED handler for logo lamp, error %i\n", > - result); > + return result; > } > > if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) && > (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) { > result = devm_led_classdev_register(&device->dev, &kblamps_led); > if (result) > - pr_err("Could not register LED handler for keyboard lamps, error %i\n", > - result); > + return result; > } > > /* > @@ -766,8 +764,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) > if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) { > result = devm_led_classdev_register(&device->dev, &radio_led); > if (result) > - pr_err("Could not register LED handler for radio LED, error %i\n", > - result); > + return result; > } > > /* Support for eco led is not always signaled in bit corresponding > @@ -779,11 +776,10 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) > (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) { > result = devm_led_classdev_register(&device->dev, &eco_led); > if (result) > - pr_err("Could not register LED handler for eco LED, error %i\n", > - result); > + return result; > } > > - return result; > + return 0; > } > > static int acpi_fujitsu_laptop_add(struct acpi_device *device) > -- > 2.12.2 > >
Jonathan, I hope this response to Darren's message also addresses your concerns. Feel free to let me know if it does not. > On Fri, Apr 07, 2017 at 03:07:12PM +0200, Michał Kępień wrote: > > If acpi_fujitsu_laptop_leds_register() returns an error, the latter will > > become the return value of acpi_fujitsu_laptop_add(), which in turn will > > be reported by driver core. Simplify code by replacing pr_err() calls > > with return statements. Return 0 instead of result when no errors occur > > in order to make the code easier to read. > > Hi Michał, > > Jonathan's comment regarding the information loss of removing the pr_err > statements seems valid to me. Based on the outer if block, I take it each > registration only fails in true error scenarios and not because some laptop > might have one but not another LED in the list. Correct. > If so, then the pr_err messages > would only appear when there was a legitimate problem. I think they're worth I am not hell-bent on removing these pr_err() calls, but allow me to briefly walk you through my thought process. devm_led_classdev_register() is basically a managed wrapper for led_classdev_register(), so let's see under what circumstances the latter may fail. While it does quite a bit, its return value can only be different than zero for one of two reasons: - there is already a LED with the same name present in the system, so the kernel automatically renames the one we are registering and the length of the generated name exceeds LED_MAX_NAME_SIZE, - device_create_with_groups() fails, either because we are out of memory or the device hierarchy is screwed up. The first case will never happen, given that the longest LED name that fujitsu-laptop tries to register is 18 bytes long, the counter used for auto-incrementation is an unsigned int and LED_MAX_NAME_SIZE is 64. In the second case, we are likely to be notified by driver core about the exact nature of the failure, but more importantly, logging which LED "caused" the failure makes us none the wiser. Actions taken by the kernel in response to each of the devm_led_classdev_register() calls are virtually identical and if any of these fails, we are more than likely to have problems way more severe than non-functioning LEDs. Have I missed anything or perhaps assumed something I should have not? > This seems to introduce a behavior change as well. Previously only the last > LED registered would determine the result - which is wrong of course and I > believe you noted a related bug in an early patch. Previously, however, if > LOGOLAMP_POWERON failed, for example, the KEYBOARD_LAMPS would still be attempted. > > So the question really comes down to this: Is there a legitimate situation in > which one LEDs registration fails and another succeeds? If so, then this would > constitute a regression for such systems. The behavior change you mentioned is intentional. As pointed out above, if any devm_led_classdev_register() call fails, it means we have reached some inconsistent state which is really unlikely to be improved by further attempts to register even more devices. What do you guys think?
On Tue, Apr 18, 2017 at 10:10:01AM +0200, Michał Kępień wrote: > Jonathan, I hope this response to Darren's message also addresses your > concerns. Feel free to let me know if it does not. > > > On Fri, Apr 07, 2017 at 03:07:12PM +0200, Michał Kępień wrote: > > > If acpi_fujitsu_laptop_leds_register() returns an error, the latter will > > > become the return value of acpi_fujitsu_laptop_add(), which in turn will > > > be reported by driver core. Simplify code by replacing pr_err() calls > > > with return statements. Return 0 instead of result when no errors occur > > > in order to make the code easier to read. > > > > Hi Michał, > > > > Jonathan's comment regarding the information loss of removing the pr_err > > statements seems valid to me. Based on the outer if block, I take it each > > registration only fails in true error scenarios and not because some laptop > > might have one but not another LED in the list. > > Correct. > > > If so, then the pr_err messages > > would only appear when there was a legitimate problem. I think they're worth > > I am not hell-bent on removing these pr_err() calls, but allow me to > briefly walk you through my thought process. > > devm_led_classdev_register() is basically a managed wrapper for > led_classdev_register(), so let's see under what circumstances the > latter may fail. While it does quite a bit, its return value can only > be different than zero for one of two reasons: > > - there is already a LED with the same name present in the system, so > the kernel automatically renames the one we are registering and the > length of the generated name exceeds LED_MAX_NAME_SIZE, > > - device_create_with_groups() fails, either because we are out of > memory or the device hierarchy is screwed up. > > The first case will never happen, given that the longest LED name that > fujitsu-laptop tries to register is 18 bytes long, the counter used for > auto-incrementation is an unsigned int and LED_MAX_NAME_SIZE is 64. > > In the second case, we are likely to be notified by driver core about > the exact nature of the failure, but more importantly, logging which LED > "caused" the failure makes us none the wiser. Actions taken by the > kernel in response to each of the devm_led_classdev_register() calls are > virtually identical and if any of these fails, we are more than likely > to have problems way more severe than non-functioning LEDs. > > Have I missed anything or perhaps assumed something I should have not? > > > This seems to introduce a behavior change as well. Previously only the last > > LED registered would determine the result - which is wrong of course and I > > believe you noted a related bug in an early patch. Previously, however, if > > LOGOLAMP_POWERON failed, for example, the KEYBOARD_LAMPS would still be attempted. > > > > So the question really comes down to this: Is there a legitimate situation in > > which one LEDs registration fails and another succeeds? If so, then this would > > constitute a regression for such systems. > > The behavior change you mentioned is intentional. As pointed out above, > if any devm_led_classdev_register() call fails, it means we have reached > some inconsistent state which is really unlikely to be improved by > further attempts to register even more devices. > > What do you guys think? Excellent rationale, I withdraw the concern. Jonathan?
On Tue, Apr 18, 2017 at 09:01:12AM -0700, Darren Hart wrote: > On Tue, Apr 18, 2017 at 10:10:01AM +0200, Micha?? K??pie?? wrote: > > Jonathan, I hope this response to Darren's message also addresses your > > concerns. Feel free to let me know if it does not. > > > > > On Fri, Apr 07, 2017 at 03:07:12PM +0200, Micha?? K??pie?? wrote: > > > > If acpi_fujitsu_laptop_leds_register() returns an error, the latter will > > > > become the return value of acpi_fujitsu_laptop_add(), which in turn will > > > > be reported by driver core. Simplify code by replacing pr_err() calls > > > > with return statements. Return 0 instead of result when no errors occur > > > > in order to make the code easier to read. > > > > > > Hi Micha??, > > > > > > Jonathan's comment regarding the information loss of removing the pr_err > > > statements seems valid to me. Based on the outer if block, I take it each > > > registration only fails in true error scenarios and not because some laptop > > > might have one but not another LED in the list. > > > > Correct. > > > > > If so, then the pr_err messages > > > would only appear when there was a legitimate problem. I think they're worth > > > > I am not hell-bent on removing these pr_err() calls, but allow me to > > briefly walk you through my thought process. > > : Thanks Michael for your detailed explanation of your rationale and the background to these changes. > > > This seems to introduce a behavior change as well. Previously only the last > > > LED registered would determine the result - which is wrong of course and I > > > believe you noted a related bug in an early patch. Previously, however, if > > > LOGOLAMP_POWERON failed, for example, the KEYBOARD_LAMPS would still be attempted. > > > > > > So the question really comes down to this: Is there a legitimate situation in > > > which one LEDs registration fails and another succeeds? If so, then this would > > > constitute a regression for such systems. > > > > The behavior change you mentioned is intentional. As pointed out above, > > if any devm_led_classdev_register() call fails, it means we have reached > > some inconsistent state which is really unlikely to be improved by > > further attempts to register even more devices. > > > > What do you guys think? > > Excellent rationale, I withdraw the concern. > Jonathan? I am happy to proceed based on Michael's subsequent explanation. The changes in this patch series are reasonably extensive but should not result in any observable changes in behaviour. They represent a significant modernisation of the code, taking advantage of the current approach to module architecture. Unfortunately I do not have hardware which includes the LEDs which these changes affect, so I cannot confirm the absence of regressions. I note however that it has been tested on a Lifebook E744 and I am therefore happy to see the patch series merged on this basis. Reviewed-by: Jonathan Woithe <jwoithe@just42.net> Regards jonathan
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index ce658789e748..177b9b57ac2f 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -739,22 +739,20 @@ static struct led_classdev eco_led = { static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) { - int result = 0; + int result; if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) { result = devm_led_classdev_register(&device->dev, &logolamp_led); if (result) - pr_err("Could not register LED handler for logo lamp, error %i\n", - result); + return result; } if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) && (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) { result = devm_led_classdev_register(&device->dev, &kblamps_led); if (result) - pr_err("Could not register LED handler for keyboard lamps, error %i\n", - result); + return result; } /* @@ -766,8 +764,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) { result = devm_led_classdev_register(&device->dev, &radio_led); if (result) - pr_err("Could not register LED handler for radio LED, error %i\n", - result); + return result; } /* Support for eco led is not always signaled in bit corresponding @@ -779,11 +776,10 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) { result = devm_led_classdev_register(&device->dev, &eco_led); if (result) - pr_err("Could not register LED handler for eco LED, error %i\n", - result); + return result; } - return result; + return 0; } static int acpi_fujitsu_laptop_add(struct acpi_device *device)
If acpi_fujitsu_laptop_leds_register() returns an error, the latter will become the return value of acpi_fujitsu_laptop_add(), which in turn will be reported by driver core. Simplify code by replacing pr_err() calls with return statements. Return 0 instead of result when no errors occur in order to make the code easier to read. Signed-off-by: Michał Kępień <kernel@kempniu.pl> --- drivers/platform/x86/fujitsu-laptop.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)