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

[PATCH v3 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 v3 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 9fd8d7a..42bbc9a 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);
@@ -381,6 +404,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;
@@ -393,6 +417,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)
@@ -402,9 +432,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;
@@ -575,6 +602,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.9.3

[PATCH v3 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
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 42bbc9a..495d1cc 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,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)
 {
@@ -406,6 +429,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;
@@ -420,6 +444,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.9.3

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

[Adrian Hunter]

On 12/04/17 21:19, 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.

What if we move the call to acpi_device_fix_up_power() for the child
devices into the ->init_card() callback? i.e. it won't happen if there
is no card:


diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index c6a9a1bfaa22..dd97978f0361 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -81,6 +81,7 @@ struct sdhci_acpi_host {
 	const struct sdhci_acpi_slot	*slot;
 	struct platform_device		*pdev;
 	bool				use_runtime_pm;
+	bool				child_fixup_power_done;
 };
 
 static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
@@ -374,11 +375,29 @@ static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
 	return NULL;
 }
 
+static void sdhci_acpi_init_card(struct mmc_host *mmc, struct mmc_card *card)
+{
+	struct device *dev = mmc_dev(mmc);
+	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *child;
+
+	if (c->child_fixup_power_done || !adev)
+		return;
+
+	/* Ensure child devices are powered on */
+	list_for_each_entry(child, &adev->children, node)
+		if (child->status.present && child->status.enabled)
+			acpi_device_fix_up_power(child);
+
+	c->child_fixup_power_done = true;
+}
+
 static int sdhci_acpi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	acpi_handle handle = ACPI_HANDLE(dev);
-	struct acpi_device *device, *child;
+	struct acpi_device *device;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
@@ -390,11 +409,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	if (acpi_bus_get_device(handle, &device))
 		return -ENODEV;
 
-	/* Power on the SDHCI controller and its children */
+	/* Power on the SDHCI controller */
 	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 (acpi_bus_get_status(device) || !device->status.present)
 		return -ENODEV;
@@ -432,6 +448,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	host->ops	= &sdhci_acpi_ops_dflt;
 	host->irq	= platform_get_irq(pdev, 0);
 
+	host->mmc_host_ops.init_card = sdhci_acpi_init_card;
+
 	host->ioaddr = devm_ioremap_nocache(dev, iomem->start,
 					    resource_size(iomem));
 	if (host->ioaddr == NULL) {

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

[Hans de Goede]

Hi,

On 13-04-17 13:50, Adrian Hunter wrote:
> On 12/04/17 21:19, 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.
>
> What if we move the call to acpi_device_fix_up_power() for the child
> devices into the ->init_card() callback? i.e. it won't happen if there
> is no card:

Interesting idea, but I think that defeats the original purpose of
calling acpi_device_fix_up_power(), AFAIK sometimes without calling
acpi_device_fix_up_power() the sdio-device will not have power, so
it will not get enumerated / seen and init_card() will not get called.

Either that or since the slot is marked as always-present init_card
gets called always and the patch won't help (I've not tried).

The above may not be entirely correct, but I'm pretty sure that
on some systems the card cannot be enumerated without first making
sure it is powered, so we cannot use card-presence detection to
decide if we should call fixup_power or not. At least I've seen
this on ARM systems (where we have the pwrseq stuff to deal with this)
I'm fine with giving you approach a try, maybe on X86 systems the
sdio-card always has enough power to at least enumerate ? But I'm
afraid it may cause regressions.

Regards,

Hans


>
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index c6a9a1bfaa22..dd97978f0361 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -81,6 +81,7 @@ struct sdhci_acpi_host {
>  	const struct sdhci_acpi_slot	*slot;
>  	struct platform_device		*pdev;
>  	bool				use_runtime_pm;
> +	bool				child_fixup_power_done;
>  };
>
>  static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
> @@ -374,11 +375,29 @@ static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>  	return NULL;
>  }
>
> +static void sdhci_acpi_init_card(struct mmc_host *mmc, struct mmc_card *card)
> +{
> +	struct device *dev = mmc_dev(mmc);
> +	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct acpi_device *child;
> +
> +	if (c->child_fixup_power_done || !adev)
> +		return;
> +
> +	/* Ensure child devices are powered on */
> +	list_for_each_entry(child, &adev->children, node)
> +		if (child->status.present && child->status.enabled)
> +			acpi_device_fix_up_power(child);
> +
> +	c->child_fixup_power_done = true;
> +}
> +
>  static int sdhci_acpi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	acpi_handle handle = ACPI_HANDLE(dev);
> -	struct acpi_device *device, *child;
> +	struct acpi_device *device;
>  	struct sdhci_acpi_host *c;
>  	struct sdhci_host *host;
>  	struct resource *iomem;
> @@ -390,11 +409,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	if (acpi_bus_get_device(handle, &device))
>  		return -ENODEV;
>
> -	/* Power on the SDHCI controller and its children */
> +	/* Power on the SDHCI controller */
>  	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 (acpi_bus_get_status(device) || !device->status.present)
>  		return -ENODEV;
> @@ -432,6 +448,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	host->ops	= &sdhci_acpi_ops_dflt;
>  	host->irq	= platform_get_irq(pdev, 0);
>
> +	host->mmc_host_ops.init_card = sdhci_acpi_init_card;
> +
>  	host->ioaddr = devm_ioremap_nocache(dev, iomem->start,
>  					    resource_size(iomem));
>  	if (host->ioaddr == NULL) {
>

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

[Adrian Hunter]

On 12/04/17 21:19, Hans de Goede 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>

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