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
