[PATCH 0/3] mmc: sdhci-acpi: Add fix_up_power module option

[PATCH 0/3] mmc: sdhci-acpi: Add fix_up_power module option

[Hans de Goede]

Hi,

Here is a patch-set fixing the wifi not working on the GPDwin.

The first patch I've send before, but I'm resending as it needs
to be applied before patch 2 and 3 to avoid conflicts.

Regards,

Hans

[PATCH 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 2/3] mmc: sdhci-acpi: Add fix_up_power 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 module option which
defaults to 1 (leaving the existing behavior intact) and can be set
to 0 disable the calling of acpi_device_fix_up_power() fixing the
wifi not working on this machine.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mmc/host/sdhci-acpi.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 9fd8d7a..8fe46ba 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -83,6 +83,8 @@ struct sdhci_acpi_host {
 	bool				use_runtime_pm;
 };
 
+static int fix_up_power = 1;
+
 static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
 {
 	return c->slot && (c->slot->flags & flag);
@@ -394,10 +396,12 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	/* 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;
@@ -575,6 +579,10 @@ static struct platform_driver sdhci_acpi_driver = {
 
 module_platform_driver(sdhci_acpi_driver);
 
+module_param(fix_up_power, int, 0444);
+MODULE_PARM_DESC(fix_up_power,
+		 "Call acpi_device_fix_up_power on sdio host and its children");
+
 MODULE_DESCRIPTION("Secure Digital Host Controller Interface ACPI driver");
 MODULE_AUTHOR("Adrian Hunter");
 MODULE_LICENSE("GPL v2");
-- 
2.9.3

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

Note as the comment added above the GPDwin entry explains unfortunately
the GPDwin dmi strings are somewhat generic, so I've added the bios-date
string for a more unique match, which means we need 3 entries for the
GPDwin for the 3 known BIOS versions out there.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mmc/host/sdhci-acpi.c | 46 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 8fe46ba..826e34f 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,7 +84,7 @@ struct sdhci_acpi_host {
 	bool				use_runtime_pm;
 };
 
-static int fix_up_power = 1;
+static int fix_up_power = -1;
 
 static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
 {
@@ -363,6 +364,42 @@ 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_blacklist[] = {
+	{
+		/*
+		 * Match for the GPDwin which unfortunately uses somewhat
+		 * generic dmi strings, which is why the bios-date match is
+		 * included and we need multiple entries :| These strings have
+		 * been checked against 6 other byt/cht boards and board_vendor
+		 * and board_name are unique to the GPDwin (in the test set)
+		 * where as only one other board has the same board_version.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
+			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
+			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
+			DMI_MATCH(DMI_BIOS_DATE, "10/25/2016"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
+			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
+			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
+			DMI_MATCH(DMI_BIOS_DATE, "11/18/2016"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
+			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
+			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
+			DMI_MATCH(DMI_BIOS_DATE, "02/21/2017"),
+		},
+	},
+	{ }
+};
+
 static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
 							 const char *uid)
 {
@@ -395,6 +432,13 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	if (acpi_bus_get_device(handle, &device))
 		return -ENODEV;
 
+	if (fix_up_power == -1) {
+		if (dmi_check_system(fix_up_power_blacklist))
+			fix_up_power = 0;
+		else
+			fix_up_power = 1;
+	}
+
 	/* Power on the SDHCI controller and its children */
 	if (fix_up_power) {
 		acpi_device_fix_up_power(device);
-- 
2.9.3

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

[Adrian Hunter]

On 04/04/17 11:50, Hans de Goede wrote:
> Add a DMI based blacklist for systems where calling
> acpi_device_fix_up_power() is harmful.
> 
> Note as the comment added above the GPDwin entry explains unfortunately
> the GPDwin dmi strings are somewhat generic, so I've added the bios-date
> string for a more unique match, which means we need 3 entries for the
> GPDwin for the 3 known BIOS versions out there.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/mmc/host/sdhci-acpi.c | 46 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 8fe46ba..826e34f 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,7 +84,7 @@ struct sdhci_acpi_host {
>  	bool				use_runtime_pm;
>  };
>  
> -static int fix_up_power = 1;
> +static int fix_up_power = -1;
>  
>  static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>  {
> @@ -363,6 +364,42 @@ 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_blacklist[] = {
> +	{
> +		/*
> +		 * Match for the GPDwin which unfortunately uses somewhat
> +		 * generic dmi strings, which is why the bios-date match is
> +		 * included and we need multiple entries :| These strings have
> +		 * been checked against 6 other byt/cht boards and board_vendor
> +		 * and board_name are unique to the GPDwin (in the test set)
> +		 * where as only one other board has the same board_version.
> +		 */
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
> +			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
> +			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
> +			DMI_MATCH(DMI_BIOS_DATE, "10/25/2016"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
> +			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
> +			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
> +			DMI_MATCH(DMI_BIOS_DATE, "11/18/2016"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
> +			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
> +			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
> +			DMI_MATCH(DMI_BIOS_DATE, "02/21/2017"),

And if there are more versions?

I would prefer to add a module parameter to blacklist the offending HID:UID

> +		},
> +	},
> +	{ }
> +};
> +
>  static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>  							 const char *uid)
>  {
> @@ -395,6 +432,13 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	if (acpi_bus_get_device(handle, &device))
>  		return -ENODEV;
>  
> +	if (fix_up_power == -1) {
> +		if (dmi_check_system(fix_up_power_blacklist))
> +			fix_up_power = 0;
> +		else
> +			fix_up_power = 1;
> +	}
> +
>  	/* Power on the SDHCI controller and its children */
>  	if (fix_up_power) {
>  		acpi_device_fix_up_power(device);
> 

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

[Hans de Goede]

Hi,

On 05-04-17 09:29, Adrian Hunter wrote:
> On 04/04/17 11:50, Hans de Goede wrote:
>> Add a DMI based blacklist for systems where calling
>> acpi_device_fix_up_power() is harmful.
>>
>> Note as the comment added above the GPDwin entry explains unfortunately
>> the GPDwin dmi strings are somewhat generic, so I've added the bios-date
>> string for a more unique match, which means we need 3 entries for the
>> GPDwin for the 3 known BIOS versions out there.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/mmc/host/sdhci-acpi.c | 46 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 8fe46ba..826e34f 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,7 +84,7 @@ struct sdhci_acpi_host {
>>  	bool				use_runtime_pm;
>>  };
>>
>> -static int fix_up_power = 1;
>> +static int fix_up_power = -1;
>>
>>  static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>>  {
>> @@ -363,6 +364,42 @@ 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_blacklist[] = {
>> +	{
>> +		/*
>> +		 * Match for the GPDwin which unfortunately uses somewhat
>> +		 * generic dmi strings, which is why the bios-date match is
>> +		 * included and we need multiple entries :| These strings have
>> +		 * been checked against 6 other byt/cht boards and board_vendor
>> +		 * and board_name are unique to the GPDwin (in the test set)
>> +		 * where as only one other board has the same board_version.
>> +		 */
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
>> +			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
>> +			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
>> +			DMI_MATCH(DMI_BIOS_DATE, "10/25/2016"),
>> +		},
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
>> +			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
>> +			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
>> +			DMI_MATCH(DMI_BIOS_DATE, "11/18/2016"),
>> +		},
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
>> +			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
>> +			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
>> +			DMI_MATCH(DMI_BIOS_DATE, "02/21/2017"),
>
> And if there are more versions?

True, I already sort of expected this, so I've asked the internets
to send me dmi strings for other Cherry Trail / Bay Trail devices,
I now have a sample set of 15 devices.

Among these the GPDwin is the only one to set DMI_BOARD_VENDOR to
"AMI Corporation", as well as the only one to set DMI_BOARD_NAME
to "Default string", one of only 2 to set DMI_BOARD_SERIAL to
"Default string" and as one of only 3 to set DMI_PRODUCT_NAME
to "Default string", so I think that if me on those 4 (the latter
2 to be on the save side) that we can then remove the BIOS date
check and use only one entry, I will do so in the next version.

I do believe that we really need a DMI quirk and not just a module
option as this is something which should just work (and wifi not working
actually is a recent regression, see below).

> I would prefer to add a module parameter to blacklist the offending HID:UID

I think that applying the module_param to only a single HID:UID is a good
idea, this will also make the DMI match more narrow, esp. since the
GPDwin uses the somewhat rare 80860F14:2 HID:UID which only got added to
the kernel recently (and promptly regressed wifi support on the GPDwin).

I will do so in the next version.

When you say "black_list" do you mean not apply the acpi_power_fix_up calls
to the selected HID:UID, or do you mean completely ignoring the device
(return -ENODEV from probe()) ?

I think only skipping the acpi_power_fix_up calls is better, because that
I've the feeling that might help on some boards which do have sdio-wifi too,
where as returning -ENODEV although it will work for the GPDwin case, is
less likely to help in other cases (although I must admit I guess it may also
help in some cases where only skipping the power-fixup might not).

Regards,

Hans



>
>> +		},
>> +	},
>> +	{ }
>> +};
>> +
>>  static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>>  							 const char *uid)
>>  {
>> @@ -395,6 +432,13 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>  	if (acpi_bus_get_device(handle, &device))
>>  		return -ENODEV;
>>
>> +	if (fix_up_power == -1) {
>> +		if (dmi_check_system(fix_up_power_blacklist))
>> +			fix_up_power = 0;
>> +		else
>> +			fix_up_power = 1;
>> +	}
>> +
>>  	/* Power on the SDHCI controller and its children */
>>  	if (fix_up_power) {
>>  		acpi_device_fix_up_power(device);
>>
>