[PATCH v3 resend 0/3] mmc: sdhci-acpi: Fix sdhci-acpi breaking pci-e based wifi on the GPD-win

[PATCH v3 resend 0/3] mmc: sdhci-acpi: Fix sdhci-acpi breaking pci-e based wifi on the GPD-win

[Hans de Goede]

Hi All,

This series seems to have fallen through the cracks, so I'm resending it.

Regards,

Hans

[PATCH v3 resend 1/3] mmc: sdhci-acpi: Remove unneeded acpi_bus_get_status() call

[Hans de Goede]

The acpi-subsys already calls acpi_bus_get_status() and checks that
device->status.present is set before even registering the platform_device
so out probe function will never get called if device->status.present is
false and there is no need for this check.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
Changes in v2:
-This is a new patch replacing "mmc: sdhci-acpi: Check device status
 before calling fix_up_power()"
---
 drivers/mmc/host/sdhci-acpi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index c6a9a1bfaa22..89d9a8c014f5 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -396,9 +396,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		if (child->status.present && child->status.enabled)
 			acpi_device_fix_up_power(child);
 
-	if (acpi_bus_get_status(device) || !device->status.present)
-		return -ENODEV;
-
 	if (sdhci_acpi_byt_defer(dev))
 		return -EPROBE_DEFER;
 
-- 
2.13.0

[PATCH v3 resend 2/3] 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)
---
 drivers/mmc/host/sdhci-acpi.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 89d9a8c014f5..3a7d979a306d 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -83,6 +83,29 @@ 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 *sep;
+
+	if (!match)
+		return false;
+
+	sep = strchr(match, ':');
+	if (!match)
+		return false;
+
+	if (strncmp(match, hid, sep - match))
+		return false;
+
+	if (strcmp(sep + 1, uid))
+		return false;
+
+	return true;
+}
+
 static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
 {
 	return c->slot && (c->slot->flags & flag);
@@ -378,6 +401,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 +414,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 +429,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 +607,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> 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 v3 resend 3/3] 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).

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"
---
 drivers/mmc/host/sdhci-acpi.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 3a7d979a306d..45455abc7ca6 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>
@@ -381,6 +382,28 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
 
+static const struct dmi_system_id fix_up_power_dmi_blacklist[] = {
+	{
+		/*
+		 * Match for the GPDwin which unfortunately uses somewhat
+		 * generic dmi strings, which is why we test for 4 strings.
+		 * Comparing against 23 other byt/cht boards, board_vendor
+		 * and board_name are unique to the GPDwin, where as only one
+		 * other board has the same board_serial and 3 others have
+		 * the same default product_name. Also the GPDwin is the
+		 * only device to have both board_ and product_name not set.
+		 */
+		.driver_data = "80860F14:2",
+		.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)
 {
@@ -403,6 +426,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	acpi_handle handle = ACPI_HANDLE(dev);
 	const char *bl = blacklist;
 	struct acpi_device *device, *child;
+	const struct dmi_system_id *dmi_id;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
@@ -417,6 +441,12 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	hid = acpi_device_hid(device);
 	uid = device->pnp.unique_id;
 
+	if (!bl) {
+		dmi_id = dmi_first_match(fix_up_power_dmi_blacklist);
+		if (dmi_id)
+			bl = dmi_id->driver_data;
+	}
+
 	if (sdhci_acpi_compare_hid_uid(bl, hid, uid))
 		return -ENODEV;
 
-- 
2.13.0

Re: [PATCH v3 resend 2/3] mmc: sdhci-acpi: Add blacklist module option

[Adrian Hunter]

On 24/05/17 13:47, 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>
> ---
> 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)
> ---
>  drivers/mmc/host/sdhci-acpi.c | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 89d9a8c014f5..3a7d979a306d 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -83,6 +83,29 @@ 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 *sep;
> +
> +	if (!match)
> +		return false;
> +
> +	sep = strchr(match, ':');
> +	if (!match)

You mean (!sep)

> +		return false;
> +
> +	if (strncmp(match, hid, sep - match))
> +		return false;
> +
> +	if (strcmp(sep + 1, uid))
> +		return false;
> +
> +	return true;

This should all go together i.e.

	return sep && !strncmp(match, hid, sep - match) &&
	       !strcmp(sep + 1, uid);

> +}
> +
>  static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>  {
>  	return c->slot && (c->slot->flags & flag);
> @@ -378,6 +401,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 +414,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))

'bl' is redundant in this patch.  This should just be

	if (sdhci_acpi_blacklist(hid, uid))
		return -ENODEV;

> +		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 +429,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 +607,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> which should be ignored");

Why not allow more than one HID:UID separated by commas.

> +
>  MODULE_DESCRIPTION("Secure Digital Host Controller Interface ACPI driver");
>  MODULE_AUTHOR("Adrian Hunter");
>  MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v3 resend 3/3] mmc: sdhci-acpi: Add DMI based blacklist

[Adrian Hunter]

On 24/05/17 13:47, 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).
> 
> 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"
> ---
>  drivers/mmc/host/sdhci-acpi.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 3a7d979a306d..45455abc7ca6 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>
> @@ -381,6 +382,28 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>  
> +static const struct dmi_system_id fix_up_power_dmi_blacklist[] = {
> +	{
> +		/*
> +		 * Match for the GPDwin which unfortunately uses somewhat
> +		 * generic dmi strings, which is why we test for 4 strings.
> +		 * Comparing against 23 other byt/cht boards, board_vendor
> +		 * and board_name are unique to the GPDwin, where as only one
> +		 * other board has the same board_serial and 3 others have
> +		 * the same default product_name. Also the GPDwin is the
> +		 * only device to have both board_ and product_name not set.
> +		 */
> +		.driver_data = "80860F14:2",
> +		.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"),

I can't accept that this is an accurate way to identify the board.

Re: [PATCH v3 resend 2/3] mmc: sdhci-acpi: Add blacklist module option

[Hans de Goede]

Hi,

On 07-06-17 14:47, Adrian Hunter wrote:
> On 24/05/17 13:47, 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>
>> ---
>> 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)
>> ---
>>   drivers/mmc/host/sdhci-acpi.c | 36 +++++++++++++++++++++++++++++++++---
>>   1 file changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 89d9a8c014f5..3a7d979a306d 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -83,6 +83,29 @@ 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 *sep;
>> +
>> +	if (!match)
>> +		return false;
>> +
>> +	sep = strchr(match, ':');
>> +	if (!match)
> 
> You mean (!sep)

Right, will fix for v4.

>> +		return false;
>> +
>> +	if (strncmp(match, hid, sep - match))
>> +		return false;
>> +
>> +	if (strcmp(sep + 1, uid))
>> +		return false;
>> +
>> +	return true;
> 
> This should all go together i.e.
 >
> 	return sep && !strncmp(match, hid, sep - match) &&
> 	       !strcmp(sep + 1, uid);

Ok.


>> +}
>> +
>>   static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>>   {
>>   	return c->slot && (c->slot->flags & flag);
>> @@ -378,6 +401,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 +414,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))
> 
> 'bl' is redundant in this patch.  This should just be
> 
> 	if (sdhci_acpi_blacklist(hid, uid))
> 		return -ENODEV;
> 
>> +		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 +429,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 +607,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> which should be ignored");
> 
> Why not allow more than one HID:UID separated by commas.

I didn't think we will ever have the need to blacklist more then
1 HID:UID pair and it makes the parsing slightly harder. But since you've
asked I'll add support for this for v4.

Regards,

Hans

Re: [PATCH v3 resend 3/3] mmc: sdhci-acpi: Add DMI based blacklist

[Hans de Goede]

Hi,

On 07-06-17 15:09, Adrian Hunter wrote:
> On 24/05/17 13:47, 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).
>>
>> 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"
>> ---
>>   drivers/mmc/host/sdhci-acpi.c | 30 ++++++++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 3a7d979a306d..45455abc7ca6 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>
>> @@ -381,6 +382,28 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>>   };
>>   MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>   
>> +static const struct dmi_system_id fix_up_power_dmi_blacklist[] = {
>> +	{
>> +		/*
>> +		 * Match for the GPDwin which unfortunately uses somewhat
>> +		 * generic dmi strings, which is why we test for 4 strings.
>> +		 * Comparing against 23 other byt/cht boards, board_vendor
>> +		 * and board_name are unique to the GPDwin, where as only one
>> +		 * other board has the same board_serial and 3 others have
>> +		 * the same default product_name. Also the GPDwin is the
>> +		 * only device to have both board_ and product_name not set.
>> +		 */
>> +		.driver_data = "80860F14:2",
>> +		.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"),
> 
> I can't accept that this is an accurate way to identify the board.

Well 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

Given the kernel's no regressions policy I see only 2 ways to fix this
something like this patch, or revert the commit causing the regression.

In the CHT dstd's I have 6 out of 8 use the 80860F14 HID rather then
the INT33BB HID one, so reverting the commit causing this regression
is not really an option.

Note that the quirk not only matches on DMI strings, but also on the
80860F14:2 pair, further narrowing the chance for duplicate matches.

I've access to 29 dmi dumps of Bay / Cherry Trail tablets now and
only 3 match DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation") (*),
and only 1 out of 29 (the GPD win) matches DMI_MATCH(DMI_BOARD_NAME,
"Default string") so after 2 of the 5 checks (including the UID
check) we already have only the GPD win matching.

Regards,

Hans



*) This is different from the comment block, I've been deliberately
looking for dmi dumps with a board_vendor of "AMI Corporation"

Re: [PATCH v3 resend 3/3] mmc: sdhci-acpi: Add DMI based blacklist

[Hans de Goede]

On 08-06-17 12:20, Hans de Goede wrote:
> Hi,
> 
> On 07-06-17 15:09, Adrian Hunter wrote:
>> On 24/05/17 13:47, 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).
>>>
>>> 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"
>>> ---
>>>   drivers/mmc/host/sdhci-acpi.c | 30 ++++++++++++++++++++++++++++++
>>>   1 file changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index 3a7d979a306d..45455abc7ca6 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>
>>> @@ -381,6 +382,28 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>>>   };
>>>   MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>> +static const struct dmi_system_id fix_up_power_dmi_blacklist[] = {
>>> +    {
>>> +        /*
>>> +         * Match for the GPDwin which unfortunately uses somewhat
>>> +         * generic dmi strings, which is why we test for 4 strings.
>>> +         * Comparing against 23 other byt/cht boards, board_vendor
>>> +         * and board_name are unique to the GPDwin, where as only one
>>> +         * other board has the same board_serial and 3 others have
>>> +         * the same default product_name. Also the GPDwin is the
>>> +         * only device to have both board_ and product_name not set.
>>> +         */
>>> +        .driver_data = "80860F14:2",
>>> +        .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"),
>>
>> I can't accept that this is an accurate way to identify the board.
> 
> Well 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
> 
> Given the kernel's no regressions policy I see only 2 ways to fix this
> something like this patch, or revert the commit causing the regression.

p.s.

Given that this is a regression it would be nice if we could speedup
the review process of this patch-set a bit so that we can get this
regression fixed soon. Sofar the review of this patch-set has been
somewhat slow I must say.

Re: [PATCH v3 resend 2/3] mmc: sdhci-acpi: Add blacklist module option

[Hans de Goede]

Hi,

On 07-06-17 14:47, Adrian Hunter wrote:
> On 24/05/17 13:47, 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>
>> ---
>> 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)
>> ---
>>   drivers/mmc/host/sdhci-acpi.c | 36 +++++++++++++++++++++++++++++++++---
>>   1 file changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 89d9a8c014f5..3a7d979a306d 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -83,6 +83,29 @@ 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 *sep;
>> +
>> +	if (!match)
>> +		return false;
>> +
>> +	sep = strchr(match, ':');
>> +	if (!match)
> 
> You mean (!sep)
> 
>> +		return false;
>> +
>> +	if (strncmp(match, hid, sep - match))
>> +		return false;
>> +
>> +	if (strcmp(sep + 1, uid))
>> +		return false;
>> +
>> +	return true;
> 
> This should all go together i.e.
> 
> 	return sep && !strncmp(match, hid, sep - match) &&
> 	       !strcmp(sep + 1, uid);
> 
>> +}
>> +
>>   static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>>   {
>>   	return c->slot && (c->slot->flags & flag);
>> @@ -378,6 +401,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 +414,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))
> 
> 'bl' is redundant in this patch.  This should just be

I forgot to respond to this bit in my previous reply. The bl is there to make
adding a dmi-quirk table for this easier, so that we can pass in the dmi-quirk
table version of blacklist is not set on the kernel cmdline.

Regards,

Hans


> 
> 	if (sdhci_acpi_blacklist(hid, uid))
> 		return -ENODEV;
> 
>> +		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 +429,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 +607,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> which should be ignored");
> 
> Why not allow more than one HID:UID separated by commas.
> 
>> +
>>   MODULE_DESCRIPTION("Secure Digital Host Controller Interface ACPI driver");
>>   MODULE_AUTHOR("Adrian Hunter");
>>   MODULE_LICENSE("GPL v2");
>>
> 

Re: [PATCH v3 resend 1/3] mmc: sdhci-acpi: Remove unneeded acpi_bus_get_status() call

[Ulf Hansson]

On 24 May 2017 at 12:47, Hans de Goede <hdegoede@redhat.com> wrote:
> The acpi-subsys already calls acpi_bus_get_status() and checks that
> device->status.present is set before even registering the platform_device
> so out probe function will never get called if device->status.present is
> false and there is no need for this check.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
> Changes in v2:
> -This is a new patch replacing "mmc: sdhci-acpi: Check device status
>  before calling fix_up_power()"
> ---
>  drivers/mmc/host/sdhci-acpi.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index c6a9a1bfaa22..89d9a8c014f5 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -396,9 +396,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>                 if (child->status.present && child->status.enabled)
>                         acpi_device_fix_up_power(child);
>
> -       if (acpi_bus_get_status(device) || !device->status.present)
> -               return -ENODEV;
> -
>         if (sdhci_acpi_byt_defer(dev))
>                 return -EPROBE_DEFER;
>
> --
> 2.13.0
>

Re: [PATCH v3 resend 3/3] mmc: sdhci-acpi: Add DMI based blacklist

[Hans de Goede]

Hi,

On 08-06-17 12:20, Hans de Goede wrote:
> Hi,
> 
> On 07-06-17 15:09, Adrian Hunter wrote:
>> On 24/05/17 13:47, 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).
>>>
>>> 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"
>>> ---
>>>   drivers/mmc/host/sdhci-acpi.c | 30 ++++++++++++++++++++++++++++++
>>>   1 file changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index 3a7d979a306d..45455abc7ca6 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>
>>> @@ -381,6 +382,28 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>>>   };
>>>   MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>> +static const struct dmi_system_id fix_up_power_dmi_blacklist[] = {
>>> +    {
>>> +        /*
>>> +         * Match for the GPDwin which unfortunately uses somewhat
>>> +         * generic dmi strings, which is why we test for 4 strings.
>>> +         * Comparing against 23 other byt/cht boards, board_vendor
>>> +         * and board_name are unique to the GPDwin, where as only one
>>> +         * other board has the same board_serial and 3 others have
>>> +         * the same default product_name. Also the GPDwin is the
>>> +         * only device to have both board_ and product_name not set.
>>> +         */
>>> +        .driver_data = "80860F14:2",
>>> +        .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"),
>>
>> I can't accept that this is an accurate way to identify the board.
> 
> Well 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
> 
> Given the kernel's no regressions policy I see only 2 ways to fix this
> something like this patch, or revert the commit causing the regression.
> 
> In the CHT dstd's I have 6 out of 8 use the 80860F14 HID rather then
> the INT33BB HID one, so reverting the commit causing this regression
> is not really an option.
> 
> Note that the quirk not only matches on DMI strings, but also on the
> 80860F14:2 pair, further narrowing the chance for duplicate matches.
> 
> I've access to 29 dmi dumps of Bay / Cherry Trail tablets now and
> only 3 match DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation") (*),
> and only 1 out of 29 (the GPD win) matches DMI_MATCH(DMI_BOARD_NAME,
> "Default string") so after 2 of the 5 checks (including the UID
> check) we already have only the GPD win matching.

So thinking more about this, one more thing which can be done to
make sure the dmi match is unique to the GPD win is also check
the BIOS date, as I did in v1 of this patch set, one problem with
that is that the manufacturer does tend to update the BIOS date in
new production batches (*) and I ended up with many classic
dmi table entries which were all identical except for the bios_date
field. So for v4 I've coded out the bios-date check as a separate
check so that we can have an array of bios-dates for a single
dmi quirk table entry.

Regards,

Hans

*) I've notified them of the BIOS bug and asked them to fix this
(as well as provide better dmi strings) they have acknowledged receipt
of my email and said they would forward it to their engineers, so
this may get fixed in a later version at which point we don't need
to add the dates of the fixed and newer bios versions.