| Message ID | 1603972048-64271-1-git-send-email-zou_wei@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [-next] platform/surface: remove status assignment without reading | expand |
On Thu, Oct 29, 2020 at 1:36 PM Zou Wei <zou_wei@huawei.com> wrote: > > The status local variable is assigned but never read: ... > mutex_lock(&s3_wmi_lock); > - status = wmi_query_block(guid, instance, &output); What are you doing?!
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
diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c index 130b6f5..ae1416c 100644 --- a/drivers/platform/surface/surface3-wmi.c +++ b/drivers/platform/surface/surface3-wmi.c @@ -57,12 +57,10 @@ static DEFINE_MUTEX(s3_wmi_lock); 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); obj = output.pointer;
The status local variable is assigned but never read: drivers/platform/surface/surface3-wmi.c:60:14: warning: variable ‘status’ set but not used [-Wunused-but-set-variable] acpi_status status; ^~~~~~ Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Zou Wei <zou_wei@huawei.com> --- drivers/platform/surface/surface3-wmi.c | 2 -- 1 file changed, 2 deletions(-)