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

["Antonio Vázquez Blanco" <antoniovazquezblanco@gmail.com>]

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