mei: vsc: Fix wrong invocation of ACPI SID method

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@redhat.com>
---
Even though this is a one-liner, figuring out what was actually going
wrong here took quite a while.
---
 drivers/misc/mei/vsc-fw-loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

> From: Hans de Goede <hdegoede@redhat.com>
> 
> 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@redhat.com>

Reviewed-by: Wentong Wu <wentong.wu@intel.com>

> ---
> Even though this is a one-liner, figuring out what was actually going wrong
> here took quite a while.

Thanks a lot

BR,
Wentong
> ---
>  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,
>  	};
>  	struct acpi_object_list arg_list = {
> --
> 2.45.1

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@redhat.com>
>> ---
>> 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@linux.intel.com>
> 
> 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@vger.kernel.org # for 6.8 and later

Ack.

Regards,

Hans



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

Hi Hans,

On Tue, Jun 04, 2024 at 11:00:10AM +0200, Hans de Goede wrote:
> 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@redhat.com>
> >> ---
> >> 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.

Yes, I didn't actually mean to suggest this.

> 
> 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.

I prefer your current patch actually, it's good as-is.

> 
> > 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@linux.intel.com>
> > 
> > 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@vger.kernel.org # for 6.8 and later
> 
> Ack.