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

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

[Sachi King]

The AMD variant of the Surface Laptop report 0 for their OEM platform
revision.  The Surface devices that require the surfacepro3_button
driver do not have the _DSM that gets the OEM platform revision.  If the
method does not exist, load surfacepro3_button.

Fixes: 64dd243d7356 ("platform/x86: surfacepro3_button: Fix device check")
Co-developed-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Sachi King <nakato@nakato.io>
---
 drivers/platform/surface/surfacepro3_button.c | 30 ++++---------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/surface/surfacepro3_button.c b/drivers/platform/surface/surfacepro3_button.c
index 242fb690dcaf..30eea54dbb47 100644
--- a/drivers/platform/surface/surfacepro3_button.c
+++ b/drivers/platform/surface/surfacepro3_button.c
@@ -149,7 +149,8 @@ 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 for the _DSM method and OEM Platform Revision DSM
+ * function.
  *
  * 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.
@@ -157,30 +158,11 @@ static int surface_button_resume(struct device *dev)
 static bool surface_button_check_MSHW0040(struct acpi_device *dev)
 {
 	acpi_handle handle = dev->handle;
-	union acpi_object *result;
-	u64 oem_platform_rev = 0;	// valid revisions are nonzero
-
-	// get OEM platform revision
-	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.
-	 */
-
-	if (result) {
-		oem_platform_rev = result->integer.value;
-		ACPI_FREE(result);
-	}
-
-	dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
 
-	return oem_platform_rev == 0;
+	// make sure that OEM platform revision DSM call does not exist
+	return !acpi_check_dsm(handle, &MSHW0040_DSM_UUID,
+			       MSHW0040_DSM_REVISION,
+			       BIT(MSHW0040_DSM_GET_OMPR));
 }
 
 
-- 
2.33.0

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

[Andy Shevchenko]

On Tue, Nov 9, 2021 at 10:11 AM Sachi King <nakato@nakato.io> wrote:
>
> The AMD variant of the Surface Laptop report 0 for their OEM platform
> revision.  The Surface devices that require the surfacepro3_button
> driver do not have the _DSM that gets the OEM platform revision.  If the
> method does not exist, load surfacepro3_button.

...

>   * 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 for the _DSM method and OEM Platform Revision DSM
> + * function.

Not sure what this change means (not a native speaker).

...

> -       dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);

I think this is useful to have.

What about leaving it as is for debugging purposes and just replacing
the last test?

...

> +       // make sure that OEM platform revision DSM call does not exist

Please, fix the comment style while at it.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi Sachi,

On 11/9/21 09:11, Sachi King wrote:
> The AMD variant of the Surface Laptop report 0 for their OEM platform
> revision.  The Surface devices that require the surfacepro3_button
> driver do not have the _DSM that gets the OEM platform revision.  If the
> method does not exist, load surfacepro3_button.
> 
> Fixes: 64dd243d7356 ("platform/x86: surfacepro3_button: Fix device check")
> Co-developed-by: Maximilian Luz <luzmaximilian@gmail.com>
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> Signed-off-by: Sachi King <nakato@nakato.io>

Thank you for your patch.

I'm afraid that I do not entirely understand the problem and thus
I also cannot review this patch properly.

Can you please send a new version with a more complete commit message,
explaining things like:

1. On which models this driver currently loads
2. On which models where it loads it should not load
3. The presence / lack of the OEM platform revision _DSM and the
   returned value by the _DSM if present on each of the models on
   which this driver currently loads.

Also I'm wondering, since this is only used on a few
systems, if it would not be better to just use
a dmi_system_id table with a list of systems on which this
should actually load, and also use that list to set the
modalias (using MODULE_DEVICE_TABLE(dmi, ...) ?

That will avoid this module auto-loading at all on devices
where it is not necessary thus speeding up booting a bit
and also not wasting memory by having an unused module sit
in memory.

Note the driver can still bind by the ACPI ids, that is fine
you would just:

1. Add a DMI table + MODULE_DEVICE_TABLE(dmi, ...)
2. Add a DMI check to surface_button_check_MSHW0040()
3. Drop MODULE_DEVICE_TABLE(acpi, surface_button_device_ids);

And then presto, the module will not even load on devices
where it should not be used.

###

2 more requests related to this patch

1. If for the next version you stick with the _DSM presence check
   approach please also add some documentation inside the driver
   in the form of a more elaborate comment.

2. I see this is part of a series for the next version please
   Cc platform-driver-x86@vger.kernel.org for the entire series.

Regards,

Hans



> ---
>  drivers/platform/surface/surfacepro3_button.c | 30 ++++---------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/platform/surface/surfacepro3_button.c b/drivers/platform/surface/surfacepro3_button.c
> index 242fb690dcaf..30eea54dbb47 100644
> --- a/drivers/platform/surface/surfacepro3_button.c
> +++ b/drivers/platform/surface/surfacepro3_button.c
> @@ -149,7 +149,8 @@ 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 for the _DSM method and OEM Platform Revision DSM
> + * function.
>   *
>   * 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.
> @@ -157,30 +158,11 @@ static int surface_button_resume(struct device *dev)
>  static bool surface_button_check_MSHW0040(struct acpi_device *dev)
>  {
>  	acpi_handle handle = dev->handle;
> -	union acpi_object *result;
> -	u64 oem_platform_rev = 0;	// valid revisions are nonzero
> -
> -	// get OEM platform revision
> -	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.
> -	 */
> -
> -	if (result) {
> -		oem_platform_rev = result->integer.value;
> -		ACPI_FREE(result);
> -	}
> -
> -	dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
>  
> -	return oem_platform_rev == 0;
> +	// make sure that OEM platform revision DSM call does not exist
> +	return !acpi_check_dsm(handle, &MSHW0040_DSM_UUID,
> +			       MSHW0040_DSM_REVISION,
> +			       BIT(MSHW0040_DSM_GET_OMPR));
>  }
>  
>  
> 

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

[Dmitry Torokhov]

On Tue, Nov 09, 2021 at 11:12:34AM +0200, Andy Shevchenko wrote:
> On Tue, Nov 9, 2021 at 10:11 AM Sachi King <nakato@nakato.io> wrote:
> >
> > The AMD variant of the Surface Laptop report 0 for their OEM platform
> > revision.  The Surface devices that require the surfacepro3_button
> > driver do not have the _DSM that gets the OEM platform revision.  If the
> > method does not exist, load surfacepro3_button.
> 
> ...
> 
> >   * 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 for the _DSM method and OEM Platform Revision DSM
> > + * function.
> 
> Not sure what this change means (not a native speaker).
> 
> ...
> 
> > -       dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
> 
> I think this is useful to have.
> 
> What about leaving it as is for debugging purposes and just replacing
> the last test?

I agree it is nice to be able to print it for debug purposes, but it has
to be fetched separately, as with the proposed change we are not reading
it.

If I am understanding the change it wants to call acpi_dsm_check()
to verify whether MSHW0040_DSM_GET_OMPR function exists at all (which is
done by reading _DSM MSHW0040_DSM_UUID, revision MSHW0040_DSM_REVISION,
function 0. Only if function 0 indicates that function
MSHW0040_DSM_GET_OMPR is supported in this _DSM, we can read it to get
the real version number, which can be 0.

Treating 0 as an invalid version as it was done in original change is
wrong.

I propose we combine the old and new code, call acpi_dsm_check() and
bail if it returns false, otherwise proceed to calling
acpi_evaluate_dsm_typed() and dev_dbg() the version.

Sachi, are you going to update the patch? You do not need to adjust the
surface driver as Hans is getting rid of it.

Thanks.

-- 
Dmitry

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

[Hans de Goede]

Hi All,

On 3/1/22 22:01, Dmitry Torokhov wrote:
> On Tue, Nov 09, 2021 at 11:12:34AM +0200, Andy Shevchenko wrote:
>> On Tue, Nov 9, 2021 at 10:11 AM Sachi King <nakato@nakato.io> wrote:
>>>
>>> The AMD variant of the Surface Laptop report 0 for their OEM platform
>>> revision.  The Surface devices that require the surfacepro3_button
>>> driver do not have the _DSM that gets the OEM platform revision.  If the
>>> method does not exist, load surfacepro3_button.
>>
>> ...
>>
>>>   * 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 for the _DSM method and OEM Platform Revision DSM
>>> + * function.
>>
>> Not sure what this change means (not a native speaker).
>>
>> ...
>>
>>> -       dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
>>
>> I think this is useful to have.
>>
>> What about leaving it as is for debugging purposes and just replacing
>> the last test?
> 
> I agree it is nice to be able to print it for debug purposes, but it has
> to be fetched separately, as with the proposed change we are not reading
> it.
> 
> If I am understanding the change it wants to call acpi_dsm_check()
> to verify whether MSHW0040_DSM_GET_OMPR function exists at all (which is
> done by reading _DSM MSHW0040_DSM_UUID, revision MSHW0040_DSM_REVISION,
> function 0. Only if function 0 indicates that function
> MSHW0040_DSM_GET_OMPR is supported in this _DSM, we can read it to get
> the real version number, which can be 0.
> 
> Treating 0 as an invalid version as it was done in original change is
> wrong.
> 
> I propose we combine the old and new code, call acpi_dsm_check() and
> bail if it returns false, otherwise proceed to calling
> acpi_evaluate_dsm_typed() and dev_dbg() the version.
> 
> Sachi, are you going to update the patch? You do not need to adjust the
> surface driver as Hans is getting rid of it.

Right, for more info on that see:

https://lore.kernel.org/linux-input/20220224110241.9613-1-hdegoede@redhat.com/

Regards,

Hans