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@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.
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 <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.
--
Kind regards,
Sakari Ailus
