lkml.org 
[lkml]   [2024]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH net-next] net: fec: Convert fec driver to use lock guards
Date
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2024年5月7日 22:01
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>; richardcochran@gmail.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH net-next] net: fec: Convert fec driver to use lock guards
>
> On Tue, May 07, 2024 at 05:05:20PM +0800, Wei Fang wrote:
> > Use guard() and scoped_guard() defined in linux/cleanup.h to automate
> > lock lifetime control in fec driver.
>
> You are probably the first to use these in netdev. Or one of the very
> early adopters. As such, you should explain in a bit more detail why
> these changes are safe.
>
Okay, I can add more in the commit message.

> > - spin_lock_irqsave(&fep->tmreg_lock, flags);
> > - ns = timecounter_cyc2time(&fep->tc, ts);
> > - spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> > + scoped_guard(spinlock_irqsave, &fep->tmreg_lock) {
> > + ns = timecounter_cyc2time(&fep->tc, ts);
> > + }
>
> This looks fine.
>
> > - mutex_lock(&fep->ptp_clk_mutex);
> > - ret = clk_prepare_enable(fep->clk_ptp);
> > - if (ret) {
> > - mutex_unlock(&fep->ptp_clk_mutex);
> > - goto failed_clk_ptp;
> > - } else {
> > - fep->ptp_clk_on = true;
> > + scoped_guard(mutex, &fep->ptp_clk_mutex) {
> > + ret = clk_prepare_enable(fep->clk_ptp);
> > + if (ret)
> > + goto failed_clk_ptp;
> > + else
> > + fep->ptp_clk_on = true;
> > }
>
> As Eric pointed out, it is not obvious what the semantics are
> here. You are leaving the scope, so i hope it does not matter it is a
> goto you are using to leave the scope. But a quick search did not find
> anything to confirm this. So i would like to see some justification in
> the commit message this is safe.
>
According to the explanation of the cleanup attribute of gcc [1] and clang [2],
the cleanup attribute runs a function when the variable goes out of scope. So
the lock will be freed when leaving the scope.
In addition, I have seen cases of using goto statements in scope_guard() in
the gpiolib driver [3].

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute
[2] https://clang.llvm.org/docs/AttributeReference.html#cleanup
[3] https://elixir.bootlin.com/linux/v6.9-rc7/source/drivers/gpio/gpiolib.c#L930

> > +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> > @@ -99,18 +99,17 @@
> > */
> > static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
> > {
> > - unsigned long flags;
> > u32 val, tempval;
> > struct timespec64 ts;
> > u64 ns;
> >
> > - if (fep->pps_enable == enable)
> > - return 0;
> > -
> > fep->pps_channel = DEFAULT_PPS_CHANNEL;
> > fep->reload_period = PPS_OUPUT_RELOAD_PERIOD;
> >
> > - spin_lock_irqsave(&fep->tmreg_lock, flags);
> > + guard(spinlock_irqsave)(&fep->tmreg_lock);
> > +
> > + if (fep->pps_enable == enable)
> > + return 0;
>
> This is not obviously correct. Why has this condition moved?
>
As you see, the assignment of ' pps_enable ' is protected by the 'tmreg_lock',
But the read operation of 'pps_enable' was not protected by the lock, so the
Coverity tool will complain a LOCK EVASION warning which may cause data
race to occur when running in a multithreaded environment. Of course, this
data race issue is almost impossible, so I modified it by the way. Because I don't
really want to fix it through another patch, unless you insist on doing so.

> I also personally don't like guard(). scoped_guard() {} is much easier
> to understand.
>
If the scope of the lock is from the time it is acquired until the function returns,
I think guard() is simpler. Of course, you may think scope_guard() is more reasonable
based on other considerations.

> In order to get my Reviewed-by: you need to drop all the plain guard()
> calls. I'm also not sure as a community we want to see changes like
> this.
>
Why I do this is because I see more and more drivers converting to using
Scope-based resource management mechanisms to manage resources,
not just locks, but memory and some other resources. I think the community
should actively embrace this new mechanism.
\
 
 \ /
  Last update: 2024-05-08 05:29    [W:0.036 / U:1.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site