[PATCH v4 1/2] mmc: sdhci-acpi: Add blacklist module option

[PATCH v4 1/2] mmc: sdhci-acpi: Add blacklist module option

[Hans de Goede]

Commit e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are
powered when probing") introduced unconditional calling of
acpi_device_fix_up_power() on the mmc controller and its children.

This broke wifi on some systems because acpi_device_fix_up_power()
was called even for disabled children sometimes leaving gpio-s in
a state where wifi would not work, this was fixed in
commit e1d070c3793a ("mmc: sdhci-acpi: Only powered up enabled acpi
child devices").

Unfortunately on some devices calling acpi_device_fix_up_power()
still causes issues. Specifically on the GPD-win mini clam-shell PC
which has a pci-e wifi module, it causes the wifi module to get
turned off. This is a BIOS bug and I've tried to get the manufacturer
to fix this but sofar they have not responded (and even if they do
then we cannot assume all users will update their BIOS).

Since the GPD-win uses a pci-e wifi module the sdhci controller for
sdio cards really should not get initialized on it at all.

This commit adds a new sdhci_acpi.blacklist module option which can
be set to an ACPI hid:uid pair, e.g. "80860F14:2" to disable probing
for the sdhci host matching the hid:uid pair, fixing the wifi not
working on this machine.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Make the module option take a hid:uid pair string, instead of it
 being a boolean option, so that it only applies to one host

Changes in v3:
-Make the module option skip probing the device entirely (return -ENODEV)

Changes in v4:
-Fix bad seperator (now hid_end) check
-Allow passing multiple HID:UID pairs seperated by commas
---
 drivers/mmc/host/sdhci-acpi.c | 46 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 89d9a8c014f5..ecc3aefd4643 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -83,6 +83,39 @@ struct sdhci_acpi_host {
 	bool				use_runtime_pm;
 };
 
+static char *blacklist;
+
+static bool sdhci_acpi_compare_hid_uid(const char *match, const char *hid,
+				       const char *uid)
+{
+	const char *hid_end, *uid_end, *end;
+
+	if (!match || !hid || !uid)
+		return false;
+
+	end = match + strlen(match);
+
+	do {
+		hid_end = strchr(match, ':');
+		if (!hid_end)
+			return false;
+
+		uid_end = strchr(hid_end, ',');
+		if (!uid_end)
+			uid_end = end;
+
+		if (strlen(hid) == (hid_end - match) &&
+		    strncmp(match, hid, hid_end - match) == 0 &&
+		    strlen(uid) == (uid_end - hid_end - 1) &&
+		    strncmp(hid_end + 1, uid, uid_end - hid_end - 1) == 0)
+			return true;
+
+		match = uid_end + 1;
+	} while (*match);
+
+	return false;
+}
+
 static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
 {
 	return c->slot && (c->slot->flags & flag);
@@ -378,6 +411,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	acpi_handle handle = ACPI_HANDLE(dev);
+	const char *bl = blacklist;
 	struct acpi_device *device, *child;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
@@ -390,6 +424,12 @@ 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;
+
+	if (sdhci_acpi_compare_hid_uid(bl, hid, uid))
+		return -ENODEV;
+
 	/* Power on the SDHCI controller and its children */
 	acpi_device_fix_up_power(device);
 	list_for_each_entry(child, &device->children, node)
@@ -399,9 +439,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;
@@ -580,6 +617,9 @@ static struct platform_driver sdhci_acpi_driver = {
 
 module_platform_driver(sdhci_acpi_driver);
 
+module_param(blacklist, charp, 0444);
+MODULE_PARM_DESC(blacklist, "ACPI <HID:UID>[,HID:UID] which should be ignored");
+
 MODULE_DESCRIPTION("Secure Digital Host Controller Interface ACPI driver");
 MODULE_AUTHOR("Adrian Hunter");
 MODULE_LICENSE("GPL v2");
-- 
2.13.0

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

[Hans de Goede]

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"),
+		},
+	},
+	{ }
+};
+
 static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
 							 const char *uid)
 {
@@ -427,6 +488,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	hid = acpi_device_hid(device);
 	uid = device->pnp.unique_id;
 
+	if (!bl)
+		bl = sdhci_acpi_get_dmi_blacklist(dmi_probe_blacklist);
+
 	if (sdhci_acpi_compare_hid_uid(bl, hid, uid))
 		return -ENODEV;
 
-- 
2.13.0

Re: [PATCH v4 1/2] mmc: sdhci-acpi: Add blacklist module option

[Adrian Hunter]

On 08/06/17 21:54, Hans de Goede wrote:
> Commit e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are
> powered when probing") introduced unconditional calling of
> acpi_device_fix_up_power() on the mmc controller and its children.
> 
> This broke wifi on some systems because acpi_device_fix_up_power()
> was called even for disabled children sometimes leaving gpio-s in
> a state where wifi would not work, this was fixed in
> commit e1d070c3793a ("mmc: sdhci-acpi: Only powered up enabled acpi
> child devices").
> 
> Unfortunately on some devices calling acpi_device_fix_up_power()
> still causes issues. Specifically on the GPD-win mini clam-shell PC
> which has a pci-e wifi module, it causes the wifi module to get
> turned off. This is a BIOS bug and I've tried to get the manufacturer
> to fix this but sofar they have not responded (and even if they do
> then we cannot assume all users will update their BIOS).
> 
> Since the GPD-win uses a pci-e wifi module the sdhci controller for
> sdio cards really should not get initialized on it at all.
> 
> This commit adds a new sdhci_acpi.blacklist module option which can
> be set to an ACPI hid:uid pair, e.g. "80860F14:2" to disable probing
> for the sdhci host matching the hid:uid pair, fixing the wifi not
> working on this machine.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

The subject reads like it is trying to blacklist a module.  How about "Add
module option to blacklist a device"

> ---
> Changes in v2:
> -Make the module option take a hid:uid pair string, instead of it
>  being a boolean option, so that it only applies to one host
> 
> Changes in v3:
> -Make the module option skip probing the device entirely (return -ENODEV)
> 
> Changes in v4:
> -Fix bad seperator (now hid_end) check
> -Allow passing multiple HID:UID pairs seperated by commas
> ---
>  drivers/mmc/host/sdhci-acpi.c | 46 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 89d9a8c014f5..ecc3aefd4643 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -83,6 +83,39 @@ struct sdhci_acpi_host {
>  	bool				use_runtime_pm;
>  };
>  
> +static char *blacklist;
> +
> +static bool sdhci_acpi_compare_hid_uid(const char *match, const char *hid,
> +				       const char *uid)
> +{
> +	const char *hid_end, *uid_end, *end;
> +
> +	if (!match || !hid || !uid)
> +		return false;
> +
> +	end = match + strlen(match);
> +
> +	do {
> +		hid_end = strchr(match, ':');
> +		if (!hid_end)
> +			return false;
> +
> +		uid_end = strchr(hid_end, ',');
> +		if (!uid_end)
> +			uid_end = end;
> +
> +		if (strlen(hid) == (hid_end - match) &&
> +		    strncmp(match, hid, hid_end - match) == 0 &&
> +		    strlen(uid) == (uid_end - hid_end - 1) &&
> +		    strncmp(hid_end + 1, uid, uid_end - hid_end - 1) == 0)
> +			return true;
> +
> +		match = uid_end + 1;

That is off by one when uid_end == end.  But it seems error prone, so how
about this:


static int __sdhci_acpi_blacklist(char *bl, char *id)
{
	char *s;

	do {
		s = strsep(&bl, ",");
		if (s && !strcmp(s, id))
			return -ENODEV;
	} while (bl);

	return 0;
}

static int sdhci_acpi_blacklist(const char *hid, const char *uid)
{
	int ret = -ENOMEM;
	char *bl, *id;

	if (!blacklist || !hid || !uid)
		return 0;

	bl = kstrdup(blacklist, GFP_KERNEL);
	id = kasprintf(GFP_KERNEL, "%s:%s", hid, uid);
	if (bl && id)
		ret = __sdhci_acpi_blacklist(bl, id);
	kfree(id);
	kfree(bl);

	return ret;
}

Then:

	err = sdhci_acpi_blacklist(hid, uid);
	if (err)
		return err;


> +	} while (*match);
> +
> +	return false;
> +}
> +
>  static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>  {
>  	return c->slot && (c->slot->flags & flag);
> @@ -378,6 +411,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	acpi_handle handle = ACPI_HANDLE(dev);
> +	const char *bl = blacklist;
>  	struct acpi_device *device, *child;
>  	struct sdhci_acpi_host *c;
>  	struct sdhci_host *host;
> @@ -390,6 +424,12 @@ 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;
> +
> +	if (sdhci_acpi_compare_hid_uid(bl, hid, uid))
> +		return -ENODEV;
> +
>  	/* Power on the SDHCI controller and its children */
>  	acpi_device_fix_up_power(device);
>  	list_for_each_entry(child, &device->children, node)
> @@ -399,9 +439,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;
> @@ -580,6 +617,9 @@ static struct platform_driver sdhci_acpi_driver = {
>  
>  module_platform_driver(sdhci_acpi_driver);
>  
> +module_param(blacklist, charp, 0444);
> +MODULE_PARM_DESC(blacklist, "ACPI <HID:UID>[,HID:UID] which should be ignored");
> +
>  MODULE_DESCRIPTION("Secure Digital Host Controller Interface ACPI driver");
>  MODULE_AUTHOR("Adrian Hunter");
>  MODULE_LICENSE("GPL v2");
> 

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

[Adrian Hunter]

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.

> +	},
> +	{ }
> +};
> +
>  static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>  							 const char *uid)
>  {
> @@ -427,6 +488,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	hid = acpi_device_hid(device);
>  	uid = device->pnp.unique_id;
>  
> +	if (!bl)
> +		bl = sdhci_acpi_get_dmi_blacklist(dmi_probe_blacklist);
> +
>  	if (sdhci_acpi_compare_hid_uid(bl, hid, uid))
>  		return -ENODEV;
>  
> 

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

[Hans de Goede]

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.

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:

1) Revert the commit causing the regressions
2) Do nothing, live with the regression.
3) Add a DMI based quirk

1. is not an option since that commit is necessary to make wifi work
on other devices

2. is what you seem to be advocating, but since the kernel has a clear
no regressions policy that is not an option either

3. is thus the only option left.

So unless you see a 4th option we really need to go with this patch,
note that in this version I've made the chance of false positives
for the DMI match even smaller then it was before because it now
needs to match a know bios-date too.

Also note that this is being hit be actual users, not just by me, see:

https://bbs.archlinux.org/viewtopic.php?id=224086

Regards,

Hans

Re: [PATCH v4 1/2] mmc: sdhci-acpi: Add blacklist module option

[Hans de Goede]

Hi,

On 12-06-17 14:04, Adrian Hunter wrote:
> On 08/06/17 21:54, Hans de Goede wrote:
>> Commit e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are
>> powered when probing") introduced unconditional calling of
>> acpi_device_fix_up_power() on the mmc controller and its children.
>>
>> This broke wifi on some systems because acpi_device_fix_up_power()
>> was called even for disabled children sometimes leaving gpio-s in
>> a state where wifi would not work, this was fixed in
>> commit e1d070c3793a ("mmc: sdhci-acpi: Only powered up enabled acpi
>> child devices").
>>
>> Unfortunately on some devices calling acpi_device_fix_up_power()
>> still causes issues. Specifically on the GPD-win mini clam-shell PC
>> which has a pci-e wifi module, it causes the wifi module to get
>> turned off. This is a BIOS bug and I've tried to get the manufacturer
>> to fix this but sofar they have not responded (and even if they do
>> then we cannot assume all users will update their BIOS).
>>
>> Since the GPD-win uses a pci-e wifi module the sdhci controller for
>> sdio cards really should not get initialized on it at all.
>>
>> This commit adds a new sdhci_acpi.blacklist module option which can
>> be set to an ACPI hid:uid pair, e.g. "80860F14:2" to disable probing
>> for the sdhci host matching the hid:uid pair, fixing the wifi not
>> working on this machine.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> The subject reads like it is trying to blacklist a module.  How about "Add
> module option to blacklist a device"

Sure that works for me.

>> ---
>> Changes in v2:
>> -Make the module option take a hid:uid pair string, instead of it
>>   being a boolean option, so that it only applies to one host
>>
>> Changes in v3:
>> -Make the module option skip probing the device entirely (return -ENODEV)
>>
>> Changes in v4:
>> -Fix bad seperator (now hid_end) check
>> -Allow passing multiple HID:UID pairs seperated by commas
>> ---
>>   drivers/mmc/host/sdhci-acpi.c | 46 ++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 89d9a8c014f5..ecc3aefd4643 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -83,6 +83,39 @@ struct sdhci_acpi_host {
>>   	bool				use_runtime_pm;
>>   };
>>   
>> +static char *blacklist;
>> +
>> +static bool sdhci_acpi_compare_hid_uid(const char *match, const char *hid,
>> +				       const char *uid)
>> +{
>> +	const char *hid_end, *uid_end, *end;
>> +
>> +	if (!match || !hid || !uid)
>> +		return false;
>> +
>> +	end = match + strlen(match);
>> +
>> +	do {
>> +		hid_end = strchr(match, ':');
>> +		if (!hid_end)
>> +			return false;
>> +
>> +		uid_end = strchr(hid_end, ',');
>> +		if (!uid_end)
>> +			uid_end = end;
>> +
>> +		if (strlen(hid) == (hid_end - match) &&
>> +		    strncmp(match, hid, hid_end - match) == 0 &&
>> +		    strlen(uid) == (uid_end - hid_end - 1) &&
>> +		    strncmp(hid_end + 1, uid, uid_end - hid_end - 1) == 0)
>> +			return true;
>> +
>> +		match = uid_end + 1;
> 
> That is off by one when uid_end == end.

Right, thank you for catching that.

> But it seems error prone, so how
> about this:

Hmm, I don't like all the needless malloc/free going
on there. I will send a v5 with the improved subject you
suggested and the off-by-one fixed.

Regards,

Hans



> 
> 
> static int __sdhci_acpi_blacklist(char *bl, char *id)
> {
> 	char *s;
> 
> 	do {
> 		s = strsep(&bl, ",");
> 		if (s && !strcmp(s, id))
> 			return -ENODEV;
> 	} while (bl);
> 
> 	return 0;
> }
> 
> static int sdhci_acpi_blacklist(const char *hid, const char *uid)
> {
> 	int ret = -ENOMEM;
> 	char *bl, *id;
> 
> 	if (!blacklist || !hid || !uid)
> 		return 0;
> 
> 	bl = kstrdup(blacklist, GFP_KERNEL);
> 	id = kasprintf(GFP_KERNEL, "%s:%s", hid, uid);
> 	if (bl && id)
> 		ret = __sdhci_acpi_blacklist(bl, id);
> 	kfree(id);
> 	kfree(bl);
> 
> 	return ret;
> }
> 
> Then:
> 
> 	err = sdhci_acpi_blacklist(hid, uid);
> 	if (err)
> 		return err;
> 
> 
>> +	} while (*match);
>> +
>> +	return false;
>> +}
>> +
>>   static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>>   {
>>   	return c->slot && (c->slot->flags & flag);
>> @@ -378,6 +411,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>>   	acpi_handle handle = ACPI_HANDLE(dev);
>> +	const char *bl = blacklist;
>>   	struct acpi_device *device, *child;
>>   	struct sdhci_acpi_host *c;
>>   	struct sdhci_host *host;
>> @@ -390,6 +424,12 @@ 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;
>> +
>> +	if (sdhci_acpi_compare_hid_uid(bl, hid, uid))
>> +		return -ENODEV;
>> +
>>   	/* Power on the SDHCI controller and its children */
>>   	acpi_device_fix_up_power(device);
>>   	list_for_each_entry(child, &device->children, node)
>> @@ -399,9 +439,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;
>> @@ -580,6 +617,9 @@ static struct platform_driver sdhci_acpi_driver = {
>>   
>>   module_platform_driver(sdhci_acpi_driver);
>>   
>> +module_param(blacklist, charp, 0444);
>> +MODULE_PARM_DESC(blacklist, "ACPI <HID:UID>[,HID:UID] which should be ignored");
>> +
>>   MODULE_DESCRIPTION("Secure Digital Host Controller Interface ACPI driver");
>>   MODULE_AUTHOR("Adrian Hunter");
>>   MODULE_LICENSE("GPL v2");
>>
> 

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

[Adrian Hunter]

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
	2. use the module option to blacklist the bad device

> 
> 1) Revert the commit causing the regressions
> 2) Do nothing, live with the regression.
> 3) Add a DMI based quirk
> 
> 1. is not an option since that commit is necessary to make wifi work
> on other devices
> 
> 2. is what you seem to be advocating, but since the kernel has a clear
> no regressions policy that is not an option either
> 
> 3. is thus the only option left.
> 
> So unless you see a 4th option we really need to go with this patch,
> note that in this version I've made the chance of false positives
> for the DMI match even smaller then it was before because it now
> needs to match a know bios-date too.
> 
> Also note that this is being hit be actual users, not just by me, see:
> 
> https://bbs.archlinux.org/viewtopic.php?id=224086

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

[Hans de Goede]

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.

Regards,

Hans

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

[Hans de Goede]

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.

Regards,

Hans

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

[Adrian Hunter]

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.

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

[Hans de Goede]

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

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

[Adrian Hunter]

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@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 ?

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;

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

[Hans de Goede]

Hi,

On 19-06-17 13:59, Adrian Hunter wrote:
> 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@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 ?
> 
> The PCI bus is first enumerated when the subsystems are initialized
> which is before driver initialization.

Ok.

> Does this work?

I'm happy to report that yes it does. If you prefer this solution
then that is fine by me:

Acked-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> 
> 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;
> 

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

[Hans de Goede]

Hi,

On 19-06-17 16:07, Hans de Goede wrote:
> Hi,
> 
> On 19-06-17 13:59, Adrian Hunter wrote:

<snip>

>>>> 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.

<snip>

>> Does this work?
> 
> I'm happy to report that yes it does. If you prefer this solution
> then that is fine by me:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>

So what is the plan moving forward with this ?

Yesterday I was running some tests on a GPD-win a friend recently bought and
I hit this issue again, my usb-disk with a test-install was running my solution
with the BIOS date check and that failed because his model had a new BIOS
build and thus a different date. So I believe that this patch is better
then mine as it won't break with BIOS updates and avoids us needing to
patch the quirk table all the time.

As such I would like to see us move forward with this patch and get it
merged for 4.13 (with a Cc stable) so that we can finally get this
regression fixed.

Adrian, can you do an official submission of this patch (with my ack
and tested-by) so that Ulf can merge this ?

Regards,

Hans



>> 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;
>>