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

[PATCH v2 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>
---
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 237f318..9fd8d7a 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -399,9 +399,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.9.3

[PATCH v2 2/3] mmc: sdhci-acpi: Add fix_up_power_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 GPDwin mini clam-shell PC
which has a pcie 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).

This commit adds a new sdhci_acpi.fix_up_power_blacklist module option
which can be set to an ACPI hid:uid pair, e.g. "80860F14:2" to disable
the calling of acpi_device_fix_up_power() 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
---
 drivers/mmc/host/sdhci-acpi.c | 48 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 9fd8d7a..5a73174 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 *fix_up_power_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);
@@ -381,6 +404,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	acpi_handle handle = ACPI_HANDLE(dev);
+	const char *fix_up_power_bl = fix_up_power_blacklist;
+	bool fix_up_power = true;
 	struct acpi_device *device, *child;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
@@ -393,18 +418,23 @@ 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(fix_up_power_bl, hid, uid))
+		fix_up_power = false;
+
 	/* 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 (fix_up_power) {
+		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_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;
@@ -575,6 +605,10 @@ static struct platform_driver sdhci_acpi_driver = {
 
 module_platform_driver(sdhci_acpi_driver);
 
+module_param(fix_up_power_blacklist, charp, 0444);
+MODULE_PARM_DESC(fix_up_power,
+		 "ACPI HID:UID for which to not call acpi_device_fix_up_power");
+
 MODULE_DESCRIPTION("Secure Digital Host Controller Interface ACPI driver");
 MODULE_AUTHOR("Adrian Hunter");
 MODULE_LICENSE("GPL v2");
-- 
2.9.3

[PATCH v2 3/3] mmc: sdhci-acpi: Add DMI fix_up_power_blacklist

[Hans de Goede]

Add a DMI based blacklist for systems where calling
acpi_device_fix_up_power() is harmful.

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

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 5a73174..ba9e688 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>
@@ -384,6 +385,27 @@ 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 15 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 2 others have
+		 * the same default product_name.
+		 */
+		.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)
 {
@@ -407,6 +429,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	const char *fix_up_power_bl = fix_up_power_blacklist;
 	bool fix_up_power = true;
 	struct acpi_device *device, *child;
+	const struct dmi_system_id *dmi_id;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
@@ -421,6 +444,12 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	hid = acpi_device_hid(device);
 	uid = device->pnp.unique_id;
 
+	if (!fix_up_power_bl) {
+		dmi_id = dmi_first_match(fix_up_power_dmi_blacklist);
+		if (dmi_id)
+			fix_up_power_bl = dmi_id->driver_data;
+	}
+
 	if (sdhci_acpi_compare_hid_uid(fix_up_power_bl, hid, uid))
 		fix_up_power = false;
 
-- 
2.9.3

Re: [PATCH v2 2/3] mmc: sdhci-acpi: Add fix_up_power_blacklist module option

[Adrian Hunter]

On 05/04/17 16:00, 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 GPDwin mini clam-shell PC
> which has a pcie 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).
> 
> This commit adds a new sdhci_acpi.fix_up_power_blacklist module option
> which can be set to an ACPI hid:uid pair, e.g. "80860F14:2" to disable
> the calling of acpi_device_fix_up_power() for the sdhci host matching
> the hid:uid pair, fixing the wifi not working on this machine.

Sorry for the slow reply :-(.  I would much prefer to blacklist the whole
device i.e. if the firmware is broken then the device is broken.

Re: [PATCH v2 3/3] mmc: sdhci-acpi: Add DMI fix_up_power_blacklist

[Adrian Hunter]

On 05/04/17 16:00, Hans de Goede wrote:
> Add a DMI based blacklist for systems where calling
> acpi_device_fix_up_power() is harmful.
> 
> 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
> ---
>  drivers/mmc/host/sdhci-acpi.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 5a73174..ba9e688 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>
> @@ -384,6 +385,27 @@ 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 15 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 2 others have
> +		 * the same default product_name.
> +		 */
> +		.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"),

That match is too broad - it looks like defaults from AMI.  Other lazy
manufacturers might also not bother to change the values.

> +		},
> +	},
> +	{ }
> +};
> +
>  static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>  							 const char *uid)
>  {
> @@ -407,6 +429,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	const char *fix_up_power_bl = fix_up_power_blacklist;
>  	bool fix_up_power = true;
>  	struct acpi_device *device, *child;
> +	const struct dmi_system_id *dmi_id;
>  	struct sdhci_acpi_host *c;
>  	struct sdhci_host *host;
>  	struct resource *iomem;
> @@ -421,6 +444,12 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	hid = acpi_device_hid(device);
>  	uid = device->pnp.unique_id;
>  
> +	if (!fix_up_power_bl) {
> +		dmi_id = dmi_first_match(fix_up_power_dmi_blacklist);
> +		if (dmi_id)
> +			fix_up_power_bl = dmi_id->driver_data;
> +	}
> +
>  	if (sdhci_acpi_compare_hid_uid(fix_up_power_bl, hid, uid))
>  		fix_up_power = false;
>  
> 

Re: [PATCH v2 2/3] mmc: sdhci-acpi: Add fix_up_power_blacklist module option

[Hans de Goede]

Hi,

On 12-04-17 15:07, Adrian Hunter wrote:
> On 05/04/17 16:00, 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 GPDwin mini clam-shell PC
>> which has a pcie 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).
>>
>> This commit adds a new sdhci_acpi.fix_up_power_blacklist module option
>> which can be set to an ACPI hid:uid pair, e.g. "80860F14:2" to disable
>> the calling of acpi_device_fix_up_power() for the sdhci host matching
>> the hid:uid pair, fixing the wifi not working on this machine.
>
> Sorry for the slow reply :-(.

No worries.

> I would much prefer to blacklist the whole
> device i.e. if the firmware is broken then the device is broken.

So I'm in 2 minds about this, for the GPDwin disabling the entire
sdio controller is the right thing to do, since the wifi is actually
attached to the pcie. OTOH I've the feeling (but no proof) that not
doing the fixup-power might actually help sdio attached wifi on some
devices (this feeling is based on my patch where I added the _STA
check before calling fixup power, which does fix some real world
issues).

Anyways you say you prefer blacklisting the entire sdhci controller,
so I will send a v3 doing that.

Regards,

Hans

Re: [PATCH v2 3/3] mmc: sdhci-acpi: Add DMI fix_up_power_blacklist

[Hans de Goede]

Hi,

On 12-04-17 15:16, Adrian Hunter wrote:
> On 05/04/17 16:00, Hans de Goede wrote:
>> Add a DMI based blacklist for systems where calling
>> acpi_device_fix_up_power() is harmful.
>>
>> 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
>> ---
>>  drivers/mmc/host/sdhci-acpi.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 5a73174..ba9e688 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>
>> @@ -384,6 +385,27 @@ 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 15 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 2 others have
>> +		 * the same default product_name.
>> +		 */
>> +		.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"),
>
> That match is too broad - it looks like defaults from AMI.  Other lazy
> manufacturers might also not bother to change the values.

Erm, no it is not too broad, that is why the largish comment is
there, to explain that it is not too broad.

My collection of DMI strings for Bay Trail / Cherry Trail devices,
which are the only ones using "80860F14:2" which also needs to match,
has been growing I now have 24 files, and:

[hans@shalem dmi-ids]$ ack "board_vendor" * | grep "AMI Corporation"
dmi.gpdwin:7:board_vendor: AMI Corporation

[hans@shalem dmi-ids]$ ack "board_name" * | grep "Default string"
dmi.gpdwin:5:board_name: Default string

[hans@shalem dmi-ids]$ ack "board_serial" * | grep "Default string"
dmi.gpdwin:6:board_serial: Default string
dmi.iwork8air:6:board_serial: Default string

[hans@shalem dmi-ids]$ ack "product_name" * | grep "Default string"
dmi.Chuwi_HiBook:15:power: product_name: Default string
dmi.chuwi.hi12:13:product_name: Default string
dmi.gpdwin:15:product_name: Default string
dmi.meegopad-t08:15:power: product_name: Default string

So as you can see the first 2 matches are unique in this set of
dmi strings from 24 boards.

Of those the following claim to have an ami bios:

[hans@shalem dmi-ids]$ grep -i -l American *
dmi.Chuwi_HiBook
dmi.Chuwi_Vi8_plus
dmi.Dexp_Ursus_GX110
dmi.asus.t200ta
dmi.chuwi.hi12
dmi.chuwi_hi10pro_cwi529
dmi.gpdwin
dmi.iwork8air
dmi.lamina-i8270
dmi.meegopad-t08
dmi.t100ta

So the uniqueness is not due to lack of AMI bios samples
in my set.

Regards,

Hans



>
>> +		},
>> +	},
>> +	{ }
>> +};
>> +
>>  static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>>  							 const char *uid)
>>  {
>> @@ -407,6 +429,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>  	const char *fix_up_power_bl = fix_up_power_blacklist;
>>  	bool fix_up_power = true;
>>  	struct acpi_device *device, *child;
>> +	const struct dmi_system_id *dmi_id;
>>  	struct sdhci_acpi_host *c;
>>  	struct sdhci_host *host;
>>  	struct resource *iomem;
>> @@ -421,6 +444,12 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>  	hid = acpi_device_hid(device);
>>  	uid = device->pnp.unique_id;
>>
>> +	if (!fix_up_power_bl) {
>> +		dmi_id = dmi_first_match(fix_up_power_dmi_blacklist);
>> +		if (dmi_id)
>> +			fix_up_power_bl = dmi_id->driver_data;
>> +	}
>> +
>>  	if (sdhci_acpi_compare_hid_uid(fix_up_power_bl, hid, uid))
>>  		fix_up_power = false;
>>
>>
>