| Message ID | 20180524180740.25084-1-venemo@msn.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
+Cc: Mario On Thu, May 24, 2018 at 9:07 PM, Timur Kristóf <timur.kristof@gmail.com> wrote: > From: Timur Kristóf <venemo@fedoraproject.org> > > The 9370 doesn't expose the necessary KBD_LED_AC_TOKEN in the > BIOS, so the driver thinks it cannot adjust the AC keyboard > backlight timeout. This patch adds a quirk to fix this until > Dell adds the missing token to their BIOS. > > For further discussion, see: > https://github.com/dell/libsmbios/issues/48 > The change per se looks good to me, though I would hear back from Pali and / or Mario on the subject. Their formal Ack would be enough. > Signed-off-by: Timur Kristóf <venemo@fedoraproject.org> > --- > drivers/platform/x86/dell-laptop.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c > index c52c6723374b..058944258161 100644 > --- a/drivers/platform/x86/dell-laptop.c > +++ b/drivers/platform/x86/dell-laptop.c > @@ -38,6 +38,7 @@ > struct quirk_entry { > bool touchpad_led; > bool kbd_led_levels_off_1; > + bool support_kbd_timeout_ac_missing_tag; I would rather try to keep kbd_ prefix and perhaps somehow make the name shorter. > > bool needs_kbd_timeouts; > /* > @@ -68,6 +69,10 @@ static struct quirk_entry quirk_dell_xps13_9333 = { > .kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 }, > }; > > +static struct quirk_entry quirk_dell_xps13_9370 = { > + .support_kbd_timeout_ac_missing_tag = true, > +}; > + > static struct quirk_entry quirk_dell_latitude_e6410 = { > .kbd_led_levels_off_1 = true, > }; > @@ -291,6 +296,15 @@ static const struct dmi_system_id dell_quirks[] __initconst = { > }, > .driver_data = &quirk_dell_xps13_9333, > }, > + { > + .callback = dmi_matched, > + .ident = "Dell XPS 13 9370", > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > + DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9370"), > + }, > + .driver_data = &quirk_dell_xps13_9370, > + }, > { > .callback = dmi_matched, > .ident = "Dell Latitude E6410", > @@ -1401,7 +1415,7 @@ static inline int kbd_init_info(void) > * timeout value which is shared for both battery and AC power > * settings. So do not try to set AC values on old models. > */ > - if (dell_smbios_find_token(KBD_LED_AC_TOKEN)) > + if (dell_smbios_find_token(KBD_LED_AC_TOKEN) || (quirks && quirks->support_kbd_timeout_ac_missing_tag)) Two lines, please. > kbd_timeout_ac_supported = true; > > kbd_get_state(&state); > -- > 2.17.0 >
On Thursday 31 May 2018 15:05:39 Andy Shevchenko wrote: > +Cc: Mario > > On Thu, May 24, 2018 at 9:07 PM, Timur Kristóf <timur.kristof@gmail.com> wrote: > > From: Timur Kristóf <venemo@fedoraproject.org> > > > > The 9370 doesn't expose the necessary KBD_LED_AC_TOKEN in the > > BIOS, so the driver thinks it cannot adjust the AC keyboard > > backlight timeout. This patch adds a quirk to fix this until > > Dell adds the missing token to their BIOS. > > > > For further discussion, see: > > https://github.com/dell/libsmbios/issues/48 > > > > The change per se looks good to me, though I would hear back from Pali > and / or Mario on the subject. > Their formal Ack would be enough. I do not know what is happening here. So Mario should comment if this is really a BIOS bug (and we need to workaround it by DMI whitelisting) or kernel has incorrect implementation (and different fix is needed). > > Signed-off-by: Timur Kristóf <venemo@fedoraproject.org> > > --- > > drivers/platform/x86/dell-laptop.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c > > index c52c6723374b..058944258161 100644 > > --- a/drivers/platform/x86/dell-laptop.c > > +++ b/drivers/platform/x86/dell-laptop.c > > @@ -38,6 +38,7 @@ > > struct quirk_entry { > > bool touchpad_led; > > bool kbd_led_levels_off_1; > > + bool support_kbd_timeout_ac_missing_tag; > > I would rather try to keep kbd_ prefix and perhaps somehow make the > name shorter. > > > > > bool needs_kbd_timeouts; > > /* > > @@ -68,6 +69,10 @@ static struct quirk_entry quirk_dell_xps13_9333 = { > > .kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 }, > > }; > > > > +static struct quirk_entry quirk_dell_xps13_9370 = { > > + .support_kbd_timeout_ac_missing_tag = true, > > +}; > > + > > static struct quirk_entry quirk_dell_latitude_e6410 = { > > .kbd_led_levels_off_1 = true, > > }; > > @@ -291,6 +296,15 @@ static const struct dmi_system_id dell_quirks[] __initconst = { > > }, > > .driver_data = &quirk_dell_xps13_9333, > > }, > > + { > > + .callback = dmi_matched, > > + .ident = "Dell XPS 13 9370", > > + .matches = { > > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > > + DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9370"), > > + }, > > + .driver_data = &quirk_dell_xps13_9370, > > + }, > > { > > .callback = dmi_matched, > > .ident = "Dell Latitude E6410", > > @@ -1401,7 +1415,7 @@ static inline int kbd_init_info(void) > > * timeout value which is shared for both battery and AC power > > * settings. So do not try to set AC values on old models. > > */ > > - if (dell_smbios_find_token(KBD_LED_AC_TOKEN)) > > > + if (dell_smbios_find_token(KBD_LED_AC_TOKEN) || (quirks && quirks->support_kbd_timeout_ac_missing_tag)) > > Two lines, please. And maybe change order of ||. If we know that token is buggy on some machines then we probably should not ask for it. > > kbd_timeout_ac_supported = true; > > > > kbd_get_state(&state); > > -- > > 2.17.0 > > > > >
On Thu, 2018-05-31 at 14:22 +0200, Pali Rohár wrote: > On Thursday 31 May 2018 15:05:39 Andy Shevchenko wrote: > > +Cc: Mario > > > > On Thu, May 24, 2018 at 9:07 PM, Timur Kristóf <timur.kristof@gmail > > .com> wrote: > > > From: Timur Kristóf <venemo@fedoraproject.org> > > > > > > The 9370 doesn't expose the necessary KBD_LED_AC_TOKEN in the > > > BIOS, so the driver thinks it cannot adjust the AC keyboard > > > backlight timeout. This patch adds a quirk to fix this until > > > Dell adds the missing token to their BIOS. > > > > > > For further discussion, see: > > > https://github.com/dell/libsmbios/issues/48 > > > > > > > The change per se looks good to me, though I would hear back from > > Pali > > and / or Mario on the subject. > > Their formal Ack would be enough. > > I do not know what is happening here. So Mario should comment if this > is > really a BIOS bug (and we need to workaround it by DMI whitelisting) > or > kernel has incorrect implementation (and different fix is needed). Discussed with Mario here: https://github.com/dell/libsmbios/issues/48 The conclusion is that this is a BIOS bug. The machine is capable of changing the AC keyboard backlight timeout setting, it is just missing the token which indicates the capability. > > > > Signed-off-by: Timur Kristóf <venemo@fedoraproject.org> > > > --- > > > drivers/platform/x86/dell-laptop.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/platform/x86/dell-laptop.c > > > b/drivers/platform/x86/dell-laptop.c > > > index c52c6723374b..058944258161 100644 > > > --- a/drivers/platform/x86/dell-laptop.c > > > +++ b/drivers/platform/x86/dell-laptop.c > > > @@ -38,6 +38,7 @@ > > > struct quirk_entry { > > > bool touchpad_led; > > > bool kbd_led_levels_off_1; > > > + bool support_kbd_timeout_ac_missing_tag; > > > > I would rather try to keep kbd_ prefix and perhaps somehow make the > > name shorter. Ok. How about kbd_missing_ac_tag? > > > > > > bool needs_kbd_timeouts; > > > /* > > > @@ -68,6 +69,10 @@ static struct quirk_entry > > > quirk_dell_xps13_9333 = { > > > .kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 }, > > > }; > > > > > > +static struct quirk_entry quirk_dell_xps13_9370 = { > > > + .support_kbd_timeout_ac_missing_tag = true, > > > +}; > > > + > > > static struct quirk_entry quirk_dell_latitude_e6410 = { > > > .kbd_led_levels_off_1 = true, > > > }; > > > @@ -291,6 +296,15 @@ static const struct dmi_system_id > > > dell_quirks[] __initconst = { > > > }, > > > .driver_data = &quirk_dell_xps13_9333, > > > }, > > > + { > > > + .callback = dmi_matched, > > > + .ident = "Dell XPS 13 9370", > > > + .matches = { > > > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > > > + DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 > > > 9370"), > > > + }, > > > + .driver_data = &quirk_dell_xps13_9370, > > > + }, > > > { > > > .callback = dmi_matched, > > > .ident = "Dell Latitude E6410", > > > @@ -1401,7 +1415,7 @@ static inline int kbd_init_info(void) > > > * timeout value which is shared for both battery > > > and AC power > > > * settings. So do not try to set AC values on old > > > models. > > > */ > > > - if (dell_smbios_find_token(KBD_LED_AC_TOKEN)) > > > + if (dell_smbios_find_token(KBD_LED_AC_TOKEN) || (quirks > > > && quirks->support_kbd_timeout_ac_missing_tag)) > > > > Two lines, please. Ok. > And maybe change order of ||. If we know that token is buggy on some > machines then we probably should not ask for it. Ok. > > > > kbd_timeout_ac_supported = true; > > > > > > kbd_get_state(&state); > > > -- > > > 2.17.0 > > > > > > > > > > >
On Thursday 31 May 2018 14:35:45 Timur Kristóf wrote: > On Thu, 2018-05-31 at 14:22 +0200, Pali Rohár wrote: > > On Thursday 31 May 2018 15:05:39 Andy Shevchenko wrote: > > > +Cc: Mario > > > > > > On Thu, May 24, 2018 at 9:07 PM, Timur Kristóf <timur.kristof@gmail > > > .com> wrote: > > > > From: Timur Kristóf <venemo@fedoraproject.org> > > > > > > > > The 9370 doesn't expose the necessary KBD_LED_AC_TOKEN in the > > > > BIOS, so the driver thinks it cannot adjust the AC keyboard > > > > backlight timeout. This patch adds a quirk to fix this until > > > > Dell adds the missing token to their BIOS. > > > > > > > > For further discussion, see: > > > > https://github.com/dell/libsmbios/issues/48 > > > > > > > > > > The change per se looks good to me, though I would hear back from > > > Pali > > > and / or Mario on the subject. > > > Their formal Ack would be enough. > > > > I do not know what is happening here. So Mario should comment if this > > is > > really a BIOS bug (and we need to workaround it by DMI whitelisting) > > or > > kernel has incorrect implementation (and different fix is needed). > > Discussed with Mario here: > https://github.com/dell/libsmbios/issues/48 > > The conclusion is that this is a BIOS bug. The machine is capable of > changing the AC keyboard backlight timeout setting, it is just missing > the token which indicates the capability. Can you try to report this problem to Dell support (if possible)? Really if Dell is interested in good Linux support (which looks like yes), then Dell should know that has a bug in BIOS/firmware which is causing problems on Linux. For me it sounds stupid to adding hacks into kernel which just due to firmware bugs about which vendor does not know. As a short term fix for one laptop it is OK, but not if Dell starts producing e.g. new generation of all laptops with same bug. Long term fix is for sure in BIOS/firmware. I already wrote this to above github issue.
On Thu, 2018-05-31 at 14:50 +0200, Pali Rohár wrote: > On Thursday 31 May 2018 14:35:45 Timur Kristóf wrote: > > On Thu, 2018-05-31 at 14:22 +0200, Pali Rohár wrote: > > > On Thursday 31 May 2018 15:05:39 Andy Shevchenko wrote: > > > > +Cc: Mario > > > > > > > > On Thu, May 24, 2018 at 9:07 PM, Timur Kristóf <timur.kristof@g > > > > mail > > > > .com> wrote: > > > > > From: Timur Kristóf <venemo@fedoraproject.org> > > > > > > > > > > The 9370 doesn't expose the necessary KBD_LED_AC_TOKEN in the > > > > > BIOS, so the driver thinks it cannot adjust the AC keyboard > > > > > backlight timeout. This patch adds a quirk to fix this until > > > > > Dell adds the missing token to their BIOS. > > > > > > > > > > For further discussion, see: > > > > > https://github.com/dell/libsmbios/issues/48 > > > > > > > > > > > > > The change per se looks good to me, though I would hear back > > > > from > > > > Pali > > > > and / or Mario on the subject. > > > > Their formal Ack would be enough. > > > > > > I do not know what is happening here. So Mario should comment if > > > this > > > is > > > really a BIOS bug (and we need to workaround it by DMI > > > whitelisting) > > > or > > > kernel has incorrect implementation (and different fix is > > > needed). > > > > Discussed with Mario here: > > https://github.com/dell/libsmbios/issues/48 > > > > The conclusion is that this is a BIOS bug. The machine is capable > > of > > changing the AC keyboard backlight timeout setting, it is just > > missing > > the token which indicates the capability. > > Can you try to report this problem to Dell support (if possible)? > Really > if Dell is interested in good Linux support (which looks like yes), > then > Dell should know that has a bug in BIOS/firmware which is causing > problems on Linux. > > For me it sounds stupid to adding hacks into kernel which just due to > firmware bugs about which vendor does not know. > > As a short term fix for one laptop it is OK, but not if Dell starts > producing e.g. new generation of all laptops with same bug. Long term > fix is for sure in BIOS/firmware. > > I already wrote this to above github issue. > Thank you Pali. I agree with your point. Let's hope they will fix it in the 9380. In the meantime I think it doesn't hurt to add the patch for people who use the 9370 now. This is probably off-topic here, but with regards to Dell's interest in good Linux support: they probably have bigger problems than this. Best regards, Tim
> -----Original Message----- > From: Timur Kristóf [mailto:timur.kristof@gmail.com] > Sent: Thursday, May 31, 2018 12:14 PM > To: Pali Rohár > Cc: Andy Shevchenko; Limonciello, Mario; Platform Driver; Timur Kristóf; Matthew > Garrett; Darren Hart; Andy Shevchenko > Subject: Re: [PATCH] Fix AC keyboard backlight timeout on Dell XPS 13 9370. > > On Thu, 2018-05-31 at 14:50 +0200, Pali Rohár wrote: > > On Thursday 31 May 2018 14:35:45 Timur Kristóf wrote: > > > On Thu, 2018-05-31 at 14:22 +0200, Pali Rohár wrote: > > > > On Thursday 31 May 2018 15:05:39 Andy Shevchenko wrote: > > > > > +Cc: Mario > > > > > > > > > > On Thu, May 24, 2018 at 9:07 PM, Timur Kristóf <timur.kristof@g > > > > > mail > > > > > .com> wrote: > > > > > > From: Timur Kristóf <venemo@fedoraproject.org> > > > > > > > > > > > > The 9370 doesn't expose the necessary KBD_LED_AC_TOKEN in the > > > > > > BIOS, so the driver thinks it cannot adjust the AC keyboard > > > > > > backlight timeout. This patch adds a quirk to fix this until > > > > > > Dell adds the missing token to their BIOS. > > > > > > > > > > > > For further discussion, see: > > > > > > https://github.com/dell/libsmbios/issues/48 > > > > > > > > > > > > > > > > The change per se looks good to me, though I would hear back > > > > > from > > > > > Pali > > > > > and / or Mario on the subject. > > > > > Their formal Ack would be enough. > > > > > > > > I do not know what is happening here. So Mario should comment if > > > > this > > > > is > > > > really a BIOS bug (and we need to workaround it by DMI > > > > whitelisting) > > > > or > > > > kernel has incorrect implementation (and different fix is > > > > needed). > > > > > > Discussed with Mario here: > > > https://github.com/dell/libsmbios/issues/48 > > > > > > The conclusion is that this is a BIOS bug. The machine is capable > > > of > > > changing the AC keyboard backlight timeout setting, it is just > > > missing > > > the token which indicates the capability. > > > > Can you try to report this problem to Dell support (if possible)? > > Really > > if Dell is interested in good Linux support (which looks like yes), > > then > > Dell should know that has a bug in BIOS/firmware which is causing > > problems on Linux. > > > > For me it sounds stupid to adding hacks into kernel which just due to > > firmware bugs about which vendor does not know. > > > > As a short term fix for one laptop it is OK, but not if Dell starts > > producing e.g. new generation of all laptops with same bug. Long term > > fix is for sure in BIOS/firmware. > > > > I already wrote this to above github issue. > > > > Thank you Pali. I agree with your point. Let's hope they will fix it in > the 9380. In the meantime I think it doesn't hurt to add the patch for > people who use the 9370 now. I of course can't make any guarantees in what bugs are introduced in new generations of hardware, but I would say that it's at least understood why this behavior occurs on the 9370. I hope it should be fixed in a future firmware version but I also can not guarantee that. I think that v2 of this patch looks fine, but I would like to ask Timur that if a firmware is released that fixes this behavior please amend with a follow up patch that restricts the quirk only to the applicable firmware (or drop the quirk).
On Mon, 2018-06-04 at 21:52 +0000, Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Timur Kristóf [mailto:timur.kristof@gmail.com] > > Sent: Thursday, May 31, 2018 12:14 PM > > To: Pali Rohár > > Cc: Andy Shevchenko; Limonciello, Mario; Platform Driver; Timur > > Kristóf; Matthew > > Garrett; Darren Hart; Andy Shevchenko > > Subject: Re: [PATCH] Fix AC keyboard backlight timeout on Dell XPS > > 13 9370. > > > > On Thu, 2018-05-31 at 14:50 +0200, Pali Rohár wrote: > > > On Thursday 31 May 2018 14:35:45 Timur Kristóf wrote: > > > > On Thu, 2018-05-31 at 14:22 +0200, Pali Rohár wrote: > > > > > On Thursday 31 May 2018 15:05:39 Andy Shevchenko wrote: > > > > > > +Cc: Mario > > > > > > > > > > > > On Thu, May 24, 2018 at 9:07 PM, Timur Kristóf <timur.krist > > > > > > of@g > > > > > > mail > > > > > > .com> wrote: > > > > > > > From: Timur Kristóf <venemo@fedoraproject.org> > > > > > > > > > > > > > > The 9370 doesn't expose the necessary KBD_LED_AC_TOKEN in > > > > > > > the > > > > > > > BIOS, so the driver thinks it cannot adjust the AC > > > > > > > keyboard > > > > > > > backlight timeout. This patch adds a quirk to fix this > > > > > > > until > > > > > > > Dell adds the missing token to their BIOS. > > > > > > > > > > > > > > For further discussion, see: > > > > > > > https://github.com/dell/libsmbios/issues/48 > > > > > > > > > > > > > > > > > > > The change per se looks good to me, though I would hear > > > > > > back > > > > > > from > > > > > > Pali > > > > > > and / or Mario on the subject. > > > > > > Their formal Ack would be enough. > > > > > > > > > > I do not know what is happening here. So Mario should comment > > > > > if > > > > > this > > > > > is > > > > > really a BIOS bug (and we need to workaround it by DMI > > > > > whitelisting) > > > > > or > > > > > kernel has incorrect implementation (and different fix is > > > > > needed). > > > > > > > > Discussed with Mario here: > > > > https://github.com/dell/libsmbios/issues/48 > > > > > > > > The conclusion is that this is a BIOS bug. The machine is > > > > capable > > > > of > > > > changing the AC keyboard backlight timeout setting, it is just > > > > missing > > > > the token which indicates the capability. > > > > > > Can you try to report this problem to Dell support (if possible)? > > > Really > > > if Dell is interested in good Linux support (which looks like > > > yes), > > > then > > > Dell should know that has a bug in BIOS/firmware which is causing > > > problems on Linux. > > > > > > For me it sounds stupid to adding hacks into kernel which just > > > due to > > > firmware bugs about which vendor does not know. > > > > > > As a short term fix for one laptop it is OK, but not if Dell > > > starts > > > producing e.g. new generation of all laptops with same bug. Long > > > term > > > fix is for sure in BIOS/firmware. > > > > > > I already wrote this to above github issue. > > > > > > > Thank you Pali. I agree with your point. Let's hope they will fix > > it in > > the 9380. In the meantime I think it doesn't hurt to add the patch > > for > > people who use the 9370 now. > > I of course can't make any guarantees in what bugs are introduced in > new generations of hardware, but I would say that it's at least > understood > why this behavior occurs on the 9370. I hope it should be fixed in a > future > firmware version but I also can not guarantee that. > > I think that v2 of this patch looks fine, but I would like to ask > Timur that if > a firmware is released that fixes this behavior please amend with a > follow up patch > that restricts the quirk only to the applicable firmware (or drop the > quirk). Thank you Mario! Sure, it makes sense to restrict the quirk to firmware which is known to be buggy. I will be happy to do that when the issue is fixed by new firmware update! In the meantime I've been running 4.17-rc7 with this patch added, and it works well for me. Though not sure if this could still make it into 4.17, but would be nice! Best regards, Timur
> Thank you Mario! > > Sure, it makes sense to restrict the quirk to firmware which is known > to be buggy. I will be happy to do that when the issue is fixed by new > firmware update! > > In the meantime I've been running 4.17-rc7 with this patch added, and > it works well for me. > > Though not sure if this could still make it into 4.17, but would be > nice! > > Best regards, > Timur Timur, I believe this behavior your encountered /may/ be resolved in FW 1.4.0 that was published recently. http://www.dell.com/support/home/us/en/19/Drivers/DriversDetails?driverId=3N2X8 https://fwupd.org/lvfs/component/720/all Thanks,
On Fri, 2018-06-15 at 15:16 +0000, Mario.Limonciello@dell.com wrote: > > Thank you Mario! > > > > Sure, it makes sense to restrict the quirk to firmware which is > > known > > to be buggy. I will be happy to do that when the issue is fixed by > > new > > firmware update! > > > > In the meantime I've been running 4.17-rc7 with this patch added, > > and > > it works well for me. > > > > Though not sure if this could still make it into 4.17, but would be > > nice! > > > > Best regards, > > Timur > > Timur, > > I believe this behavior your encountered /may/ be resolved in FW > 1.4.0 that was published recently. > > http://www.dell.com/support/home/us/en/19/Drivers/DriversDetails?driv > erId=3N2X8 > https://fwupd.org/lvfs/component/720/all > > Thanks, > Hi Mario, Yes, it appears that the necessary token is now there as of BIOS 1.4.0 How would you like to proceed? I would suggest to leave the patch as-is for the sake of users who don't update their BIOS. I've looked into how to restrict the solution for just older BIOS versions, but as far as I could tell there is no way to express "less than 1.4.0" with DMI_MATCH. What do you think? Thanks & best regards, Tim
> -----Original Message----- > From: Timur Kristóf [mailto:timur.kristof@gmail.com] > Sent: Sunday, June 17, 2018 2:51 PM > To: Limonciello, Mario; pali.rohar@gmail.com > Cc: andy.shevchenko@gmail.com; platform-driver-x86@vger.kernel.org; > venemo@fedoraproject.org; mjg59@srcf.ucam.org; dvhart@infradead.org; > andy@infradead.org > Subject: Re: [PATCH] Fix AC keyboard backlight timeout on Dell XPS 13 9370. > > On Fri, 2018-06-15 at 15:16 +0000, Mario.Limonciello@dell.com wrote: > > > Thank you Mario! > > > > > > Sure, it makes sense to restrict the quirk to firmware which is > > > known > > > to be buggy. I will be happy to do that when the issue is fixed by > > > new > > > firmware update! > > > > > > In the meantime I've been running 4.17-rc7 with this patch added, > > > and > > > it works well for me. > > > > > > Though not sure if this could still make it into 4.17, but would be > > > nice! > > > > > > Best regards, > > > Timur > > > > Timur, > > > > I believe this behavior your encountered /may/ be resolved in FW > > 1.4.0 that was published recently. > > > > http://www.dell.com/support/home/us/en/19/Drivers/DriversDetails?driv > > erId=3N2X8 > > https://fwupd.org/lvfs/component/720/all > > > > Thanks, > > > > Hi Mario, > > Yes, it appears that the necessary token is now there as of BIOS 1.4.0 > How would you like to proceed? I would suggest to leave the patch as-is > for the sake of users who don't update their BIOS. I've looked into how > to restrict the solution for just older BIOS versions, but as far as I > could tell there is no way to express "less than 1.4.0" with DMI_MATCH. > > What do you think? > > Thanks & best regards, > Tim Timur, Thanks for confirming. My personal opinion is that the kernel shouldn't set precedent to this type of blanket quirk (all BIOS version) for machines with BIOS not matching kernel behavior but are still getting updates that can be fixed. The proper thing to do is to get BIOS fixed in that instance. Since Dell publishes FW updates to LVFS for this platform, fwupd is integrated into most major distros and it's really easy to install the updates from Linux I have confidence that affected people will be installing this BIOS update to fix the issue. So I would say that either: 1) Drop this patch. 2) Create a new macro that can match < $VERSION and re-configure your patch to do that too. 3) Change your patch to detect if running on XPS 9370 and missing this token and show a warning in kernel log that there is a FW problem and you will want to check for a FW update to fix it.
On Mon, Jun 18, 2018 at 3:59 AM <Mario.Limonciello@dell.com> wrote: > Thanks for confirming. My personal opinion is that the kernel shouldn't set > precedent to this type of blanket quirk (all BIOS version) for machines with > BIOS not matching kernel behavior but are still getting updates that can be > fixed. The proper thing to do is to get BIOS fixed in that instance. > > Since Dell publishes FW updates to LVFS for this platform, fwupd is integrated > into most major distros and it's really easy to install the updates from Linux I > have confidence that affected people will be installing this BIOS update to > fix the issue. > > So I would say that either: > 1) Drop this patch. > 2) Create a new macro that can match < $VERSION and re-configure your patch > to do that too. > 3) Change your patch to detect if running on XPS 9370 and missing this token > and show a warning in kernel log that there is a FW problem and you > will want to check for a FW update to fix it. Thanks, Mario, for your input! I like options 1) or 3), though 3), if possible to reliably detect, I prefer more. User needs to be informed that there is a BIOS fix.
On Mon, 2018-06-18 at 11:00 +0300, Andy Shevchenko wrote: > On Mon, Jun 18, 2018 at 3:59 AM <Mario.Limonciello@dell.com> wrote: > > > Thanks for confirming. My personal opinion is that the kernel > > shouldn't set > > precedent to this type of blanket quirk (all BIOS version) for > > machines with > > BIOS not matching kernel behavior but are still getting updates > > that can be > > fixed. The proper thing to do is to get BIOS fixed in that > > instance. > > > > Since Dell publishes FW updates to LVFS for this platform, fwupd is > > integrated > > into most major distros and it's really easy to install the updates > > from Linux I > > have confidence that affected people will be installing this BIOS > > update to > > fix the issue. > > > > So I would say that either: > > 1) Drop this patch. > > 2) Create a new macro that can match < $VERSION and re-configure > > your patch > > to do that too. > > 3) Change your patch to detect if running on XPS 9370 and missing > > this token > > and show a warning in kernel log that there is a FW problem and you > > will want to check for a FW update to fix it. > > Thanks, Mario, for your input! > > I like options 1) or 3), though 3), if possible to reliably detect, I > prefer more. User needs to be informed that there is a BIOS fix. I think 3) is not that hard to do. I'll look into it and send a patch (but probably not until next week).
On Mon, Jun 18, 2018 at 11:15:27AM +0200, Timur Kristóf wrote: > On Mon, 2018-06-18 at 11:00 +0300, Andy Shevchenko wrote: > > On Mon, Jun 18, 2018 at 3:59 AM <Mario.Limonciello@dell.com> wrote: > > > > > Thanks for confirming. My personal opinion is that the kernel > > > shouldn't set > > > precedent to this type of blanket quirk (all BIOS version) for > > > machines with > > > BIOS not matching kernel behavior but are still getting updates > > > that can be > > > fixed. The proper thing to do is to get BIOS fixed in that > > > instance. > > > > > > Since Dell publishes FW updates to LVFS for this platform, fwupd is > > > integrated > > > into most major distros and it's really easy to install the updates > > > from Linux I > > > have confidence that affected people will be installing this BIOS > > > update to > > > fix the issue. > > > > > > So I would say that either: > > > 1) Drop this patch. > > > 2) Create a new macro that can match < $VERSION and re-configure > > > your patch > > > to do that too. > > > 3) Change your patch to detect if running on XPS 9370 and missing > > > this token > > > and show a warning in kernel log that there is a FW problem and you > > > will want to check for a FW update to fix it. > > > > Thanks, Mario, for your input! > > > > I like options 1) or 3), though 3), if possible to reliably detect, I > > prefer more. User needs to be informed that there is a BIOS fix. > > I think 3) is not that hard to do. I'll look into it and send a patch > (but probably not until next week). Agreed. "Users that don't update their BIOS"... need to update their BIOS. Mario - thank you for your continued involvement in improving Linux support on Dell, it makes a big difference. Much appreciated.
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c index c52c6723374b..058944258161 100644 --- a/drivers/platform/x86/dell-laptop.c +++ b/drivers/platform/x86/dell-laptop.c @@ -38,6 +38,7 @@ struct quirk_entry { bool touchpad_led; bool kbd_led_levels_off_1; + bool support_kbd_timeout_ac_missing_tag; bool needs_kbd_timeouts; /* @@ -68,6 +69,10 @@ static struct quirk_entry quirk_dell_xps13_9333 = { .kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 }, }; +static struct quirk_entry quirk_dell_xps13_9370 = { + .support_kbd_timeout_ac_missing_tag = true, +}; + static struct quirk_entry quirk_dell_latitude_e6410 = { .kbd_led_levels_off_1 = true, }; @@ -291,6 +296,15 @@ static const struct dmi_system_id dell_quirks[] __initconst = { }, .driver_data = &quirk_dell_xps13_9333, }, + { + .callback = dmi_matched, + .ident = "Dell XPS 13 9370", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), + DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9370"), + }, + .driver_data = &quirk_dell_xps13_9370, + }, { .callback = dmi_matched, .ident = "Dell Latitude E6410", @@ -1401,7 +1415,7 @@ static inline int kbd_init_info(void) * timeout value which is shared for both battery and AC power * settings. So do not try to set AC values on old models. */ - if (dell_smbios_find_token(KBD_LED_AC_TOKEN)) + if (dell_smbios_find_token(KBD_LED_AC_TOKEN) || (quirks && quirks->support_kbd_timeout_ac_missing_tag)) kbd_timeout_ac_supported = true; kbd_get_state(&state);