Received-SPF: pass (google.com: domain of linux-kernel+bounces-200306-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 4 Jun 2024 09:07:28 +0000
From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Rafael J . Wysocki" <rafael@kernel.org>,
	Wentong Wu <wentong.wu@intel.com>,
	Tomas Winkler <tomas.winkler@intel.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-acpi@vger.kernel.org, Kate Hsuan <hpa@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mei: vsc: Fix wrong invocation of ACPI SID method
Message-ID: <Zl7ZUOOj12zkDSik@kekkonen.localdomain>
References: <20240603205050.505389-1-hdegoede@redhat.com>
 <Zl42vYs40MJiPxl7@kekkonen.localdomain>
 <77625f15-e183-49e8-8496-b12002cc7dbb@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <77625f15-e183-49e8-8496-b12002cc7dbb@redhat.com>

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.

-- 
Kind regards,

Sakari Ailus