Re: [PATCH v4 2/2] mmc: sdhci-acpi: Add DMI based blacklist

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 16-06-17 14:34, Adrian Hunter wrote:
> On 16/06/17 15:33, Hans de Goede wrote:
>> Hi,
>>
>> On 14-06-17 15:20, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 14-06-17 09:43, Adrian Hunter wrote:
>>>> On 12/06/17 16:27, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 12-06-17 14:11, Adrian Hunter wrote:
>>>>>> On 08/06/17 21:55, Hans de Goede wrote:
>>>>>>> Add a DMI based blacklist for systems where probing some sdio interfaces
>>>>>>> is harmful (e.g. causes pci-e based wifi to not work).
>>>>>>>
>>>>>>> BugLink: https://bbs.archlinux.org/viewtopic.php?id=224086
>>>>>>> Fixes: db52d4f8a4bd ("mmc: sdhci-acpi: support 80860F14 UID 2 SDIO bus")
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> -Adjust for changes in mmc: sdhci-acpi: Add fix_up_power_blacklist module
>>>>>>> option
>>>>>>> -Only use a single fix_up_power_dmi_blacklist for the GPDwin further
>>>>>>> testing
>>>>>>>     has shown that the DMI strings are unique enough that we do not
>>>>>>> need the
>>>>>>>     bios-date in there
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>> -Adjust for changes to "mmc: sdhci-acpi: Add blacklist module option"
>>>>>>>
>>>>>>> Changes in v4:
>>>>>>> -Rename blacklist to dmi_probe_blacklist as it now blacklists probing,
>>>>>>>     rather then calling acpi_device_fix_up_power.
>>>>>>> -Also check bios-date against known bios-dates for the GPD win, to avoid
>>>>>>>     possible false positives due to the use of quite generic DMI strings
>>>>>>> -Add Fixes and BugLink tags
>>>>>>> ---
>>>>>>>     drivers/mmc/host/sdhci-acpi.c | 64
>>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>>>     1 file changed, 64 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c
>>>>>>> b/drivers/mmc/host/sdhci-acpi.c
>>>>>>> index ecc3aefd4643..3e12a6a8ad99 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>
>>>>>>> @@ -83,6 +84,11 @@ struct sdhci_acpi_host {
>>>>>>>         bool                use_runtime_pm;
>>>>>>>     };
>>>>>>>     +struct dmi_probe_blacklist_data {
>>>>>>> +    const char *hid_uid;
>>>>>>> +    const char * const *bios_dates;
>>>>>>> +};
>>>>>>> +
>>>>>>>     static char *blacklist;
>>>>>>>       static bool sdhci_acpi_compare_hid_uid(const char *match, const char
>>>>>>> *hid,
>>>>>>> @@ -116,6 +122,34 @@ static bool sdhci_acpi_compare_hid_uid(const char
>>>>>>> *match, const char *hid,
>>>>>>>         return false;
>>>>>>>     }
>>>>>>>     +static const char *sdhci_acpi_get_dmi_blacklist(const struct
>>>>>>> dmi_system_id *bl)
>>>>>>> +{
>>>>>>> +    const struct dmi_system_id *dmi_id;
>>>>>>> +    const struct dmi_probe_blacklist_data *bl_data;
>>>>>>> +    const char *bios_date;
>>>>>>> +    int i;
>>>>>>> +
>>>>>>> +    dmi_id = dmi_first_match(bl);
>>>>>>> +    if (!dmi_id)
>>>>>>> +        return NULL;
>>>>>>> +
>>>>>>> +    bl_data = dmi_id->driver_data;
>>>>>>> +
>>>>>>> +    if (!bl_data->bios_dates)
>>>>>>> +        return bl_data->hid_uid;
>>>>>>> +
>>>>>>> +    bios_date = dmi_get_system_info(DMI_BIOS_DATE);
>>>>>>> +    if (!bios_date)
>>>>>>> +        return NULL;
>>>>>>> +
>>>>>>> +    for (i = 0; bl_data->bios_dates[i]; i++) {
>>>>>>> +        if (strcmp(bl_data->bios_dates[i], bios_date) == 0)
>>>>>>> +            return bl_data->hid_uid;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return NULL;
>>>>>>> +}
>>>>>>> +
>>>>>>>     static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned
>>>>>>> int flag)
>>>>>>>     {
>>>>>>>         return c->slot && (c->slot->flags & flag);
>>>>>>> @@ -391,6 +425,33 @@ static const struct acpi_device_id
>>>>>>> sdhci_acpi_ids[] = {
>>>>>>>     };
>>>>>>>     MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>>>>>>     +const struct dmi_probe_blacklist_data gpd_win_bl_data = {
>>>>>>> +    .hid_uid = "80860F14:2",
>>>>>>> +    .bios_dates = (const char * const []){
>>>>>>> +        "10/25/2016", "11/18/2016", "02/21/2017", NULL },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct dmi_system_id dmi_probe_blacklist[] = {
>>>>>>> +    {
>>>>>>> +        /*
>>>>>>> +         * Match for the GPDwin which unfortunately uses somewhat
>>>>>>> +         * generic dmi strings, which is why we test for 4 strings
>>>>>>> +         * and a known BIOS date.
>>>>>>> +         * Comparing against 29 other byt/cht boards, board_name is
>>>>>>> +         * unique to the GPDwin, where as only 2 other boards have the
>>>>>>> +         * same board_serial and 3 others have the same board_vendor
>>>>>>> +         */
>>>>>>> +        .driver_data = (void *)&gpd_win_bl_data,
>>>>>>> +        .matches = {
>>>>>>> +            DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
>>>>>>> +            DMI_MATCH(DMI_BOARD_NAME, "Default string"),
>>>>>>> +            DMI_MATCH(DMI_BOARD_SERIAL, "Default string"),
>>>>>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
>>>>>>> +        },
>>>>>>
>>>>>> To me this is matching by accident rather than by design, which is not
>>>>>> acceptable.
>>>>>
>>>>> I already explained why we need this dmi quirk in your reply of v3,
>>>>> it would have been nice if you replied there.
>>>>
>>>> I understand what you are saying, but that doesn't make the patch
>>>> acceptable, so I cannot Ack it.
>>>>
>>>>>
>>>>> As I already mentioned when I first submitted this patch-set this
>>>>> patch-set fixes a regression. When I first installed Linux on this
>>>>> system, the wifi just worked, until this commit got merged:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=db52d4f8a4bde36263a7cc9d46ff20b243562ac9
>>>>>
>>>>>
>>>>>
>>>>> So that gives us 3 options:
>>>>
>>>> In the absence of another solution, the options are:
>>>>      1. get the BIOS fixed
>>>
>>> a. That is not going to happen (I've already contacted the vendor).
>>> b. Even if that were to happen, almost no-one will update the BIOS, so
>>>      this does not help
>>>
>>>>      2. use the module option to blacklist the bad device
>>>
>>> Needing to use a module-option, where before none was necessary
>>> is still a regression. I've personally had a commit of mine
>>> reverted by Torvalds himself because I changed something which
>>> would require the use a of a kernel cmdline option in certain
>>> corner-cases where no cmdline option was needed before.
>>>
>>> Basically your solutions boil down to my:
>>>
>>>>> 2) Do nothing, live with the regression.
>>>
>>>>> 2. is what you seem to be advocating, but since the kernel has a clear
>>>>> no regressions policy that is not an option either
>>>
>>> So your advocating we just live with the REGRESSION, because that
>>> is what this is a REGRESSION and nothing else. That is simply
>>> not acceptable (and clearly against kernel policy).
>>>
>>> I've compared DMI data to 29 other boards using the same chipset
>>> to prove the DMI match is unique, then since you are still worried
>>> about the match being too generic I also added BIOS date checking,
>>> which certainly makes the match more then unique enough, something to
>>> which you've not even responded...
>>>
>>> In the mean time users have been suffering from this regression
>>> for 3 months now:
>>> https://bbs.archlinux.org/viewtopic.php?id=224086
>>>
>>> I've no words for this, other then that your blocking of fixing
>>> this REGRESSION, without you even addressing my factual arguments
>>> why this match is not too generic, vs you're feeling that it is
>>> too generic, simply is unacceptable.
>>
>> To be clear, I understand that needing DMI quirks in the first place
>> is undesirable, and that this vendor using way too generic strings
>> is adding extra ugliness to the ugliness of needing DMI quirks in
>> the first place, so I understand your reluctance here.
>>
>> But to me making this "just" work for users trumps my desire to
>> avoid ugliness like this. I really want to see Linux used by as much
>> users as possible and in order for that to happen we need to have
>> Ubunutu / Fedora just work with their hardware, if users first need
>> to google a kernel cmdline option, then they will just stop using
>> Linux.
> 
> Perhaps there is something else we can match on, like the presence of the
> PCIe wifi device since we only use SDIO for wifi.  Can you send a copy of
> the ACPI DSDT table, or an acpidump file.  Also lspci output.

Full acpidump is here:

https://fedorapeople.org/~jwrdegoede/GPDwin.acpidump.20161025

dsdt.dsl:

https://fedorapeople.org/~jwrdegoede/GPD-win/dsdt.dsl.orig

lspci -nn:

00:00.0 Host bridge [0600]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series SoC Transaction Register [8086:2280] (rev 20)
00:02.0 VGA compatible controller [0300]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series PCI Configuration Registers [8086:22b0] (rev 20)
00:0b.0 Signal processing controller [1180]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series Power Management Controller [8086:22dc] (rev 20)
00:14.0 USB controller [0c03]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series USB xHCI Controller [8086:22b5] (rev 20)
00:16.0 USB controller [0c03]: Intel Corporation Device [8086:22b7] (rev 20)
00:1a.0 Encryption controller [1080]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series Trusted Execution Engine [8086:2298] (rev 20)
00:1c.0 PCI bridge [0604]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series PCI Express Port #1 [8086:22c8] (rev 20)
00:1f.0 ISA bridge [0601]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series PCU [8086:229c] (rev 20)
01:00.0 Network controller [0280]: Broadcom Limited BCM4356 802.11ac Wireless Network Adapter [14e4:43ec] (rev 02)

Note that one of the issues with matching on something else
is probe ordering, so matching on say a pci device is tricky,
what if the pci-bus is not yet (fully) enumerated ?

Regards,

Hans