Received-SPF: pass (google.com: domain of linux-kernel+bounces-200299-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <77625f15-e183-49e8-8496-b12002cc7dbb@redhat.com>
Date: Tue, 4 Jun 2024 11:00:10 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mei: vsc: Fix wrong invocation of ACPI SID method
To: Sakari Ailus <sakari.ailus@linux.intel.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
References: <20240603205050.505389-1-hdegoede@redhat.com>
 <Zl42vYs40MJiPxl7@kekkonen.localdomain>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <Zl42vYs40MJiPxl7@kekkonen.localdomain>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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 = {
>