Messages in this thread | | | Date | Tue, 7 May 2024 16:01:10 +0200 | From | Andrew Lunn <> | 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.
> - 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.
> +++ 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?
I also personally don't like guard(). scoped_guard() {} is much easier to understand.
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.
Andrew
| |