Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH -next] platform/surface: remove status assignment without
 reading
To:     Zou Wei <zou_wei@huawei.com>, hdegoede@redhat.com,
        mgross@linux.intel.com
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>
References: <1603972048-64271-1-git-send-email-zou_wei@huawei.com>
From:   Maximilian Luz <luzmaximilian@gmail.com>
Message-ID: <25642c19-6598-42c0-36ad-a2b2d507183a@gmail.com>
Date:   Thu, 29 Oct 2020 14:05:31 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <1603972048-64271-1-git-send-email-zou_wei@huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/29/20 12:47 PM, Zou Wei wrote:
> The status local variable is assigned but never read:

[...]

>   static int s3_wmi_query_block(const char *guid, int instance, int *ret)
>   {
>   	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> -	acpi_status status;
>   	union acpi_object *obj;
>   	int error = 0;
>   
>   	mutex_lock(&s3_wmi_lock);
> -	status = wmi_query_block(guid, instance, &output);

I assume you want to remove the "status =" and not the whole line...

Regardless, I personally would prefer explicit error handling (as is
done in other contexts with wmi_query_block(), see e.g.
platform/x86/intel-wmi-sbl-fw-update.c. Although, yes, the "if (!obj |
..." should cover the error case as far as I can tell.

Also there seems to be the same issue in platform/x86/msi-wmi.c.

On a related note:

Does anyone know if the mutex here is really required? If there are any
problems, I believe it should rather go to s3_wmi_send_lid_state() and
also wrap the input_report_switch() / input_sync(). Otherwise, if there
were any race-type situations, not covering that could lead to wrong
updates:

     Thread 1                            Thread 2
     
     s3_query_lid() returns old
                                         s3_query_lid() returns new
                                         input_report_switch()
                                         input_sync()
     input_report_switch()
     input_sync()

I don't really expect those situations here as s3_wmi_send_lid_state()
is only used in probe(), resume(), and notify() (please correct me if
I'm wrong), so the mutex does seem weird to me here.

Otherwise if it's needed, a comment explaining why wouldn't hurt.

Regards,
Max