[PATCH 5.16 regression fix 0/5] ACPI: scan: Skip turning off some unused objects during scan

[PATCH 5.16 regression fix 0/5] ACPI: scan: Skip turning off some unused objects during scan

[Hans de Goede]

Hi Rafael,

Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by
unused objects") adds a:

	bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused);

call to acpi_scan_init(). On some devices with buggy DSDTs calling
_PS3 for one device may result in it turning off another device.

Specifically the DSDT of the GPD win and GPD pocket devices has a
"\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module
which _PS3 method sets a GPIO causing the PCI wifi card to turn off.

I've an earlier, in some ways simpler, fix for this here:
https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch

But the sdhci-acpi.c MMC host code already has an older workaround
for it to not toggle power on this broken ACPI object; and this
simpler fix would require keeping that workaround. So then we would
have 2 workarounds for the same issue in the kernel.

Thus instead I've come up with a slightly different approach which
IMHO has ended up pretty well.

Patches 1-3 of this series are this different approach and assuming
they are considered ok must be merged into 5.16 to fix the regression
caused by commit c10383e8ddf4 on these devices.

Patch 4 removes the now no longer necessary workaround for the same
issue from the sdhci-acpi.c code. Once 1-3 are merged this could
also go to 5.16 but 5.17 is fine too.

Patch 5 is a small bonus cleanup to the sdhci-acpi.c code.

Regards,

Hans


Hans de Goede (5):
  ACPI: Change acpi_device_always_present() into
    acpi_device_override_status()
  ACPI: x86: Allow specifying acpi_device_override_status() quirks by
    path
  ACPI: x86: Add not-present quirk for the PCI0.SDHB.BRC1 device on the
    GPD win
  mmc: sdhci-acpi: Remove special handling for GPD win/pocket devices
  mmc: sdhci-acpi: Use the new soc_intel_is_byt() helper

 drivers/acpi/bus.c            |   4 +-
 drivers/acpi/x86/utils.c      | 114 ++++++++++++++++++++++++----------
 drivers/mmc/host/sdhci-acpi.c |  78 ++---------------------
 include/acpi/acpi_bus.h       |   5 +-
 4 files changed, 91 insertions(+), 110 deletions(-)

-- 
2.31.1

[PATCH 5.16 regression fix 1/5] ACPI: Change acpi_device_always_present() into acpi_device_override_status()

[Hans de Goede]

ATM acpi_bus_get_status() calls acpi_device_always_present() to allow
platform quirks to override the _STA return to report that a device
is present (status = ACPI_STA_DEFAULT) independent of the _STA return.

In some cases it might also be useful to have the opposite functionality
and have a platform quirk which marks a device as not present (status = 0)
to work around ACPI table bugs.

Change acpi_device_always_present() into a more generic
acpi_device_override_status() function to allow this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/bus.c       |  4 +--
 drivers/acpi/x86/utils.c | 66 +++++++++++++++++++++++-----------------
 include/acpi/acpi_bus.h  |  5 +--
 3 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index fa923a929224..dd535b4b9a16 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -98,8 +98,8 @@ int acpi_bus_get_status(struct acpi_device *device)
 	acpi_status status;
 	unsigned long long sta;
 
-	if (acpi_device_always_present(device)) {
-		acpi_set_device_status(device, ACPI_STA_DEFAULT);
+	if (acpi_device_override_status(device, &sta)) {
+		acpi_set_device_status(device, sta);
 		return 0;
 	}
 
diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index bfcb76888ca7..165110210750 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -22,40 +22,49 @@
  * Some BIOS-es (temporarily) hide specific APCI devices to work around Windows
  * driver bugs. We use DMI matching to match known cases of this.
  *
- * We work around this by always reporting ACPI_STA_DEFAULT for these
- * devices. Note this MUST only be done for devices where this is safe.
+ * Likewise sometimes some not-actually present devices are sometimes
+ * reported as present, which may cause issues.
  *
- * This forcing of devices to be present is limited to specific CPU (SoC)
- * models both to avoid potentially causing trouble on other models and
- * because some HIDs are re-used on different SoCs for completely
- * different devices.
+ * We work around this by using the below quirk list to override the status
+ * reported by the _STA method with a fixed value (ACPI_STA_DEFAULT or 0).
+ * Note this MUST only be done for devices where this is safe.
+ *
+ * This status overriding is limited to specific CPU (SoC) models both to
+ * avoid potentially causing trouble on other models and because some HIDs
+ * are re-used on different SoCs for completely different devices.
  */
-struct always_present_id {
+struct override_status_id {
 	struct acpi_device_id hid[2];
 	struct x86_cpu_id cpu_ids[2];
 	struct dmi_system_id dmi_ids[2]; /* Optional */
 	const char *uid;
+	unsigned long long status;
 };
 
-#define X86_MATCH(model)	X86_MATCH_INTEL_FAM6_MODEL(model, NULL)
-
-#define ENTRY(hid, uid, cpu_models, dmi...) {				\
+#define ENTRY(status, hid, uid, cpu_model, dmi...) {			\
 	{ { hid, }, {} },						\
-	{ cpu_models, {} },						\
+	{ X86_MATCH_INTEL_FAM6_MODEL(cpu_model, NULL), {} },		\
 	{ { .matches = dmi }, {} },					\
 	uid,								\
+	status,								\
 }
 
-static const struct always_present_id always_present_ids[] = {
+#define PRESENT_ENTRY_HID(hid, uid, cpu_model, dmi...) \
+	ENTRY(ACPI_STA_DEFAULT, hid, uid, cpu_model, dmi)
+
+#define NOT_PRESENT_ENTRY_HID(hid, uid, cpu_model, dmi...) \
+	ENTRY(0, hid, uid, cpu_model, dmi)
+
+static const struct override_status_id override_status_ids[] = {
 	/*
 	 * Bay / Cherry Trail PWM directly poked by GPU driver in win10,
 	 * but Linux uses a separate PWM driver, harmless if not used.
 	 */
-	ENTRY("80860F09", "1", X86_MATCH(ATOM_SILVERMONT), {}),
-	ENTRY("80862288", "1", X86_MATCH(ATOM_AIRMONT), {}),
+	PRESENT_ENTRY_HID("80860F09", "1", ATOM_SILVERMONT, {}),
+	PRESENT_ENTRY_HID("80862288", "1", ATOM_AIRMONT, {}),
 
 	/* The Xiaomi Mi Pad 2 uses PWM2 for touchkeys backlight control */
-	ENTRY("80862289", "2", X86_MATCH(ATOM_AIRMONT), {
+	PRESENT_ENTRY_HID("80862289", "2", ATOM_AIRMONT, {
 		DMI_MATCH(DMI_SYS_VENDOR, "Xiaomi Inc"),
 		DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"),
 	      }),
@@ -63,18 +72,18 @@ static const struct always_present_id always_present_ids[] = {
 	 * The INT0002 device is necessary to clear wakeup interrupt sources
 	 * on Cherry Trail devices, without it we get nobody cared IRQ msgs.
 	 */
-	ENTRY("INT0002", "1", X86_MATCH(ATOM_AIRMONT), {}),
+	PRESENT_ENTRY_HID("INT0002", "1", ATOM_AIRMONT, {}),
 	/*
 	 * On the Dell Venue 11 Pro 7130 and 7139, the DSDT hides
 	 * the touchscreen ACPI device until a certain time
 	 * after _SB.PCI0.GFX0.LCD.LCD1._ON gets called has passed
 	 * *and* _STA has been called at least 3 times since.
 	 */
-	ENTRY("SYNA7500", "1", X86_MATCH(HASWELL_L), {
+	PRESENT_ENTRY_HID("SYNA7500", "1", HASWELL_L, {
 		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 		DMI_MATCH(DMI_PRODUCT_NAME, "Venue 11 Pro 7130"),
 	      }),
-	ENTRY("SYNA7500", "1", X86_MATCH(HASWELL_L), {
+	PRESENT_ENTRY_HID("SYNA7500", "1", HASWELL_L, {
 		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 		DMI_MATCH(DMI_PRODUCT_NAME, "Venue 11 Pro 7139"),
 	      }),
@@ -90,19 +99,19 @@ static const struct always_present_id always_present_ids[] = {
 	 * was copy-pasted from the GPD win, so it has a disabled KIOX000A
 	 * node which we should not enable, thus we also check the BIOS date.
 	 */
-	ENTRY("KIOX000A", "1", X86_MATCH(ATOM_AIRMONT), {
+	PRESENT_ENTRY_HID("KIOX000A", "1", ATOM_AIRMONT, {
 		DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
 		DMI_MATCH(DMI_BOARD_NAME, "Default string"),
 		DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
 		DMI_MATCH(DMI_BIOS_DATE, "02/21/2017")
 	      }),
-	ENTRY("KIOX000A", "1", X86_MATCH(ATOM_AIRMONT), {
+	PRESENT_ENTRY_HID("KIOX000A", "1", ATOM_AIRMONT, {
 		DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
 		DMI_MATCH(DMI_BOARD_NAME, "Default string"),
 		DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
 		DMI_MATCH(DMI_BIOS_DATE, "03/20/2017")
 	      }),
-	ENTRY("KIOX000A", "1", X86_MATCH(ATOM_AIRMONT), {
+	PRESENT_ENTRY_HID("KIOX000A", "1", ATOM_AIRMONT, {
 		DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
 		DMI_MATCH(DMI_BOARD_NAME, "Default string"),
 		DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
@@ -110,26 +119,27 @@ static const struct always_present_id always_present_ids[] = {
 	      }),
 };
 
-bool acpi_device_always_present(struct acpi_device *adev)
+bool acpi_device_override_status(struct acpi_device *adev, unsigned long long *status)
 {
 	bool ret = false;
 	unsigned int i;
 
-	for (i = 0; i < ARRAY_SIZE(always_present_ids); i++) {
-		if (acpi_match_device_ids(adev, always_present_ids[i].hid))
+	for (i = 0; i < ARRAY_SIZE(override_status_ids); i++) {
+		if (acpi_match_device_ids(adev, override_status_ids[i].hid))
 			continue;
 
 		if (!adev->pnp.unique_id ||
-		    strcmp(adev->pnp.unique_id, always_present_ids[i].uid))
+		    strcmp(adev->pnp.unique_id, override_status_ids[i].uid))
 			continue;
 
-		if (!x86_match_cpu(always_present_ids[i].cpu_ids))
+		if (!x86_match_cpu(override_status_ids[i].cpu_ids))
 			continue;
 
-		if (always_present_ids[i].dmi_ids[0].matches[0].slot &&
-		    !dmi_check_system(always_present_ids[i].dmi_ids))
+		if (override_status_ids[i].dmi_ids[0].matches[0].slot &&
+		    !dmi_check_system(override_status_ids[i].dmi_ids))
 			continue;
 
+		*status = override_status_ids[i].status;
 		ret = true;
 		break;
 	}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 2f93ecf05dac..5895f6c7f6db 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -615,9 +615,10 @@ int acpi_enable_wakeup_device_power(struct acpi_device *dev, int state);
 int acpi_disable_wakeup_device_power(struct acpi_device *dev);
 
 #ifdef CONFIG_X86
-bool acpi_device_always_present(struct acpi_device *adev);
+bool acpi_device_override_status(struct acpi_device *adev, unsigned long long *status);
 #else
-static inline bool acpi_device_always_present(struct acpi_device *adev)
+static inline bool acpi_device_override_status(struct acpi_device *adev,
+					       unsigned long long *status)
 {
 	return false;
 }
-- 
2.31.1

[PATCH 5.16 regression fix 2/5] ACPI: x86: Allow specifying acpi_device_override_status() quirks by path

[Hans de Goede]

Not all ACPI-devices have a HID + UID, allow specifying quirks for
acpi_device_override_status() by path too.

Note this moves the path/HID+UID check to after the CPU + DMI checks
since the path lookup is somewhat costly.

This way this lookup is only done on devices where the other checks
match.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/x86/utils.c | 42 ++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index 165110210750..a854c28047f2 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -38,22 +38,30 @@ struct override_status_id {
 	struct x86_cpu_id cpu_ids[2];
 	struct dmi_system_id dmi_ids[2]; /* Optional */
 	const char *uid;
+	const char *path;
 	unsigned long long status;
 };
 
-#define ENTRY(status, hid, uid, cpu_model, dmi...) {			\
+#define ENTRY(status, hid, uid, path, cpu_model, dmi...) {		\
 	{ { hid, }, {} },						\
 	{ X86_MATCH_INTEL_FAM6_MODEL(cpu_model, NULL), {} },		\
 	{ { .matches = dmi }, {} },					\
 	uid,								\
+	path,								\
 	status,								\
 }
 
 #define PRESENT_ENTRY_HID(hid, uid, cpu_model, dmi...) \
-	ENTRY(ACPI_STA_DEFAULT, hid, uid, cpu_model, dmi)
+	ENTRY(ACPI_STA_DEFAULT, hid, uid, NULL, cpu_model, dmi)
 
 #define NOT_PRESENT_ENTRY_HID(hid, uid, cpu_model, dmi...) \
-	ENTRY(0, hid, uid, cpu_model, dmi)
+	ENTRY(0, hid, uid, NULL, cpu_model, dmi)
+
+#define PRESENT_ENTRY_PATH(path, cpu_model, dmi...) \
+	ENTRY(ACPI_STA_DEFAULT, "", NULL, path, cpu_model, dmi)
+
+#define NOT_PRESENT_ENTRY_PATH(path, cpu_model, dmi...) \
+	ENTRY(0, "", NULL, path, cpu_model, dmi)
 
 static const struct override_status_id override_status_ids[] = {
 	/*
@@ -125,13 +133,6 @@ bool acpi_device_override_status(struct acpi_device *adev, unsigned long long *s
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(override_status_ids); i++) {
-		if (acpi_match_device_ids(adev, override_status_ids[i].hid))
-			continue;
-
-		if (!adev->pnp.unique_id ||
-		    strcmp(adev->pnp.unique_id, override_status_ids[i].uid))
-			continue;
-
 		if (!x86_match_cpu(override_status_ids[i].cpu_ids))
 			continue;
 
@@ -139,6 +140,27 @@ bool acpi_device_override_status(struct acpi_device *adev, unsigned long long *s
 		    !dmi_check_system(override_status_ids[i].dmi_ids))
 			continue;
 
+		if (override_status_ids[i].path) {
+			struct acpi_buffer path = { ACPI_ALLOCATE_BUFFER, NULL };
+			bool match;
+
+			if (acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, &path))
+				continue;
+
+			match = strcmp((char *)path.pointer, override_status_ids[i].path) == 0;
+			kfree(path.pointer);
+
+			if (!match)
+				continue;
+		} else {
+			if (acpi_match_device_ids(adev, override_status_ids[i].hid))
+				continue;
+
+			if (!adev->pnp.unique_id ||
+			    strcmp(adev->pnp.unique_id, override_status_ids[i].uid))
+				continue;
+		}
+
 		*status = override_status_ids[i].status;
 		ret = true;
 		break;
-- 
2.31.1

[PATCH 5.16 regression fix 3/5] ACPI: x86: Add not-present quirk for the PCI0.SDHB.BRC1 device on the GPD win

[Hans de Goede]

The GPD win and its sibling the GPD pocket (99% the same electronics in a
different case) use a PCI wifi card. But the ACPI tables on both variants
contain a bug where the SDIO MMC controller for SDIO wifi cards is enabled
despite this. This SDIO MMC controller has a PCI0.SDHB.BRC1 child-device
which _PS3 method sets a GPIO causing the PCI wifi card to turn off.

Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by
unused objects") adds a:

	bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused);

call to acpi_scan_init() which causes the buggy _PS3 method to get called
causing the wifi to no longer work.

Add a quirk to override the _STA method for the PCI0.SDHB.BRC1 child
with a status value of 0, so that its power_manageable gets cleared,
avoiding this problem.

This fixes the wifi no longer working on the GPD win and GPD pocket
devices afer this change.

Note that even though it is not used, the _STA method for the MMC
controller is deliberately not overridden. If the status of the MMC
controller were forced to 0 it would never get suspended, which would
cause these mini-laptops to not reach S0i3 level when suspended.

Fixes: c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by unused objects")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/x86/utils.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index a854c28047f2..047fb4747ea8 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -99,9 +99,10 @@ static const struct override_status_id override_status_ids[] = {
 	/*
 	 * The GPD win BIOS dated 20170221 has disabled the accelerometer, the
 	 * drivers sometimes cause crashes under Windows and this is how the
-	 * manufacturer has solved this :| Note that the the DMI data is less
-	 * generic then it seems, a board_vendor of "AMI Corporation" is quite
-	 * rare and a board_name of "Default String" also is rare.
+	 * manufacturer has solved this :|  The DMI match may not seem unique,
+	 * but it is. In the 67000+ DMI decode dumps from linux-hardware.org
+	 * only 116 have board_vendor set to "AMI Corporation" and of those 116
+	 * only the GPD win and pocket entries' board_name is "Default string".
 	 *
 	 * Unfortunately the GPD pocket also uses these strings and its BIOS
 	 * was copy-pasted from the GPD win, so it has a disabled KIOX000A
@@ -125,6 +126,19 @@ static const struct override_status_id override_status_ids[] = {
 		DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
 		DMI_MATCH(DMI_BIOS_DATE, "05/25/2017")
 	      }),
+
+	/*
+	 * The GPD win/pocket have a PCI wifi card, but its DSDT has the SDIO
+	 * mmc controller enabled and that has a child-device which _PS3
+	 * method sets a GPIO causing the PCI wifi card to turn off.
+	 * See above remark about uniqueness of the DMI match.
+	 */
+	NOT_PRESENT_ENTRY_PATH("\\_SB_.PCI0.SDHB.BRC1", ATOM_AIRMONT, {
+		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
+		DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
+		DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
+		DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
+	      }),
 };
 
 bool acpi_device_override_status(struct acpi_device *adev, unsigned long long *status)
-- 
2.31.1

[PATCH 4/5] mmc: sdhci-acpi: Remove special handling for GPD win/pocket devices

[Hans de Goede]

Remove the special sdhci_acpi_no_fixup_child_power() helper which was
added to avoid triggering an ACPI tables bug on the GPD win/pocket
devices.

The ACPI child-device triggering this bug has now been added to the
acpi_device_override_status() quirk table, so that its status
field is set to all 0 (instead of the wrong return value from the _STA
ACPI method). This removes the need for the special handling in
the sdhci-acpi code.

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

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index f1ef0d28b0dd..1461aae13c19 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -34,7 +34,6 @@
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/iosf_mbi.h>
-#include <linux/pci.h>
 #endif
 
 #include "sdhci.h"
@@ -250,16 +249,6 @@ 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_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL),
-		{}
-	};
-
-	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)
@@ -304,43 +293,6 @@ 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(struct acpi_device *adev)
-{
-	return sdhci_acpi_cht() &&
-	       acpi_dev_hid_uid_match(adev, "80860F14", "2") &&
-	       sdhci_acpi_cht_pci_wifi(0x14e4, 0x43ec, 0, 28);
-}
-
 #else
 
 static inline void sdhci_acpi_byt_setting(struct device *dev)
@@ -352,11 +304,6 @@ static inline bool sdhci_acpi_byt_defer(struct device *dev)
 	return false;
 }
 
-static inline bool sdhci_acpi_no_fixup_child_power(struct acpi_device *adev)
-{
-	return false;
-}
-
 #endif
 
 static int bxt_get_cd(struct mmc_host *mmc)
@@ -861,11 +808,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 
 	/* Power on the SDHCI controller and its children */
 	acpi_device_fix_up_power(device);
-	if (!sdhci_acpi_no_fixup_child_power(device)) {
-		list_for_each_entry(child, &device->children, node)
-			if (child->status.present && child->status.enabled)
-				acpi_device_fix_up_power(child);
-	}
+	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;
-- 
2.31.1

[PATCH 5/5] mmc: sdhci-acpi: Use the new soc_intel_is_byt() helper

[Hans de Goede]

Use the new soc_intel_is_byt() helper function from
include/linux/platform_data/x86/soc.h .

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

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 1461aae13c19..c0350e9c03f3 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -31,8 +31,7 @@
 #include <linux/mmc/slot-gpio.h>
 
 #ifdef CONFIG_X86
-#include <asm/cpu_device_id.h>
-#include <asm/intel-family.h>
+#include <linux/platform_data/x86/soc.h>
 #include <asm/iosf_mbi.h>
 #endif
 
@@ -239,16 +238,6 @@ static const struct sdhci_acpi_chip sdhci_acpi_chip_int = {
 
 #ifdef CONFIG_X86
 
-static bool sdhci_acpi_byt(void)
-{
-	static const struct x86_cpu_id byt[] = {
-		X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
-		{}
-	};
-
-	return x86_match_cpu(byt);
-}
-
 #define BYT_IOSF_SCCEP			0x63
 #define BYT_IOSF_OCP_NETCTRL0		0x1078
 #define BYT_IOSF_OCP_TIMEOUT_BASE	GENMASK(10, 8)
@@ -257,7 +246,7 @@ static void sdhci_acpi_byt_setting(struct device *dev)
 {
 	u32 val = 0;
 
-	if (!sdhci_acpi_byt())
+	if (!soc_intel_is_byt())
 		return;
 
 	if (iosf_mbi_read(BYT_IOSF_SCCEP, MBI_CR_READ, BYT_IOSF_OCP_NETCTRL0,
@@ -282,7 +271,7 @@ static void sdhci_acpi_byt_setting(struct device *dev)
 
 static bool sdhci_acpi_byt_defer(struct device *dev)
 {
-	if (!sdhci_acpi_byt())
+	if (!soc_intel_is_byt())
 		return false;
 
 	if (!iosf_mbi_available())
-- 
2.31.1

Re: [PATCH 5.16 regression fix 0/5] ACPI: scan: Skip turning off some unused objects during scan

[Rafael J. Wysocki]

On Wed, Nov 17, 2021 at 11:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Rafael,
>
> Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by
> unused objects") adds a:
>
>         bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused);
>
> call to acpi_scan_init(). On some devices with buggy DSDTs calling
> _PS3 for one device may result in it turning off another device.

Well, I'm going to revert this commit.  I'm sending a pull request
with the revert later today.

> Specifically the DSDT of the GPD win and GPD pocket devices has a
> "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module
> which _PS3 method sets a GPIO causing the PCI wifi card to turn off.
>
> I've an earlier, in some ways simpler, fix for this here:
> https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch
>
> But the sdhci-acpi.c MMC host code already has an older workaround
> for it to not toggle power on this broken ACPI object; and this
> simpler fix would require keeping that workaround. So then we would
> have 2 workarounds for the same issue in the kernel.
>
> Thus instead I've come up with a slightly different approach which
> IMHO has ended up pretty well.
>
> Patches 1-3 of this series are this different approach and assuming
> they are considered ok must be merged into 5.16 to fix the regression
> caused by commit c10383e8ddf4 on these devices.

So I'll have a look at these and if they look good, we can do that
instead of the problematic commit in 5.17.

> Patch 4 removes the now no longer necessary workaround for the same
> issue from the sdhci-acpi.c code. Once 1-3 are merged this could
> also go to 5.16 but 5.17 is fine too.
>
> Patch 5 is a small bonus cleanup to the sdhci-acpi.c code.

Thanks!

Re: [PATCH 5.16 regression fix 0/5] ACPI: scan: Skip turning off some unused objects during scan

[Hans de Goede]

Hi,

On 11/18/21 12:08, Rafael J. Wysocki wrote:
> On Wed, Nov 17, 2021 at 11:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Rafael,
>>
>> Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by
>> unused objects") adds a:
>>
>>         bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused);
>>
>> call to acpi_scan_init(). On some devices with buggy DSDTs calling
>> _PS3 for one device may result in it turning off another device.
> 
> Well, I'm going to revert this commit.  I'm sending a pull request
> with the revert later today.
> 
>> Specifically the DSDT of the GPD win and GPD pocket devices has a
>> "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module
>> which _PS3 method sets a GPIO causing the PCI wifi card to turn off.
>>
>> I've an earlier, in some ways simpler, fix for this here:
>> https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch
>>
>> But the sdhci-acpi.c MMC host code already has an older workaround
>> for it to not toggle power on this broken ACPI object; and this
>> simpler fix would require keeping that workaround. So then we would
>> have 2 workarounds for the same issue in the kernel.
>>
>> Thus instead I've come up with a slightly different approach which
>> IMHO has ended up pretty well.
>>
>> Patches 1-3 of this series are this different approach and assuming
>> they are considered ok must be merged into 5.16 to fix the regression
>> caused by commit c10383e8ddf4 on these devices.
> 
> So I'll have a look at these and if they look good, we can do that
> instead of the problematic commit in 5.17.

I'm a bit confused now, if the problematic commit is going to get
reversed then technically we don't need this series anymore ?

Or are you planning on re-introducing it in some form for 5.17 ?

With that said getting this series merged would still be good,
patch 1 + 2 make the existing always_present quirk code more generic
which might be useful later. And then patch 3 (which is small)
allows dropping some ugliness from the sdhci-acpi.c code since
the DSDT bug we are hitting will now be solved by the
new acpi-dev-status-override mechanism.

Regards,

Hans

Re: [PATCH 5.16 regression fix 0/5] ACPI: scan: Skip turning off some unused objects during scan

[Rafael J. Wysocki]

On Thursday, November 18, 2021 12:15:28 PM CET Hans de Goede wrote:
> Hi,
> 
> On 11/18/21 12:08, Rafael J. Wysocki wrote:
> > On Wed, Nov 17, 2021 at 11:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by
> >> unused objects") adds a:
> >>
> >>         bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused);
> >>
> >> call to acpi_scan_init(). On some devices with buggy DSDTs calling
> >> _PS3 for one device may result in it turning off another device.
> > 
> > Well, I'm going to revert this commit.  I'm sending a pull request
> > with the revert later today.
> > 
> >> Specifically the DSDT of the GPD win and GPD pocket devices has a
> >> "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module
> >> which _PS3 method sets a GPIO causing the PCI wifi card to turn off.
> >>
> >> I've an earlier, in some ways simpler, fix for this here:
> >> https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch
> >>
> >> But the sdhci-acpi.c MMC host code already has an older workaround
> >> for it to not toggle power on this broken ACPI object; and this
> >> simpler fix would require keeping that workaround. So then we would
> >> have 2 workarounds for the same issue in the kernel.
> >>
> >> Thus instead I've come up with a slightly different approach which
> >> IMHO has ended up pretty well.
> >>
> >> Patches 1-3 of this series are this different approach and assuming
> >> they are considered ok must be merged into 5.16 to fix the regression
> >> caused by commit c10383e8ddf4 on these devices.
> > 
> > So I'll have a look at these and if they look good, we can do that
> > instead of the problematic commit in 5.17.
> 
> I'm a bit confused now, if the problematic commit is going to get
> reversed then technically we don't need this series anymore ?

That's correct.

> Or are you planning on re-introducing it in some form for 5.17 ?

I have been considering this.

> With that said getting this series merged would still be good,
> patch 1 + 2 make the existing always_present quirk code more generic
> which might be useful later. And then patch 3 (which is small)
> allows dropping some ugliness from the sdhci-acpi.c code since
> the DSDT bug we are hitting will now be solved by the
> new acpi-dev-status-override mechanism.

OK, so this would be applicable for 5.17, but a couple of changelogs
need to be updated if I'm not mistaken.

Can you please do that and resend the series?

Re: [PATCH 5.16 regression fix 0/5] ACPI: scan: Skip turning off some unused objects during scan

[Hans de Goede]

Hi,

On 11/18/21 15:51, Rafael J. Wysocki wrote:
> On Thursday, November 18, 2021 12:15:28 PM CET Hans de Goede wrote:
>> Hi,
>>
>> On 11/18/21 12:08, Rafael J. Wysocki wrote:
>>> On Wed, Nov 17, 2021 at 11:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by
>>>> unused objects") adds a:
>>>>
>>>>         bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused);
>>>>
>>>> call to acpi_scan_init(). On some devices with buggy DSDTs calling
>>>> _PS3 for one device may result in it turning off another device.
>>>
>>> Well, I'm going to revert this commit.  I'm sending a pull request
>>> with the revert later today.
>>>
>>>> Specifically the DSDT of the GPD win and GPD pocket devices has a
>>>> "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module
>>>> which _PS3 method sets a GPIO causing the PCI wifi card to turn off.
>>>>
>>>> I've an earlier, in some ways simpler, fix for this here:
>>>> https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch
>>>>
>>>> But the sdhci-acpi.c MMC host code already has an older workaround
>>>> for it to not toggle power on this broken ACPI object; and this
>>>> simpler fix would require keeping that workaround. So then we would
>>>> have 2 workarounds for the same issue in the kernel.
>>>>
>>>> Thus instead I've come up with a slightly different approach which
>>>> IMHO has ended up pretty well.
>>>>
>>>> Patches 1-3 of this series are this different approach and assuming
>>>> they are considered ok must be merged into 5.16 to fix the regression
>>>> caused by commit c10383e8ddf4 on these devices.
>>>
>>> So I'll have a look at these and if they look good, we can do that
>>> instead of the problematic commit in 5.17.
>>
>> I'm a bit confused now, if the problematic commit is going to get
>> reversed then technically we don't need this series anymore ?
> 
> That's correct.
> 
>> Or are you planning on re-introducing it in some form for 5.17 ?
> 
> I have been considering this.
> 
>> With that said getting this series merged would still be good,
>> patch 1 + 2 make the existing always_present quirk code more generic
>> which might be useful later. And then patch 3 (which is small)
>> allows dropping some ugliness from the sdhci-acpi.c code since
>> the DSDT bug we are hitting will now be solved by the
>> new acpi-dev-status-override mechanism.
> 
> OK, so this would be applicable for 5.17, but a couple of changelogs
> need to be updated if I'm not mistaken.
> 
> Can you please do that and resend the series?

I will update some of the commit messages and send a v2 tomorrow.

Regards,

Hans