Hi Maximilian,
FYI, the error/warning still remains.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: dea8dcf2a9fa8cc540136a6cd885c3beece16ec3
commit: f23027ca3d48b6f93c5994069fb25b73539fdf34 platform/surface: Move Surface 3 WMI driver to platform/surface
date: 9 weeks ago
config: x86_64-randconfig-r001-20201221 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23027ca3d48b6f93c5994069fb25b73539fdf34
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git fetch --no-tags linus master
git checkout f23027ca3d48b6f93c5994069fb25b73539fdf34
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/platform/surface/surface3-wmi.c: In function 's3_wmi_query_block':
>> drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
60 | acpi_status status;
| ^~~~~~
vim +/status +60 drivers/platform/surface/surface3-wmi.c
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 56
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 57 static int s3_wmi_query_block(const char *guid, int instance, int *ret)
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 58 {
83da6b59919a71a drivers/platform/x86/surface3-wmi.c Andy Shevchenko 2016-12-15 59 struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 @60 acpi_status status;
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 61 union acpi_object *obj;
83da6b59919a71a drivers/platform/x86/surface3-wmi.c Andy Shevchenko 2016-12-15 62 int error = 0;
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 63
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 64 mutex_lock(&s3_wmi_lock);
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 65 status = wmi_query_block(guid, instance, &output);
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 66
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 67 obj = output.pointer;
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 68
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 69 if (!obj || obj->type != ACPI_TYPE_INTEGER) {
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 70 if (obj) {
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 71 pr_err("query block returned object type: %d - buffer length:%d\n",
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 72 obj->type,
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 73 obj->type == ACPI_TYPE_BUFFER ?
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 74 obj->buffer.length : 0);
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 75 }
83da6b59919a71a drivers/platform/x86/surface3-wmi.c Andy Shevchenko 2016-12-15 76 error = -EINVAL;
83da6b59919a71a drivers/platform/x86/surface3-wmi.c Andy Shevchenko 2016-12-15 77 goto out_free_unlock;
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 78 }
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 79 *ret = obj->integer.value;
83da6b59919a71a drivers/platform/x86/surface3-wmi.c Andy Shevchenko 2016-12-15 80 out_free_unlock:
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 81 kfree(obj);
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 82 mutex_unlock(&s3_wmi_lock);
83da6b59919a71a drivers/platform/x86/surface3-wmi.c Andy Shevchenko 2016-12-15 83 return error;
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 84 }
3dda3b3798f96d2 drivers/platform/x86/surface3-wmi.c Benjamin Tissoires 2016-11-25 85
:::::: The code at line 60 was first introduced by commit
:::::: 3dda3b3798f96d2974b5f60811142d3e25547807 platform/x86: Add custom surface3 platform device for controlling LID
:::::: TO: Benjamin Tissoires <[email protected]>
:::::: CC: Andy Shevchenko <[email protected]>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi,
On 12/29/20 4:58 AM, kernel test robot wrote:
> Hi Maximilian,
>
> FYI, the error/warning still remains.
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: dea8dcf2a9fa8cc540136a6cd885c3beece16ec3
> commit: f23027ca3d48b6f93c5994069fb25b73539fdf34 platform/surface: Move Surface 3 WMI driver to platform/surface
> date: 9 weeks ago
> config: x86_64-randconfig-r001-20201221 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> reproduce (this is a W=1 build):
> # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23027ca3d48b6f93c5994069fb25b73539fdf34
> git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> git fetch --no-tags linus master
> git checkout f23027ca3d48b6f93c5994069fb25b73539fdf34
> # save the attached .config to linux build tree
> make W=1 ARCH=x86_64
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> drivers/platform/surface/surface3-wmi.c: In function 's3_wmi_query_block':
>>> drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
> 60 | acpi_status status;
> | ^~~~~~
I guess fixing this would require something like this:
From: Hans de Goede <[email protected]>
Subject: [PATCH] platform/surface: surface3-wmi: Fix variable 'status' set but not used compiler warning
Explictly check the status rather then relying on output.pointer staying
NULL on an error. This silences the following compiler warning:
drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/platform/surface/surface3-wmi.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c
index 130b6f52a600..4b7f79c0b78e 100644
--- a/drivers/platform/surface/surface3-wmi.c
+++ b/drivers/platform/surface/surface3-wmi.c
@@ -63,6 +63,10 @@ static int s3_wmi_query_block(const char *guid, int instance, int *ret)
mutex_lock(&s3_wmi_lock);
status = wmi_query_block(guid, instance, &output);
+ if (ACPI_FAILURE(status)) {
+ error = -EIO;
+ goto out_free_unlock;
+ }
obj = output.pointer;
Maximilian, can you review and/or test this fix please ?
Regards,
Hans
On 1/4/21 3:52 PM, Hans de Goede wrote:
> Hi,
>
> On 12/29/20 4:58 AM, kernel test robot wrote:
>> Hi Maximilian,
>>
>> FYI, the error/warning still remains.
>>
>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>> head: dea8dcf2a9fa8cc540136a6cd885c3beece16ec3
>> commit: f23027ca3d48b6f93c5994069fb25b73539fdf34 platform/surface: Move Surface 3 WMI driver to platform/surface
>> date: 9 weeks ago
>> config: x86_64-randconfig-r001-20201221 (attached as .config)
>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>> reproduce (this is a W=1 build):
>> # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23027ca3d48b6f93c5994069fb25b73539fdf34
>> git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>> git fetch --no-tags linus master
>> git checkout f23027ca3d48b6f93c5994069fb25b73539fdf34
>> # save the attached .config to linux build tree
>> make W=1 ARCH=x86_64
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <[email protected]>
>>
>> All warnings (new ones prefixed by >>):
>>
>> drivers/platform/surface/surface3-wmi.c: In function 's3_wmi_query_block':
>>>> drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
>> 60 | acpi_status status;
>> | ^~~~~~
>
> I guess fixing this would require something like this:
>
> From: Hans de Goede <[email protected]>
> Subject: [PATCH] platform/surface: surface3-wmi: Fix variable 'status' set but not used compiler warning
>
> Explictly check the status rather then relying on output.pointer staying
> NULL on an error. This silences the following compiler warning:
>
> drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> drivers/platform/surface/surface3-wmi.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c
> index 130b6f52a600..4b7f79c0b78e 100644
> --- a/drivers/platform/surface/surface3-wmi.c
> +++ b/drivers/platform/surface/surface3-wmi.c
> @@ -63,6 +63,10 @@ static int s3_wmi_query_block(const char *guid, int instance, int *ret)
>
> mutex_lock(&s3_wmi_lock);
> status = wmi_query_block(guid, instance, &output);
> + if (ACPI_FAILURE(status)) {
> + error = -EIO;
> + goto out_free_unlock;
> + }
>
> obj = output.pointer;
>
>
> Maximilian, can you review and/or test this fix please ?
Ah, this was on my TODO list (among looking at some other things in this
driver), sorry that I haven't gotten to it yet. I'd have proposed pretty
much the exact same thing.
One thing to note though: You should initialize obj with NULL. Keeping
it uninitialized may mess with kfree() under out_free_unlock.
Unfortunately I don't have access to a Surface 3 to test, but apart from
not initializing obj, this patch looks good to me. You may add my
reviewed-by tag once you've fixed that.
Also note that drivers/platform/x86/msi-wmi.c suffers from the same
problem in msi_wmi_query_block() and should receive a similar fix.
Regards,
Max
Hi,
On 1/4/21 4:24 PM, Maximilian Luz wrote:
> On 1/4/21 3:52 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 12/29/20 4:58 AM, kernel test robot wrote:
>>> Hi Maximilian,
>>>
>>> FYI, the error/warning still remains.
>>>
>>> tree:?? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>>> head:?? dea8dcf2a9fa8cc540136a6cd885c3beece16ec3
>>> commit: f23027ca3d48b6f93c5994069fb25b73539fdf34 platform/surface: Move Surface 3 WMI driver to platform/surface
>>> date:?? 9 weeks ago
>>> config: x86_64-randconfig-r001-20201221 (attached as .config)
>>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>>> reproduce (this is a W=1 build):
>>> ???????? # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23027ca3d48b6f93c5994069fb25b73539fdf34
>>> ???????? git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>>> ???????? git fetch --no-tags linus master
>>> ???????? git checkout f23027ca3d48b6f93c5994069fb25b73539fdf34
>>> ???????? # save the attached .config to linux build tree
>>> ???????? make W=1 ARCH=x86_64
>>>
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <[email protected]>
>>>
>>> All warnings (new ones prefixed by >>):
>>>
>>> ??? drivers/platform/surface/surface3-wmi.c: In function 's3_wmi_query_block':
>>>>> drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
>>> ?????? 60 |? acpi_status status;
>>> ????????? |????????????? ^~~~~~
>>
>> I guess fixing this would require something like this:
>>
>> From: Hans de Goede <[email protected]>
>> Subject: [PATCH] platform/surface: surface3-wmi: Fix variable 'status' set but not used compiler warning
>>
>> Explictly check the status rather then relying on output.pointer staying
>> NULL on an error. This silences the following compiler warning:
>>
>> drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
>>
>> Reported-by: kernel test robot <[email protected]>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> ? drivers/platform/surface/surface3-wmi.c | 4 ++++
>> ? 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c
>> index 130b6f52a600..4b7f79c0b78e 100644
>> --- a/drivers/platform/surface/surface3-wmi.c
>> +++ b/drivers/platform/surface/surface3-wmi.c
>> @@ -63,6 +63,10 @@ static int s3_wmi_query_block(const char *guid, int instance, int *ret)
>> ? ????? mutex_lock(&s3_wmi_lock);
>> ????? status = wmi_query_block(guid, instance, &output);
>> +??? if (ACPI_FAILURE(status)) {
>> +??????? error = -EIO;
>> +??????? goto out_free_unlock;
>> +??? }
>> ? ????? obj = output.pointer;
>> ?
>> Maximilian, can you review and/or test this fix please ?
>
> Ah, this was on my TODO list (among looking at some other things in this
> driver), sorry that I haven't gotten to it yet. I'd have proposed pretty
> much the exact same thing.
>
> One thing to note though: You should initialize obj with NULL. Keeping
> it uninitialized may mess with kfree() under out_free_unlock.
>
> Unfortunately I don't have access to a Surface 3 to test, but apart from
> not initializing obj, this patch looks good to me. You may add my
> reviewed-by tag once you've fixed that.
Thank you, and sorry for being a bit slow with my follow up.
I've added the obj = NULL initialization and added your reviewed-by tag.
I'm sending this out as an official patch submission for the archives now
and then I'll apply it to my review-hans branch right away, so no need
for you to do anything :)
> Also note that drivers/platform/x86/msi-wmi.c suffers from the same
> problem in msi_wmi_query_block() and should receive a similar fix.
Thanks, I'll prep a patch for that too.
Regards,
Hans
On 2/4/21 12:36 PM, Hans de Goede wrote:
> Hi,
>
> On 1/4/21 4:24 PM, Maximilian Luz wrote:
>> On 1/4/21 3:52 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 12/29/20 4:58 AM, kernel test robot wrote:
>>>> Hi Maximilian,
>>>>
>>>> FYI, the error/warning still remains.
>>>>
>>>> tree:?? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>>>> head:?? dea8dcf2a9fa8cc540136a6cd885c3beece16ec3
>>>> commit: f23027ca3d48b6f93c5994069fb25b73539fdf34 platform/surface: Move Surface 3 WMI driver to platform/surface
>>>> date:?? 9 weeks ago
>>>> config: x86_64-randconfig-r001-20201221 (attached as .config)
>>>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>>>> reproduce (this is a W=1 build):
>>>> ???????? # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23027ca3d48b6f93c5994069fb25b73539fdf34
>>>> ???????? git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>>>> ???????? git fetch --no-tags linus master
>>>> ???????? git checkout f23027ca3d48b6f93c5994069fb25b73539fdf34
>>>> ???????? # save the attached .config to linux build tree
>>>> ???????? make W=1 ARCH=x86_64
>>>>
>>>> If you fix the issue, kindly add following tag as appropriate
>>>> Reported-by: kernel test robot <[email protected]>
>>>>
>>>> All warnings (new ones prefixed by >>):
>>>>
>>>> ??? drivers/platform/surface/surface3-wmi.c: In function 's3_wmi_query_block':
>>>>>> drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
>>>> ?????? 60 |? acpi_status status;
>>>> ????????? |????????????? ^~~~~~
>>>
>>> I guess fixing this would require something like this:
>>>
>>> From: Hans de Goede <[email protected]>
>>> Subject: [PATCH] platform/surface: surface3-wmi: Fix variable 'status' set but not used compiler warning
>>>
>>> Explictly check the status rather then relying on output.pointer staying
>>> NULL on an error. This silences the following compiler warning:
>>>
>>> drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
>>>
>>> Reported-by: kernel test robot <[email protected]>
>>> Signed-off-by: Hans de Goede <[email protected]>
>>> ---
>>> ? drivers/platform/surface/surface3-wmi.c | 4 ++++
>>> ? 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c
>>> index 130b6f52a600..4b7f79c0b78e 100644
>>> --- a/drivers/platform/surface/surface3-wmi.c
>>> +++ b/drivers/platform/surface/surface3-wmi.c
>>> @@ -63,6 +63,10 @@ static int s3_wmi_query_block(const char *guid, int instance, int *ret)
>>> ? ????? mutex_lock(&s3_wmi_lock);
>>> ????? status = wmi_query_block(guid, instance, &output);
>>> +??? if (ACPI_FAILURE(status)) {
>>> +??????? error = -EIO;
>>> +??????? goto out_free_unlock;
>>> +??? }
>>> ? ????? obj = output.pointer;
>>>
>>> Maximilian, can you review and/or test this fix please ?
>>
>> Ah, this was on my TODO list (among looking at some other things in this
>> driver), sorry that I haven't gotten to it yet. I'd have proposed pretty
>> much the exact same thing.
>>
>> One thing to note though: You should initialize obj with NULL. Keeping
>> it uninitialized may mess with kfree() under out_free_unlock.
>>
>> Unfortunately I don't have access to a Surface 3 to test, but apart from
>> not initializing obj, this patch looks good to me. You may add my
>> reviewed-by tag once you've fixed that.
>
> Thank you, and sorry for being a bit slow with my follow up.
No worries.
> I've added the obj = NULL initialization and added your reviewed-by tag.
>
> I'm sending this out as an official patch submission for the archives now
> and then I'll apply it to my review-hans branch right away, so no need
> for you to do anything :)
>
>> Also note that drivers/platform/x86/msi-wmi.c suffers from the same
>> problem in msi_wmi_query_block() and should receive a similar fix.
>
> Thanks, I'll prep a patch for that too.
Perfect, thanks!
Regards,
Max
>
> Regards,
>
> Hans
>