[PATCH v2 0/2] Fix MSHW0040 button driver selection on AMD Surface devices

[PATCH v2 0/2] Fix MSHW0040 button driver selection on AMD Surface devices

[Antonio Vázquez Blanco]

This is a rebase and rework of Sachi King's series from November 2021 [1].
It was reviewed then, the review was never acted on, and the thread went
quiet in March 2022. Sachi has confirmed they are happy for it to be
carried and no longer have the hardware. Both patches keep their original
authorship and sign-offs.

The problem, restated, because Hans asked for this in [1] and never got a
clear answer:

The AMD variants of the Surface Laptop 3 and 4 use the same MSHW0040 ACPI
device ID as the Intel Surface devices from the 5th generation on. Both
soc_button_array and surfacepro3_button match that ID and tell the groups
apart inside probe, by evaluating function 2 of the MSHW0040 _DSM, the OEM
Platform Revision, and treating a returned 0 as proof that the method is
absent. The AMD variants implement that function and legitimately return 0,
so both drivers draw the wrong conclusion, and surfacepro3_button binds
where soc_button_array should.

The sound test is whether the device implements that function at all, which
function 0 of the same _DSM advertises and acpi_check_dsm() reports.

To answer the question of which models are affected, every ACPI dump in
linux-surface/acpidumps [2] carrying MSHW0040 was surveyed, 32 models, for
UUID 6fd05c69-cde3-49f4-95ed-ab1665498035 revision 1:

  Model                        fn 0 mask fn 2 advertised
  Surface Book 1               0x03      no
  Surface Book 2               0x0d      yes
  Surface Book 3               0x0f      yes
  Surface Laptop 1             0x05      yes
  Surface Laptop 2             0x05      yes
  Surface Laptop 3 AMD         0x0f      yes
  Surface Laptop 3 Intel       0x0f      yes
  Surface Laptop 4 AMD         0x0f      yes
  Surface Laptop 4 Intel       0x0f      yes
  Surface Laptop 5             0x0f      yes
  Surface Laptop 6             0x0f      yes
  Surface Laptop 7 Qualcomm    0x0f      yes
  Surface Laptop Go 1          0x0f      yes
  Surface Laptop Go 2          0x0f      yes
  Surface Laptop Go 3          0x0f      yes
  Surface Laptop SE            0x0d      yes
  Surface Laptop Studio 1      0x0f      yes
  Surface Laptop Studio 2      0x0f      yes
  Surface Pro 10 Business      0x0f      yes
  Surface Pro 11 Qualcomm      0x0f      yes
  Surface Pro 4                no _DSM   no
  Surface Pro 5                0x0f      yes
  Surface Pro 6                0x0f      yes
  Surface Pro 7                0x0f      yes
  Surface Pro 7 Plus           0x0f      yes
  Surface Pro 8                0x0f      yes
  Surface Pro 9 5G             0x0f      yes
  Surface Pro 9 Intel          0x0f      yes
  Surface Pro X SQ1            0x0f      yes
  Surface Pro X SQ2            0x0f      yes
  Surface Studio 2             0x0d      yes
  Windows Dev Kit 2023         0x0f      yes
Exactly two do not advertise function 2, and they are exactly the two that
need surfacepro3_button:

  - Surface Pro 4: its MSHW0040 device carries no _DSM at all, only _HID
    and _STA.
  - Surface Book 1: it does carry the _DSM with this UUID, but function 0
    returns {0x03,0,0,0}, advertising functions 0 and 1 only. Function 1
    there is dGPU base attach and detach handling.

Worth noting that v1 said the devices needing surfacepro3_button "do not
have the _DSM". That is not true of the Surface Book 1, and the inaccuracy
is probably why the problem was hard to follow. The commit messages now
describe what is actually tested.

Changes in v2:
 - Dmitry: a returned 0 is no longer treated as invalid. acpi_check_dsm()
   makes the decision, and the revision is still evaluated and dev_dbg()'d
   afterwards, instead of that evaluation being removed.
 - Andy: the dev_dbg() of the OEM Platform Revision is kept, and the new
   comments use kernel comment style.
 - Hans: the commit messages now state which models load which driver and
   what each one's _DSM advertises, and the in-driver comments are
   expanded.
 - Rebased onto v7.4-rc2: surface_button_check_MSHW0040() now takes
   (struct device *, acpi_handle) and the driver has become a platform
   driver.

Not done, and I would welcome guidance. Hans also suggested replacing the
ACPI match with a DMI table plus MODULE_DEVICE_TABLE(dmi, ...), so that
surfacepro3_button stops auto-loading on machines that do not need it. That
is a larger change, independent of this fix, and the survey above suggests
the _DSM test is reliable across the whole range. That may be a good
follow-up if it is still wanted.

On merging: please do not separate the two patches. Both drivers match
MSHW0040 and discriminate only in probe, so with only one of them applied,
which driver claims the device on the AMD variants depends on module load
order. Hans asked for the same arrangement for his own two patch series in
2022, so either tree with an ack from the other would work.

On testing. Tested on a Surface Pro 4, which is the "no _DSM" side of the
check. Its own DSDT agrees with the survey above: the MSHW0040 device
carries only _HID and _STA, with no _DSM at all. With both patches applied,
MSHW0040 still binds surfacepro3_button, soc_button_array still declines,
exactly one input device is registered, and the volume keys still work. No
"OEM Platform Revision" line appears, which is the correct result here: on
this model both drivers return before reaching that evaluation.

That is a regression check, not a confirmation of the fix. On a Pro 4, v1,
v2 and current mainline all take the same path, because the _DSM is simply
absent and acpi_check_dsm() returns false. The Surface Book 1 is the case
that separates them and I do not have one.

Worth knowing, though: the out of tree linux-surface kernel has carried v1
of both patches since 2021-10-05 and still ships them today on its 6.19,
7.2 and 7.3 branches. This logic has therefore been in continuous use on
Surface hardware, including the AMD Laptops it fixes, for about four years.
The Pro 4 above had been running it before this test.

Neither I nor Sachi has an AMD Surface Laptop, so the half this actually
fixes is still untested by us directly. If anyone with an AMD Surface
Laptop 3 or 4 can test it, that would be very welcome.

[1] https://lore.kernel.org/all/20211109081125.41410-1-nakato@nakato.io/
[2] https://github.com/linux-surface/acpidumps

Sachi King (2):
  Input: soc_button_array - support AMD variant Surface devices
  platform/surface: surfacepro3_button: don't load on amd variant

 drivers/input/misc/soc_button_array.c         | 31 ++++++++++---------
 drivers/platform/surface/surfacepro3_button.c | 31 ++++++++++---------
 2 files changed, 34 insertions(+), 28 deletions(-)


base-commit: 8a65162514fbad21a2c58ffed65f42b27e92b163
-- 
2.56.0

[PATCH v2 1/2] Input: soc_button_array - support AMD variant Surface devices

[Antonio Vázquez Blanco]

From: Sachi King <nakato@nakato.io>

The power button on the AMD variants of the Surface Laptop 3 and 4 uses
the same MSHW0040 device ID as the 5th and later generations of Surface
devices, so soc_button_array matches it. The device check then rejects
it, and surfacepro3_button binds instead, which is the wrong driver.

soc_device_check_MSHW0040() evaluates function 2 of the MSHW0040 _DSM,
the OEM Platform Revision, and treats a returned 0 as proof that the
_DSM is not there. That is wrong: 0 is a valid revision. The AMD
variants implement the function and return 0, so the check rejects
exactly the devices it is meant to accept.

What separates the two groups is whether the device implements that
function at all, which function 0 of the same _DSM advertises. Of the 32
Surface models carrying MSHW0040 in the ACPI dumps at [1]:

  fn 0 mask   fn 2 advertised   models
  no _DSM     no                Surface Pro 4
  0x03        no                Surface Book 1
  0x05        yes               Surface Laptop 1, 2
  0x0d        yes               Surface Book 2, Laptop SE, Studio 2
  0x0f        yes               the remaining 25

Only two models do not advertise it, and they are exactly the two that
need surfacepro3_button: the Surface Pro 4, whose MSHW0040 device
carries no _DSM at all, and the Surface Book 1, whose _DSM advertises
functions 0 and 1 only, function 1 there being dGPU base attach
handling.

Use acpi_check_dsm() to make the decision, and keep evaluating the
revision afterwards so that it remains available through dev_dbg().

[1] https://github.com/linux-surface/acpidumps

Fixes: c394159310d0 ("Input: soc_button_array - add support for newer surface devices")
Link: https://lore.kernel.org/all/20211109081125.41410-1-nakato@nakato.io/
Co-developed-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Sachi King <nakato@nakato.io>
Co-developed-by: Antonio Vázquez Blanco <antoniovazquezblanco@gmail.com>
Signed-off-by: Antonio Vázquez Blanco <antoniovazquezblanco@gmail.com>
---
 drivers/input/misc/soc_button_array.c | 31 +++++++++++++++------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index dbf86ea778e6..75946e18cc80 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -546,9 +546,17 @@ static const struct soc_device_data soc_device_MSHW0028 = {
  * Special device check for Surface Book 2 and Surface Pro (2017).
  * Both, the Surface Pro 4 (surfacepro3_button.c) and the above mentioned
  * devices use MSHW0040 for power and volume buttons, however the way they
- * have to be addressed differs. Make sure that we only load this drivers
- * for the correct devices by checking the OEM Platform Revision provided by
- * the _DSM method.
+ * have to be addressed differs. Make sure that we only load this driver for
+ * the correct devices.
+ *
+ * The OEM Platform Revision _DSM function tells them apart, but its return
+ * value cannot: it is a revision number and 0 is a valid one, which the AMD
+ * variants of the Surface Laptop 3 and 4 report while still needing this
+ * driver. Check whether the function is implemented at all instead, which
+ * function 0 of the same _DSM advertises. The devices that need
+ * surfacepro3_button.c do not advertise it: the Surface Pro 4 has no _DSM on
+ * this device, and the Surface Book 1 has one that advertises functions 0
+ * and 1 only.
  */
 #define MSHW0040_DSM_REVISION		0x01
 #define MSHW0040_DSM_GET_OMPR		0x02	// get OEM Platform Revision
@@ -560,27 +568,22 @@ static int soc_device_check_MSHW0040(struct device *dev)
 {
 	acpi_handle handle = ACPI_HANDLE(dev);
 	union acpi_object *result;
-	u64 oem_platform_rev = 0;	// valid revisions are nonzero
+	u64 oem_platform_rev = 0;
+
+	if (!acpi_check_dsm(handle, &MSHW0040_DSM_UUID, MSHW0040_DSM_REVISION,
+			    BIT(MSHW0040_DSM_GET_OMPR)))
+		return -ENODEV;
 
-	// get OEM platform revision
+	/* Fetch the revision separately, it is only used for debugging. */
 	result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
 					 MSHW0040_DSM_REVISION,
 					 MSHW0040_DSM_GET_OMPR, NULL,
 					 ACPI_TYPE_INTEGER);
-
 	if (result) {
 		oem_platform_rev = result->integer.value;
 		ACPI_FREE(result);
 	}
 
-	/*
-	 * If the revision is zero here, the _DSM evaluation has failed. This
-	 * indicates that we have a Pro 4 or Book 1 and this driver should not
-	 * be used.
-	 */
-	if (oem_platform_rev == 0)
-		return -ENODEV;
-
 	dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev);
 
 	return 0;
-- 
2.56.0

[PATCH v2 2/2] platform/surface: surfacepro3_button: don't load on amd variant

[Antonio Vázquez Blanco]

From: Sachi King <nakato@nakato.io>

surfacepro3_button binds to MSHW0040 on the AMD variants of the Surface
Laptop 3 and 4, where soc_button_array is the right driver.

surface_button_check_MSHW0040() evaluates function 2 of the MSHW0040
_DSM, the OEM Platform Revision, and binds when the result is 0,
assuming that only a failed evaluation yields 0. The AMD variants
implement that function and legitimately return 0, so this driver binds
there as well.

Decide instead on whether the device implements the function at all,
which function 0 of the same _DSM advertises and acpi_check_dsm()
reports. Of the 32 Surface models carrying MSHW0040 in the ACPI dumps at
[1], only two do not advertise it, and they are exactly the two this
driver is for: the Surface Pro 4, whose MSHW0040 device carries no _DSM,
and the Surface Book 1, whose _DSM advertises functions 0 and 1 only.

The driver currently reaches the right answer on the Surface Book 1 by
accident rather than by that test. Function 2 is not implemented there,
so the _DSM falls through to a trailing Return (Buffer), and
acpi_evaluate_dsm_typed() rejects the type and returns NULL rather than
an integer. The revision is left at 0 and the driver binds.

Keep evaluating the revision when it is advertised, so that it remains
available through dev_dbg().

[1] https://github.com/linux-surface/acpidumps

Fixes: 64dd243d7356 ("platform/x86: surfacepro3_button: Fix device check")
Link: https://lore.kernel.org/all/20211109081125.41410-1-nakato@nakato.io/
Co-developed-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Sachi King <nakato@nakato.io>
Co-developed-by: Antonio Vázquez Blanco <antoniovazquezblanco@gmail.com>
Signed-off-by: Antonio Vázquez Blanco <antoniovazquezblanco@gmail.com>
---
 drivers/platform/surface/surfacepro3_button.c | 31 ++++++++++---------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/surface/surfacepro3_button.c b/drivers/platform/surface/surfacepro3_button.c
index b38aa010053e..2d95a8ef3e23 100644
--- a/drivers/platform/surface/surfacepro3_button.c
+++ b/drivers/platform/surface/surfacepro3_button.c
@@ -148,7 +148,14 @@ static int surface_button_resume(struct device *dev)
 /*
  * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device
  * ID (MSHW0040) for the power/volume buttons. Make sure this is the right
- * device by checking for the _DSM method and OEM Platform Revision.
+ * device by checking whether the OEM Platform Revision _DSM function is
+ * implemented, which function 0 of that _DSM advertises.
+ *
+ * The returned revision cannot be used for this: 0 is a valid revision, and
+ * the AMD variants of the Surface Laptop 3 and 4 report it while needing
+ * soc_button_array instead. The devices handled here do not advertise the
+ * function at all: the Surface Pro 4 has no _DSM on this device, and the
+ * Surface Book 1 has one that advertises functions 0 and 1 only.
  *
  * Returns true if the driver should bind to this device, i.e. the device is
  * either MSWH0028 (Pro 3) or MSHW0040 on a Pro 4 or Book 1.
@@ -156,21 +163,17 @@ static int surface_button_resume(struct device *dev)
 static bool surface_button_check_MSHW0040(struct device *dev, acpi_handle handle)
 {
 	union acpi_object *result;
-	u64 oem_platform_rev = 0;	// valid revisions are nonzero
+	u64 oem_platform_rev = 0;
+
+	if (!acpi_check_dsm(handle, &MSHW0040_DSM_UUID, MSHW0040_DSM_REVISION,
+			    BIT(MSHW0040_DSM_GET_OMPR)))
+		return true;
 
-	// get OEM platform revision
+	/* Fetch the revision separately, it is only used for debugging. */
 	result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
 					 MSHW0040_DSM_REVISION,
-					 MSHW0040_DSM_GET_OMPR,
-					 NULL, ACPI_TYPE_INTEGER);
-
-	/*
-	 * If evaluating the _DSM fails, the method is not present. This means
-	 * that we have either MSHW0028 or MSHW0040 on Pro 4 or Book 1, so we
-	 * should use this driver. We use revision 0 indicating it is
-	 * unavailable.
-	 */
-
+					 MSHW0040_DSM_GET_OMPR, NULL,
+					 ACPI_TYPE_INTEGER);
 	if (result) {
 		oem_platform_rev = result->integer.value;
 		ACPI_FREE(result);
@@ -178,7 +181,7 @@ static bool surface_button_check_MSHW0040(struct device *dev, acpi_handle handle
 
 	dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev);
 
-	return oem_platform_rev == 0;
+	return false;
 }
 
 
-- 
2.56.0