Simply enable one to write code like:

int foo(struct my_drv *adapter)
{
	scoped_guard(spinlock, &adapter->some_spinlock)
		return adapter->spinlock_protected_var;
}

Current scoped_guard() implementation does not support that,
due to compiler complaining:
error: control reaches end of non-void function [-Werror=return-type]

One could argue that for such use case it would be better to use
guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say
that coding with my proposed locking style is also very pleasant, as I'm
doing so for a few weeks already.

Technical stuff about the change:
scoped_guard() macro uses common idiom of using "for" statement to declare
a scoped variable. Unfortunately, current logic is too hard for compiler
diagnostics to be sure that there is exactly one loop step; fix that.

To make any loop so trivial that there is no above warning, it must not
depend on any variable to tell if there are more steps. There is no
obvious solution for that in C, but one could use the compound statement
expression with "goto" jumping past the "loop", effectively leaving only
the subscope part of the loop semantics.

More impl details:
one more level of macro indirection is now needed to avoid duplicating
label names;
I didn't spot any other place that is using
	if (0) past_the_loop:; else for (...; 1; ({goto past_the_loop}))
idiom, so it's not packed for reuse what makes actual macros code cleaner.

NAKed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
Andy believes that this change is completely wrong C, and wants me
to keep the following 4 corncers attached (I either don't agree
or they are irrelevant), but here we go:
1. wrong usage of scoped_guard().
   In the described cases the guard() needs to be used.
2. the code like:
	int foo(...)
	{
		my_macro(...)
			return X;
	}
   without return 0; (which is a known dead code) is counter intuitive
   from the C language perspective.
3. [about netdev not liking guard()]
   I do not buy "too hard" when it's too easy to get a preprocessed *.i
   file if needed for any diagnosis which makes things quite clear.
   Moreover, once done the developer will much easier understands how this
   "magic" works (there is no rocket science, but yes, the initial
   threshold probably a bit higher than just pure C).
4. Besides that (if you was following the minmax discussion in LKML) the
   macro expansion may be problematic and lead to the unbelievable huge .i
   files that compiles dozens of seconds on modern CPUs (I experienced
   myself that with AtomISP driver which drove the above mentioned minmax
   discussion).
   [Przemek - nested scoped_guard() usage expands linearly]
---
 include/linux/cleanup.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index d9e613803df1..6b568a8a7f9c 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
-#define scoped_guard(_name, args...)					\
-	for (CLASS(_name, scope)(args),					\
-	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
+#define scoped_guard(_name, args...)	\
+	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
+
+#define __scoped_guard_labeled(_label, _name, args...)	\
+	if (0)						\
+		_label: ;				\
+	else						\
+		for (CLASS(_name, scope)(args);		\
+		     __guard_ptr(_name)(&scope), 1;	\
+		     ({ goto _label; }))
 
 #define scoped_cond_guard(_name, _fail, args...) \
 	for (CLASS(_name, scope)(args), \

base-commit: 151ac45348afc5b56baa584c7cd4876addf461ff
-- 
2.46.0

…
> Current scoped_guard() implementation does not support that,
> due to compiler complaining:
…
> +++ b/include/linux/cleanup.h
> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
>
> -#define scoped_guard(_name, args...)					\
> -	for (CLASS(_name, scope)(args),					\
> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> +#define scoped_guard(_name, args...)	\
> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
> +
> +#define __scoped_guard_labeled(_label, _name, args...)	\
> +	if (0)						\
> +		_label: ;				\
> +	else						\
> +		for (CLASS(_name, scope)(args);		\
> +		     __guard_ptr(_name)(&scope), 1;	\
> +		     ({ goto _label; }))
>
>  #define scoped_cond_guard(_name, _fail, args...) \
>  	for (CLASS(_name, scope)(args), \

* How do you think about to define such macros before their use?

* Would you ever like to avoid reserved identifiers in such source code?
  https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier


Regards,
Markus

On 9/30/24 13:30, Markus Elfring wrote:
> …
>> Current scoped_guard() implementation does not support that,
>> due to compiler complaining:
> …
>> +++ b/include/linux/cleanup.h
>> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>>
>>   #define __guard_ptr(_name) class_##_name##_lock_ptr
>>
>> -#define scoped_guard(_name, args...)					\
>> -	for (CLASS(_name, scope)(args),					\
>> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
>> +#define scoped_guard(_name, args...)	\
>> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
>> +
>> +#define __scoped_guard_labeled(_label, _name, args...)	\
>> +	if (0)						\
>> +		_label: ;				\
>> +	else						\
>> +		for (CLASS(_name, scope)(args);		\
>> +		     __guard_ptr(_name)(&scope), 1;	\
>> +		     ({ goto _label; }))
>>
>>   #define scoped_cond_guard(_name, _fail, args...) \
>>   	for (CLASS(_name, scope)(args), \
> 
> * How do you think about to define such macros before their use?

will do, no problem

> 
> * Would you ever like to avoid reserved identifiers in such source code?
>    https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

we already don't care about this guideline (see __guard_ptr())

OTOH there is DCL37-C-EX3 that says "does not apply for std lib",
and here (in the kernel) we don' need stdlib, (or for the purpose of
bureaucracy we ARE the stdlib for ourselves)

> 
> 
> Regards,
> Markus

>> * Would you ever like to avoid reserved identifiers in such source code?
>>   https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
>
> we already don't care about this guideline (see __guard_ptr())

Please take another look at name conventions.
How do you think about to avoid that this software depends on undefined behaviour?

Regards,
Markus

On Thu, Sep 26, 2024 at 03:41:38PM +0200, Przemek Kitszel wrote:
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index d9e613803df1..6b568a8a7f9c 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
>  
> -#define scoped_guard(_name, args...)					\
> -	for (CLASS(_name, scope)(args),					\
> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> +#define scoped_guard(_name, args...)	\
> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
> +
> +#define __scoped_guard_labeled(_label, _name, args...)	\
> +	if (0)						\
> +		_label: ;				\
> +	else						\
> +		for (CLASS(_name, scope)(args);		\
> +		     __guard_ptr(_name)(&scope), 1;	\
                                               ^^^
> +		     ({ goto _label; }))
>  

Remove the ", 1".  The point of the __guard_ptr() condition is for try_locks
but the ", 1" means they always succeed.  The only try lock I can find in
the current tree is tsc200x_esd_work().

regards,
dan carpenter

On 9/27/24 09:31, Dan Carpenter wrote:
> On Thu, Sep 26, 2024 at 03:41:38PM +0200, Przemek Kitszel wrote:
>> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
>> index d9e613803df1..6b568a8a7f9c 100644
>> --- a/include/linux/cleanup.h
>> +++ b/include/linux/cleanup.h
>> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>>   
>>   #define __guard_ptr(_name) class_##_name##_lock_ptr
>>   
>> -#define scoped_guard(_name, args...)					\
>> -	for (CLASS(_name, scope)(args),					\
>> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
>> +#define scoped_guard(_name, args...)	\
>> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
>> +
>> +#define __scoped_guard_labeled(_label, _name, args...)	\
>> +	if (0)						\
>> +		_label: ;				\
>> +	else						\
>> +		for (CLASS(_name, scope)(args);		\
>> +		     __guard_ptr(_name)(&scope), 1;	\
>                                                 ^^^
>> +		     ({ goto _label; }))
>>   
> 
> Remove the ", 1".  The point of the __guard_ptr() condition is for try_locks
> but the ", 1" means they always succeed.  The only try lock I can find in

You are right that the __guard_ptr() is conditional for the benefit of
try_locks. But here we have unconditional lock. And removing ", 1" part
makes compiler complaining with the very same message:
error: control reaches end of non-void function [-Werror=return-type]

so ", 1" part is on purpose and must stay there to aid compiler.

> the current tree is tsc200x_esd_work().
> 
> regards,
> dan carpenter
> 
>

On Fri, Sep 27, 2024 at 04:08:30PM +0200, Przemek Kitszel wrote:
> On 9/27/24 09:31, Dan Carpenter wrote:
> > On Thu, Sep 26, 2024 at 03:41:38PM +0200, Przemek Kitszel wrote:
> > > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> > > index d9e613803df1..6b568a8a7f9c 100644
> > > --- a/include/linux/cleanup.h
> > > +++ b/include/linux/cleanup.h
> > > @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
> > >   #define __guard_ptr(_name) class_##_name##_lock_ptr
> > > -#define scoped_guard(_name, args...)					\
> > > -	for (CLASS(_name, scope)(args),					\
> > > -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> > > +#define scoped_guard(_name, args...)	\
> > > +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
> > > +
> > > +#define __scoped_guard_labeled(_label, _name, args...)	\
> > > +	if (0)						\
> > > +		_label: ;				\
> > > +	else						\
> > > +		for (CLASS(_name, scope)(args);		\
> > > +		     __guard_ptr(_name)(&scope), 1;	\
> >                                                 ^^^
> > > +		     ({ goto _label; }))
> > 
> > Remove the ", 1".  The point of the __guard_ptr() condition is for try_locks
> > but the ", 1" means they always succeed.  The only try lock I can find in
> 
> You are right that the __guard_ptr() is conditional for the benefit of
> try_locks. But here we have unconditional lock. And removing ", 1" part
> makes compiler complaining with the very same message:
> error: control reaches end of non-void function [-Werror=return-type]
> 
> so ", 1" part is on purpose and must stay there to aid compiler.
> 
> > the current tree is tsc200x_esd_work().

Obviously, we can't break stuff and also checking __guard_ptr(_name)(&scope) is
pointless if we're going to ignore the return value.

But, sure, I get that we want to the compiler to know that regular spin_lock()
is going to succeed and spin_trylock() might not.  As a static checker
developer, I want that as well.  Currently, whenever someone creates a new class
of locks, I have to add a couple lines to Smatch to add this information.  It's
not a huge deal, but it would be nice to avoid this.

I did a `git grep scoped_guard | grep try` and I think tsc200x_esd_work() is the
only place which actually uses try locks with scoped_guard().  If it's just the
one, then why don't we create a scoped_guard_trylock() macro?

regards,
dan carpenter

On 9/27/24 17:04, Dan Carpenter wrote:
> On Fri, Sep 27, 2024 at 04:08:30PM +0200, Przemek Kitszel wrote:
>> On 9/27/24 09:31, Dan Carpenter wrote:
>>> On Thu, Sep 26, 2024 at 03:41:38PM +0200, Przemek Kitszel wrote:
>>>> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
>>>> index d9e613803df1..6b568a8a7f9c 100644
>>>> --- a/include/linux/cleanup.h
>>>> +++ b/include/linux/cleanup.h
>>>> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>>>>    #define __guard_ptr(_name) class_##_name##_lock_ptr
>>>> -#define scoped_guard(_name, args...)					\
>>>> -	for (CLASS(_name, scope)(args),					\
>>>> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
>>>> +#define scoped_guard(_name, args...)	\
>>>> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
>>>> +
>>>> +#define __scoped_guard_labeled(_label, _name, args...)	\
>>>> +	if (0)						\
>>>> +		_label: ;				\
>>>> +	else						\
>>>> +		for (CLASS(_name, scope)(args);		\
>>>> +		     __guard_ptr(_name)(&scope), 1;	\
>>>                                                  ^^^
>>>> +		     ({ goto _label; }))
>>>
>>> Remove the ", 1".  The point of the __guard_ptr() condition is for try_locks
>>> but the ", 1" means they always succeed.  The only try lock I can find in
>>
>> You are right that the __guard_ptr() is conditional for the benefit of
>> try_locks. But here we have unconditional lock. And removing ", 1" part
>> makes compiler complaining with the very same message:
>> error: control reaches end of non-void function [-Werror=return-type]
>>
>> so ", 1" part is on purpose and must stay there to aid compiler.
>>
>>> the current tree is tsc200x_esd_work().
> 
> Obviously, we can't break stuff and also checking __guard_ptr(_name)(&scope) is
> pointless if we're going to ignore the return value.
> 
> But, sure, I get that we want to the compiler to know that regular spin_lock()
> is going to succeed and spin_trylock() might not.  As a static checker
> developer, I want that as well.  Currently, whenever someone creates a new class
> of locks, I have to add a couple lines to Smatch to add this information.  It's
> not a huge deal, but it would be nice to avoid this.
> 
> I did a `git grep scoped_guard | grep try` and I think tsc200x_esd_work() is the
> only place which actually uses try locks with scoped_guard().  If it's just the
> one, then why don't we create a scoped_guard_trylock() macro?

Most of the time it is just easier to bend your driver than change or
extend the core of the kernel.

There is actually scoped_cond_guard() which is a trylock variant.

scoped_guard(mutex_try, &ts->mutex) you have found is semantically
wrong and must be fixed.

---
I have received also a bot message about "if (x) scoped_guard(y, z)"
usage (without braces), so will need to adjust it too.

On Mon, Sep 30, 2024 at 12:21:44PM +0200, Przemek Kitszel wrote:
> 
> Most of the time it is just easier to bend your driver than change or
> extend the core of the kernel.
> 
> There is actually scoped_cond_guard() which is a trylock variant.
> 
> scoped_guard(mutex_try, &ts->mutex) you have found is semantically
> wrong and must be fixed.

What?  I'm so puzzled by this conversation.

Anyway, I don't have a problem with your goal, but your macro is wrong and will
need to be re-written.  You will need to update any drivers which use the
scoped_guard() for try locks.  I don't care how you do that.  Use
scoped_cond_guard() if you want or invent a new macro.  But that work always
falls on the person changing the API.  Plus, it's only the one tsc200x-core.c
driver so I don't understand why you're making a big deal about it.

regards,
dan carpenter

On 9/30/24 13:08, Dan Carpenter wrote:
> On Mon, Sep 30, 2024 at 12:21:44PM +0200, Przemek Kitszel wrote:
>>
>> Most of the time it is just easier to bend your driver than change or
>> extend the core of the kernel.
>>
>> There is actually scoped_cond_guard() which is a trylock variant.
>>
>> scoped_guard(mutex_try, &ts->mutex) you have found is semantically
>> wrong and must be fixed.
> 
> What?  I'm so puzzled by this conversation.

there are two variants of scoped_guard() and you have found a place
where the wrong one is used

> 
> Anyway, I don't have a problem with your goal, but your macro is wrong and will
> need to be re-written.  You will need to update any drivers which use the
> scoped_guard() for try locks.  I don't care how you do that.  Use
> scoped_cond_guard() if you want or invent a new macro.  But that work always
> falls on the person changing the API.  Plus, it's only the one tsc200x-core.c
> driver so I don't understand why you're making a big deal about it.

apologies for upsetting you
I will send next iteration of this series with additional patches fixing
current code (thanks you for finding it for me in this case!)

I didn't said so in prev mail to leave you an option to send the fix for
the usage bug you have reported, just confirmed it. But by all means I'm
happy to fix current code myself.

> but your macro is wrong and will need to be re-written

could you please elaborate here?

On Mon, Sep 30, 2024 at 01:30:58PM +0200, Przemek Kitszel wrote:
> > but your macro is wrong and will need to be re-written
> 
> could you please elaborate here?

- 		__guard_ptr(_name)(&scope) && !done;
+		__guard_ptr(_name)(&scope), 1;     \

The __guard_ptr(_name)(&scope) check is checking whether lock function
succeeded.  With the new macro we only use scoped_guard() for locks which can't
fail.  We can (basically must) remove the __guard_ptr(_name)(&scope) check since
we're ignoring the result.

There are only four drivers which rely on conditional scoped_guard() locks.

$ git grep scoped_guard | egrep '(try|_intr)'
drivers/input/keyboard/atkbd.c: scoped_guard(mutex_intr, &atkbd->mutex) {
drivers/input/touchscreen/tsc200x-core.c:       scoped_guard(mutex_try, &ts->mutex) {
drivers/input/touchscreen/wacom_w8001.c:        scoped_guard(mutex_intr, &w8001->mutex) {
drivers/platform/x86/ideapad-laptop.c:  scoped_guard(mutex_intr, &dytc->mutex) {

This change breaks the drivers at runtime, but you need to ensure that drivers
using the old API will break at compile time so that people don't introduce new
bugs during the transition.  In other words, you will need to update the
DEFINE_GUARD_COND() stuff as well.

$ git grep DEFINE_GUARD_COND
include/linux/cleanup.h: * DEFINE_GUARD_COND(name, ext, condlock)
include/linux/cleanup.h:#define DEFINE_GUARD_COND(_name, _ext, _condlock) \
include/linux/iio/iio.h:DEFINE_GUARD_COND(iio_claim_direct, _try, ({
include/linux/mutex.h:DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
include/linux/mutex.h:DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))

I propose that you use scoped_try_guard() and scoped_guard_interruptible() to
support conditional locking.  Creating different macros for conditional locks is
the only way you can silence your GCC warnings and it makes life easier for me
as a static checker developer as well.  It's probably more complicated than I
have described so I'll leave that up to you, but this first draft doesn't work.

regards,
dan carpenter

On Mon, Sep 30, 2024 at 03:57:08PM +0300, Dan Carpenter wrote:
> On Mon, Sep 30, 2024 at 01:30:58PM +0200, Przemek Kitszel wrote:
> > > but your macro is wrong and will need to be re-written
> > 
> > could you please elaborate here?
> 
> - 		__guard_ptr(_name)(&scope) && !done;
> +		__guard_ptr(_name)(&scope), 1;     \
> 
> The __guard_ptr(_name)(&scope) check is checking whether lock function
> succeeded.  With the new macro we only use scoped_guard() for locks which can't
> fail.  We can (basically must) remove the __guard_ptr(_name)(&scope) check since
> we're ignoring the result.
> 
> There are only four drivers which rely on conditional scoped_guard() locks.
> 
> $ git grep scoped_guard | egrep '(try|_intr)'
> drivers/input/keyboard/atkbd.c: scoped_guard(mutex_intr, &atkbd->mutex) {
> drivers/input/touchscreen/tsc200x-core.c:       scoped_guard(mutex_try, &ts->mutex) {
> drivers/input/touchscreen/wacom_w8001.c:        scoped_guard(mutex_intr, &w8001->mutex) {
> drivers/platform/x86/ideapad-laptop.c:  scoped_guard(mutex_intr, &dytc->mutex) {

FTR I have many more pending changes using scoped_guard() this way.

> 
> This change breaks the drivers at runtime, but you need to ensure that drivers
> using the old API will break at compile time so that people don't introduce new
> bugs during the transition.  In other words, you will need to update the
> DEFINE_GUARD_COND() stuff as well.
> 
> $ git grep DEFINE_GUARD_COND
> include/linux/cleanup.h: * DEFINE_GUARD_COND(name, ext, condlock)
> include/linux/cleanup.h:#define DEFINE_GUARD_COND(_name, _ext, _condlock) \
> include/linux/iio/iio.h:DEFINE_GUARD_COND(iio_claim_direct, _try, ({
> include/linux/mutex.h:DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
> include/linux/mutex.h:DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
> include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
> include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
> include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))
> 
> I propose that you use scoped_try_guard() and scoped_guard_interruptible() to
> support conditional locking.  Creating different macros for conditional locks is
> the only way you can silence your GCC warnings and it makes life easier for me
> as a static checker developer as well.  It's probably more complicated than I
> have described so I'll leave that up to you, but this first draft doesn't work.

No, please do not. Right now the whether resource acquisition can fail
or not is decided by a particular class (which in turn can be used in
various guard macros). You are proposing to bubble this knowledge up and
make specialized "try" and "interruptible" and "killable" and "fallible"
version of the previously generic macros.

Now, the whole issue is because GCC is unable to figure out the flow
control of a complex macro. Please either fix GCC or rework that one
call site to shut GCC up. Do not wreck nice generic guard
implementation, that is flexible and supports several styles of use.

Again, the fact that scoped_guard() body can be skipped if the
constructor fails is explicitly documented as a desirable property.

Thanks.

-- 
Dmitry

On Mon, Sep 30, 2024 at 01:30:58PM +0200, Przemek Kitszel wrote:
> On 9/30/24 13:08, Dan Carpenter wrote:
> > On Mon, Sep 30, 2024 at 12:21:44PM +0200, Przemek Kitszel wrote:
> > > 
> > > Most of the time it is just easier to bend your driver than change or
> > > extend the core of the kernel.
> > > 
> > > There is actually scoped_cond_guard() which is a trylock variant.
> > > 
> > > scoped_guard(mutex_try, &ts->mutex) you have found is semantically
> > > wrong and must be fixed.
> > 
> > What?  I'm so puzzled by this conversation.
> 
> there are two variants of scoped_guard() and you have found a place
> where the wrong one is used

"Yeah? Well, you know, that's just like uh, your opinion, man."

From include/linux/cleanup.h:

 * scoped_guard (name, args...) { }:
 *	similar to CLASS(name, scope)(args), except the variable (with the
 *	explicit name 'scope') is declard in a for-loop such that its scope is
 *	bound to the next (compound) statement.
 *
 *	for conditional locks the loop body is skipped when the lock is not
 *	acquired.

Please note the 2nd paragraph that explains this particular usage and
that it was done this way on purpose.

> 
> > 
> > Anyway, I don't have a problem with your goal, but your macro is wrong and will
> > need to be re-written.  You will need to update any drivers which use the
> > scoped_guard() for try locks.  I don't care how you do that.  Use
> > scoped_cond_guard() if you want or invent a new macro.  But that work always
> > falls on the person changing the API.  Plus, it's only the one tsc200x-core.c
> > driver so I don't understand why you're making a big deal about it.

I think if you also count uses of "scoped_guard(mutex_intr, ...)" you
will find more of such examples.

> 
> apologies for upsetting you
> I will send next iteration of this series with additional patches fixing
> current code (thanks you for finding it for me in this case!)

No, please do not. Your "fix" it looks like will prevent writing
code like:

	scoped_guard(mutex_intr, &some_mutex) {
		do_stuff();

		return 0;
	}

	return -EINTR;

You might not like it, but it is a valid pattern.

> 
> I didn't said so in prev mail to leave you an option to send the fix for
> the usage bug you have reported, just confirmed it. But by all means I'm
> happy to fix current code myself.
> 
> > but your macro is wrong and will need to be re-written
> 
> could you please elaborate here?
i
Dan explained that you are changing the behavior of the guards, in an
undesirable way, breaking users. Please re-read what was written before.

Thanks.

-- 
Dmitry