| Message ID | 20240829165105.1609180-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v1,1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope | expand |
Hi Andy, Thank you for addressing this. On Thu, 2024-08-29 at 19:50 +0300, Andy Shevchenko wrote: > First of all, it's a bit counterintuitive to have something like > > int err; > ... > scoped_guard(...) > err = foo(...); > if (err) > return err; > > Second, with a particular kernel configuration and compiler version in > one of such cases the objtool is not happy: > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > I'm not an expert on all this, but the theory is that compiler and > linker in this case can't understand that 'result' variable will be > always initialized as long as no error has been returned. Assigning > 'result' to a dummy value helps with this. Note, that fixing the > scoped_guard() scope (as per above) does not make issue gone. > > That said, assign dummy value and make the scope_guard() clear of its scope. > For the sake of consistency do it in the entire file. > Interestingly, if I open a scope manually and use the plain guard, the warning disappears. ... unsigned long result; int err; { guard(mutex)(&priv->vpc_mutex); err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result); if (err) return err; } ... This looks a bit strange, but is probably easier for the compiler than the for loop of scoped_guard. But I don't know how well this style fits into the kernel. Best regards, Gergo Koteles
On Tue, Sep 03, 2024 at 05:00:51PM +0200, Gergo Koteles wrote: > On Thu, 2024-08-29 at 19:50 +0300, Andy Shevchenko wrote: > > First of all, it's a bit counterintuitive to have something like > > > > int err; > > ... > > scoped_guard(...) > > err = foo(...); > > if (err) > > return err; > > > > Second, with a particular kernel configuration and compiler version in > > one of such cases the objtool is not happy: > > > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > > > I'm not an expert on all this, but the theory is that compiler and > > linker in this case can't understand that 'result' variable will be > > always initialized as long as no error has been returned. Assigning > > 'result' to a dummy value helps with this. Note, that fixing the > > scoped_guard() scope (as per above) does not make issue gone. > > > > That said, assign dummy value and make the scope_guard() clear of its scope. > > For the sake of consistency do it in the entire file. > > > > Interestingly, if I open a scope manually and use the plain guard, the > warning disappears. Yes, that's what I also have, but I avoid that approach because in that case the printing will be done inside the lock, widening the critical section for no benefits. > ... > unsigned long result; > int err; > > { > guard(mutex)(&priv->vpc_mutex); > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, > &result); > if (err) > return err; > } > ... > > This looks a bit strange, but is probably easier for the compiler than > the for loop of scoped_guard. > > But I don't know how well this style fits into the kernel.
On Tue, 2024-09-03 at 18:14 +0300, Andy Shevchenko wrote: > On Tue, Sep 03, 2024 at 05:00:51PM +0200, Gergo Koteles wrote: > > On Thu, 2024-08-29 at 19:50 +0300, Andy Shevchenko wrote: > > > First of all, it's a bit counterintuitive to have something like > > > > > > int err; > > > ... > > > scoped_guard(...) > > > err = foo(...); > > > if (err) > > > return err; > > > > > > Second, with a particular kernel configuration and compiler version in > > > one of such cases the objtool is not happy: > > > > > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > > > > > I'm not an expert on all this, but the theory is that compiler and > > > linker in this case can't understand that 'result' variable will be > > > always initialized as long as no error has been returned. Assigning > > > 'result' to a dummy value helps with this. Note, that fixing the > > > scoped_guard() scope (as per above) does not make issue gone. > > > > > > That said, assign dummy value and make the scope_guard() clear of its scope. > > > For the sake of consistency do it in the entire file. > > > > > > > Interestingly, if I open a scope manually and use the plain guard, the > > warning disappears. > > Yes, that's what I also have, but I avoid that approach because in that case > the printing will be done inside the lock, widening the critical section for > no benefits. > This is intended to be an inner block scope within the function, it does not expand the critical section. > > ... > > unsigned long result; > > int err; > > > > { > > guard(mutex)(&priv->vpc_mutex); > > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, > > &result); > > if (err) > > return err; > > } > > ... > > > > This looks a bit strange, but is probably easier for the compiler than > > the for loop of scoped_guard. > > > > But I don't know how well this style fits into the kernel. >
On Tue, Sep 03, 2024 at 05:29:02PM +0200, Gergo Koteles wrote: > On Tue, 2024-09-03 at 18:14 +0300, Andy Shevchenko wrote: > > On Tue, Sep 03, 2024 at 05:00:51PM +0200, Gergo Koteles wrote: > > > On Thu, 2024-08-29 at 19:50 +0300, Andy Shevchenko wrote: > > > > First of all, it's a bit counterintuitive to have something like > > > > > > > > int err; > > > > ... > > > > scoped_guard(...) > > > > err = foo(...); > > > > if (err) > > > > return err; > > > > > > > > Second, with a particular kernel configuration and compiler version in > > > > one of such cases the objtool is not happy: > > > > > > > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > > > > > > > I'm not an expert on all this, but the theory is that compiler and > > > > linker in this case can't understand that 'result' variable will be > > > > always initialized as long as no error has been returned. Assigning > > > > 'result' to a dummy value helps with this. Note, that fixing the > > > > scoped_guard() scope (as per above) does not make issue gone. > > > > > > > > That said, assign dummy value and make the scope_guard() clear of its scope. > > > > For the sake of consistency do it in the entire file. > > > > > > > > > > Interestingly, if I open a scope manually and use the plain guard, the > > > warning disappears. > > > > Yes, that's what I also have, but I avoid that approach because in that case > > the printing will be done inside the lock, widening the critical section for > > no benefits. > > > > This is intended to be an inner block scope within the function, it > does not expand the critical section. I'm not sure I understand. scoped_guard() has a marked scope (with {} or just a line coupled with it). The guard() has a scope starting at it till the end of the function. In the latter case the sysfs_emit() becomes part of the critical section. > > > unsigned long result; > > > int err; > > > > > > { > > > guard(mutex)(&priv->vpc_mutex); > > > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, > > > &result); > > > if (err) > > > return err; > > > } But looking again into the code above now I got what you meant. You have added a nested scope inside the function, like do { ... } while (0); Yes, this is strange and not what we want to have either. So I prefer to hear what objtool / clang people may comment on this. Sorry that I missed this. > > > This looks a bit strange, but is probably easier for the compiler than > > > the for loop of scoped_guard. > > > > > > But I don't know how well this style fits into the kernel.
Hi Andy, On Tue, Sep 03, 2024 at 06:40:16PM +0300, Andy Shevchenko wrote: > On Tue, Sep 03, 2024 at 05:29:02PM +0200, Gergo Koteles wrote: > > On Tue, 2024-09-03 at 18:14 +0300, Andy Shevchenko wrote: > > > On Tue, Sep 03, 2024 at 05:00:51PM +0200, Gergo Koteles wrote: > > > > On Thu, 2024-08-29 at 19:50 +0300, Andy Shevchenko wrote: > > > > > First of all, it's a bit counterintuitive to have something like > > > > > > > > > > int err; > > > > > ... > > > > > scoped_guard(...) > > > > > err = foo(...); > > > > > if (err) > > > > > return err; > > > > > > > > > > Second, with a particular kernel configuration and compiler version in > > > > > one of such cases the objtool is not happy: > > > > > > > > > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > > > > > > > > > I'm not an expert on all this, but the theory is that compiler and > > > > > linker in this case can't understand that 'result' variable will be > > > > > always initialized as long as no error has been returned. Assigning > > > > > 'result' to a dummy value helps with this. Note, that fixing the > > > > > scoped_guard() scope (as per above) does not make issue gone. > > > > > > > > > > That said, assign dummy value and make the scope_guard() clear of its scope. > > > > > For the sake of consistency do it in the entire file. > > > > > > > > > > > > > Interestingly, if I open a scope manually and use the plain guard, the > > > > warning disappears. > > > > > > Yes, that's what I also have, but I avoid that approach because in that case > > > the printing will be done inside the lock, widening the critical section for > > > no benefits. > > > > > > > This is intended to be an inner block scope within the function, it > > does not expand the critical section. > > I'm not sure I understand. > > scoped_guard() has a marked scope (with {} or just a line coupled with it). > The guard() has a scope starting at it till the end of the function. In the > latter case the sysfs_emit() becomes part of the critical section. > > > > > unsigned long result; > > > > int err; > > > > > > > > { > > > > guard(mutex)(&priv->vpc_mutex); > > > > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, > > > > &result); > > > > if (err) > > > > return err; > > > > } > > But looking again into the code above now I got what you meant. > You have added a nested scope inside the function, like > > do { > ... > } while (0); > > Yes, this is strange and not what we want to have either. So I prefer to hear > what objtool / clang people may comment on this. So this does not appear to happen when CONFIG_KCOV is disabled with the configuration from the original report. I have spent some time looking at the disassembly but I am a little out of my element there. If I remember correctly, the "unexpected end of section" warning from objtool can appear when optimizations play fast and loose with the presence of potential undefined behavior (or cannot prove that there is no undefined behavior through inlining or analysis). In this case, I wonder if KCOV prevents LLVM from realizing that the for loop that scoped_guard() results in will run at least once, meaning that err and result would be potentially used uninitialized? That could explain why this change resolves the warning, as it ensures that no undefined behavior could happen regardless of whether or not the loop runs? Josh and Peter may have more insight. Cheers, Nathan
On Thu, Aug 29, 2024 at 07:50:32PM +0300, Andy Shevchenko wrote: > First of all, it's a bit counterintuitive to have something like > > int err; > ... > scoped_guard(...) > err = foo(...); > if (err) > return err; > > Second, with a particular kernel configuration and compiler version in > one of such cases the objtool is not happy: > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > I'm not an expert on all this, but the theory is that compiler and > linker in this case can't understand that 'result' variable will be > always initialized as long as no error has been returned. Assigning > 'result' to a dummy value helps with this. Note, that fixing the > scoped_guard() scope (as per above) does not make issue gone. I'm not sure I buy that, we should look closer to understand what the issue is. Can you share the config and/or toolchain version(s) need to trigger the warning?
On Tue, Sep 03, 2024 at 09:52:01PM -0700, Josh Poimboeuf wrote: > On Thu, Aug 29, 2024 at 07:50:32PM +0300, Andy Shevchenko wrote: > > First of all, it's a bit counterintuitive to have something like > > > > int err; > > ... > > scoped_guard(...) > > err = foo(...); > > if (err) > > return err; > > > > Second, with a particular kernel configuration and compiler version in > > one of such cases the objtool is not happy: > > > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > > > I'm not an expert on all this, but the theory is that compiler and > > linker in this case can't understand that 'result' variable will be > > always initialized as long as no error has been returned. Assigning > > 'result' to a dummy value helps with this. Note, that fixing the > > scoped_guard() scope (as per above) does not make issue gone. > > I'm not sure I buy that, we should look closer to understand what the > issue is. Can you share the config and/or toolchain version(s) need to > trigger the warning? .config is from the original report [1], toolchain is Debian clang version 18.1.8 (9) Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin (Just whatever Debian unstable provides) [1]: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/
On Tue, Sep 03, 2024 at 06:22:42PM -0700, Nathan Chancellor wrote: > On Tue, Sep 03, 2024 at 06:40:16PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 03, 2024 at 05:29:02PM +0200, Gergo Koteles wrote: > > > On Tue, 2024-09-03 at 18:14 +0300, Andy Shevchenko wrote: > > > > On Tue, Sep 03, 2024 at 05:00:51PM +0200, Gergo Koteles wrote: > > > > > On Thu, 2024-08-29 at 19:50 +0300, Andy Shevchenko wrote: > > > > > > First of all, it's a bit counterintuitive to have something like > > > > > > > > > > > > int err; > > > > > > ... > > > > > > scoped_guard(...) > > > > > > err = foo(...); > > > > > > if (err) > > > > > > return err; > > > > > > > > > > > > Second, with a particular kernel configuration and compiler version in > > > > > > one of such cases the objtool is not happy: > > > > > > > > > > > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > > > > > > > > > > > I'm not an expert on all this, but the theory is that compiler and > > > > > > linker in this case can't understand that 'result' variable will be > > > > > > always initialized as long as no error has been returned. Assigning > > > > > > 'result' to a dummy value helps with this. Note, that fixing the > > > > > > scoped_guard() scope (as per above) does not make issue gone. > > > > > > > > > > > > That said, assign dummy value and make the scope_guard() clear of its scope. > > > > > > For the sake of consistency do it in the entire file. > > > > > > > > > > > > > > > > Interestingly, if I open a scope manually and use the plain guard, the > > > > > warning disappears. > > > > > > > > Yes, that's what I also have, but I avoid that approach because in that case > > > > the printing will be done inside the lock, widening the critical section for > > > > no benefits. > > > > > > > > > > This is intended to be an inner block scope within the function, it > > > does not expand the critical section. > > > > I'm not sure I understand. > > > > scoped_guard() has a marked scope (with {} or just a line coupled with it). > > The guard() has a scope starting at it till the end of the function. In the > > latter case the sysfs_emit() becomes part of the critical section. > > > > > > > unsigned long result; > > > > > int err; > > > > > > > > > > { > > > > > guard(mutex)(&priv->vpc_mutex); > > > > > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, > > > > > &result); > > > > > if (err) > > > > > return err; > > > > > } > > > > But looking again into the code above now I got what you meant. > > You have added a nested scope inside the function, like > > > > do { > > ... > > } while (0); > > > > Yes, this is strange and not what we want to have either. So I prefer to hear > > what objtool / clang people may comment on this. > > So this does not appear to happen when CONFIG_KCOV is disabled with the > configuration from the original report. I have spent some time looking > at the disassembly but I am a little out of my element there. If I > remember correctly, the "unexpected end of section" warning from objtool > can appear when optimizations play fast and loose with the presence of > potential undefined behavior (or cannot prove that there is no undefined > behavior through inlining or analysis). In this case, I wonder if KCOV > prevents LLVM from realizing that the for loop that scoped_guard() > results in will run at least once, meaning that err and result would be > potentially used uninitialized? That could explain why this change > resolves the warning, as it ensures that no undefined behavior could > happen regardless of whether or not the loop runs? > > Josh and Peter may have more insight. Thanks for looking into this. Josh already keeps an eye on this.
On Tue, 3 Sep 2024, Gergo Koteles wrote: > On Tue, 2024-09-03 at 18:14 +0300, Andy Shevchenko wrote: > > On Tue, Sep 03, 2024 at 05:00:51PM +0200, Gergo Koteles wrote: > > > On Thu, 2024-08-29 at 19:50 +0300, Andy Shevchenko wrote: > > > > First of all, it's a bit counterintuitive to have something like > > > > > > > > int err; > > > > ... > > > > scoped_guard(...) > > > > err = foo(...); > > > > if (err) > > > > return err; > > > > > > > > Second, with a particular kernel configuration and compiler version in > > > > one of such cases the objtool is not happy: > > > > > > > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > > > > > > > I'm not an expert on all this, but the theory is that compiler and > > > > linker in this case can't understand that 'result' variable will be > > > > always initialized as long as no error has been returned. Assigning > > > > 'result' to a dummy value helps with this. Note, that fixing the > > > > scoped_guard() scope (as per above) does not make issue gone. > > > > > > > > That said, assign dummy value and make the scope_guard() clear of its scope. > > > > For the sake of consistency do it in the entire file. > > > > > > > > > > Interestingly, if I open a scope manually and use the plain guard, the > > > warning disappears. > > > > Yes, that's what I also have, but I avoid that approach because in that case > > the printing will be done inside the lock, widening the critical section for > > no benefits. > > > > This is intended to be an inner block scope within the function, it > does not expand the critical section. > > > > > ... > > > unsigned long result; > > > int err; > > > > > > { > > > guard(mutex)(&priv->vpc_mutex); > > > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, > > > &result); > > > if (err) > > > return err; > > > } > > > ... > > > > > > This looks a bit strange, but is probably easier for the compiler than > > > the for loop of scoped_guard. > > > > > > But I don't know how well this style fits into the kernel. It's ugly enough that I'd prefer the initialization of results variable as done in Andy's patch.
Hi, On 8/29/24 6:50 PM, Andy Shevchenko wrote: > First of all, it's a bit counterintuitive to have something like > > int err; > ... > scoped_guard(...) > err = foo(...); > if (err) > return err; > > Second, with a particular kernel configuration and compiler version in > one of such cases the objtool is not happy: > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > I'm not an expert on all this, but the theory is that compiler and > linker in this case can't understand that 'result' variable will be > always initialized as long as no error has been returned. Assigning > 'result' to a dummy value helps with this. Note, that fixing the > scoped_guard() scope (as per above) does not make issue gone. > > That said, assign dummy value and make the scope_guard() clear of its scope. > For the sake of consistency do it in the entire file. > > Fixes: 7cc06e729460 ("platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/ > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > > This has been Cc'ed to objtool and clang maintainers to have a look and > tell if this can be addressed in a better way. > > drivers/platform/x86/ideapad-laptop.c | 48 +++++++++++++++------------ > 1 file changed, 27 insertions(+), 21 deletions(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 35c75bcff195..c64dfc56651d 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -554,13 +554,14 @@ static ssize_t camera_power_show(struct device *dev, > char *buf) > { > struct ideapad_private *priv = dev_get_drvdata(dev); > - unsigned long result; > + unsigned long result = 0; > int err; > > - scoped_guard(mutex, &priv->vpc_mutex) > + scoped_guard(mutex, &priv->vpc_mutex) { > err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result); > - if (err) > - return err; > + if (err) > + return err; > + } > > return sysfs_emit(buf, "%d\n", !!result); > } > @@ -577,10 +578,11 @@ static ssize_t camera_power_store(struct device *dev, > if (err) > return err; > > - scoped_guard(mutex, &priv->vpc_mutex) > + scoped_guard(mutex, &priv->vpc_mutex) { > err = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state); > - if (err) > - return err; > + if (err) > + return err; > + } > > return count; > } > @@ -628,13 +630,14 @@ static ssize_t fan_mode_show(struct device *dev, > char *buf) > { > struct ideapad_private *priv = dev_get_drvdata(dev); > - unsigned long result; > + unsigned long result = 0; > int err; > > - scoped_guard(mutex, &priv->vpc_mutex) > + scoped_guard(mutex, &priv->vpc_mutex) { > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result); > - if (err) > - return err; > + if (err) > + return err; > + } > > return sysfs_emit(buf, "%lu\n", result); > } > @@ -654,10 +657,11 @@ static ssize_t fan_mode_store(struct device *dev, > if (state > 4 || state == 3) > return -EINVAL; > > - scoped_guard(mutex, &priv->vpc_mutex) > + scoped_guard(mutex, &priv->vpc_mutex) { > err = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state); > - if (err) > - return err; > + if (err) > + return err; > + } > > return count; > } > @@ -737,13 +741,14 @@ static ssize_t touchpad_show(struct device *dev, > char *buf) > { > struct ideapad_private *priv = dev_get_drvdata(dev); > - unsigned long result; > + unsigned long result = 0; > int err; > > - scoped_guard(mutex, &priv->vpc_mutex) > + scoped_guard(mutex, &priv->vpc_mutex) { > err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result); > - if (err) > - return err; > + if (err) > + return err; > + } > > priv->r_touchpad_val = result; > > @@ -762,10 +767,11 @@ static ssize_t touchpad_store(struct device *dev, > if (err) > return err; > > - scoped_guard(mutex, &priv->vpc_mutex) > + scoped_guard(mutex, &priv->vpc_mutex) { > err = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state); > - if (err) > - return err; > + if (err) > + return err; > + } > > priv->r_touchpad_val = state; >
Wed, Sep 04, 2024 at 08:14:53PM +0200, Hans de Goede kirjoitti: > Hi, > > On 8/29/24 6:50 PM, Andy Shevchenko wrote: > > First of all, it's a bit counterintuitive to have something like > > > > int err; > > ... > > scoped_guard(...) > > err = foo(...); > > if (err) > > return err; > > > > Second, with a particular kernel configuration and compiler version in > > one of such cases the objtool is not happy: > > > > ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > > > > I'm not an expert on all this, but the theory is that compiler and > > linker in this case can't understand that 'result' variable will be > > always initialized as long as no error has been returned. Assigning > > 'result' to a dummy value helps with this. Note, that fixing the > > scoped_guard() scope (as per above) does not make issue gone. > > > > That said, assign dummy value and make the scope_guard() clear of its scope. > > For the sake of consistency do it in the entire file. > > > > Fixes: 7cc06e729460 ("platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands") > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/ > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Thank you for your patch, I've applied this patch to my review-hans > branch: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Have you had a chance to go through the discussion? TL;DR: please defer this. There is still no clear understanding of the root cause and the culprit.
Hi Andy, On 9/4/24 10:18 PM, Andy Shevchenko wrote: > Wed, Sep 04, 2024 at 08:14:53PM +0200, Hans de Goede kirjoitti: >> Hi, >> >> On 8/29/24 6:50 PM, Andy Shevchenko wrote: >>> First of all, it's a bit counterintuitive to have something like >>> >>> int err; >>> ... >>> scoped_guard(...) >>> err = foo(...); >>> if (err) >>> return err; >>> >>> Second, with a particular kernel configuration and compiler version in >>> one of such cases the objtool is not happy: >>> >>> ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section >>> >>> I'm not an expert on all this, but the theory is that compiler and >>> linker in this case can't understand that 'result' variable will be >>> always initialized as long as no error has been returned. Assigning >>> 'result' to a dummy value helps with this. Note, that fixing the >>> scoped_guard() scope (as per above) does not make issue gone. >>> >>> That said, assign dummy value and make the scope_guard() clear of its scope. >>> For the sake of consistency do it in the entire file. >>> >>> Fixes: 7cc06e729460 ("platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands") >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/ >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> Thank you for your patch, I've applied this patch to my review-hans >> branch: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > Have you had a chance to go through the discussion? Yes I did read the entire discussion. > TL;DR: please defer this. There is still no clear understanding of the root > cause and the culprit. My gist from the discussion was that this was good to have regardless of the root cause. IMHO the old construction where the scoped-guard only guards the function-call and not the "if (ret)" on the return value of the guarded call was quite ugly / convoluted / hard to read and this patch is an improvement regardless. Regards, Hans
On Thu, Sep 05, 2024 at 10:33:22AM +0200, Hans de Goede wrote: > On 9/4/24 10:18 PM, Andy Shevchenko wrote: > > Wed, Sep 04, 2024 at 08:14:53PM +0200, Hans de Goede kirjoitti: > >> On 8/29/24 6:50 PM, Andy Shevchenko wrote: > >>> First of all, it's a bit counterintuitive to have something like > >>> > >>> int err; > >>> ... > >>> scoped_guard(...) > >>> err = foo(...); > >>> if (err) > >>> return err; > >>> > >>> Second, with a particular kernel configuration and compiler version in > >>> one of such cases the objtool is not happy: > >>> > >>> ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section > >>> > >>> I'm not an expert on all this, but the theory is that compiler and > >>> linker in this case can't understand that 'result' variable will be > >>> always initialized as long as no error has been returned. Assigning > >>> 'result' to a dummy value helps with this. Note, that fixing the > >>> scoped_guard() scope (as per above) does not make issue gone. > >>> > >>> That said, assign dummy value and make the scope_guard() clear of its scope. > >>> For the sake of consistency do it in the entire file. > >>> > >>> Fixes: 7cc06e729460 ("platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands") > >>> Reported-by: kernel test robot <lkp@intel.com> > >>> Closes: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/ > >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > >> Thank you for your patch, I've applied this patch to my review-hans > >> branch: > >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > > > Have you had a chance to go through the discussion? > > Yes I did read the entire discussion. > > > TL;DR: please defer this. There is still no clear understanding of the root > > cause and the culprit. > > My gist from the discussion was that this was good to have regardless of > the root cause. > > IMHO the old construction where the scoped-guard only guards the function-call > and not the "if (ret)" on the return value of the guarded call was quite ugly / > convoluted / hard to read and this patch is an improvement regardless. Okay, if you think it's good to go, you are welcome! Thanks!
On Wed, Sep 04, 2024 at 01:26:11PM +0300, Andy Shevchenko wrote: > On Tue, Sep 03, 2024 at 09:52:01PM -0700, Josh Poimboeuf wrote: > > I'm not sure I buy that, we should look closer to understand what the > > issue is. Can you share the config and/or toolchain version(s) need to > > trigger the warning? > > .config is from the original report [1], toolchain is > Debian clang version 18.1.8 (9) > Target: x86_64-pc-linux-gnu > Thread model: posix > InstalledDir: /usr/bin > > (Just whatever Debian unstable provides) > > [1]: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/ The warning is due to a (minor?) Clang bug, almost like it tried to optimize but didn't quite finish. Here's the disassembly: 0000000000000010 <fan_mode_show>: 10: e8 00 00 00 00 call 15 <fan_mode_show+0x5> 11: R_X86_64_PLT32 __fentry__-0x4 15: 55 push %rbp 16: 48 89 e5 mov %rsp,%rbp 19: 41 57 push %r15 1b: 41 56 push %r14 1d: 53 push %rbx 1e: 50 push %rax 1f: 48 89 d3 mov %rdx,%rbx 22: 49 89 fe mov %rdi,%r14 25: e8 00 00 00 00 call 2a <fan_mode_show+0x1a> 26: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4 2a: 4d 8b 76 78 mov 0x78(%r14),%r14 2e: 31 f6 xor %esi,%esi 30: 49 8d 7e 08 lea 0x8(%r14),%rdi 34: e8 00 00 00 00 call 39 <fan_mode_show+0x29> 35: R_X86_64_PLT32 mutex_lock_nested-0x4 39: 4d 89 f7 mov %r14,%r15 3c: 49 83 c7 08 add $0x8,%r15 40: 74 5b je 9d <fan_mode_show+0x8d> 42: 49 8b 06 mov (%r14),%rax 45: 48 8d 55 e0 lea -0x20(%rbp),%rdx 49: be 2b 00 00 00 mov $0x2b,%esi 4e: 48 8b 78 08 mov 0x8(%rax),%rdi 52: e8 00 00 00 00 call 57 <fan_mode_show+0x47> 53: R_X86_64_PLT32 .text.read_ec_data-0x4 57: 4c 89 ff mov %r15,%rdi 5a: 41 89 c6 mov %eax,%r14d 5d: e8 00 00 00 00 call 62 <fan_mode_show+0x52> 5e: R_X86_64_PLT32 mutex_unlock-0x4 62: 45 85 f6 test %r14d,%r14d 65: 74 07 je 6e <fan_mode_show+0x5e> 67: e8 00 00 00 00 call 6c <fan_mode_show+0x5c> 68: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4 6c: eb 1b jmp 89 <fan_mode_show+0x79> 6e: e8 00 00 00 00 call 73 <fan_mode_show+0x63> 6f: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4 73: 48 8b 55 e0 mov -0x20(%rbp),%rdx 77: 48 89 df mov %rbx,%rdi 7a: 48 c7 c6 00 00 00 00 mov $0x0,%rsi 7d: R_X86_64_32S .rodata.str1.1+0x508 81: e8 00 00 00 00 call 86 <fan_mode_show+0x76> 82: R_X86_64_PLT32 sysfs_emit-0x4 86: 41 89 c6 mov %eax,%r14d 89: 49 63 c6 movslq %r14d,%rax 8c: 48 83 c4 08 add $0x8,%rsp 90: 5b pop %rbx 91: 41 5e pop %r14 93: 41 5f pop %r15 95: 5d pop %rbp 96: 31 ff xor %edi,%edi 98: 31 d2 xor %edx,%edx 9a: 31 f6 xor %esi,%esi 9c: c3 ret 9d: e8 00 00 00 00 call a2 <__param_ctrl_ps2_aux_port+0x2> 9e: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4 <end of function> And the interesting bit: 30: 49 8d 7e 08 lea 0x8(%r14),%rdi # rdi = &priv->vpc_mutex 34: e8 00 00 00 00 call mutex_lock_nested 39: 4d 89 f7 mov %r14,%r15 # r15 = r14 = priv 3c: 49 83 c7 08 add $0x8,%r15 # r15 = &priv->vpc_mutex 40: 74 5b je 9d <fan_mode_show+0x8d> # if &priv->vpc_mutex == NULL, goto 9d ... 9d: e8 00 00 00 00 call a2 <__param_ctrl_ps2_aux_port+0x2> 9e: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4 <oof> If '&priv->vpc_mutex' is NULL, it jumps to 9d, where it calls __sanitizer_cov_trace_pc(). After that returns, it runs off the rails. Apparently Clang decided somehow (LTO?) that '&priv->vpc_mutex' can never be NULL, but it didn't quite finish the optimization. Maybe some bad interaction between LTO and KCOV? Here's the triggering code: > static ssize_t fan_mode_show(struct device *dev, > struct device_attribute *attr, > char *buf) > { > struct ideapad_private *priv = dev_get_drvdata(dev); > unsigned long result; > int err; > > scoped_guard(mutex, &priv->vpc_mutex) > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result); Here's the pre-processed code for the scoped_guard() invocation and its DEFINE_GUARD() dependency, edited for readability: > typedef struct mutex * class_mutex_t; > > static inline void class_mutex_destructor(struct mutex **p) > { > struct mutex *_T = *p; > if (_T) > mutex_unlock(_T); > } > > static inline struct mutex *class_mutex_constructor(struct mutex *_T) > { > struct mutex *t = ({ mutex_lock_nested(_T, 0); _T; }); > return t; > } > > static inline void *class_mutex_lock_ptr(struct mutex **_T) > { > return *_T; > } > > for (struct mutex *scope = class_mutex_constructor(&priv->vpc_mutex), *done = ((void *)0); > class_mutex_lock_ptr(&scope) && !done; > done = (void *)1) { > > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result); > } The fake "for loop" is needed to be able to initialize the scope variable inline. But basically it's doing this: > if (class_mutex_constructor(&priv->vpc_mutex)) > err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result); In this case, mutex is an unconditional guard, so the constructor just returns the original value of '&priv->vpc_mutex'. So if the original '&priv->vpc_mutex' is never NULL, the condition would always be true.
On Thu, Sep 05, 2024 at 08:16:01PM -0700, Josh Poimboeuf wrote: > On Wed, Sep 04, 2024 at 01:26:11PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 03, 2024 at 09:52:01PM -0700, Josh Poimboeuf wrote: > > > I'm not sure I buy that, we should look closer to understand what the > > > issue is. Can you share the config and/or toolchain version(s) need to > > > trigger the warning? > > > > .config is from the original report [1], toolchain is > > Debian clang version 18.1.8 (9) > > Target: x86_64-pc-linux-gnu > > Thread model: posix > > InstalledDir: /usr/bin > > > > (Just whatever Debian unstable provides) > > > > [1]: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/ > > The warning is due to a (minor?) Clang bug, almost like it tried to > optimize but didn't quite finish. ... > In this case, mutex is an unconditional guard, so the constructor just > returns the original value of '&priv->vpc_mutex'. So if the original > '&priv->vpc_mutex' is never NULL, the condition would always be true. Thanks a lot for that Josh. I have a somewhat trivial reproducer for the clang folks to take a look at. I should have some time on Monday to get that reported upstream and I will see if I can find anyone to take a look at it. For what it is worth, I don't think the workaround for this is too bad and it seems like it only shows up with KCOV. Cheers, Nathan
On Fri, Sep 13, 2024 at 04:33:24PM -0700, Nathan Chancellor wrote: > On Thu, Sep 05, 2024 at 08:16:01PM -0700, Josh Poimboeuf wrote: > > On Wed, Sep 04, 2024 at 01:26:11PM +0300, Andy Shevchenko wrote: > > > On Tue, Sep 03, 2024 at 09:52:01PM -0700, Josh Poimboeuf wrote: > > > > I'm not sure I buy that, we should look closer to understand what the > > > > issue is. Can you share the config and/or toolchain version(s) need to > > > > trigger the warning? > > > > > > .config is from the original report [1], toolchain is > > > Debian clang version 18.1.8 (9) > > > Target: x86_64-pc-linux-gnu > > > Thread model: posix > > > InstalledDir: /usr/bin > > > > > > (Just whatever Debian unstable provides) > > > > > > [1]: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/ > > > > The warning is due to a (minor?) Clang bug, almost like it tried to > > optimize but didn't quite finish. > ... > > In this case, mutex is an unconditional guard, so the constructor just > > returns the original value of '&priv->vpc_mutex'. So if the original > > '&priv->vpc_mutex' is never NULL, the condition would always be true. > > Thanks a lot for that Josh. I have a somewhat trivial reproducer for the > clang folks to take a look at. I should have some time on Monday to get > that reported upstream and I will see if I can find anyone to take a > look at it. > > For what it is worth, I don't think the workaround for this is too bad > and it seems like it only shows up with KCOV. FWIW, Hans queued the workaround.
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index 35c75bcff195..c64dfc56651d 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -554,13 +554,14 @@ static ssize_t camera_power_show(struct device *dev, char *buf) { struct ideapad_private *priv = dev_get_drvdata(dev); - unsigned long result; + unsigned long result = 0; int err; - scoped_guard(mutex, &priv->vpc_mutex) + scoped_guard(mutex, &priv->vpc_mutex) { err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result); - if (err) - return err; + if (err) + return err; + } return sysfs_emit(buf, "%d\n", !!result); } @@ -577,10 +578,11 @@ static ssize_t camera_power_store(struct device *dev, if (err) return err; - scoped_guard(mutex, &priv->vpc_mutex) + scoped_guard(mutex, &priv->vpc_mutex) { err = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state); - if (err) - return err; + if (err) + return err; + } return count; } @@ -628,13 +630,14 @@ static ssize_t fan_mode_show(struct device *dev, char *buf) { struct ideapad_private *priv = dev_get_drvdata(dev); - unsigned long result; + unsigned long result = 0; int err; - scoped_guard(mutex, &priv->vpc_mutex) + scoped_guard(mutex, &priv->vpc_mutex) { err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result); - if (err) - return err; + if (err) + return err; + } return sysfs_emit(buf, "%lu\n", result); } @@ -654,10 +657,11 @@ static ssize_t fan_mode_store(struct device *dev, if (state > 4 || state == 3) return -EINVAL; - scoped_guard(mutex, &priv->vpc_mutex) + scoped_guard(mutex, &priv->vpc_mutex) { err = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state); - if (err) - return err; + if (err) + return err; + } return count; } @@ -737,13 +741,14 @@ static ssize_t touchpad_show(struct device *dev, char *buf) { struct ideapad_private *priv = dev_get_drvdata(dev); - unsigned long result; + unsigned long result = 0; int err; - scoped_guard(mutex, &priv->vpc_mutex) + scoped_guard(mutex, &priv->vpc_mutex) { err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result); - if (err) - return err; + if (err) + return err; + } priv->r_touchpad_val = result; @@ -762,10 +767,11 @@ static ssize_t touchpad_store(struct device *dev, if (err) return err; - scoped_guard(mutex, &priv->vpc_mutex) + scoped_guard(mutex, &priv->vpc_mutex) { err = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state); - if (err) - return err; + if (err) + return err; + } priv->r_touchpad_val = state;
First of all, it's a bit counterintuitive to have something like int err; ... scoped_guard(...) err = foo(...); if (err) return err; Second, with a particular kernel configuration and compiler version in one of such cases the objtool is not happy: ideapad-laptop.o: warning: objtool: .text.fan_mode_show: unexpected end of section I'm not an expert on all this, but the theory is that compiler and linker in this case can't understand that 'result' variable will be always initialized as long as no error has been returned. Assigning 'result' to a dummy value helps with this. Note, that fixing the scoped_guard() scope (as per above) does not make issue gone. That said, assign dummy value and make the scope_guard() clear of its scope. For the sake of consistency do it in the entire file. Fixes: 7cc06e729460 ("platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands") Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202408290219.BrPO8twi-lkp@intel.com/ Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- This has been Cc'ed to objtool and clang maintainers to have a look and tell if this can be addressed in a better way. drivers/platform/x86/ideapad-laptop.c | 48 +++++++++++++++------------ 1 file changed, 27 insertions(+), 21 deletions(-)