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

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

[Zou Wei]

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

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;
 
-- 
2.6.2

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

[Andy Shevchenko]

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


-- 
With Best Regards,
Andy Shevchenko

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