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

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

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