* [PATCH 0/3] Allow setting dmi-product-name on the cmdline + dmi sdhci quirk @ 2017-02-25 17:23 Hans de Goede 2017-02-25 17:23 ` [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option Hans de Goede ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Hans de Goede @ 2017-02-25 17:23 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson, Jean Delvare Cc: Hans de Goede, Takashi Iwai, russianneuromancer @ ya . ru, linux-mmc, linux-kernel Hi All, So patch 1/3 and 2+3/3 are only somewhat related and they can be merged independent of each other. The catch is that the GPD win for which patch 3/3 adds a dmi based quirk does not have any usable dmi strings. I've contacted the manufacturer for a BIOS update (no clue how that will go) but even with the BIOS update I believe patch 1/3 will be useful for similar cases and for people who do not want to flash their BIOS. So if Jean is happy with patch 1/3 then 2+3/3 can be merged independently. Regards, Hans p.s. Takashi this means that you can add a dmi based quirk for the headphone detection too, my goal is to make everything just work on the GPD win if the user supplies dmi_product_name=GPD-WINI55 on the kernel cmdline (we will also need a udev hwdb entry to fix the accelerometer orientation). ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option 2017-02-25 17:23 [PATCH 0/3] Allow setting dmi-product-name on the cmdline + dmi sdhci quirk Hans de Goede @ 2017-02-25 17:23 ` Hans de Goede 2017-03-03 9:24 ` Jean Delvare 2017-02-25 17:23 ` [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() Hans de Goede 2017-02-25 17:23 ` [PATCH 3/3] mmc: sdhci-acpi: Add a quirk to not break wifi on GPD WIN I55 machines Hans de Goede 2 siblings, 1 reply; 15+ messages in thread From: Hans de Goede @ 2017-02-25 17:23 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson, Jean Delvare Cc: Hans de Goede, Takashi Iwai, russianneuromancer @ ya . ru, linux-mmc, linux-kernel Unfortunately some firmware has all the DMI strings filled with: "Default String" (or something equally useless). This makes it impossible to apply DMI based quirks to certain machines. This commit adds a dmi_product_name kernel cmdline option which can be used to override the DMI_PRODUCT_NAME string, so that DMI based quirks can still be used on such boards, there are 3 reasons for this: 1) Rather then add cmdline options for all things which can be DMI quirked and thus may need to be specified, this only requires adding code for a single extra cmdline option 2) Some devices can be quite quirky, e.g. the GPD win mini laptop / clamshell x86 machine, needing several quirks in both kernel and userspace (udev hwdb) in this case being able to fake a unique dmi product name allows making these devices usable with a single kernel cmdline option rather then requiring multiple kernel cmdline options + manual userspace tweaking 3) In some case we may be able to successfully get the manufacturer to fix the DMI strings with a firmware update, but not all users may want to update, this will allow users to use DMI based quirks without forcing them to update their firmware Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Note the GPD win: http://www.gpd.hk/gpdwin.asp is the main reason I wrote this patch. I've requested the manufacturer to do a BIOS update fixing the DMI strings, but I cannot guarantee that that will happen. --- drivers/firmware/dmi_scan.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c index 54be60e..c99e753 100644 --- a/drivers/firmware/dmi_scan.c +++ b/drivers/firmware/dmi_scan.c @@ -1047,3 +1047,17 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device) } } EXPORT_SYMBOL_GPL(dmi_memdev_name); + +static int __init dmi_parse_dmi_product_name(char *str) +{ + static char prod_name[32]; + + if (!str) + return -EINVAL; + + strlcpy(prod_name, str, sizeof(prod_name)); + dmi_ident[DMI_PRODUCT_NAME] = prod_name; + + return 0; +} +early_param("dmi_product_name", dmi_parse_dmi_product_name); -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option 2017-02-25 17:23 ` [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option Hans de Goede @ 2017-03-03 9:24 ` Jean Delvare 2017-03-03 14:27 ` Hans de Goede 0 siblings, 1 reply; 15+ messages in thread From: Jean Delvare @ 2017-03-03 9:24 UTC (permalink / raw) To: Hans de Goede Cc: Adrian Hunter, Ulf Hansson, Jean Delvare, Takashi Iwai, russianneuromancer, linux-mmc, linux-kernel Hi Hans, On Sat, 25 Feb 2017 18:23:55 +0100, Hans de Goede wrote: > Unfortunately some firmware has all the DMI strings filled with: > "Default String" (or something equally useless). This makes it impossible > to apply DMI based quirks to certain machines. Sad but true :-( > This commit adds a dmi_product_name kernel cmdline option which can > be used to override the DMI_PRODUCT_NAME string, so that DMI based > quirks can still be used on such boards, there are 3 reasons for this: > > 1) Rather then add cmdline options for all things which can be DMI quirked > and thus may need to be specified, this only requires adding code for > a single extra cmdline option This cuts both ways. It also means that, if you get a new machine which needs some of the quirks needed by an older machine, but not all of them, you can't get it to work without modifying your kernel first. If quirk options are addressed directly to the relevant subsystem, then there is a chance that they can be reused directly for other machines later. > 2) Some devices can be quite quirky, e.g. the GPD win mini laptop / > clamshell x86 machine, needing several quirks in both kernel and userspace > (udev hwdb) in this case being able to fake a unique dmi product name > allows making these devices usable with a single kernel cmdline option > rather then requiring multiple kernel cmdline options + manual userspace > tweaking > > 3) In some case we may be able to successfully get the manufacturer to > fix the DMI strings with a firmware update, but not all users may want > to update, this will allow users to use DMI based quirks without forcing > them to update their firmware But that's the only right thing to do. The easier we make it for manufacturers shipping crappy BIOSes, the lesser motivated they will be to fix them. Writing a decent DMI table is a one hour job, there's no excuse for not doing it. So I am very reluctant to accept this patch, sorry. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note the GPD win: http://www.gpd.hk/gpdwin.asp is the main reason I wrote > this patch. I've requested the manufacturer to do a BIOS update fixing the > DMI strings, but I cannot guarantee that that will happen. > --- > drivers/firmware/dmi_scan.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index 54be60e..c99e753 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -1047,3 +1047,17 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device) > } > } > EXPORT_SYMBOL_GPL(dmi_memdev_name); > + > +static int __init dmi_parse_dmi_product_name(char *str) > +{ > + static char prod_name[32]; > + > + if (!str) > + return -EINVAL; > + > + strlcpy(prod_name, str, sizeof(prod_name)); > + dmi_ident[DMI_PRODUCT_NAME] = prod_name; > + > + return 0; > +} > +early_param("dmi_product_name", dmi_parse_dmi_product_name); -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option 2017-03-03 9:24 ` Jean Delvare @ 2017-03-03 14:27 ` Hans de Goede 2017-03-09 9:59 ` Jean Delvare 0 siblings, 1 reply; 15+ messages in thread From: Hans de Goede @ 2017-03-03 14:27 UTC (permalink / raw) To: Jean Delvare Cc: Adrian Hunter, Ulf Hansson, Jean Delvare, Takashi Iwai, russianneuromancer, linux-mmc, linux-kernel Hi, On 03-03-17 10:24, Jean Delvare wrote: > Hi Hans, > > On Sat, 25 Feb 2017 18:23:55 +0100, Hans de Goede wrote: >> Unfortunately some firmware has all the DMI strings filled with: >> "Default String" (or something equally useless). This makes it impossible >> to apply DMI based quirks to certain machines. > > Sad but true :-( > >> This commit adds a dmi_product_name kernel cmdline option which can >> be used to override the DMI_PRODUCT_NAME string, so that DMI based >> quirks can still be used on such boards, there are 3 reasons for this: >> >> 1) Rather then add cmdline options for all things which can be DMI quirked >> and thus may need to be specified, this only requires adding code for >> a single extra cmdline option > > This cuts both ways. It also means that, if you get a new machine which > needs some of the quirks needed by an older machine, but not all of > them, you can't get it to work without modifying your kernel first. If > quirk options are addressed directly to the relevant subsystem, then > there is a chance that they can be reused directly for other machines > later. Quirks being applied for issues which may be fixed by e.g. a BIOS update already always being applied even if the new BIOS is there already is the case for any DMI based quirks. >> 2) Some devices can be quite quirky, e.g. the GPD win mini laptop / >> clamshell x86 machine, needing several quirks in both kernel and userspace >> (udev hwdb) in this case being able to fake a unique dmi product name >> allows making these devices usable with a single kernel cmdline option >> rather then requiring multiple kernel cmdline options + manual userspace >> tweaking >> >> 3) In some case we may be able to successfully get the manufacturer to >> fix the DMI strings with a firmware update, but not all users may want >> to update, this will allow users to use DMI based quirks without forcing >> them to update their firmware > > But that's the only right thing to do. The easier we make it for > manufacturers shipping crappy BIOSes, the lesser motivated they will be > to fix them. Writing a decent DMI table is a one hour job, there's no > excuse for not doing it. So I am very reluctant to accept this patch, > sorry. This whole we are going to make users live miserable to pressure manufacturers into doing the right thing does not work. We (the kernel community) have tried that for 10 years and not a whole lot has changed and in the cases where things have changed it was not because of this it was because there was a business case for FOSS drivers. Unfortunately there is not a whole lot of business case for correct DMI strings (for consumer hw). So lets not make users live harder then it needs to be. We're talking about either giving the users this set of instructions: a) Add dmi_product_name=GPD-WINI55 to your kernel commandline, then reboot or: b) 1) Add "quirk1=foo quirk2=bar quirk3=argh quirk4=uhoh" to your kernel cmdline. 2) Edit /lib/udev/hwdb/99-local.hwdb, Add: sensor:modalias:acpi:KIOX000A*:dmi*: ACCEL_MOUNT_MATRIX=1, 0, 0; 0, -1, 0; 0, 0, 1 3) As root run: "udevadm hwdb --update" 4) reboot Which one is more userfriendly? Esp. the ability to have a single kernel cmdline option influence both kernel and userspace behavior (by allowing a pre-existing rule for the hw to exist in hwdb) is important since now a days quirks are more or less split 50/50 between the 2. So I would really like to see support for this kernel cmdline option merged. Takashi Iwai has been working on some quirks for headphone detection for the GPDwin machine, which also rely on being able to use a fake dmi_id to identify the machine. And we will likely hit the same problem with intel based tables using silead touchscreens which also require extra platform info not available in the ACPI tables, which currently also gets automatically added based on dmi data, see: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/silead_dmi.c If you look at the amount of info we need per tablet here doing this through the kernel cmdline is going to be quite painful. Also the needed extra code to be able to specify this and many other quirks which are currently only settable through dmi on the kernel cmdline will be many times more then the code for the dmi_product_name kernel cmdline option. Regards, Hans > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Note the GPD win: http://www.gpd.hk/gpdwin.asp is the main reason I wrote >> this patch. I've requested the manufacturer to do a BIOS update fixing the >> DMI strings, but I cannot guarantee that that will happen. >> --- >> drivers/firmware/dmi_scan.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c >> index 54be60e..c99e753 100644 >> --- a/drivers/firmware/dmi_scan.c >> +++ b/drivers/firmware/dmi_scan.c >> @@ -1047,3 +1047,17 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device) >> } >> } >> EXPORT_SYMBOL_GPL(dmi_memdev_name); >> + >> +static int __init dmi_parse_dmi_product_name(char *str) >> +{ >> + static char prod_name[32]; >> + >> + if (!str) >> + return -EINVAL; >> + >> + strlcpy(prod_name, str, sizeof(prod_name)); >> + dmi_ident[DMI_PRODUCT_NAME] = prod_name; >> + >> + return 0; >> +} >> +early_param("dmi_product_name", dmi_parse_dmi_product_name); > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option 2017-03-03 14:27 ` Hans de Goede @ 2017-03-09 9:59 ` Jean Delvare 2017-03-09 10:43 ` Hans de Goede 0 siblings, 1 reply; 15+ messages in thread From: Jean Delvare @ 2017-03-09 9:59 UTC (permalink / raw) To: Hans de Goede Cc: Adrian Hunter, Ulf Hansson, Takashi Iwai, russianneuromancer, linux-mmc, linux-kernel Hi Hans, On ven., 2017-03-03 at 15:27 +0100, Hans de Goede wrote: > On 03-03-17 10:24, Jean Delvare wrote: > > On Sat, 25 Feb 2017 18:23:55 +0100, Hans de Goede wrote: > > > 1) Rather then add cmdline options for all things which can be DMI quirked > > > and thus may need to be specified, this only requires adding code for > > > a single extra cmdline option > > > > This cuts both ways. It also means that, if you get a new machine which > > needs some of the quirks needed by an older machine, but not all of > > them, you can't get it to work without modifying your kernel first. If > > quirk options are addressed directly to the relevant subsystem, then > > there is a chance that they can be reused directly for other machines > > later. > > Quirks being applied for issues which may be fixed by e.g. a BIOS update > already always being applied even if the new BIOS is there already is the > case for any DMI based quirks. I'm afraid you did not get my point. By "new machine", I did not mean a new hardware or BIOS iteration of the same machine. I meant a completely different machine. Say you implemented 4 quirks for machine Foo from vendor A, and mapped them to dmi_product_name=A-Foo. Then vendor B releases machine Bar, which needs 2 of the quirks that machine Foo needed, but not the other 2. You can't pass dmi_product_name=A-Foo for machine Bar, you have to add kernel code to handle dmi_product_name=B-Bar. If instead you had implemented 4 individually-enabled quirks for machine Foo, you could reuse them directly for machine Bar, without requiring kernel changes. And as a side note, your claim that we keep applying quirks on newer BIOS iterations isn't entirely true. Grep the kernel tree for "dmi_get_system_info(DMI_BIOS_VERSION)" or "DMI_MATCH(DMI_BIOS_VERSION". Sometimes applying a quirk which is not needed is harmless, but most of the time it must be avoided. > > > 2) Some devices can be quite quirky, e.g. the GPD win mini laptop / > > > clamshell x86 machine, needing several quirks in both kernel and userspace > > > (udev hwdb) in this case being able to fake a unique dmi product name > > > allows making these devices usable with a single kernel cmdline option > > > rather then requiring multiple kernel cmdline options + manual userspace > > > tweaking > > > > > > 3) In some case we may be able to successfully get the manufacturer to > > > fix the DMI strings with a firmware update, but not all users may want > > > to update, this will allow users to use DMI based quirks without forcing > > > them to update their firmware > > > > But that's the only right thing to do. The easier we make it for > > manufacturers shipping crappy BIOSes, the lesser motivated they will be > > to fix them. Writing a decent DMI table is a one hour job, there's no > > excuse for not doing it. So I am very reluctant to accept this patch, > > sorry. > > This whole we are going to make users live miserable to pressure manufacturers > into doing the right thing does not work. We (the kernel community) have > tried that for 10 years and not a whole lot has changed and in the cases > where things have changed it was not because of this it was because > there was a business case for FOSS drivers. Unfortunately there is not > a whole lot of business case for correct DMI strings (for consumer hw). Actually, as the maintainer of dmidecode, my feeling is that the quality of DMI tables has increased significantly over time. That a vendor dared to ship a system in 2016 with not even a valid product name in the DMI table stunned me, I have to admit. Thankfully it is an exception. What you have in your system is the lowest possible degree of DMI table quality :-( I really believe that Linux is where it is now because we said no when we had to say no. Quality and maintainability comes at this price. > So lets not make users live harder then it needs to be. Users who buy crappy hardware have made their decision. I am not going to make their life harder on purpose, but I am also not going to let them influence the direction of Linux kernel development. You also have to consider what kind of user is going to install Linux on such machines. If it requires any kind of manual tweaking (as even your proposal does) then you can exclude the majority of owners. The remaining few percents will find their way through instructions, even if they require several steps. > We're talking about either giving the users this set of instructions: > > a) Add dmi_product_name=GPD-WINI55 to your kernel commandline, then > reboot > > or: > > b) > 1) Add "quirk1=foo quirk2=bar quirk3=argh quirk4=uhoh" to your kernel cmdline. > 2) Edit /lib/udev/hwdb/99-local.hwdb, Add: > > sensor:modalias:acpi:KIOX000A*:dmi*: > ACCEL_MOUNT_MATRIX=1, 0, 0; 0, -1, 0; 0, 0, 1 > > 3) As root run: "udevadm hwdb --update" > 4) reboot > > Which one is more userfriendly? The answer is obvious, but it would be more honest to present the two options with a neutral point of view. You made "reboot" a separate step in b), not in a). Also steps 2 and 3 of b) are the same thing, really. With more neutrality, a) would still be shorter than b), but the difference would be smaller. > Esp. the ability to have a single kernel cmdline option influence both > kernel and userspace behavior (by allowing a pre-existing rule for the > hw to exist in hwdb) is important since now a days quirks are > more or less split 50/50 between the 2. > > So I would really like to see support for this kernel cmdline option merged. > Takashi Iwai has been working on some quirks for headphone detection for > the GPDwin machine, which also rely on being able to use a fake dmi_id to > identify the machine. I'll discuss that with Takashi when he returns from vacation. > And we will likely hit the same problem with intel based tables using > silead touchscreens which also require extra platform info not available > in the ACPI tables, which currently also gets automatically added based > on dmi data, see: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/silead_dmi.c And what makes you think that systems with crappy DMI data using such touchscreen devices will exist? > If you look at the amount of info we need per tablet here doing this > through the kernel cmdline is going to be quite painful. Also the needed > extra code to be able to specify this and many other quirks which are > currently only settable through dmi on the kernel cmdline will be many > times more then the code for the dmi_product_name kernel cmdline > option. I'm not disputing the potential usefulness of the feature. But I see that you are implementing the minimal solution that addresses your immediate problem, with little consideration for where it will lead us in the long term. One thing that strikes me is that "GPD-WINI55" isn't a "DMI product name". For one thing, you insist on mapping it to DMI for convenience, but it means you are excluding all architectures which do not support DMI. Also GPD is the manufacturer name, not the product name. I'm not sure what the "I55" part comes from, I couldn't find any reference to it on the vendor site. Did you craft it from the fact that this model comes with a 5.5 inches screen? You bundle the whole thing to a single arbitrary string, it looks like a hack, it's not clean. And it's not safe either, as the product name space is per-vendor, so checking for the DMI product name without first checking for the system vendor could lead to unexpected matches (imagine another vendor releases a machine where the full product name is "GPD-WINI55".) Where are you with the vendor, when do they plan to ship that BIOS update that would solve the problem so nicely? -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option 2017-03-09 9:59 ` Jean Delvare @ 2017-03-09 10:43 ` Hans de Goede 2017-03-20 17:35 ` Takashi Iwai 2017-04-04 10:21 ` Hans de Goede 0 siblings, 2 replies; 15+ messages in thread From: Hans de Goede @ 2017-03-09 10:43 UTC (permalink / raw) To: Jean Delvare Cc: Adrian Hunter, Ulf Hansson, Takashi Iwai, russianneuromancer, linux-mmc, linux-kernel Hi, On 09-03-17 10:59, Jean Delvare wrote: > Hi Hans, > > On ven., 2017-03-03 at 15:27 +0100, Hans de Goede wrote: >> On 03-03-17 10:24, Jean Delvare wrote: >>> On Sat, 25 Feb 2017 18:23:55 +0100, Hans de Goede wrote: >>>> 1) Rather then add cmdline options for all things which can be DMI quirked >>>> and thus may need to be specified, this only requires adding code for >>>> a single extra cmdline option >>> >>> This cuts both ways. It also means that, if you get a new machine which >>> needs some of the quirks needed by an older machine, but not all of >>> them, you can't get it to work without modifying your kernel first. If >>> quirk options are addressed directly to the relevant subsystem, then >>> there is a chance that they can be reused directly for other machines >>> later. >> >> Quirks being applied for issues which may be fixed by e.g. a BIOS update >> already always being applied even if the new BIOS is there already is the >> case for any DMI based quirks. > > I'm afraid you did not get my point. By "new machine", I did not mean a > new hardware or BIOS iteration of the same machine. I meant a > completely different machine. I see. > Say you implemented 4 quirks for machine Foo from vendor A, and mapped > them to dmi_product_name=A-Foo. Then vendor B releases machine Bar, > which needs 2 of the quirks that machine Foo needed, but not the other > 2. You can't pass dmi_product_name=A-Foo for machine Bar, you have to > add kernel code to handle dmi_product_name=B-Bar. Right, just as we would if the new machine had proper dmi strings in the first place and chances are really good (luckily) that machine-B will have a proper dmi string, so we would add the new quirks even without the A machine having them via the dmi_product_name hack, since we will want things to work ootb where-ever possible. > If instead you had implemented 4 individually-enabled quirks for > machine Foo, you could reuse them directly for machine Bar, without > requiring kernel changes. True, but as said many many drivers now have quirks which can only be enabled through dmi since that is always the end game, so that things will work ootb. This seems orthogonal to out discussion. > And as a side note, your claim that we keep applying quirks on newer > BIOS iterations isn't entirely true. Grep the kernel tree for > "dmi_get_system_info(DMI_BIOS_VERSION)" or > "DMI_MATCH(DMI_BIOS_VERSION". Sometimes applying a quirk which is not > needed is harmless, but most of the time it must be avoided. True. >>>> 2) Some devices can be quite quirky, e.g. the GPD win mini laptop / >>>> clamshell x86 machine, needing several quirks in both kernel and userspace >>>> (udev hwdb) in this case being able to fake a unique dmi product name >>>> allows making these devices usable with a single kernel cmdline option >>>> rather then requiring multiple kernel cmdline options + manual userspace >>>> tweaking >>>> >>>> 3) In some case we may be able to successfully get the manufacturer to >>>> fix the DMI strings with a firmware update, but not all users may want >>>> to update, this will allow users to use DMI based quirks without forcing >>>> them to update their firmware >>> >>> But that's the only right thing to do. The easier we make it for >>> manufacturers shipping crappy BIOSes, the lesser motivated they will be >>> to fix them. Writing a decent DMI table is a one hour job, there's no >>> excuse for not doing it. So I am very reluctant to accept this patch, >>> sorry. >> >> This whole we are going to make users live miserable to pressure manufacturers >> into doing the right thing does not work. We (the kernel community) have >> tried that for 10 years and not a whole lot has changed and in the cases >> where things have changed it was not because of this it was because >> there was a business case for FOSS drivers. Unfortunately there is not >> a whole lot of business case for correct DMI strings (for consumer hw). > > Actually, as the maintainer of dmidecode, my feeling is that the > quality of DMI tables has increased significantly over time. That a > vendor dared to ship a system in 2016 with not even a valid product > name in the DMI table stunned me, I have to admit. Thankfully it is an > exception. What you have in your system is the lowest possible degree > of DMI table quality :-( Yeah. > I really believe that Linux is where it is now because we said no when > we had to say no. Quality and maintainability comes at this price. > >> So lets not make users live harder then it needs to be. > > Users who buy crappy hardware have made their decision. I am not going > to make their life harder on purpose, but I am also not going to let > them influence the direction of Linux kernel development. This specific machine is not crappy (although the DMI tables are) and fills a niche where there is almost 0 choice. > You also have to consider what kind of user is going to install Linux > on such machines. If it requires any kind of manual tweaking (as even > your proposal does) then you can exclude the majority of owners. The > remaining few percents will find their way through instructions, even > if they require several steps. Still I would like to make things as easy as possible and unfortunately adding kernel cmdline tweaks is something which quite a few users eventually end up doing, so this is well documented. >> We're talking about either giving the users this set of instructions: >> >> a) Add dmi_product_name=GPD-WINI55 to your kernel commandline, then >> reboot >> >> or: >> >> b) >> 1) Add "quirk1=foo quirk2=bar quirk3=argh quirk4=uhoh" to your kernel cmdline. >> 2) Edit /lib/udev/hwdb/99-local.hwdb, Add: >> >> sensor:modalias:acpi:KIOX000A*:dmi*: >> ACCEL_MOUNT_MATRIX=1, 0, 0; 0, -1, 0; 0, 0, 1 >> >> 3) As root run: "udevadm hwdb --update" >> 4) reboot >> >> Which one is more userfriendly? > > The answer is obvious, but it would be more honest to present the two > options with a neutral point of view. You made "reboot" a separate step > in b), not in a). Also steps 2 and 3 of b) are the same thing, really. > With more neutrality, a) would still be shorter than b), but the > difference would be smaller. > >> Esp. the ability to have a single kernel cmdline option influence both >> kernel and userspace behavior (by allowing a pre-existing rule for the >> hw to exist in hwdb) is important since now a days quirks are >> more or less split 50/50 between the 2. This to me is the most important reason for wanting this, without the dmi_product_name= hack (I admit it is a hack) we cannot have userspace quirks unless we get the user to unconditionally add them. Another big advantage of the dmi_product_name= hack (I admit it is a hack) is that we can add new quirks as we discover the need for them, e.g as we do hardware enablement for more bits and pieces (such as battery monitoring) on the machine and users will then get these automatically applied when they install a new kernel. >> So I would really like to see support for this kernel cmdline option merged. >> Takashi Iwai has been working on some quirks for headphone detection for >> the GPDwin machine, which also rely on being able to use a fake dmi_id to >> identify the machine. > > I'll discuss that with Takashi when he returns from vacation. Ok, lets wait for that then. >> And we will likely hit the same problem with intel based tables using >> silead touchscreens which also require extra platform info not available >> in the ACPI tables, which currently also gets automatically added based >> on dmi data, see: >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/silead_dmi.c > > And what makes you think that systems with crappy DMI data using such > touchscreen devices will exist? Because these touchscreens are used in the cheapest tablets available, so I'm afraid we will eventually hit one with DMI data as bad as the GPD-win >> If you look at the amount of info we need per tablet here doing this >> through the kernel cmdline is going to be quite painful. Also the needed >> extra code to be able to specify this and many other quirks which are >> currently only settable through dmi on the kernel cmdline will be many >> times more then the code for the dmi_product_name kernel cmdline >> option. > > I'm not disputing the potential usefulness of the feature. But I see > that you are implementing the minimal solution that addresses your > immediate problem, with little consideration for where it will lead us > in the long term. That is a bit of a vague statement if you foresee specific problems with this patch in the future please state them. > One thing that strikes me is that "GPD-WINI55" isn't a "DMI product > name". AFAICT there really is no clear definition (in practice) of what goes into the product-name people in general identify this machine as the GPDwin or GPD win so to me that is the product name, also I did not want to also add a dmi_sys_vendor kernel cmdline argument since just having a unique product name string is enough and that would double the code added for this hack. > For one thing, you insist on mapping it to DMI for convenience, > but it means you are excluding all architectures which do not support > DMI. True, but other architectures have e.g. devicetree where we can fix this with an updated .dtb file. > Also GPD is the manufacturer name, not the product name. See my answer to why GPD is included in the product name above, in practice everyone calls the product the "GPD win" just doing a web- search for "win" is not going to find you anything useful. > I'm not sure > what the "I55" part comes from, I couldn't find any reference to it on > the vendor site. Did you craft it from the fact that this model comes > with a 5.5 inches screen? I wanted something more specific then GPD win in case GPD releases a new significantly different version. The PCB is marked WINI55 so that is where that comes from. > You bundle the whole thing to a single > arbitrary string, it looks like a hack, it's not clean. And it's not > safe either, as the product name space is per-vendor, so checking for > the DMI product name without first checking for the system vendor could > lead to unexpected matches (imagine another vendor releases a machine > where the full product name is "GPD-WINI55".) There are many examples where dmi matches are done only on the product_name, or any other dmi string as long as it seems unique, I don't see this bit as a problem, but if you want to also add a dmi_sys_vendor kernel cmdline argument, sure we can do that. > Where are you with the vendor, when do they plan to ship that BIOS > update that would solve the problem so nicely? They are not answering on either their forum or their contact email address from there webpage :| Anyways lets wait for Takashi to get back from vacation and you've had a discussion with him about this and then we will see from there. Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option 2017-03-09 10:43 ` Hans de Goede @ 2017-03-20 17:35 ` Takashi Iwai 2017-04-04 10:21 ` Hans de Goede 1 sibling, 0 replies; 15+ messages in thread From: Takashi Iwai @ 2017-03-20 17:35 UTC (permalink / raw) To: Hans de Goede Cc: Jean Delvare, Adrian Hunter, Ulf Hansson, russianneuromancer, linux-mmc, linux-kernel On Thu, 09 Mar 2017 11:43:52 +0100, Hans de Goede wrote: > > Hi, > > On 09-03-17 10:59, Jean Delvare wrote: > > Hi Hans, > > > > On ven., 2017-03-03 at 15:27 +0100, Hans de Goede wrote: > >> On 03-03-17 10:24, Jean Delvare wrote: > >>> On Sat, 25 Feb 2017 18:23:55 +0100, Hans de Goede wrote: > >>>> 1) Rather then add cmdline options for all things which can be DMI quirked > >>>> and thus may need to be specified, this only requires adding code for > >>>> a single extra cmdline option > >>> > >>> This cuts both ways. It also means that, if you get a new machine which > >>> needs some of the quirks needed by an older machine, but not all of > >>> them, you can't get it to work without modifying your kernel first. If > >>> quirk options are addressed directly to the relevant subsystem, then > >>> there is a chance that they can be reused directly for other machines > >>> later. > >> > >> Quirks being applied for issues which may be fixed by e.g. a BIOS update > >> already always being applied even if the new BIOS is there already is the > >> case for any DMI based quirks. > > > > I'm afraid you did not get my point. By "new machine", I did not mean a > > new hardware or BIOS iteration of the same machine. I meant a > > completely different machine. > > I see. > > > Say you implemented 4 quirks for machine Foo from vendor A, and mapped > > them to dmi_product_name=A-Foo. Then vendor B releases machine Bar, > > which needs 2 of the quirks that machine Foo needed, but not the other > > 2. You can't pass dmi_product_name=A-Foo for machine Bar, you have to > > add kernel code to handle dmi_product_name=B-Bar. > > Right, just as we would if the new machine had proper dmi strings in > the first place and chances are really good (luckily) that machine-B > will have a proper dmi string, so we would add the new quirks even > without the A machine having them via the dmi_product_name hack, since > we will want things to work ootb where-ever possible. > > > If instead you had implemented 4 individually-enabled quirks for > > machine Foo, you could reuse them directly for machine Bar, without > > requiring kernel changes. > > True, but as said many many drivers now have quirks which can only > be enabled through dmi since that is always the end game, so that > things will work ootb. This seems orthogonal to out discussion. > > > And as a side note, your claim that we keep applying quirks on newer > > BIOS iterations isn't entirely true. Grep the kernel tree for > > "dmi_get_system_info(DMI_BIOS_VERSION)" or > > "DMI_MATCH(DMI_BIOS_VERSION". Sometimes applying a quirk which is not > > needed is harmless, but most of the time it must be avoided. > > True. > > >>>> 2) Some devices can be quite quirky, e.g. the GPD win mini laptop / > >>>> clamshell x86 machine, needing several quirks in both kernel and userspace > >>>> (udev hwdb) in this case being able to fake a unique dmi product name > >>>> allows making these devices usable with a single kernel cmdline option > >>>> rather then requiring multiple kernel cmdline options + manual userspace > >>>> tweaking > >>>> > >>>> 3) In some case we may be able to successfully get the manufacturer to > >>>> fix the DMI strings with a firmware update, but not all users may want > >>>> to update, this will allow users to use DMI based quirks without forcing > >>>> them to update their firmware > >>> > >>> But that's the only right thing to do. The easier we make it for > >>> manufacturers shipping crappy BIOSes, the lesser motivated they will be > >>> to fix them. Writing a decent DMI table is a one hour job, there's no > >>> excuse for not doing it. So I am very reluctant to accept this patch, > >>> sorry. > >> > >> This whole we are going to make users live miserable to pressure manufacturers > >> into doing the right thing does not work. We (the kernel community) have > >> tried that for 10 years and not a whole lot has changed and in the cases > >> where things have changed it was not because of this it was because > >> there was a business case for FOSS drivers. Unfortunately there is not > >> a whole lot of business case for correct DMI strings (for consumer hw). > > > > Actually, as the maintainer of dmidecode, my feeling is that the > > quality of DMI tables has increased significantly over time. That a > > vendor dared to ship a system in 2016 with not even a valid product > > name in the DMI table stunned me, I have to admit. Thankfully it is an > > exception. What you have in your system is the lowest possible degree > > of DMI table quality :-( > > Yeah. > > > I really believe that Linux is where it is now because we said no when > > we had to say no. Quality and maintainability comes at this price. > > > >> So lets not make users live harder then it needs to be. > > > > Users who buy crappy hardware have made their decision. I am not going > > to make their life harder on purpose, but I am also not going to let > > them influence the direction of Linux kernel development. > > This specific machine is not crappy (although the DMI tables are) > and fills a niche where there is almost 0 choice. > > > You also have to consider what kind of user is going to install Linux > > on such machines. If it requires any kind of manual tweaking (as even > > your proposal does) then you can exclude the majority of owners. The > > remaining few percents will find their way through instructions, even > > if they require several steps. > > Still I would like to make things as easy as possible and unfortunately > adding kernel cmdline tweaks is something which quite a few users > eventually end up doing, so this is well documented. > > >> We're talking about either giving the users this set of instructions: > >> > >> a) Add dmi_product_name=GPD-WINI55 to your kernel commandline, then > >> reboot > >> > >> or: > >> > >> b) > >> 1) Add "quirk1=foo quirk2=bar quirk3=argh quirk4=uhoh" to your kernel cmdline. > >> 2) Edit /lib/udev/hwdb/99-local.hwdb, Add: > >> > >> sensor:modalias:acpi:KIOX000A*:dmi*: > >> ACCEL_MOUNT_MATRIX=1, 0, 0; 0, -1, 0; 0, 0, 1 > >> > >> 3) As root run: "udevadm hwdb --update" > >> 4) reboot > >> > >> Which one is more userfriendly? > > > > The answer is obvious, but it would be more honest to present the two > > options with a neutral point of view. You made "reboot" a separate step > > in b), not in a). Also steps 2 and 3 of b) are the same thing, really. > > With more neutrality, a) would still be shorter than b), but the > > difference would be smaller. > > > >> Esp. the ability to have a single kernel cmdline option influence both > >> kernel and userspace behavior (by allowing a pre-existing rule for the > >> hw to exist in hwdb) is important since now a days quirks are > >> more or less split 50/50 between the 2. > > This to me is the most important reason for wanting this, without > the dmi_product_name= hack (I admit it is a hack) we cannot have userspace > quirks unless we get the user to unconditionally add them. > > Another big advantage of the dmi_product_name= hack (I admit it is > a hack) is that we can add new quirks as we discover the need for > them, e.g as we do hardware enablement for more bits and pieces > (such as battery monitoring) on the machine and users will then get > these automatically applied when they install a new kernel. > > >> So I would really like to see support for this kernel cmdline option merged. > >> Takashi Iwai has been working on some quirks for headphone detection for > >> the GPDwin machine, which also rely on being able to use a fake dmi_id to > >> identify the machine. > > > > I'll discuss that with Takashi when he returns from vacation. > > Ok, lets wait for that then. Sorry to be late for joining to the game. About the sound issue: it's for the jack detection logic implemented in the audio codec driver, and the setup is specific to the hardware implementation. On ARM systems, this could be well informed via DT as we like. But on Intel, it's not listed in ACPI, either. So currently the quirk is provided only via DMI matching. There is no module option, so far. As Hans already explained, the problem is that GPD Win device gives literally no information as identification. There is no specific PCI SSID, no proper DMI set. So, from that POV, manually tweaking the DMI is the simplest solution. If we don't use this, it will need a lot of harder efforts -- implement the module option or such tunable knobs per codec driver, which isn't always things the developers want to have. > >> And we will likely hit the same problem with intel based tables using > >> silead touchscreens which also require extra platform info not available > >> in the ACPI tables, which currently also gets automatically added based > >> on dmi data, see: > >> > >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/silead_dmi.c > > > > And what makes you think that systems with crappy DMI data using such > > touchscreen devices will exist? > > Because these touchscreens are used in the cheapest tablets available, > so I'm afraid we will eventually hit one with DMI data as bad as the > GPD-win Right. Because things on Intel Cherrytrail or Baytrail systems have no other information but ACPI, and ACPI implmentations on such devices are really crappy, mostly just copying from the reference code, we're facing lots of problems day-by-day. The pragmatic solutions we've taken in many fields are quirks based on DMI matching in the end. Will this be still maintainable in future even if the number of devices continue to grow? I don't know. But to make the things working for now, the DMI override hack looks like the simplest way to me. > >> If you look at the amount of info we need per tablet here doing this > >> through the kernel cmdline is going to be quite painful. Also the needed > >> extra code to be able to specify this and many other quirks which are > >> currently only settable through dmi on the kernel cmdline will be many > >> times more then the code for the dmi_product_name kernel cmdline > >> option. > > > > I'm not disputing the potential usefulness of the feature. But I see > > that you are implementing the minimal solution that addresses your > > immediate problem, with little consideration for where it will lead us > > in the long term. > > That is a bit of a vague statement if you foresee specific problems > with this patch in the future please state them. Indeed. An override of DMI strings is hacky, but I see no big problems in it. You can break the running system by that, but this isn't the only way to do so :) > > One thing that strikes me is that "GPD-WINI55" isn't a "DMI product > > name". > > AFAICT there really is no clear definition (in practice) of what goes > into the product-name people in general identify this machine as the > GPDwin or GPD win so to me that is the product name, also I did not > want to also add a dmi_sys_vendor kernel cmdline argument since just > having a unique product name string is enough and that would double > the code added for this hack. Maybe we can set both fields with some separator (e.g. ":") in a shot? Also some sanitization of strings would be safer, too. > > For one thing, you insist on mapping it to DMI for convenience, > > but it means you are excluding all architectures which do not support > > DMI. > > True, but other architectures have e.g. devicetree where we can fix > this with an updated .dtb file. Right, it's specific to Intel SoC devices, so far. > > Also GPD is the manufacturer name, not the product name. > > See my answer to why GPD is included in the product name above, in > practice everyone calls the product the "GPD win" just doing a web- > search for "win" is not going to find you anything useful. > > > I'm not sure > > what the "I55" part comes from, I couldn't find any reference to it on > > the vendor site. Did you craft it from the fact that this model comes > > with a 5.5 inches screen? > > I wanted something more specific then GPD win in case GPD releases > a new significantly different version. The PCB is marked WINI55 > so that is where that comes from. > > > You bundle the whole thing to a single > > arbitrary string, it looks like a hack, it's not clean. And it's not > > safe either, as the product name space is per-vendor, so checking for > > the DMI product name without first checking for the system vendor could > > lead to unexpected matches (imagine another vendor releases a machine > > where the full product name is "GPD-WINI55".) > > There are many examples where dmi matches are done only on the > product_name, or any other dmi string as long as it seems unique, > I don't see this bit as a problem, but if you want to also add a > dmi_sys_vendor kernel cmdline argument, sure we can do that. > > > Where are you with the vendor, when do they plan to ship that BIOS > > update that would solve the problem so nicely? > > They are not answering on either their forum or their contact email > address from there webpage :| > > > Anyways lets wait for Takashi to get back from vacation and you've > had a discussion with him about this and then we will see from > there. So I'm back, and let the discussion fly :) thanks, Takashi ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option 2017-03-09 10:43 ` Hans de Goede @ 2017-04-04 10:21 ` Hans de Goede 2017-04-04 10:21 ` Hans de Goede 1 sibling, 0 replies; 15+ messages in thread From: Hans de Goede @ 2017-04-04 10:21 UTC (permalink / raw) To: Jean Delvare Cc: Adrian Hunter, Ulf Hansson, Takashi Iwai, russianneuromancer, linux-mmc, linux-kernel, Takashi Iwai Hi, On 09-03-17 11:43, Hans de Goede wrote: > Hi, > > On 09-03-17 10:59, Jean Delvare wrote: <snip> >>> So I would really like to see support for this kernel cmdline option merged. >>> Takashi Iwai has been working on some quirks for headphone detection for >>> the GPDwin machine, which also rely on being able to use a fake dmi_id to >>> identify the machine. >> >> I'll discuss that with Takashi when he returns from vacation. > > Ok, lets wait for that then. So Takashi is back (and has responded) but in the mean time I've been thinking about this and tried to find a better solution as I really want the kernel to do the right thing automatically. So I've been dumping all the /sys/class/dmi/id strings on all Bay Trail and Cherry Trail devices I've (7 different models) and seeing if anything stands out. The GPDwin for which I mainly wrote this patch is the only one to set board_vendor to "AMI Corporation", which is a string one would usually expect in bios_vendor. So I decided that we can use normal DMI matching for this after all, to play things safe I've also added a check for the bios_date (which should be reasonable unique) + 2 other strings, resulting in: +static const struct dmi_system_id fix_up_power_blacklist[] = { + { + /* + * Match for the GPDwin which unfortunately uses somewhat + * generic dmi strings, which is why the bios-date match is + * included and we need multiple entries :| These strings have + * been checked against 6 other byt/cht boards and board_vendor + * and board_name are unique to the GPDwin (in the test set) + * where as only one other board has the same board_version. + */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Default string"), + DMI_MATCH(DMI_BOARD_VERSION, "Default string"), + DMI_MATCH(DMI_BIOS_DATE, "10/25/2016"), + }, + }, + { + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Default string"), + DMI_MATCH(DMI_BOARD_VERSION, "Default string"), + DMI_MATCH(DMI_BIOS_DATE, "11/18/2016"), + }, + }, + { + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Default string"), + DMI_MATCH(DMI_BOARD_VERSION, "Default string"), + DMI_MATCH(DMI_BIOS_DATE, "02/21/2017"), + }, + }, + { } +}; Which is not the prettiest but gets the job done and has the big advantage over my dmi_product_name kernel cmdline option proposal that it will just work. So you can consider this patch dropped from my pov. Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option @ 2017-04-04 10:21 ` Hans de Goede 0 siblings, 0 replies; 15+ messages in thread From: Hans de Goede @ 2017-04-04 10:21 UTC (permalink / raw) To: Jean Delvare Cc: Adrian Hunter, Ulf Hansson, Takashi Iwai, russianneuromancer, linux-mmc, linux-kernel Hi, On 09-03-17 11:43, Hans de Goede wrote: > Hi, > > On 09-03-17 10:59, Jean Delvare wrote: <snip> >>> So I would really like to see support for this kernel cmdline option merged. >>> Takashi Iwai has been working on some quirks for headphone detection for >>> the GPDwin machine, which also rely on being able to use a fake dmi_id to >>> identify the machine. >> >> I'll discuss that with Takashi when he returns from vacation. > > Ok, lets wait for that then. So Takashi is back (and has responded) but in the mean time I've been thinking about this and tried to find a better solution as I really want the kernel to do the right thing automatically. So I've been dumping all the /sys/class/dmi/id strings on all Bay Trail and Cherry Trail devices I've (7 different models) and seeing if anything stands out. The GPDwin for which I mainly wrote this patch is the only one to set board_vendor to "AMI Corporation", which is a string one would usually expect in bios_vendor. So I decided that we can use normal DMI matching for this after all, to play things safe I've also added a check for the bios_date (which should be reasonable unique) + 2 other strings, resulting in: +static const struct dmi_system_id fix_up_power_blacklist[] = { + { + /* + * Match for the GPDwin which unfortunately uses somewhat + * generic dmi strings, which is why the bios-date match is + * included and we need multiple entries :| These strings have + * been checked against 6 other byt/cht boards and board_vendor + * and board_name are unique to the GPDwin (in the test set) + * where as only one other board has the same board_version. + */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Default string"), + DMI_MATCH(DMI_BOARD_VERSION, "Default string"), + DMI_MATCH(DMI_BIOS_DATE, "10/25/2016"), + }, + }, + { + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Default string"), + DMI_MATCH(DMI_BOARD_VERSION, "Default string"), + DMI_MATCH(DMI_BIOS_DATE, "11/18/2016"), + }, + }, + { + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Default string"), + DMI_MATCH(DMI_BOARD_VERSION, "Default string"), + DMI_MATCH(DMI_BIOS_DATE, "02/21/2017"), + }, + }, + { } +}; Which is not the prettiest but gets the job done and has the big advantage over my dmi_product_name kernel cmdline option proposal that it will just work. So you can consider this patch dropped from my pov. Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() 2017-02-25 17:23 [PATCH 0/3] Allow setting dmi-product-name on the cmdline + dmi sdhci quirk Hans de Goede 2017-02-25 17:23 ` [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option Hans de Goede @ 2017-02-25 17:23 ` Hans de Goede 2017-02-25 18:31 ` Hans de Goede ` (2 more replies) 2017-02-25 17:23 ` [PATCH 3/3] mmc: sdhci-acpi: Add a quirk to not break wifi on GPD WIN I55 machines Hans de Goede 2 siblings, 3 replies; 15+ messages in thread From: Hans de Goede @ 2017-02-25 17:23 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson, Jean Delvare Cc: Hans de Goede, Takashi Iwai, russianneuromancer @ ya . ru, linux-mmc, linux-kernel Calling acpi_device_fix_up_power() on a device which is not present is not a good idea. While at it also call acpi_bus_get_status() on the children before the status check to make sure that child->status contains valid data. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/mmc/host/sdhci-acpi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c index 96465ff..873beae 100644 --- a/drivers/mmc/host/sdhci-acpi.c +++ b/drivers/mmc/host/sdhci-acpi.c @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) if (acpi_bus_get_device(handle, &device)) return -ENODEV; + if (acpi_bus_get_status(device) || !device->status.present) + return -ENODEV; + /* Power on the SDHCI controller and its children */ acpi_device_fix_up_power(device); list_for_each_entry(child, &device->children, node) if (child->status.present && child->status.enabled) acpi_device_fix_up_power(child); - if (acpi_bus_get_status(device) || !device->status.present) - return -ENODEV; - if (sdhci_acpi_byt_defer(dev)) return -EPROBE_DEFER; -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() 2017-02-25 17:23 ` [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() Hans de Goede @ 2017-02-25 18:31 ` Hans de Goede 2017-03-02 11:53 ` Adrian Hunter 2017-03-03 9:09 ` Jean Delvare 2 siblings, 0 replies; 15+ messages in thread From: Hans de Goede @ 2017-02-25 18:31 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson, Jean Delvare Cc: Takashi Iwai, russianneuromancer @ ya . ru, linux-mmc, linux-kernel HI, On 25-02-17 18:23, Hans de Goede wrote: > Calling acpi_device_fix_up_power() on a device which is not present > is not a good idea. > > While at it also call acpi_bus_get_status() on the children before > the status check to make sure that child->status contains valid data. This paragraph of the commit msg should be removed, I dropped that part of the patch since acpi_init_device_object() already sets status and all acpi_device's get that called upon creation. Regards, Hans > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/mmc/host/sdhci-acpi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index 96465ff..873beae 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > if (acpi_bus_get_device(handle, &device)) > return -ENODEV; > > + if (acpi_bus_get_status(device) || !device->status.present) > + return -ENODEV; > + > /* Power on the SDHCI controller and its children */ > acpi_device_fix_up_power(device); > list_for_each_entry(child, &device->children, node) > if (child->status.present && child->status.enabled) > acpi_device_fix_up_power(child); > > - if (acpi_bus_get_status(device) || !device->status.present) > - return -ENODEV; > - > if (sdhci_acpi_byt_defer(dev)) > return -EPROBE_DEFER; > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() 2017-02-25 17:23 ` [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() Hans de Goede 2017-02-25 18:31 ` Hans de Goede @ 2017-03-02 11:53 ` Adrian Hunter 2017-03-03 9:09 ` Jean Delvare 2 siblings, 0 replies; 15+ messages in thread From: Adrian Hunter @ 2017-03-02 11:53 UTC (permalink / raw) To: Hans de Goede, Ulf Hansson, Jean Delvare Cc: Takashi Iwai, russianneuromancer @ ya . ru, linux-mmc, linux-kernel On 25/02/17 19:23, Hans de Goede wrote: > Calling acpi_device_fix_up_power() on a device which is not present > is not a good idea. > > While at it also call acpi_bus_get_status() on the children before > the status check to make sure that child->status contains valid data. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-acpi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index 96465ff..873beae 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > if (acpi_bus_get_device(handle, &device)) > return -ENODEV; > > + if (acpi_bus_get_status(device) || !device->status.present) > + return -ENODEV; > + > /* Power on the SDHCI controller and its children */ > acpi_device_fix_up_power(device); > list_for_each_entry(child, &device->children, node) > if (child->status.present && child->status.enabled) > acpi_device_fix_up_power(child); > > - if (acpi_bus_get_status(device) || !device->status.present) > - return -ENODEV; > - > if (sdhci_acpi_byt_defer(dev)) > return -EPROBE_DEFER; > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() 2017-02-25 17:23 ` [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() Hans de Goede 2017-02-25 18:31 ` Hans de Goede 2017-03-02 11:53 ` Adrian Hunter @ 2017-03-03 9:09 ` Jean Delvare 2017-03-03 13:59 ` Hans de Goede 2 siblings, 1 reply; 15+ messages in thread From: Jean Delvare @ 2017-03-03 9:09 UTC (permalink / raw) To: Hans de Goede Cc: Adrian Hunter, Ulf Hansson, Jean Delvare, Takashi Iwai, russianneuromancer, linux-mmc, linux-kernel Hi Hans, Adrian, On Sat, 25 Feb 2017 18:23:56 +0100, Hans de Goede wrote: > Calling acpi_device_fix_up_power() on a device which is not present > is not a good idea. How bad is it? This was introduced by commit e5bbf30733f9, which was backported to several stable branches. If it causes real trouble then this fix-up patch should be annotated with Fixes: e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are powered when") and Cc's to stable@, so it can be propagated to all affected trees. > While at it also call acpi_bus_get_status() on the children before > the status check to make sure that child->status contains valid data. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/mmc/host/sdhci-acpi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index 96465ff..873beae 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > if (acpi_bus_get_device(handle, &device)) > return -ENODEV; > > + if (acpi_bus_get_status(device) || !device->status.present) > + return -ENODEV; > + > /* Power on the SDHCI controller and its children */ > acpi_device_fix_up_power(device); > list_for_each_entry(child, &device->children, node) > if (child->status.present && child->status.enabled) > acpi_device_fix_up_power(child); > > - if (acpi_bus_get_status(device) || !device->status.present) > - return -ENODEV; > - > if (sdhci_acpi_byt_defer(dev)) > return -EPROBE_DEFER; > -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() 2017-03-03 9:09 ` Jean Delvare @ 2017-03-03 13:59 ` Hans de Goede 0 siblings, 0 replies; 15+ messages in thread From: Hans de Goede @ 2017-03-03 13:59 UTC (permalink / raw) To: Jean Delvare Cc: Adrian Hunter, Ulf Hansson, Jean Delvare, Takashi Iwai, russianneuromancer, linux-mmc, linux-kernel, Hans de Goede Hi, On 03-03-17 10:09, Jean Delvare wrote: > Hi Hans, Adrian, > > On Sat, 25 Feb 2017 18:23:56 +0100, Hans de Goede wrote: >> Calling acpi_device_fix_up_power() on a device which is not present >> is not a good idea. > > How bad is it? Not that bad really, thinking more about this, since drivers/mmc/host/sdhci-acpi.c is a platform driver, its probe function will never get called if !device->status.present, since the acpi-subsys then will not instantiate a platform device for the acpi-dev in question at all. So a better fix would be to just drop the superfluous check. I'll send a new version doing this. Regards, Hans > > This was introduced by commit e5bbf30733f9, which was backported to > several stable branches. If it causes real trouble then this fix-up > patch should be annotated with > > Fixes: e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are powered when") > > and Cc's to stable@, so it can be propagated to all affected trees. > >> While at it also call acpi_bus_get_status() on the children before >> the status check to make sure that child->status contains valid data. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/mmc/host/sdhci-acpi.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c >> index 96465ff..873beae 100644 >> --- a/drivers/mmc/host/sdhci-acpi.c >> +++ b/drivers/mmc/host/sdhci-acpi.c >> @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) >> if (acpi_bus_get_device(handle, &device)) >> return -ENODEV; >> >> + if (acpi_bus_get_status(device) || !device->status.present) >> + return -ENODEV; >> + >> /* Power on the SDHCI controller and its children */ >> acpi_device_fix_up_power(device); >> list_for_each_entry(child, &device->children, node) >> if (child->status.present && child->status.enabled) >> acpi_device_fix_up_power(child); >> >> - if (acpi_bus_get_status(device) || !device->status.present) >> - return -ENODEV; >> - >> if (sdhci_acpi_byt_defer(dev)) >> return -EPROBE_DEFER; >> > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] mmc: sdhci-acpi: Add a quirk to not break wifi on GPD WIN I55 machines 2017-02-25 17:23 [PATCH 0/3] Allow setting dmi-product-name on the cmdline + dmi sdhci quirk Hans de Goede 2017-02-25 17:23 ` [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option Hans de Goede 2017-02-25 17:23 ` [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() Hans de Goede @ 2017-02-25 17:23 ` Hans de Goede 2 siblings, 0 replies; 15+ messages in thread From: Hans de Goede @ 2017-02-25 17:23 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson, Jean Delvare Cc: Hans de Goede, Takashi Iwai, russianneuromancer @ ya . ru, linux-mmc, linux-kernel The GPD WIN (I55) has a bug in its ACPI tables where putting the unused 80860F14 UID 2 (SDIO) device in PS0 toggles a gpio disabling the pcie wifi until the next reboot. Add a quirk to skip probing of the unused SDIO controller, fixing this. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/mmc/host/sdhci-acpi.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c index 873beae..957d0b8 100644 --- a/drivers/mmc/host/sdhci-acpi.c +++ b/drivers/mmc/host/sdhci-acpi.c @@ -36,6 +36,7 @@ #include <linux/pm.h> #include <linux/pm_runtime.h> #include <linux/delay.h> +#include <linux/dmi.h> #include <linux/mmc/host.h> #include <linux/mmc/pm.h> @@ -397,6 +398,18 @@ static int sdhci_acpi_probe(struct platform_device *pdev) if (acpi_bus_get_status(device) || !device->status.present) return -ENODEV; + hid = acpi_device_hid(device); + uid = device->pnp.unique_id; + + /* + * The GPD WIN (I55) has a bug in its ACPI tables where putting the + * unused 80860F14 UID 2 (SDIO) device in PS0 toggles a gpio + * disabling the pcie wifi. + */ + if (strcmp(hid, "80860F14") == 0 && strcmp(uid, "2") == 0 && + dmi_match(DMI_PRODUCT_NAME, "GPD-WINI55")) + return -ENODEV; + /* Power on the SDHCI controller and its children */ acpi_device_fix_up_power(device); list_for_each_entry(child, &device->children, node) @@ -406,9 +419,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev) if (sdhci_acpi_byt_defer(dev)) return -EPROBE_DEFER; - hid = acpi_device_hid(device); - uid = device->pnp.unique_id; - iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!iomem) return -ENOMEM; -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-04 10:21 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-25 17:23 [PATCH 0/3] Allow setting dmi-product-name on the cmdline + dmi sdhci quirk Hans de Goede 2017-02-25 17:23 ` [PATCH 1/3] firmware: dmi_scan: Add dmi_product_name kernel cmdline option Hans de Goede 2017-03-03 9:24 ` Jean Delvare 2017-03-03 14:27 ` Hans de Goede 2017-03-09 9:59 ` Jean Delvare 2017-03-09 10:43 ` Hans de Goede 2017-03-20 17:35 ` Takashi Iwai 2017-04-04 10:21 ` Hans de Goede 2017-04-04 10:21 ` Hans de Goede 2017-02-25 17:23 ` [PATCH 2/3] mmc: sdhci-acpi: Check device status before calling fix_up_power() Hans de Goede 2017-02-25 18:31 ` Hans de Goede 2017-03-02 11:53 ` Adrian Hunter 2017-03-03 9:09 ` Jean Delvare 2017-03-03 13:59 ` Hans de Goede 2017-02-25 17:23 ` [PATCH 3/3] mmc: sdhci-acpi: Add a quirk to not break wifi on GPD WIN I55 machines Hans de Goede
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.