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

From: Dmitry Torokhov
Date: Mon Sep 30 2024 - 09:01:22 EST


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