On 16/06/17 17:37, Hans de Goede wrote: > 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@xxxxxxxxxx> >>>>>>>> --- >>>>>>>> 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 ? The PCI bus is first enumerated when the subsystems are initialized which is before driver initialization. Does this work? diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c index cf66a3db71b8..ac678e9fb19a 100644 --- a/drivers/mmc/host/sdhci-acpi.c +++ b/drivers/mmc/host/sdhci-acpi.c @@ -45,6 +45,7 @@ #include <asm/cpu_device_id.h> #include <asm/intel-family.h> #include <asm/iosf_mbi.h> +#include <linux/pci.h> #endif #include "sdhci.h" @@ -134,6 +135,16 @@ static bool sdhci_acpi_byt(void) return x86_match_cpu(byt); } +static bool sdhci_acpi_cht(void) +{ + static const struct x86_cpu_id cht[] = { + { X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_AIRMONT }, + {} + }; + + return x86_match_cpu(cht); +} + #define BYT_IOSF_SCCEP 0x63 #define BYT_IOSF_OCP_NETCTRL0 0x1078 #define BYT_IOSF_OCP_TIMEOUT_BASE GENMASK(10, 8) @@ -178,6 +189,45 @@ static bool sdhci_acpi_byt_defer(struct device *dev) return false; } +static bool sdhci_acpi_cht_pci_wifi(unsigned int vendor, unsigned int device, + unsigned int slot, unsigned int parent_slot) +{ + struct pci_dev *dev, *parent, *from = NULL; + + while (1) { + dev = pci_get_device(vendor, device, from); + pci_dev_put(from); + if (!dev) + break; + parent = pci_upstream_bridge(dev); + if (ACPI_COMPANION(&dev->dev) && PCI_SLOT(dev->devfn) == slot && + parent && PCI_SLOT(parent->devfn) == parent_slot && + !pci_upstream_bridge(parent)) { + pci_dev_put(dev); + return true; + } + from = dev; + } + + return false; +} + +/* + * GPDwin uses PCI wifi which conflicts with SDIO's use of + * acpi_device_fix_up_power() on child device nodes. Identifying GPDwin is + * problematic, but since SDIO is only used for wifi, the presence of the PCI + * wifi card in the expected slot with an ACPI companion node, is used to + * indicate that acpi_device_fix_up_power() should be avoided. + */ +static inline bool sdhci_acpi_no_fixup_child_power(const char *hid, + const char *uid) +{ + return sdhci_acpi_cht() && + !strcmp(hid, "80860F14") && + !strcmp(uid, "2") && + sdhci_acpi_cht_pci_wifi(0x14e4, 0x43ec, 0, 28); +} + #else static inline void sdhci_acpi_byt_setting(struct device *dev) @@ -189,6 +239,12 @@ static inline bool sdhci_acpi_byt_defer(struct device *dev) return false; } +static inline bool sdhci_acpi_no_fixup_child_power(const char *hid, + const char *uid) +{ + return false; +} + #endif static int bxt_get_cd(struct mmc_host *mmc) @@ -389,18 +445,20 @@ static int sdhci_acpi_probe(struct platform_device *pdev) if (acpi_bus_get_device(handle, &device)) return -ENODEV; + hid = acpi_device_hid(device); + uid = device->pnp.unique_id; + /* 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 (!sdhci_acpi_no_fixup_child_power(hid, uid)) { + list_for_each_entry(child, &device->children, node) + if (child->status.present && child->status.enabled) + acpi_device_fix_up_power(child); + } 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; -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
