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 <[email protected]>
---
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(-)
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 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 <[email protected]>
> ---
> 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)? Maybe moving setting type
below setting integer.value might do the trick as well? ;-)
It'd be useful to be warned by the compiler in such a case. There are much
less useful warnings out there.
Excellent finding indeed.
Reviewed-by: Sakari Ailus <[email protected]>
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: [email protected] # for 6.8 and later
> };
> struct acpi_object_list arg_list = {
--
Kind regards,
Sakari Ailus
> From: Hans de Goede <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Wentong Wu <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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: [email protected] # 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 <[email protected]>
> >> ---
> >> 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 = ¶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.
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 <[email protected]>
> >
> > 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: [email protected] # for 6.8 and later
>
> Ack.
--
Kind regards,
Sakari Ailus