Received-SPF: pass (google.com: domain of linux-kernel+bounces-199736-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 3 Jun 2024 21:33:49 +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: <Zl42vYs40MJiPxl7@kekkonen.localdomain>
References: <20240603205050.505389-1-hdegoede@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240603205050.505389-1-hdegoede@redhat.com>

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)? 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 <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

>  	};
>  	struct acpi_object_list arg_list = {

-- 
Kind regards,

Sakari Ailus