Re: [PATCH -next] platform/surface: remove status assignment without reading

[Maximilian Luz]

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