[PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Andy Shevchenko]

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

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;
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Gergo Koteles]

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

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Gergo Koteles]

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.
> 

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Nathan Chancellor]

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

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Ilpo Järvinen]

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.

-- 
 i.

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Josh Poimboeuf]

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?

-- 
Josh

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Andy Shevchenko]

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/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Josh Poimboeuf]

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.

-- 
Josh

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Nathan Chancellor]

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

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Hans de Goede]

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

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Hans de Goede]

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

Re: [PATCH v1 1/1] platform/x86: ideapad-laptop: Make the scope_guard() clear of its scope

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko