Messages in this thread | | | From | Wei Fang <> | Subject | RE: [PATCH net-next] net: fec: Convert fec driver to use lock guards | Date | Wed, 8 May 2024 03:29:10 +0000 |
| |
> -----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.
| |