Re: [PATCH] mei: vsc: Fix wrong invocation of ACPI SID method

[Hans de Goede]

Hi,

On 6/3/24 11:33 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Mon, Jun 03, 2024 at 10:50:50PM +0200, Hans de Goede wrote:
>> When using an initializer for a union only one of the union members
>> must be initialized. The initializer for the acpi_object union variable
>> passed as argument to the SID ACPI method was initializing both
>> the type and the integer members of the union.
>>
>> Unfortunately rather then complaining about this gcc simply ignores
>> the first initializer and only used the second integer.value = 1
>> initializer. Leaving type set to 0 which leads to the argument being
>> skipped by acpi acpi_ns_evaluate() resulting in:
>>
>> ACPI Warning: \_SB.PC00.SPI1.SPFD.CVFD.SID: Insufficient arguments -
>> Caller passed 0, method requires 1 (20240322/nsarguments-232)
>>
>> Fix this by initializing only the integer struct part of the union
>> and initializing both members of the integer struct.
>>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> Even though this is a one-liner, figuring out what was actually going
>> wrong here took quite a while.
> 
> I was wondering this with Wentong, too...!
> 
>> ---
>>  drivers/misc/mei/vsc-fw-loader.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/mei/vsc-fw-loader.c b/drivers/misc/mei/vsc-fw-loader.c
>> index ffa4ccd96a10..596a9d695dfc 100644
>> --- a/drivers/misc/mei/vsc-fw-loader.c
>> +++ b/drivers/misc/mei/vsc-fw-loader.c
>> @@ -252,7 +252,7 @@ static int vsc_get_sensor_name(struct vsc_fw_loader *fw_loader,
>>  {
>>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER };
>>  	union acpi_object obj = {
>> -		.type = ACPI_TYPE_INTEGER,
>> +		.integer.type = ACPI_TYPE_INTEGER,
>>  		.integer.value = 1,
> 
> I guess initialising integer.value implies that all integer fields are set
> to zero (and so zeroing type set the line above)?

Yes I was thinking that might be happening too.

> Maybe moving setting type
> below setting integer.value might do the trick as well? ;-)

I was wondering the same thing, but that seems error-prone /
something which may break with different compiler versions.

Actually most code using union acpi_object variables simply
does not initialize them at declaration time.

So I was also considering to maybe change the code like this:

        struct acpi_object_list arg_list;
        union acpi_object *ret_obj;
        union acpi_object param;

	...

        param.type = ACPI_TYPE_INTEGER;
        param.integer.value = 1;

        arg_list.count = 1;
        arg_list.pointer = &param;

        status = acpi_evaluate_object(handle, "SID", &arg_list, &buffer);

Slightly more verbose, but this is basically what all other
callers of acpi_evaluate_object() (with a non NULL arg_list) do.

> It'd be useful to be warned by the compiler in such a case. There are much
> less useful warnings out there.

Ack.

> Excellent finding indeed.
> 
> Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> 
> This could be cc'd to stable, this warning will display for a lot of
> systems. I.e. I think:
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Cc: stable@xxxxxxxxxxxxxxx # for 6.8 and later

Ack.

Regards,

Hans



>>  	};
>>  	struct acpi_object_list arg_list = {
>