[PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Michał Kępień]

Radio LED detection method implemented in commit 4f62568c1fcf
("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
which has a slide switch (and thus no radio LED).  Use bit 17 of
flags_supported (the value returned by method S000 of ACPI device
FUJ02E3) to determine whether a radio LED is present as it seems to be a
more reliable indicator, based on comparing DSDT tables of four Fujitsu
Lifebook models (E744, E751, S7110, S8420).

Reported-by: Heinrich Siebmanns <harv@gmx.de>
Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
I do not have a Fujitsu laptop with a radio LED for testing, so I was
only able to check that this patch still does not cause a radio LED to
be detected on a Lifebook S7020.

Harvey, could you please try this patch on your Lifebook E751 and see
whether the log messages you reported disappear?  I will be happy to
assist you off-list in case you need help with it.

 drivers/platform/x86/fujitsu-laptop.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 56a8195096a2..2cfbd3fa5136 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -691,6 +691,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
 
 static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 {
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 	struct led_classdev *led;
 	int result;
 
@@ -724,12 +725,15 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	}
 
 	/*
-	 * BTNI bit 24 seems to indicate the presence of a radio toggle
-	 * button in place of a slide switch, and all such machines appear
-	 * to also have an RF LED.  Therefore use bit 24 as an indicator
-	 * that an RF LED is present.
+	 * Some Fujitsu laptops have a radio toggle button in place of a slide
+	 * switch and all such machines appear to also have an RF LED.  Based on
+	 * comparing DSDT tables of four Fujitsu Lifebook models (E744, E751,
+	 * S7110, S8420; the first one has a radio toggle button, the other
+	 * three have slide switches), bit 17 of flags_supported (the value
+	 * returned by method S000 of ACPI device FUJ02E3) seems to indicate
+	 * whether given model has a radio toggle button.
 	 */
-	if (call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
+	if (priv->flags_supported & BIT(17)) {
 		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
 		if (!led)
 			return -ENOMEM;
-- 
2.14.2

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Jonathan Woithe]

On Wed, Oct 25, 2017 at 06:29:46AM +0200, Micha?? K??pie?? wrote:
> Radio LED detection method implemented in commit 4f62568c1fcf
> ("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
> causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
> which has a slide switch (and thus no radio LED).  Use bit 17 of
> flags_supported (the value returned by method S000 of ACPI device
> FUJ02E3) to determine whether a radio LED is present as it seems to be a
> more reliable indicator, based on comparing DSDT tables of four Fujitsu
> Lifebook models (E744, E751, S7110, S8420).
> 
> Reported-by: Heinrich Siebmanns <harv@gmx.de>
> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>

This seems to be a reasonable approach given the most recent set of
observations.  Assuming it tests ok on the E751:

  Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

Regards
  jonathan

> ---
> I do not have a Fujitsu laptop with a radio LED for testing, so I was
> only able to check that this patch still does not cause a radio LED to
> be detected on a Lifebook S7020.
> 
> Harvey, could you please try this patch on your Lifebook E751 and see
> whether the log messages you reported disappear?  I will be happy to
> assist you off-list in case you need help with it.
> 
>  drivers/platform/x86/fujitsu-laptop.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 56a8195096a2..2cfbd3fa5136 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -691,6 +691,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
>  
>  static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>  {
> +	struct fujitsu_laptop *priv = acpi_driver_data(device);
>  	struct led_classdev *led;
>  	int result;
>  
> @@ -724,12 +725,15 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>  	}
>  
>  	/*
> -	 * BTNI bit 24 seems to indicate the presence of a radio toggle
> -	 * button in place of a slide switch, and all such machines appear
> -	 * to also have an RF LED.  Therefore use bit 24 as an indicator
> -	 * that an RF LED is present.
> +	 * Some Fujitsu laptops have a radio toggle button in place of a slide
> +	 * switch and all such machines appear to also have an RF LED.  Based on
> +	 * comparing DSDT tables of four Fujitsu Lifebook models (E744, E751,
> +	 * S7110, S8420; the first one has a radio toggle button, the other
> +	 * three have slide switches), bit 17 of flags_supported (the value
> +	 * returned by method S000 of ACPI device FUJ02E3) seems to indicate
> +	 * whether given model has a radio toggle button.
>  	 */
> -	if (call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
> +	if (priv->flags_supported & BIT(17)) {
>  		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
>  		if (!led)
>  			return -ENOMEM;
> -- 
> 2.14.2

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Harvey]

[-- Attachment #1.1: Type: text/plain, Size: 3541 bytes --]

Am 25.10.2017 um 08:33 schrieb Jonathan Woithe:
> On Wed, Oct 25, 2017 at 06:29:46AM +0200, Micha?? K??pie?? wrote:
>> Radio LED detection method implemented in commit 4f62568c1fcf
>> ("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
>> causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
>> which has a slide switch (and thus no radio LED).  Use bit 17 of
>> flags_supported (the value returned by method S000 of ACPI device
>> FUJ02E3) to determine whether a radio LED is present as it seems to be a
>> more reliable indicator, based on comparing DSDT tables of four Fujitsu
>> Lifebook models (E744, E751, S7110, S8420).
>>
>> Reported-by: Heinrich Siebmanns <harv@gmx.de>
>> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> 
> This seems to be a reasonable approach given the most recent set of
> observations.  Assuming it tests ok on the E751:
> 
>   Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

It does. I applied the patch against an Archlinux vanilla 4.13.9 kernel.
The resulting module is clean and the error messages are gone, while
anything else seems to work as expected.

So this looks ok for me.

Tested-by: Heinrich Siebmanns <harv@gmx.de>

> Regards
>   jonathan
> 
>> ---
>> I do not have a Fujitsu laptop with a radio LED for testing, so I was
>> only able to check that this patch still does not cause a radio LED to
>> be detected on a Lifebook S7020.
>>
>> Harvey, could you please try this patch on your Lifebook E751 and see
>> whether the log messages you reported disappear?  I will be happy to
>> assist you off-list in case you need help with it.
>>
>>  drivers/platform/x86/fujitsu-laptop.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
>> index 56a8195096a2..2cfbd3fa5136 100644
>> --- a/drivers/platform/x86/fujitsu-laptop.c
>> +++ b/drivers/platform/x86/fujitsu-laptop.c
>> @@ -691,6 +691,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
>>  
>>  static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>>  {
>> +	struct fujitsu_laptop *priv = acpi_driver_data(device);
>>  	struct led_classdev *led;
>>  	int result;
>>  
>> @@ -724,12 +725,15 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>>  	}
>>  
>>  	/*
>> -	 * BTNI bit 24 seems to indicate the presence of a radio toggle
>> -	 * button in place of a slide switch, and all such machines appear
>> -	 * to also have an RF LED.  Therefore use bit 24 as an indicator
>> -	 * that an RF LED is present.
>> +	 * Some Fujitsu laptops have a radio toggle button in place of a slide
>> +	 * switch and all such machines appear to also have an RF LED.  Based on
>> +	 * comparing DSDT tables of four Fujitsu Lifebook models (E744, E751,
>> +	 * S7110, S8420; the first one has a radio toggle button, the other
>> +	 * three have slide switches), bit 17 of flags_supported (the value
>> +	 * returned by method S000 of ACPI device FUJ02E3) seems to indicate
>> +	 * whether given model has a radio toggle button.
>>  	 */
>> -	if (call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
>> +	if (priv->flags_supported & BIT(17)) {
>>  		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
>>  		if (!led)
>>  			return -ENOMEM;
>> -- 
>> 2.14.2
> 

-- 
I am root. If you see me laughing, you'd better have a backup!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Andy Shevchenko]

On Wed, Oct 25, 2017 at 7:29 AM, Michał Kępień <kernel@kempniu.pl> wrote:
> Radio LED detection method implemented in commit 4f62568c1fcf
> ("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
> causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
> which has a slide switch (and thus no radio LED).  Use bit 17 of
> flags_supported (the value returned by method S000 of ACPI device
> FUJ02E3) to determine whether a radio LED is present as it seems to be a
> more reliable indicator, based on comparing DSDT tables of four Fujitsu
> Lifebook models (E744, E751, S7110, S8420).
>

Pushed to my review and testing queue, thanks!

> Reported-by: Heinrich Siebmanns <harv@gmx.de>
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
> I do not have a Fujitsu laptop with a radio LED for testing, so I was
> only able to check that this patch still does not cause a radio LED to
> be detected on a Lifebook S7020.
>
> Harvey, could you please try this patch on your Lifebook E751 and see
> whether the log messages you reported disappear?  I will be happy to
> assist you off-list in case you need help with it.
>
>  drivers/platform/x86/fujitsu-laptop.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 56a8195096a2..2cfbd3fa5136 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -691,6 +691,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
>
>  static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>  {
> +       struct fujitsu_laptop *priv = acpi_driver_data(device);
>         struct led_classdev *led;
>         int result;
>
> @@ -724,12 +725,15 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>         }
>
>         /*
> -        * BTNI bit 24 seems to indicate the presence of a radio toggle
> -        * button in place of a slide switch, and all such machines appear
> -        * to also have an RF LED.  Therefore use bit 24 as an indicator
> -        * that an RF LED is present.
> +        * Some Fujitsu laptops have a radio toggle button in place of a slide
> +        * switch and all such machines appear to also have an RF LED.  Based on
> +        * comparing DSDT tables of four Fujitsu Lifebook models (E744, E751,
> +        * S7110, S8420; the first one has a radio toggle button, the other
> +        * three have slide switches), bit 17 of flags_supported (the value
> +        * returned by method S000 of ACPI device FUJ02E3) seems to indicate
> +        * whether given model has a radio toggle button.
>          */
> -       if (call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
> +       if (priv->flags_supported & BIT(17)) {
>                 led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
>                 if (!led)
>                         return -ENOMEM;
> --
> 2.14.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Michał Kępień]

> On Wed, Oct 25, 2017 at 7:29 AM, Michał Kępień <kernel@kempniu.pl> wrote:
> > Radio LED detection method implemented in commit 4f62568c1fcf
> > ("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
> > causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
> > which has a slide switch (and thus no radio LED).  Use bit 17 of
> > flags_supported (the value returned by method S000 of ACPI device
> > FUJ02E3) to determine whether a radio LED is present as it seems to be a
> > more reliable indicator, based on comparing DSDT tables of four Fujitsu
> > Lifebook models (E744, E751, S7110, S8420).
> >
> 
> Pushed to my review and testing queue, thanks!

I forgot that this patch can also be tagged with:

Fixes: 4f62568c1fcf ("fujitsu-laptop: Support radio LED")

-- 
Best regards,
Michał Kępień

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Andy Shevchenko]

On Sun, Oct 29, 2017 at 11:28 PM, Michał Kępień <kernel@kempniu.pl> wrote:
>> On Wed, Oct 25, 2017 at 7:29 AM, Michał Kępień <kernel@kempniu.pl> wrote:
>> > Radio LED detection method implemented in commit 4f62568c1fcf
>> > ("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
>> > causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
>> > which has a slide switch (and thus no radio LED).  Use bit 17 of
>> > flags_supported (the value returned by method S000 of ACPI device
>> > FUJ02E3) to determine whether a radio LED is present as it seems to be a
>> > more reliable indicator, based on comparing DSDT tables of four Fujitsu
>> > Lifebook models (E744, E751, S7110, S8420).
>> >
>>
>> Pushed to my review and testing queue, thanks!
>
> I forgot that this patch can also be tagged with:
>
> Fixes: 4f62568c1fcf ("fujitsu-laptop: Support radio LED")

Added.

Do you consider this an important fix? We are at -rc7 now, I'm not
sure it's so critical. Tell me if you consider otherwise.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Jonathan Woithe]

On Mon, Oct 30, 2017 at 01:21:50PM +0200, Andy Shevchenko wrote:
> On Sun, Oct 29, 2017 at 11:28 PM, Micha?? K??pie?? <kernel@kempniu.pl> wrote:
> >> On Wed, Oct 25, 2017 at 7:29 AM, Micha?? K??pie?? <kernel@kempniu.pl> wrote:
> >> > Radio LED detection method implemented in commit 4f62568c1fcf
> >> > ("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
> >> > causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
> >> > which has a slide switch (and thus no radio LED).  Use bit 17 of
> >> > flags_supported (the value returned by method S000 of ACPI device
> >> > FUJ02E3) to determine whether a radio LED is present as it seems to be a
> >> > more reliable indicator, based on comparing DSDT tables of four Fujitsu
> >> > Lifebook models (E744, E751, S7110, S8420).
> >> >
> >>
> >> Pushed to my review and testing queue, thanks!
> >
> > I forgot that this patch can also be tagged with:
> >
> > Fixes: 4f62568c1fcf ("fujitsu-laptop: Support radio LED")
> 
> Added.
> 
> Do you consider this an important fix? We are at -rc7 now, I'm not
> sure it's so critical. Tell me if you consider otherwise.

I agree - from my perspective I wouldn't have thought it so critical as to
push it out this late in the development cycle.  It's not a regression as
such and is largely cosmetic.  Others may argue differently though.

BTW, it looks like you may have missed my Reviewed-by tag on this patch,
sent on 25 Oct.  There was also a Tested-by added by Heinrich Siebmanns on
the same day:

Reviewed-by: Jonathan Woithe <jwoithe@just42.net>
Tested-by: Heinrich Siebmanns <harv@gmx.de>

Or perhaps such peripheral tags aren't carried forward on patches like this,
in which case it's a moot point.

Regards
  jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Andy Shevchenko]

On Mon, Oct 30, 2017 at 2:12 PM, Jonathan Woithe <jwoithe@just42.net> wrote:
> On Mon, Oct 30, 2017 at 01:21:50PM +0200, Andy Shevchenko wrote:
>> On Sun, Oct 29, 2017 at 11:28 PM, Micha?? K??pie?? <kernel@kempniu.pl> wrote:
>> >> On Wed, Oct 25, 2017 at 7:29 AM, Micha?? K??pie?? <kernel@kempniu.pl> wrote:
>> >> > Radio LED detection method implemented in commit 4f62568c1fcf
>> >> > ("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
>> >> > causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
>> >> > which has a slide switch (and thus no radio LED).  Use bit 17 of
>> >> > flags_supported (the value returned by method S000 of ACPI device
>> >> > FUJ02E3) to determine whether a radio LED is present as it seems to be a
>> >> > more reliable indicator, based on comparing DSDT tables of four Fujitsu
>> >> > Lifebook models (E744, E751, S7110, S8420).
>> >> >
>> >>
>> >> Pushed to my review and testing queue, thanks!
>> >
>> > I forgot that this patch can also be tagged with:
>> >
>> > Fixes: 4f62568c1fcf ("fujitsu-laptop: Support radio LED")
>>
>> Added.
>>
>> Do you consider this an important fix? We are at -rc7 now, I'm not
>> sure it's so critical. Tell me if you consider otherwise.
>
> I agree - from my perspective I wouldn't have thought it so critical as to
> push it out this late in the development cycle.  It's not a regression as
> such and is largely cosmetic.  Others may argue differently though.
>

Got it!

> BTW, it looks like you may have missed my Reviewed-by tag on this patch,
> sent on 25 Oct.  There was also a Tested-by added by Heinrich Siebmanns on
> the same day:
>
> Reviewed-by: Jonathan Woithe <jwoithe@just42.net>
> Tested-by: Heinrich Siebmanns <harv@gmx.de>

The first one is indeed missed and sorry, can't touch the change
anymore (it is published now).
The second one is there.

So, I applied a patch with tags that were parsed by patchwork at that time.
Since Heinrich's ones are there, perhaps something happened to your
tag that it wasn't recognized, sorry.

> Or perhaps such peripheral tags aren't carried forward on patches like this,
> in which case it's a moot point.

It's okay to have them.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Michał Kępień]

> > Do you consider this an important fix? We are at -rc7 now, I'm not
> > sure it's so critical. Tell me if you consider otherwise.
> 
> I agree - from my perspective I wouldn't have thought it so critical as to
> push it out this late in the development cycle.  It's not a regression as
> such and is largely cosmetic.  Others may argue differently though.

I agree as well.  The erroneous log messages will only be generated when
any rfkill device in the system is enabled or disabled, so IMHO this is
mostly a nuisance thus can be handled when convenient.

-- 
Best regards,
Michał Kępień

Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Harvey]

[-- Attachment #1.1: Type: text/plain, Size: 1200 bytes --]

> I agree as well.  The erroneous log messages will only be generated when
> any rfkill device in the system is enabled or disabled, so IMHO this is
> mostly a nuisance thus can be handled when convenient.

FWIW: They are generated several times during every startup of the machine:

sudo journalctl -b | grep radio_led
718:Okt 31 11:46:21 gruenix kernel: leds fujitsu::radio_led: Setting an
LED's brightness failed (-2147483648)
744:Okt 31 11:46:22 gruenix kernel: leds fujitsu::radio_led: Setting an
LED's brightness failed (-2147483648)
760:Okt 31 11:46:22 gruenix kernel: leds fujitsu::radio_led: Setting an
LED's brightness failed (-2147483648)
790:Okt 31 11:46:22 gruenix kernel: leds fujitsu::radio_led: Setting an
LED's brightness failed (-2147483648)
793:Okt 31 11:46:22 gruenix kernel: leds fujitsu::radio_led: Setting an
LED's brightness failed (-2147483648)
893:Okt 31 11:46:29 gruenix kernel: leds fujitsu::radio_led: Setting an
LED's brightness failed (-2147483648)

But, yes. This is only a cosmetic issue. There don't seem to be too many
people reportig this as well.

Greetings
Harvey

-- 
I am root. If you see me laughing, you'd better have a backup!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]