Re: [PATCH] platform/x86: intel: int1092: intel_sar: fix _DSM argument4 type mismatch issue

[Hans de Goede]

Hi Ivan,

On 8/15/23 12:24, Ivan Hu wrote:
> Encountered a type mismatch as described below:
> \_SB.WCCD._DSM: Argument #4 type mismatch - Found [Integer], ACPI requires
> [Package]
> This is because the argument#4(arg3) is integer.
> According to the ACPI specification, the arg3 should be a package.
> _DSM (Device Specific Method)
> This optional object is a control method that enables devices to provide device
> specific control functions that are consumed by the device driver.
> Arguments: (4)
> Arg0 - A Buffer containing a UUID
> Arg1 - An Integer containing the Revision ID
> Arg2 - An Integer containing the Function Index
> Arg3 - A Package that contains function-specific arguments
> 
> The solution involves rectifying arg3 to be a package for the _DSM method.
> Furthermore, the firmware needs to ensure that ACPI table arg3 is a package as
> well. The suggested amendment is as follows:
> If ((Arg3 == Zero))
> {
>     WDMC [0x02] = WCS0
> }
> should modify as,
> If (((ToInteger(Derefof (Arg3 [Zero]))) == Zero))
> {
>     WDMC [0x02] = WCS0
> }
> 
> Signed-off-by: Ivan Hu <ivan.hu@xxxxxxxxxxxxx>

Thank you for your patch.

As you have mentioned in your commit message in order for this to
NOT break things the ACPI tables on *all* laptops using the
intel_sar driver would need to be updated to match.

Which is never going to happen, so merging this patch would
break intel_sar functionality on all existing laptop models
and likely also on future models since presumably Windows
will keep passing an Integer and this is what the ACPI tables
will keep expecting.

The "Argument #4 type mismatch - Found [Integer], ACPI requires [Package]"
messages is just warning and it is not that unusual for devices
to have a _DSM which goes against the ACPI specification
and instead e.g. directly expects an Integer as Arg3.

In this case we MUST always pass what the ACPI tables expect,
so that things *actually* work and we will just have to
live with the warning thrown by ACPICA.

NACK for this patch because we don't break working things
on purpose. We never ever break things on purpose but especially
not just to silence a warning in dmesg.

If you really want to get rid of these warnings then I suggest
you write a patch lowering the ACPI log message to a debug level.

Regards,

Hans





> ---
>  drivers/platform/x86/intel/int1092/intel_sar.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int1092/intel_sar.c b/drivers/platform/x86/intel/int1092/intel_sar.c
> index 6246c066ade2..8fffdce994aa 100644
> --- a/drivers/platform/x86/intel/int1092/intel_sar.c
> +++ b/drivers/platform/x86/intel/int1092/intel_sar.c
> @@ -215,13 +215,17 @@ static void sar_notify(acpi_handle handle, u32 event, void *data)
>  
>  static void sar_get_data(int reg, struct wwan_sar_context *context)
>  {
> -	union acpi_object *out, req;
> +	union acpi_object *out, req, argv4;
>  	u32 rev = 0;
>  
> -	req.type = ACPI_TYPE_INTEGER;
> +	argv4.type = ACPI_TYPE_PACKAGE;
> +	argv4.package.count = 1;
> +	argv4.package.elements = &req;
> +	req.integer.type = ACPI_TYPE_INTEGER;
>  	req.integer.value = reg;
> +
>  	out = acpi_evaluate_dsm_typed(context->handle, &context->guid, rev,
> -				      COMMAND_ID_CONFIG_TABLE, &req, ACPI_TYPE_PACKAGE);
> +				      COMMAND_ID_CONFIG_TABLE, &argv4, ACPI_TYPE_PACKAGE);
>  	if (!out)
>  		return;
>  	if (out->package.count >= 3 &&