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 = ¶m;
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 = {
>
