Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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