[PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[Hans de Goede]

Hi All,

Some apple laptop models have an ACPI device with a HID of APP000B
and that device has an IO resource (so it does not describe the new
unsupported MMIO based gmux type), but there actually is no gmux
in the laptop at all.

This patch-series improves the drivers/acpi/video_detect.c so that
it no longer tries to use the non present gmux in this case.

Note I'm still waiting for testing feedback from the reporter of
this problem. But from the logs the problem is clear
(the logs show: "apple_gmux: gmux device not present") and
the detection code is not changed, just moved so these patches
should be fine, but they can definitely use a good review.

Aditya, can you perhaps test this on a model macbook which does
actually use the apple-gmux driver for backlight control
(assuming you have such a model) ?

Rafael, since this ultimately fixes a drivers/acpi/video_detect.c
bug I think it is best if you take this entire series including
the platform/x86 changes.

Regards,

Hans


Hans de Goede (3):
  platform/x86: apple-gmux: Move port defines to apple-gmux.h
  platform/x86: apple-gmux: Add apple_gmux_detect() helper
  ACPI: video: Fix apple gmux detection

 drivers/acpi/video_detect.c       |  24 +------
 drivers/platform/x86/apple-gmux.c |  93 +++++--------------------
 include/linux/apple-gmux.h        | 108 +++++++++++++++++++++++++++++-
 3 files changed, 127 insertions(+), 98 deletions(-)

-- 
2.39.0

[PATCH 1/3] platform/x86: apple-gmux: Move port defines to apple-gmux.h

[Hans de Goede]

This is a preparation patch for adding a new static inline
apple_gmux_detect() helper which actually checks a supported
gmux is present, rather then only checking an ACPI device with
the HID is there as apple_gmux_present() does.

Fixes: 21245df307cb ("ACPI: video: Add Apple GMUX brightness control detection")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/apple-gmux.c | 23 -----------------------
 include/linux/apple-gmux.h        | 23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index ca33df7ea550..a0af01f6a0fd 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -64,29 +64,6 @@ struct apple_gmux_data {
 
 static struct apple_gmux_data *apple_gmux_data;
 
-/*
- * gmux port offsets. Many of these are not yet used, but may be in the
- * future, and it's useful to have them documented here anyhow.
- */
-#define GMUX_PORT_VERSION_MAJOR		0x04
-#define GMUX_PORT_VERSION_MINOR		0x05
-#define GMUX_PORT_VERSION_RELEASE	0x06
-#define GMUX_PORT_SWITCH_DISPLAY	0x10
-#define GMUX_PORT_SWITCH_GET_DISPLAY	0x11
-#define GMUX_PORT_INTERRUPT_ENABLE	0x14
-#define GMUX_PORT_INTERRUPT_STATUS	0x16
-#define GMUX_PORT_SWITCH_DDC		0x28
-#define GMUX_PORT_SWITCH_EXTERNAL	0x40
-#define GMUX_PORT_SWITCH_GET_EXTERNAL	0x41
-#define GMUX_PORT_DISCRETE_POWER	0x50
-#define GMUX_PORT_MAX_BRIGHTNESS	0x70
-#define GMUX_PORT_BRIGHTNESS		0x74
-#define GMUX_PORT_VALUE			0xc2
-#define GMUX_PORT_READ			0xd0
-#define GMUX_PORT_WRITE			0xd4
-
-#define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
-
 #define GMUX_INTERRUPT_ENABLE		0xff
 #define GMUX_INTERRUPT_DISABLE		0x00
 
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index ddb10aa67b14..80efaaf89e07 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -11,6 +11,29 @@
 
 #define GMUX_ACPI_HID "APP000B"
 
+/*
+ * gmux port offsets. Many of these are not yet used, but may be in the
+ * future, and it's useful to have them documented here anyhow.
+ */
+#define GMUX_PORT_VERSION_MAJOR		0x04
+#define GMUX_PORT_VERSION_MINOR		0x05
+#define GMUX_PORT_VERSION_RELEASE	0x06
+#define GMUX_PORT_SWITCH_DISPLAY	0x10
+#define GMUX_PORT_SWITCH_GET_DISPLAY	0x11
+#define GMUX_PORT_INTERRUPT_ENABLE	0x14
+#define GMUX_PORT_INTERRUPT_STATUS	0x16
+#define GMUX_PORT_SWITCH_DDC		0x28
+#define GMUX_PORT_SWITCH_EXTERNAL	0x40
+#define GMUX_PORT_SWITCH_GET_EXTERNAL	0x41
+#define GMUX_PORT_DISCRETE_POWER	0x50
+#define GMUX_PORT_MAX_BRIGHTNESS	0x70
+#define GMUX_PORT_BRIGHTNESS		0x74
+#define GMUX_PORT_VALUE			0xc2
+#define GMUX_PORT_READ			0xd0
+#define GMUX_PORT_WRITE			0xd4
+
+#define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
+
 #if IS_ENABLED(CONFIG_APPLE_GMUX)
 
 /**
-- 
2.39.0

[PATCH 2/3] platform/x86: apple-gmux: Add apple_gmux_detect() helper

[Hans de Goede]

Add a new (static inline) apple_gmux_detect() helper to apple-gmux.h
which can be used for gmux detection instead of apple_gmux_present().

The latter is not really reliable since an ACPI device with a HID
of APP000B is present on some devices without a gmux at all, as well
as on devices with a newer (unsupported) MMIO based gmux model.

This causes apple_gmux_present() to return false-postives on
a number of different Apple laptop models.

This new helper uses the same probing as the actual apple-gmux
driver, so that it does not return false positives.

To avoid code duplication the gmux_probe() function of the actual
driver is also moved over to using the new apple_gmux_detect() helper.

Fixes: 21245df307cb ("ACPI: video: Add Apple GMUX brightness control detection")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/apple-gmux.c | 70 +++++++------------------
 include/linux/apple-gmux.h        | 85 ++++++++++++++++++++++++++++++-
 2 files changed, 101 insertions(+), 54 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index a0af01f6a0fd..9333f82cfa8a 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -226,23 +226,6 @@ static void gmux_write32(struct apple_gmux_data *gmux_data, int port,
 		gmux_pio_write32(gmux_data, port, val);
 }
 
-static bool gmux_is_indexed(struct apple_gmux_data *gmux_data)
-{
-	u16 val;
-
-	outb(0xaa, gmux_data->iostart + 0xcc);
-	outb(0x55, gmux_data->iostart + 0xcd);
-	outb(0x00, gmux_data->iostart + 0xce);
-
-	val = inb(gmux_data->iostart + 0xcc) |
-		(inb(gmux_data->iostart + 0xcd) << 8);
-
-	if (val == 0x55aa)
-		return true;
-
-	return false;
-}
-
 /**
  * DOC: Backlight control
  *
@@ -582,60 +565,43 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	int ret = -ENXIO;
 	acpi_status status;
 	unsigned long long gpe;
+	bool indexed = false;
+	u32 version;
 
 	if (apple_gmux_data)
 		return -EBUSY;
 
+	if (!apple_gmux_detect(pnp, &indexed)) {
+		pr_info("gmux device not present\n");
+		return -ENODEV;
+	}
+
 	gmux_data = kzalloc(sizeof(*gmux_data), GFP_KERNEL);
 	if (!gmux_data)
 		return -ENOMEM;
 	pnp_set_drvdata(pnp, gmux_data);
 
 	res = pnp_get_resource(pnp, IORESOURCE_IO, 0);
-	if (!res) {
-		pr_err("Failed to find gmux I/O resource\n");
-		goto err_free;
-	}
-
 	gmux_data->iostart = res->start;
 	gmux_data->iolen = resource_size(res);
 
-	if (gmux_data->iolen < GMUX_MIN_IO_LEN) {
-		pr_err("gmux I/O region too small (%lu < %u)\n",
-		       gmux_data->iolen, GMUX_MIN_IO_LEN);
-		goto err_free;
-	}
-
 	if (!request_region(gmux_data->iostart, gmux_data->iolen,
 			    "Apple gmux")) {
 		pr_err("gmux I/O already in use\n");
 		goto err_free;
 	}
 
-	/*
-	 * Invalid version information may indicate either that the gmux
-	 * device isn't present or that it's a new one that uses indexed
-	 * io
-	 */
-
-	ver_major = gmux_read8(gmux_data, GMUX_PORT_VERSION_MAJOR);
-	ver_minor = gmux_read8(gmux_data, GMUX_PORT_VERSION_MINOR);
-	ver_release = gmux_read8(gmux_data, GMUX_PORT_VERSION_RELEASE);
-	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
-		if (gmux_is_indexed(gmux_data)) {
-			u32 version;
-			mutex_init(&gmux_data->index_lock);
-			gmux_data->indexed = true;
-			version = gmux_read32(gmux_data,
-				GMUX_PORT_VERSION_MAJOR);
-			ver_major = (version >> 24) & 0xff;
-			ver_minor = (version >> 16) & 0xff;
-			ver_release = (version >> 8) & 0xff;
-		} else {
-			pr_info("gmux device not present\n");
-			ret = -ENODEV;
-			goto err_release;
-		}
+	if (indexed) {
+		mutex_init(&gmux_data->index_lock);
+		gmux_data->indexed = true;
+		version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
+		ver_major = (version >> 24) & 0xff;
+		ver_minor = (version >> 16) & 0xff;
+		ver_release = (version >> 8) & 0xff;
+	} else {
+		ver_major = gmux_read8(gmux_data, GMUX_PORT_VERSION_MAJOR);
+		ver_minor = gmux_read8(gmux_data, GMUX_PORT_VERSION_MINOR);
+		ver_release = gmux_read8(gmux_data, GMUX_PORT_VERSION_RELEASE);
 	}
 	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor,
 		ver_release, (gmux_data->indexed ? "indexed" : "classic"));
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index 80efaaf89e07..47c04e7e3672 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -8,6 +8,8 @@
 #define LINUX_APPLE_GMUX_H
 
 #include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/pnp.h>
 
 #define GMUX_ACPI_HID "APP000B"
 
@@ -35,14 +37,88 @@
 #define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
 
 #if IS_ENABLED(CONFIG_APPLE_GMUX)
+static inline bool apple_gmux_is_indexed(unsigned long iostart)
+{
+	u16 val;
+
+	outb(0xaa, iostart + 0xcc);
+	outb(0x55, iostart + 0xcd);
+	outb(0x00, iostart + 0xce);
+
+	val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8);
+	if (val == 0x55aa)
+		return true;
+
+	return false;
+}
 
 /**
- * apple_gmux_present() - detect if gmux is built into the machine
+ * apple_gmux_detect() - detect if gmux is built into the machine
+ *
+ * @pnp_dev:     Device to probe or NULL to use the first matching device
+ * @indexed_ret: Returns (by reference) if the gmux is indexed or not
+ *
+ * Detect if a supported gmux device is present by actually probing it.
+ * This avoids the false positives returned on some models by
+ * apple_gmux_present().
+ *
+ * Return: %true if a supported gmux ACPI device is detected and the kernel
+ * was configured with CONFIG_APPLE_GMUX, %false otherwise.
+ */
+static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
+{
+	u8 ver_major, ver_minor, ver_release;
+	struct resource *res;
+	bool indexed = false;
+
+	if (!pnp_dev) {
+		struct acpi_device *adev;
+		struct device *dev;
+
+		adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
+		if (!adev)
+			return false;
+
+		dev = acpi_get_first_physical_node(adev);
+		if (!dev)
+			return false;
+
+		pnp_dev = to_pnp_dev(dev);
+	}
+
+	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
+	if (!res)
+		return false;
+
+	if (resource_size(res) < GMUX_MIN_IO_LEN)
+		return false;
+
+	/*
+	 * Invalid version information may indicate either that the gmux
+	 * device isn't present or that it's a new one that uses indexed io.
+	 */
+	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
+	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
+	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
+	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
+		indexed = apple_gmux_is_indexed(res->start);
+		if (!indexed)
+			return false;
+	}
+
+	if (indexed_ret)
+		*indexed_ret = indexed;
+
+	return true;
+}
+
+/**
+ * apple_gmux_present() - check if gmux ACPI device is present
  *
  * Drivers may use this to activate quirks specific to dual GPU MacBook Pros
  * and Mac Pros, e.g. for deferred probing, runtime pm and backlight.
  *
- * Return: %true if gmux is present and the kernel was configured
+ * Return: %true if gmux ACPI device is present and the kernel was configured
  * with CONFIG_APPLE_GMUX, %false otherwise.
  */
 static inline bool apple_gmux_present(void)
@@ -57,6 +133,11 @@ static inline bool apple_gmux_present(void)
 	return false;
 }
 
+static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
+{
+	return false;
+}
+
 #endif /* !CONFIG_APPLE_GMUX */
 
 #endif /* LINUX_APPLE_GMUX_H */
-- 
2.39.0

[PATCH 3/3] ACPI: video: Fix apple gmux detection

[Hans de Goede]

Some apple laptop models have an ACPI device with a HID of APP000B
and that device has an IO resource (so it does not describe the new
unsupported MMIO based gmux type), but there actually is no gmux
in the laptop at all.

The gmux_probe() function of the actual apple-gmux driver has code
to detect this, this code has been factored out into a new
apple_gmux_detect() helper in apple-gmux.h.

Use this new function to fix acpi_video_get_backlight_type() wrongly
returning apple_gmux as type on these new laptops.

Fixes: 21245df307cb ("ACPI: video: Add Apple GMUX brightness control detection")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/video_detect.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 64eab35037c3..a8c02608dde4 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -110,26 +110,6 @@ static bool nvidia_wmi_ec_supported(void)
 }
 #endif
 
-static bool apple_gmux_backlight_present(void)
-{
-	struct acpi_device *adev;
-	struct device *dev;
-
-	adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
-	if (!adev)
-		return false;
-
-	dev = acpi_get_first_physical_node(adev);
-	if (!dev)
-		return false;
-
-	/*
-	 * drivers/platform/x86/apple-gmux.c only supports old style
-	 * Apple GMUX with an IO-resource.
-	 */
-	return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL;
-}
-
 /* Force to use vendor driver when the ACPI device is known to be
  * buggy */
 static int video_detect_force_vendor(const struct dmi_system_id *d)
@@ -791,6 +771,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
 {
 	static DEFINE_MUTEX(init_mutex);
 	static bool nvidia_wmi_ec_present;
+	static bool apple_gmux_present;
 	static bool native_available;
 	static bool init_done;
 	static long video_caps;
@@ -804,6 +785,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
 				    ACPI_UINT32_MAX, find_video, NULL,
 				    &video_caps, NULL);
 		nvidia_wmi_ec_present = nvidia_wmi_ec_supported();
+		apple_gmux_present = apple_gmux_detect(NULL, NULL);
 		init_done = true;
 	}
 	if (native)
@@ -825,7 +807,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
 	if (nvidia_wmi_ec_present)
 		return acpi_backlight_nvidia_wmi_ec;
 
-	if (apple_gmux_backlight_present())
+	if (apple_gmux_present)
 		return acpi_backlight_apple_gmux;
 
 	/* Use ACPI video if available, except when native should be preferred. */
-- 
2.39.0

Re: [PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[Lukas Wunner]

On Mon, Jan 23, 2023 at 12:37:47PM +0100, Hans de Goede wrote:
> Some apple laptop models have an ACPI device with a HID of APP000B
> and that device has an IO resource (so it does not describe the new
> unsupported MMIO based gmux type), but there actually is no gmux
> in the laptop at all.
> 
> This patch-series improves the drivers/acpi/video_detect.c so that
> it no longer tries to use the non present gmux in this case.
> 
> Note I'm still waiting for testing feedback from the reporter of
> this problem. But from the logs the problem is clear
> (the logs show: "apple_gmux: gmux device not present")

Please provide a link to the original report.  I would also like to
know the exact MacBook model used and I would like to see full dmesg
output as well as an acpidump.

What you're saying here is that there's a fake APP000B device present
in DSDT and I have a hard time believng that.  We need to find out
what's really going on and how macOS handles this.

Thanks,

Lukas

Re: [PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[Hans de Goede]

Hi,

On 1/23/23 13:09, Lukas Wunner wrote:
> On Mon, Jan 23, 2023 at 12:37:47PM +0100, Hans de Goede wrote:
>> Some apple laptop models have an ACPI device with a HID of APP000B
>> and that device has an IO resource (so it does not describe the new
>> unsupported MMIO based gmux type), but there actually is no gmux
>> in the laptop at all.
>>
>> This patch-series improves the drivers/acpi/video_detect.c so that
>> it no longer tries to use the non present gmux in this case.
>>
>> Note I'm still waiting for testing feedback from the reporter of
>> this problem. But from the logs the problem is clear
>> (the logs show: "apple_gmux: gmux device not present")
> 
> Please provide a link to the original report.  I would also like to
> know the exact MacBook model used and I would like to see full dmesg
> output as well as an acpidump.

I only have a report by private email. This does include full dmesg
output and an acpidump. I will forward this to you in a private
email.

The reporter describes their model as a macbookpro8,1.

> What you're saying here is that there's a fake APP000B device present
> in DSDT

Yes that is exactly what I'm saying.

> and I have a hard time believng that.

Yes that seems to be exactly what is happening. If you look at
the drivers/platform/x86/apple-gmux.c code that is pretty much
the only scenario which leads to the:

apple_gmux: gmux device not present

message getting logged; and it seems to me that the reason that
check is there likely is exactly because of such machines
actually existing.

And /sys/class/backlight contains only intel_backlight
suggesting that this is not a hybrid gfx machine. Which also
matches with the specs which I can find for the  macbookpro8,1.

Yet there is an APP000B device present in the DSDT, here is
the relevant bit of the DSDT:

                Device (GMUX)
                {
                    Name (_HID, EisaId ("APP000B"))  // _HID: Hardware ID
                    Name (_CID, "gmux")  // _CID: Compatible ID
                    Name (_STA, 0x0B)  // _STA: Status
                    Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
                    {
                        IO (Decode16,
                            0x0700,             // Range Minimum
                            0x07FF,             // Range Maximum
                            0x01,               // Alignment
                            0xFF,               // Length
                            )
                    })
                    Name (_PRW, Package (0x02)  // _PRW: Power Resources for Wake
                    {
                        0x16, 
                        0x03
                    })
                    Scope (\_GPE)
                    {
                        Method (_L16, 0, NotSerialized)  // _Lxx: Level-Triggered GPE, xx=0x00-0xFF
                        {
                            Notify (\_SB.PCI0.LPCB.GMUX, 0x80) // Status Change
                        }
                    }

                    Name (GMGP, 0x16)
                    Method (GMSP, 1, NotSerialized)
                    {
                        If ((Arg0 <= 0x01))
                        {
                            GP06 |= Arg0
                        }
                    }

                    Method (GMLV, 0, NotSerialized)
                    {
                        Return (GP06) /* \GP06 */
                    }
                }
            }

Regards,

Hans

Re: [PATCH 2/3] platform/x86: apple-gmux: Add apple_gmux_detect() helper

[Lukas Wunner]

On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote:
> --- a/include/linux/apple-gmux.h
> +++ b/include/linux/apple-gmux.h
[...]
> +static inline bool apple_gmux_is_indexed(unsigned long iostart)
> +{
> +	u16 val;
> +
> +	outb(0xaa, iostart + 0xcc);
> +	outb(0x55, iostart + 0xcd);
> +	outb(0x00, iostart + 0xce);
> +
> +	val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8);
> +	if (val == 0x55aa)
> +		return true;
> +
> +	return false;
> +}

Something like this, and especially the large apple_gmux_detect() below,
should not live in a header file.

Why can't apple_gmux.ko just export a detection function which is used
both internally and as a helper by the backlight detection?

Thanks,

Lukas

>  
>  /**
> - * apple_gmux_present() - detect if gmux is built into the machine
> + * apple_gmux_detect() - detect if gmux is built into the machine
> + *
> + * @pnp_dev:     Device to probe or NULL to use the first matching device
> + * @indexed_ret: Returns (by reference) if the gmux is indexed or not
> + *
> + * Detect if a supported gmux device is present by actually probing it.
> + * This avoids the false positives returned on some models by
> + * apple_gmux_present().
> + *
> + * Return: %true if a supported gmux ACPI device is detected and the kernel
> + * was configured with CONFIG_APPLE_GMUX, %false otherwise.
> + */
> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
> +{
> +	u8 ver_major, ver_minor, ver_release;
> +	struct resource *res;
> +	bool indexed = false;
> +
> +	if (!pnp_dev) {
> +		struct acpi_device *adev;
> +		struct device *dev;
> +
> +		adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
> +		if (!adev)
> +			return false;
> +
> +		dev = acpi_get_first_physical_node(adev);
> +		if (!dev)
> +			return false;
> +
> +		pnp_dev = to_pnp_dev(dev);
> +	}
> +
> +	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
> +	if (!res)
> +		return false;
> +
> +	if (resource_size(res) < GMUX_MIN_IO_LEN)
> +		return false;
> +
> +	/*
> +	 * Invalid version information may indicate either that the gmux
> +	 * device isn't present or that it's a new one that uses indexed io.
> +	 */
> +	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> +	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> +	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> +	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
> +		indexed = apple_gmux_is_indexed(res->start);
> +		if (!indexed)
> +			return false;
> +	}
> +
> +	if (indexed_ret)
> +		*indexed_ret = indexed;
> +
> +	return true;
> +}
> +
> +/**
> + * apple_gmux_present() - check if gmux ACPI device is present
>   *
>   * Drivers may use this to activate quirks specific to dual GPU MacBook Pros
>   * and Mac Pros, e.g. for deferred probing, runtime pm and backlight.
>   *
> - * Return: %true if gmux is present and the kernel was configured
> + * Return: %true if gmux ACPI device is present and the kernel was configured
>   * with CONFIG_APPLE_GMUX, %false otherwise.
>   */
>  static inline bool apple_gmux_present(void)
> @@ -57,6 +133,11 @@ static inline bool apple_gmux_present(void)
>  	return false;
>  }
>  
> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
> +{
> +	return false;
> +}
> +
>  #endif /* !CONFIG_APPLE_GMUX */
>  
>  #endif /* LINUX_APPLE_GMUX_H */
> -- 
> 2.39.0

Re: [PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[Lukas Wunner]

On Mon, Jan 23, 2023 at 01:38:37PM +0100, Hans de Goede wrote:
> On 1/23/23 13:09, Lukas Wunner wrote:
> > On Mon, Jan 23, 2023 at 12:37:47PM +0100, Hans de Goede wrote:
> > > Some apple laptop models have an ACPI device with a HID of APP000B
> > > and that device has an IO resource (so it does not describe the new
> > > unsupported MMIO based gmux type), but there actually is no gmux
> > > in the laptop at all.
> > >
> > > This patch-series improves the drivers/acpi/video_detect.c so that
> > > it no longer tries to use the non present gmux in this case.
> > >
> > > Note I'm still waiting for testing feedback from the reporter of
> > > this problem. But from the logs the problem is clear
> > > (the logs show: "apple_gmux: gmux device not present")
> > 
> > Please provide a link to the original report.  I would also like to
> > know the exact MacBook model used and I would like to see full dmesg
> > output as well as an acpidump.
> 
> I only have a report by private email. This does include full dmesg
> output and an acpidump. I will forward this to you in a private
> email.
> 
> The reporter describes their model as a macbookpro8,1.
> 
> > What you're saying here is that there's a fake APP000B device present
> > in DSDT
> 
> Yes that is exactly what I'm saying.

That's a 2011 13" MacBook Pro which indeed does not have dual GPUs.

I searched for other affected models and this seems to be more common
than I thought:

MacBookPro5,4
https://pastebin.com/8Xjq7RhS

MacBookPro8,1
https://linux-hardware.org/?probe=e513cfbadb&log=dmesg

MacBookPro9,2
https://bugzilla.kernel.org/attachment.cgi?id=278961

MacBookPro10,2
https://lkml.org/lkml/2014/9/22/657

MacBookPro11,2
https://forums.fedora-fr.org/viewtopic.php?id=70142

MacBookPro11,4
https://raw.githubusercontent.com/im-0/investigate-card-reader-suspend-problem-on-mbp11.4/master/test-16/dmesg

These are 13" and 15" models from the pre-retina and retina era
(2009 - 2015).  None of them have dual GPUs.  (Only a subset of
the 15" and 17" models had dual GPUs.)  Apple sloppily included
a GMUX device on all of them and it wasn't a problem so far
because the gmux driver detects non-presence and bails out,
but it throws off the new backlight algorithm.

This is really sad. :(

Please add a Reported-by to your commits as well as the list I've
provided above so that we've got a complete record in the git history.

Thanks,

Lukas

Re: [PATCH 2/3] platform/x86: apple-gmux: Add apple_gmux_detect() helper

[Hans de Goede]

Hi,

On 1/23/23 14:49, Lukas Wunner wrote:
> On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote:
>> --- a/include/linux/apple-gmux.h
>> +++ b/include/linux/apple-gmux.h
> [...]
>> +static inline bool apple_gmux_is_indexed(unsigned long iostart)
>> +{
>> +	u16 val;
>> +
>> +	outb(0xaa, iostart + 0xcc);
>> +	outb(0x55, iostart + 0xcd);
>> +	outb(0x00, iostart + 0xce);
>> +
>> +	val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8);
>> +	if (val == 0x55aa)
>> +		return true;
>> +
>> +	return false;
>> +}
> 
> Something like this, and especially the large apple_gmux_detect() below,
> should not live in a header file.

I understand where you are coming from, but these functions really
are not that large.

> Why can't apple_gmux.ko just export a detection function which is used
> both internally and as a helper by the backlight detection?

Both the acpi-video code and the apple-gmux code can be built as
modules. So this will break if the acpi-video code get builtin
and the apple-gmux code does not.

This can be worked around in Kconfig by adding something like:

	depends on APPLE_GMUX || APPLE_GMUX=n

to the ACPI_VIDEO Kconfig bits and then cross our fingers
we don't get some Kconfig circular dep thing causing things
to error out.

The whole idea behind the video-detect.c code is that it does
as little as possible to figure out which backlight control
method to use. It e.g. on purpose does not depend on
the GPU drivers to see if native GPU backlight control is
available that would bring in a whole lot of dependencies.

So the do not depend on other kernel-modules / driver-code
is part of the design of video-detect.c (in so far as it
was ever designed, since it also very much has organically
grown / evolved into its current code).

If we forgo this design principle then we evt would end
adding similar Kconfig snippets  for each backlight device-type
which the video-detect code supports this quickly gets unwieldly.

And doing this also means that video.ko now starts depending
on not just apple-gmux.ko but also on its dependencies, although
in this case that would not bring in any extra dependencies.
But for ohter types there might very well be significant
dependencies.

So waying the cons and pros here, as well as trying to be
consistent and not add dependencies on other kernel-modules
just for detection purposes, I would prefer to keep using
the static inline approach for this.

Regards,

Hans



>>  
>>  /**
>> - * apple_gmux_present() - detect if gmux is built into the machine
>> + * apple_gmux_detect() - detect if gmux is built into the machine
>> + *
>> + * @pnp_dev:     Device to probe or NULL to use the first matching device
>> + * @indexed_ret: Returns (by reference) if the gmux is indexed or not
>> + *
>> + * Detect if a supported gmux device is present by actually probing it.
>> + * This avoids the false positives returned on some models by
>> + * apple_gmux_present().
>> + *
>> + * Return: %true if a supported gmux ACPI device is detected and the kernel
>> + * was configured with CONFIG_APPLE_GMUX, %false otherwise.
>> + */
>> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
>> +{
>> +	u8 ver_major, ver_minor, ver_release;
>> +	struct resource *res;
>> +	bool indexed = false;
>> +
>> +	if (!pnp_dev) {
>> +		struct acpi_device *adev;
>> +		struct device *dev;
>> +
>> +		adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
>> +		if (!adev)
>> +			return false;
>> +
>> +		dev = acpi_get_first_physical_node(adev);
>> +		if (!dev)
>> +			return false;
>> +
>> +		pnp_dev = to_pnp_dev(dev);
>> +	}
>> +
>> +	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
>> +	if (!res)
>> +		return false;
>> +
>> +	if (resource_size(res) < GMUX_MIN_IO_LEN)
>> +		return false;
>> +
>> +	/*
>> +	 * Invalid version information may indicate either that the gmux
>> +	 * device isn't present or that it's a new one that uses indexed io.
>> +	 */
>> +	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
>> +	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
>> +	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
>> +	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
>> +		indexed = apple_gmux_is_indexed(res->start);
>> +		if (!indexed)
>> +			return false;
>> +	}
>> +
>> +	if (indexed_ret)
>> +		*indexed_ret = indexed;
>> +
>> +	return true;
>> +}
>> +
>> +/**
>> + * apple_gmux_present() - check if gmux ACPI device is present
>>   *
>>   * Drivers may use this to activate quirks specific to dual GPU MacBook Pros
>>   * and Mac Pros, e.g. for deferred probing, runtime pm and backlight.
>>   *
>> - * Return: %true if gmux is present and the kernel was configured
>> + * Return: %true if gmux ACPI device is present and the kernel was configured
>>   * with CONFIG_APPLE_GMUX, %false otherwise.
>>   */
>>  static inline bool apple_gmux_present(void)
>> @@ -57,6 +133,11 @@ static inline bool apple_gmux_present(void)
>>  	return false;
>>  }
>>  
>> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
>> +{
>> +	return false;
>> +}
>> +
>>  #endif /* !CONFIG_APPLE_GMUX */
>>  
>>  #endif /* LINUX_APPLE_GMUX_H */
>> -- 
>> 2.39.0
> 

Re: [PATCH 2/3] platform/x86: apple-gmux: Add apple_gmux_detect() helper

[Lukas Wunner]

On Mon, Jan 23, 2023 at 03:13:28PM +0100, Hans de Goede wrote:
> On 1/23/23 14:49, Lukas Wunner wrote:
> > On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote:
> > > --- a/include/linux/apple-gmux.h
> > > +++ b/include/linux/apple-gmux.h
> > [...]
> > > +static inline bool apple_gmux_is_indexed(unsigned long iostart)
> > > +{
> > > +	u16 val;
> > > +
> > > +	outb(0xaa, iostart + 0xcc);
> > > +	outb(0x55, iostart + 0xcd);
> > > +	outb(0x00, iostart + 0xce);
> > > +
> > > +	val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8);
> > > +	if (val == 0x55aa)
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > 
> > Something like this, and especially the large apple_gmux_detect() below,
> > should not live in a header file.
> 
> I understand where you are coming from, but these functions really
> are not that large.
> 
> > Why can't apple_gmux.ko just export a detection function which is used
> > both internally and as a helper by the backlight detection?
> 
> Both the acpi-video code and the apple-gmux code can be built as
> modules. So this will break if the acpi-video code get builtin
> and the apple-gmux code does not.
> 
> This can be worked around in Kconfig by adding something like:
> 
> 	depends on APPLE_GMUX || APPLE_GMUX=n
> 
> to the ACPI_VIDEO Kconfig bits and then cross our fingers
> we don't get some Kconfig circular dep thing causing things
> to error out.

Can we force APPLE_GMUX to be built-in if ACPI_VIDEO is?

Would making APPLE_GMUX depend on ACPI_VIDEO (instead of
"ACPI_VIDEO=n || ACPI_VIDEO") achieve that?  I believe
APPLE_GMUX would then inherit the setting of ACPI_VIDEO?

Thanks,

Lukas

Re: [PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[Aditya Garg]

> On 23-Jan-2023, at 5:07 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> Hi All,
> 
> Some apple laptop models have an ACPI device with a HID of APP000B
> and that device has an IO resource (so it does not describe the new
> unsupported MMIO based gmux type), but there actually is no gmux
> in the laptop at all.
> 
> This patch-series improves the drivers/acpi/video_detect.c so that
> it no longer tries to use the non present gmux in this case.
> 
> Note I'm still waiting for testing feedback from the reporter of
> this problem. But from the logs the problem is clear
> (the logs show: "apple_gmux: gmux device not present") and
> the detection code is not changed, just moved so these patches
> should be fine, but they can definitely use a good review.
> 
> Aditya, can you perhaps test this on a model macbook which does
> actually use the apple-gmux driver for backlight control
> (assuming you have such a model) ?
> 

Hi Hans

Since I own a T2 MacBook, I’ll have to apply some out of tree patches as well from https://github.com/Redecorating/linux/commits/bits/010-gmux. I’ve requested the maintainer although he believes they shouldn't break anything on T2 Macs. I’ve still let a kernel compile without the out of tree patches, just with yours and shall send the outputs by tomorrow.

Re: [PATCH 2/3] platform/x86: apple-gmux: Add apple_gmux_detect() helper

[Hans de Goede]

Hi,

On 1/23/23 15:23, Lukas Wunner wrote:
> On Mon, Jan 23, 2023 at 03:13:28PM +0100, Hans de Goede wrote:
>> On 1/23/23 14:49, Lukas Wunner wrote:
>>> On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote:
>>>> --- a/include/linux/apple-gmux.h
>>>> +++ b/include/linux/apple-gmux.h
>>> [...]
>>>> +static inline bool apple_gmux_is_indexed(unsigned long iostart)
>>>> +{
>>>> +	u16 val;
>>>> +
>>>> +	outb(0xaa, iostart + 0xcc);
>>>> +	outb(0x55, iostart + 0xcd);
>>>> +	outb(0x00, iostart + 0xce);
>>>> +
>>>> +	val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8);
>>>> +	if (val == 0x55aa)
>>>> +		return true;
>>>> +
>>>> +	return false;
>>>> +}
>>>
>>> Something like this, and especially the large apple_gmux_detect() below,
>>> should not live in a header file.
>>
>> I understand where you are coming from, but these functions really
>> are not that large.
>>
>>> Why can't apple_gmux.ko just export a detection function which is used
>>> both internally and as a helper by the backlight detection?
>>
>> Both the acpi-video code and the apple-gmux code can be built as
>> modules. So this will break if the acpi-video code get builtin
>> and the apple-gmux code does not.
>>
>> This can be worked around in Kconfig by adding something like:
>>
>> 	depends on APPLE_GMUX || APPLE_GMUX=n
>>
>> to the ACPI_VIDEO Kconfig bits and then cross our fingers
>> we don't get some Kconfig circular dep thing causing things
>> to error out.
> 
> Can we force APPLE_GMUX to be built-in if ACPI_VIDEO is?
> 
> Would making APPLE_GMUX depend on ACPI_VIDEO (instead of
> "ACPI_VIDEO=n || ACPI_VIDEO") achieve that?  I believe
> APPLE_GMUX would then inherit the setting of ACPI_VIDEO?

I'm afraid that won't work, make it depend on ACPI_VIDEO would not
make it inherit ACPI_VIDEO's setting instead it would be limited
to ACPI_VIDEO's setting.

So if we make APPLE_GMUX "depends on ACPI_VIDEO" and ACPI_VIDEO=y
then APPLE_GMUX can be both Y or M, where as if ACPI_VIDEO=m
then APPLE_GMUX can only be m.

Note that the APPLE_GMUX Kconfig "ACPI_VIDEO=n || ACPI_VIDEO"
bit is obsolete and should be dropped (I have already prepared
a patch for this), the apple_gmux code no longer depends on any
of the ACPI_VIDEO symbols.  Which does make it possible to
add a dependency the other way.

I just tried the following:

--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -210,6 +210,8 @@ config ACPI_VIDEO
 	depends on BACKLIGHT_CLASS_DEVICE
 	depends on INPUT
 	depends on ACPI_WMI || !X86
+	# ACPI_VIDEO uses symbols from APPLE_GMUX if that is enabled
+	depends on APPLE_GMUX || APPLE_GMUX=n
 	select THERMAL
 	help
 	  This driver implements the ACPI Extensions For Display Adapters

And that does not cause any circular dep issues it seems, so 
If we really want to have the detection code inside apple_gmux then
we could use the above and have the acpi-video code depend on
apple_gmux.ko.  I'm not a fan of that though, as mentioned before
the intent for the acpi-video code's detection parts is to be
as much standalone code as possible.

Regards,

Hans

Re: [PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[Hans de Goede]

Hi,

On 1/23/23 14:58, Lukas Wunner wrote:
> On Mon, Jan 23, 2023 at 01:38:37PM +0100, Hans de Goede wrote:
>> On 1/23/23 13:09, Lukas Wunner wrote:
>>> On Mon, Jan 23, 2023 at 12:37:47PM +0100, Hans de Goede wrote:
>>>> Some apple laptop models have an ACPI device with a HID of APP000B
>>>> and that device has an IO resource (so it does not describe the new
>>>> unsupported MMIO based gmux type), but there actually is no gmux
>>>> in the laptop at all.
>>>>
>>>> This patch-series improves the drivers/acpi/video_detect.c so that
>>>> it no longer tries to use the non present gmux in this case.
>>>>
>>>> Note I'm still waiting for testing feedback from the reporter of
>>>> this problem. But from the logs the problem is clear
>>>> (the logs show: "apple_gmux: gmux device not present")
>>>
>>> Please provide a link to the original report.  I would also like to
>>> know the exact MacBook model used and I would like to see full dmesg
>>> output as well as an acpidump.
>>
>> I only have a report by private email. This does include full dmesg
>> output and an acpidump. I will forward this to you in a private
>> email.
>>
>> The reporter describes their model as a macbookpro8,1.
>>
>>> What you're saying here is that there's a fake APP000B device present
>>> in DSDT
>>
>> Yes that is exactly what I'm saying.
> 
> That's a 2011 13" MacBook Pro which indeed does not have dual GPUs.
> 
> I searched for other affected models and this seems to be more common
> than I thought:
> 
> MacBookPro5,4
> https://pastebin.com/8Xjq7RhS
> 
> MacBookPro8,1
> https://linux-hardware.org/?probe=e513cfbadb&log=dmesg
> 
> MacBookPro9,2
> https://bugzilla.kernel.org/attachment.cgi?id=278961
> 
> MacBookPro10,2
> https://lkml.org/lkml/2014/9/22/657
> 
> MacBookPro11,2
> https://forums.fedora-fr.org/viewtopic.php?id=70142
> 
> MacBookPro11,4
> https://raw.githubusercontent.com/im-0/investigate-card-reader-suspend-problem-on-mbp11.4/master/test-16/dmesg
> 
> These are 13" and 15" models from the pre-retina and retina era
> (2009 - 2015).  None of them have dual GPUs.  (Only a subset of
> the 15" and 17" models had dual GPUs.)  Apple sloppily included
> a GMUX device on all of them and it wasn't a problem so far
> because the gmux driver detects non-presence and bails out,
> but it throws off the new backlight algorithm.
> 
> This is really sad. :(
> 
> Please add a Reported-by to your commits

I was about to say that Emmanouil may want to keep their email
private. But I see you've already added them to the Cc, so
now the email is part of the platform-driver-x86 archives.

Emmanouil, is it ok if I add a line like this:

Reported-by: Emmanouil Kouroupakis <kartxxx@gmail.com>

to the commit message of v2 of the patches ? This gives you credit
for reporting the bug, but it also exposes your email address
in public places.

> as well as the list I've
> provided above so that we've got a complete record in the git history.

Ack, I'll add the list of devices to v2 of the patches.

Regards,

Hans

Re: [PATCH 2/3] platform/x86: apple-gmux: Add apple_gmux_detect() helper

[Hans de Goede]

Hi,

On 1/23/23 16:05, Hans de Goede wrote:
> Hi,
> 
> On 1/23/23 15:23, Lukas Wunner wrote:
>> On Mon, Jan 23, 2023 at 03:13:28PM +0100, Hans de Goede wrote:
>>> On 1/23/23 14:49, Lukas Wunner wrote:
>>>> On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote:
>>>>> --- a/include/linux/apple-gmux.h
>>>>> +++ b/include/linux/apple-gmux.h
>>>> [...]
>>>>> +static inline bool apple_gmux_is_indexed(unsigned long iostart)
>>>>> +{
>>>>> +	u16 val;
>>>>> +
>>>>> +	outb(0xaa, iostart + 0xcc);
>>>>> +	outb(0x55, iostart + 0xcd);
>>>>> +	outb(0x00, iostart + 0xce);
>>>>> +
>>>>> +	val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8);
>>>>> +	if (val == 0x55aa)
>>>>> +		return true;
>>>>> +
>>>>> +	return false;
>>>>> +}
>>>>
>>>> Something like this, and especially the large apple_gmux_detect() below,
>>>> should not live in a header file.
>>>
>>> I understand where you are coming from, but these functions really
>>> are not that large.
>>>
>>>> Why can't apple_gmux.ko just export a detection function which is used
>>>> both internally and as a helper by the backlight detection?
>>>
>>> Both the acpi-video code and the apple-gmux code can be built as
>>> modules. So this will break if the acpi-video code get builtin
>>> and the apple-gmux code does not.
>>>
>>> This can be worked around in Kconfig by adding something like:
>>>
>>> 	depends on APPLE_GMUX || APPLE_GMUX=n
>>>
>>> to the ACPI_VIDEO Kconfig bits and then cross our fingers
>>> we don't get some Kconfig circular dep thing causing things
>>> to error out.
>>
>> Can we force APPLE_GMUX to be built-in if ACPI_VIDEO is?
>>
>> Would making APPLE_GMUX depend on ACPI_VIDEO (instead of
>> "ACPI_VIDEO=n || ACPI_VIDEO") achieve that?  I believe
>> APPLE_GMUX would then inherit the setting of ACPI_VIDEO?
> 
> I'm afraid that won't work, make it depend on ACPI_VIDEO would not
> make it inherit ACPI_VIDEO's setting instead it would be limited
> to ACPI_VIDEO's setting.
> 
> So if we make APPLE_GMUX "depends on ACPI_VIDEO" and ACPI_VIDEO=y
> then APPLE_GMUX can be both Y or M, where as if ACPI_VIDEO=m
> then APPLE_GMUX can only be m.
> 
> Note that the APPLE_GMUX Kconfig "ACPI_VIDEO=n || ACPI_VIDEO"
> bit is obsolete and should be dropped (I have already prepared
> a patch for this), the apple_gmux code no longer depends on any
> of the ACPI_VIDEO symbols.  Which does make it possible to
> add a dependency the other way.
> 
> I just tried the following:
> 
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -210,6 +210,8 @@ config ACPI_VIDEO
>  	depends on BACKLIGHT_CLASS_DEVICE
>  	depends on INPUT
>  	depends on ACPI_WMI || !X86
> +	# ACPI_VIDEO uses symbols from APPLE_GMUX if that is enabled
> +	depends on APPLE_GMUX || APPLE_GMUX=n
>  	select THERMAL
>  	help
>  	  This driver implements the ACPI Extensions For Display Adapters
> 
> And that does not cause any circular dep issues it seems

And 10 seconds after hitting send I realized that things are not this simple,
because a bunch of other Kconfig bits do "select ACPI_VIDEO" and Kconfig
bits doing "select FOO" much make sure all the dependencies of FOO are met
and we have just added a new dependency...

So all those other places then would need something similar. Kconfig is
great, but with intra dependencies it really can get quite hairy.

So IMHO the (really not that big) static inline detect function really
is the best solution here.

Regards,

Hans

Re: [PATCH 2/3] platform/x86: apple-gmux: Add apple_gmux_detect() helper

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add a new (static inline) apple_gmux_detect() helper to apple-gmux.h
> which can be used for gmux detection instead of apple_gmux_present().
>
> The latter is not really reliable since an ACPI device with a HID
> of APP000B is present on some devices without a gmux at all, as well
> as on devices with a newer (unsupported) MMIO based gmux model.
>
> This causes apple_gmux_present() to return false-postives on

positives

> a number of different Apple laptop models.
>
> This new helper uses the same probing as the actual apple-gmux
> driver, so that it does not return false positives.
>
> To avoid code duplication the gmux_probe() function of the actual
> driver is also moved over to using the new apple_gmux_detect() helper.

...

> +       if (!apple_gmux_detect(pnp, &indexed)) {
> +               pr_info("gmux device not present\n");

You may start using dev_info(&pnp->dev, ...) if I'm not mistaken.

> +               return -ENODEV;
> +       }

...

> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
> +{
> +       u8 ver_major, ver_minor, ver_release;
> +       struct resource *res;
> +       bool indexed = false;
> +
> +       if (!pnp_dev) {
> +               struct acpi_device *adev;
> +               struct device *dev;
> +
> +               adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
> +               if (!adev)
> +                       return false;
> +
> +               dev = acpi_get_first_physical_node(adev);
> +               if (!dev)

I remember I saw something like this in your tree(?). I hope it's not
pending upstream (yet) because of a leak here. Don't forget to call
acpi_dev_put() after you finish with adev. Recently I have fixed a
bunch of similar issues in ASoC Intel.

> +                       return false;
> +
> +               pnp_dev = to_pnp_dev(dev);
> +       }
> +
> +       res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
> +       if (!res)
> +               return false;
> +
> +       if (resource_size(res) < GMUX_MIN_IO_LEN)
> +               return false;
> +
> +       /*
> +        * Invalid version information may indicate either that the gmux
> +        * device isn't present or that it's a new one that uses indexed io.
> +        */
> +       ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> +       ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> +       ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> +       if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
> +               indexed = apple_gmux_is_indexed(res->start);
> +               if (!indexed)
> +                       return false;
> +       }
> +
> +       if (indexed_ret)
> +               *indexed_ret = indexed;
> +
> +       return true;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] ACPI: video: Fix apple gmux detection

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Some apple laptop models have an ACPI device with a HID of APP000B
> and that device has an IO resource (so it does not describe the new
> unsupported MMIO based gmux type), but there actually is no gmux
> in the laptop at all.
>
> The gmux_probe() function of the actual apple-gmux driver has code
> to detect this, this code has been factored out into a new
> apple_gmux_detect() helper in apple-gmux.h.
>
> Use this new function to fix acpi_video_get_backlight_type() wrongly
> returning apple_gmux as type on these new laptops.

...

> -static bool apple_gmux_backlight_present(void)
> -{
> -       struct acpi_device *adev;
> -       struct device *dev;
> -
> -       adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
> -       if (!adev)
> -               return false;
> -
> -       dev = acpi_get_first_physical_node(adev);
> -       if (!dev)
> -               return false;
> -
> -       /*
> -        * drivers/platform/x86/apple-gmux.c only supports old style
> -        * Apple GMUX with an IO-resource.
> -        */
> -       return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL;
> -}

Oh, the leak is already somewhere... Lemme cook a patch...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] ACPI: video: Fix apple gmux detection

[Hans de Goede]

Hi Andy,

On 1/23/23 17:37, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Some apple laptop models have an ACPI device with a HID of APP000B
>> and that device has an IO resource (so it does not describe the new
>> unsupported MMIO based gmux type), but there actually is no gmux
>> in the laptop at all.
>>
>> The gmux_probe() function of the actual apple-gmux driver has code
>> to detect this, this code has been factored out into a new
>> apple_gmux_detect() helper in apple-gmux.h.
>>
>> Use this new function to fix acpi_video_get_backlight_type() wrongly
>> returning apple_gmux as type on these new laptops.
> 
> ...
> 
>> -static bool apple_gmux_backlight_present(void)
>> -{
>> -       struct acpi_device *adev;
>> -       struct device *dev;
>> -
>> -       adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
>> -       if (!adev)
>> -               return false;
>> -
>> -       dev = acpi_get_first_physical_node(adev);
>> -       if (!dev)
>> -               return false;
>> -
>> -       /*
>> -        * drivers/platform/x86/apple-gmux.c only supports old style
>> -        * Apple GMUX with an IO-resource.
>> -        */
>> -       return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL;
>> -}
> 
> Oh, the leak is already somewhere... Lemme cook a patch...

This code is very likely going away, with a Fixes tag
for the original commit which introduced it.

So IMHO it is best to let the leak be (I will fix it
in the replacement in this patch-set).

This patch-set fixes an actual regression and is intended
for stable. So also having a leak fix "flying around" is
only going to confuse things.

Regards,

Hans

Re: [PATCH 3/3] ACPI: video: Fix apple gmux detection

[Rafael J. Wysocki]

On Mon, Jan 23, 2023 at 12:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Some apple laptop models have an ACPI device with a HID of APP000B
> and that device has an IO resource (so it does not describe the new
> unsupported MMIO based gmux type), but there actually is no gmux
> in the laptop at all.
>
> The gmux_probe() function of the actual apple-gmux driver has code
> to detect this, this code has been factored out into a new
> apple_gmux_detect() helper in apple-gmux.h.
>
> Use this new function to fix acpi_video_get_backlight_type() wrongly
> returning apple_gmux as type on these new laptops.
>
> Fixes: 21245df307cb ("ACPI: video: Add Apple GMUX brightness control detection")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Please feel free to add

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

to this one and route it through platform/x86, thanks!

> ---
>  drivers/acpi/video_detect.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index 64eab35037c3..a8c02608dde4 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -110,26 +110,6 @@ static bool nvidia_wmi_ec_supported(void)
>  }
>  #endif
>
> -static bool apple_gmux_backlight_present(void)
> -{
> -       struct acpi_device *adev;
> -       struct device *dev;
> -
> -       adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
> -       if (!adev)
> -               return false;
> -
> -       dev = acpi_get_first_physical_node(adev);
> -       if (!dev)
> -               return false;
> -
> -       /*
> -        * drivers/platform/x86/apple-gmux.c only supports old style
> -        * Apple GMUX with an IO-resource.
> -        */
> -       return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL;
> -}
> -
>  /* Force to use vendor driver when the ACPI device is known to be
>   * buggy */
>  static int video_detect_force_vendor(const struct dmi_system_id *d)
> @@ -791,6 +771,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
>  {
>         static DEFINE_MUTEX(init_mutex);
>         static bool nvidia_wmi_ec_present;
> +       static bool apple_gmux_present;
>         static bool native_available;
>         static bool init_done;
>         static long video_caps;
> @@ -804,6 +785,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
>                                     ACPI_UINT32_MAX, find_video, NULL,
>                                     &video_caps, NULL);
>                 nvidia_wmi_ec_present = nvidia_wmi_ec_supported();
> +               apple_gmux_present = apple_gmux_detect(NULL, NULL);
>                 init_done = true;
>         }
>         if (native)
> @@ -825,7 +807,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
>         if (nvidia_wmi_ec_present)
>                 return acpi_backlight_nvidia_wmi_ec;
>
> -       if (apple_gmux_backlight_present())
> +       if (apple_gmux_present)
>                 return acpi_backlight_apple_gmux;
>
>         /* Use ACPI video if available, except when native should be preferred. */
> --
> 2.39.0
>

Re: [PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[Orlando Chamberlain]

> > Aditya, can you perhaps test this on a model macbook which does
> > actually use the apple-gmux driver for backlight control
> > (assuming you have such a model) ?
> > 
>
> Hi Hans
>
> Since I own a T2 MacBook, I’ll have to apply some out of tree patches as well
> from https://github.com/Redecorating/linux/commits/bits/010 gmux. I’ve
> requested the maintainer although he believes they shouldn't break anything on
> T2 Macs. I’ve still let a kernel compile without the out of tree patches, just
> with yours and shall send the outputs by tomorrow.

I'm the one that wrote those patches for gmux on the mmio T2 macs, and
I've rebased them onto Hans' patchset with patch numbers 2004-2006 here
https://github.com/t2linux/linux-t2-patches/tree/gmux

It seems to work fine on my T2 Macbookpro16,1, the display backlight
controls work and there aren't any other, not working, backlights
present.

> ls /sys/class/backlight
gmux_backlight@

Re: [PATCH 0/3] ACPI: video/apple-gmux: Improve apple-gmux backlight detection

[Aditya Garg]

> I'm the one that wrote those patches for gmux on the mmio T2 macs, and
> I've rebased them onto Hans' patchset with patch numbers 2004-2006 here
> https://github.com/t2linux/linux-t2-patches/tree/gmux
> 
> It seems to work fine on my T2 Macbookpro16,1, the display backlight
> controls work and there aren't any other, not working, backlights
> present.
> 
>> ls /sys/class/backlight
> gmux_backlight@

Thanks Orlando

Since I own the same Mac as Orlando, his testing should be good enough.