[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZvqgVeOe9jE02b1r@google.com>
Date: Mon, 30 Sep 2024 05:57:57 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, linux-kernel@...r.kernel.org,
Peter Zijlstra <peterz@...radead.org>,
amadeuszx.slawinski@...ux.intel.com,
Tony Nguyen <anthony.l.nguyen@...el.com>,
nex.sw.ncis.osdt.itp.upstreaming@...el.com, netdev@...r.kernel.org,
Andy Shevchenko <andriy.shevchenko@...el.com>
Subject: Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly
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
Powered by blists - more mailing lists