| Message ID | 20171025042946.13225-1-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
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
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 >
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 >
> 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")
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.
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
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.
> > 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.
> 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
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;
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(-)