| Message ID | 20240511030229.628287-1-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net-next] net: fec: Convert fec driver to use lock guards | expand |
On Sat, May 11, 2024 at 11:02:29AM +0800, Wei Fang wrote: > The Scope-based resource management mechanism has been introduced into > kernel since the commit 54da6a092431 ("locking: Introduce __cleanup() > based infrastructure"). The mechanism leverages the 'cleanup' attribute > provided by GCC and Clang, which allows resources to be automatically > released when they go out of scope. > Therefore, convert the fec driver to use guard() and scoped_guard() > defined in linux/cleanup.h to automate lock lifetime control in the > fec driver. Sorry, it has been decided for netdev we don't want these sort of conversions, at least not yet. The main worry is backporting fixes. It is likely such bcakports are going to be harder, and also more error prone, since the context is quite different. If done correctly, scoped_guard() {} could be useful, and avoid issues. So we are O.K. with that in new code. That will also allow us to get some experience with it over the next few years. Maybe we will then re-evaluate this decision about converting existing code. Andrew --- pw-bot: cr
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2024年5月11日 22:30 > 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 v2 net-next] net: fec: Convert fec driver to use lock guards > > On Sat, May 11, 2024 at 11:02:29AM +0800, Wei Fang wrote: > > The Scope-based resource management mechanism has been introduced > into > > kernel since the commit 54da6a092431 ("locking: Introduce __cleanup() > > based infrastructure"). The mechanism leverages the 'cleanup' > > attribute provided by GCC and Clang, which allows resources to be > > automatically released when they go out of scope. > > Therefore, convert the fec driver to use guard() and scoped_guard() > > defined in linux/cleanup.h to automate lock lifetime control in the > > fec driver. > > Sorry, it has been decided for netdev we don't want these sort of conversions, > at least not yet. The main worry is backporting fixes. It is likely such bcakports > are going to be harder, and also more error prone, since the context is quite > different. > > If done correctly, scoped_guard() {} could be useful, and avoid issues. So we are > O.K. with that in new code. That will also allow us to get some experience with > it over the next few years. Maybe we will then re-evaluate this decision about > converting existing code. > Okay, it's fine, thanks. >
> The Scope-based resource management mechanism has been introduced into … scope? was? … > +++ 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; > > if (enable) { > /* clear capture or output compare interrupt status if have. … Was this source code adjustment influenced also by a hint about “LOCK EVASION” from the analysis tool “Coverity”? https://lore.kernel.org/linux-kernel/AM0PR0402MB38910DB23A6DABF1C074EF1D88E52@AM0PR0402MB3891.eurprd04.prod.outlook.com/ https://lkml.org/lkml/2024/5/8/77 Will any tags (like “Fixes” and “Cc”) become relevant here? How do you think about to take the known advice “Solve only one problem per patch” better into account? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n81 Under which circumstances will development interests grow for further approaches according to the presentation of similar change combinations? Regards, Markus
> Was this source code adjustment influenced also by a hint about “LOCK EVASION” > from the analysis tool “Coverity”? > https://lore.kernel.org/linux-kernel/AM0PR0402MB38910DB23A6DABF1C074EF1D88E52@AM0PR0402MB3891.eurprd04.prod.outlook.com/ > https://lkml.org/lkml/2024/5/8/77 A corresponding software improvement was integrated on 2024-05-23. net: fec: avoid lock evasion when reading pps_enable https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/freescale/fec_ptp.c?h=v6.10-rc4&id=3b1c92f8e5371700fada307cc8fd2c51fa7bc8c1 Will further collateral evolution become interesting accordingly? Regards, Markus
> -----Original Message----- > From: Markus Elfring <Markus.Elfring@web.de> > Sent: 2024年6月23日 19:01 > To: Wei Fang <wei.fang@nxp.com>; netdev@vger.kernel.org; > kernel-janitors@vger.kernel.org; imx@lists.linux.dev; Andrew Lunn > <andrew@lunn.ch>; Clark Wang <xiaoning.wang@nxp.com>; David S. Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard > Cochran <richardcochran@gmail.com>; Shenwei Wang > <shenwei.wang@nxp.com> > Cc: Julia Lawall <julia.lawall@inria.fr>; Peter Zijlstra <peterz@infradead.org>; > Simon Horman <horms@kernel.org>; Waiman Long <longman@redhat.com> > Subject: Re: [PATCH v2 net-next] net: fec: Convert fec driver to use lock > guards > > > The Scope-based resource management mechanism has been introduced > into > … > scope? was? > > > … > > +++ 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; > > > > if (enable) { > > /* clear capture or output compare interrupt status if have. > … > > Was this source code adjustment influenced also by a hint about “LOCK > EVASION” > from the analysis tool “Coverity”? > https://lore.k/ > ernel.org%2Flinux-kernel%2FAM0PR0402MB38910DB23A6DABF1C074EF1D > 88E52%40AM0PR0402MB3891.eurprd04.prod.outlook.com%2F&data=05%7 > C02%7Cwei.fang%40nxp.com%7Cd6bfb97d5f854c511a3c08dc9373d064%7 > C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6385473728254555 > 50%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=m9oRx%2FI > z%2FLu1%2BlcQMRUlHsDfNznfVlJgbVwPramiEdA%3D&reserved=0 > https://lkml.o/ > rg%2Flkml%2F2024%2F5%2F8%2F77&data=05%7C02%7Cwei.fang%40nxp.c > om%7Cd6bfb97d5f854c511a3c08dc9373d064%7C686ea1d3bc2b4c6fa92cd > 99c5c301635%7C0%7C0%7C638547372825465871%7CUnknown%7CTWFp > bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC > I6Mn0%3D%7C0%7C%7C%7C&sdata=yEmDaP33Ss4FzUHv2IFkmGLV8udn9z1 > kAFzi01idssU%3D&reserved=0 > > Will any tags (like “Fixes” and “Cc”) become relevant here? > > How do you think about to take the known advice “Solve only one problem > per patch” > better into account? > https://git.ker/ > nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2 > Ftree%2FDocumentation%2Fprocess%2Fsubmitting-patches.rst%3Fh%3Dv6. > 10-rc4%23n81&data=05%7C02%7Cwei.fang%40nxp.com%7Cd6bfb97d5f854 > c511a3c08dc9373d064%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C > 0%7C638547372825472413%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4 > wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C% > 7C%7C&sdata=Lc2BsYHMzGkAgSsZ3pNdnvl8OlDtTL7pb0CNhD%2Bz2fg%3D& > reserved=0 > > Under which circumstances will development interests grow for further > approaches according to the presentation of similar change combinations? > Hi Markus, This patch has been rejected because netdev people don't want these sort of conversions at present which will make backporting more difficult. The LOCK EVASION issue has been fixed by another patch.
> This patch has been rejected because netdev people don't want these sort of > conversions at present which will make backporting more difficult. Advanced development tools can help to adjust involved concerns another bit. Some contributors got used to capabilities of the semantic patch language (Coccinelle software) for example. Will any clarifications become more helpful here? Would you get useful insights from special information sources? Looking at guard usage (with SmPL) https://lore.kernel.org/cocci/2dc6a1c7-79bf-42e3-95cc-599a1e154f57@web.de/ https://sympa.inria.fr/sympa/arc/cocci/2024-05/msg00090.html > The LOCK EVASION issue has been fixed by another patch. https://lore.kernel.org/r/20240521023800.17102-1-wei.fang@nxp.com Further software evolution might become more interesting also around the commit 3b1c92f8e5371700fada307cc8fd2c51fa7bc8c1 ("net: fec: avoid lock evasion when reading pps_enable"). Regards, Markus
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 8bd213da8fb6..8bf1490c07e1 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1397,12 +1397,10 @@ static void fec_enet_hwtstamp(struct fec_enet_private *fep, unsigned ts, struct skb_shared_hwtstamps *hwtstamps) { - unsigned long flags; u64 ns; - 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); memset(hwtstamps, 0, sizeof(*hwtstamps)); hwtstamps->hwtstamp = ns_to_ktime(ns); @@ -2313,15 +2311,13 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable) return ret; if (fep->clk_ptp) { - 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; } - mutex_unlock(&fep->ptp_clk_mutex); } ret = clk_prepare_enable(fep->clk_ref); @@ -2336,10 +2332,10 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable) } else { clk_disable_unprepare(fep->clk_enet_out); if (fep->clk_ptp) { - mutex_lock(&fep->ptp_clk_mutex); - clk_disable_unprepare(fep->clk_ptp); - fep->ptp_clk_on = false; - mutex_unlock(&fep->ptp_clk_mutex); + scoped_guard(mutex, &fep->ptp_clk_mutex) { + clk_disable_unprepare(fep->clk_ptp); + fep->ptp_clk_on = false; + } } clk_disable_unprepare(fep->clk_ref); clk_disable_unprepare(fep->clk_2x_txclk); @@ -2352,10 +2348,10 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable) clk_disable_unprepare(fep->clk_ref); failed_clk_ref: if (fep->clk_ptp) { - mutex_lock(&fep->ptp_clk_mutex); - clk_disable_unprepare(fep->clk_ptp); - fep->ptp_clk_on = false; - mutex_unlock(&fep->ptp_clk_mutex); + scoped_guard(mutex, &fep->ptp_clk_mutex) { + clk_disable_unprepare(fep->clk_ptp); + fep->ptp_clk_on = false; + } } failed_clk_ptp: clk_disable_unprepare(fep->clk_enet_out); diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c index 181d9bfbee22..0b447795734a 100644 --- a/drivers/net/ethernet/freescale/fec_ptp.c +++ 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; if (enable) { /* clear capture or output compare interrupt status if have. @@ -195,7 +194,6 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable) } fep->pps_enable = enable; - spin_unlock_irqrestore(&fep->tmreg_lock, flags); return 0; } @@ -204,9 +202,8 @@ static int fec_ptp_pps_perout(struct fec_enet_private *fep) { u32 compare_val, ptp_hc, temp_val; u64 curr_time; - unsigned long flags; - spin_lock_irqsave(&fep->tmreg_lock, flags); + guard(spinlock_irqsave)(&fep->tmreg_lock); /* Update time counter */ timecounter_read(&fep->tc); @@ -229,7 +226,6 @@ static int fec_ptp_pps_perout(struct fec_enet_private *fep) */ if (fep->perout_stime < curr_time + 100 * NSEC_PER_MSEC) { dev_err(&fep->pdev->dev, "Current time is too close to the start time!\n"); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); return -1; } @@ -257,7 +253,6 @@ static int fec_ptp_pps_perout(struct fec_enet_private *fep) */ writel(fep->next_counter, fep->hwp + FEC_TCCR(fep->pps_channel)); fep->next_counter = (fep->next_counter + fep->reload_period) & fep->cc.mask; - spin_unlock_irqrestore(&fep->tmreg_lock, flags); return 0; } @@ -307,13 +302,12 @@ static u64 fec_ptp_read(const struct cyclecounter *cc) void fec_ptp_start_cyclecounter(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); - unsigned long flags; int inc; inc = 1000000000 / fep->cycle_speed; /* grab the ptp lock */ - spin_lock_irqsave(&fep->tmreg_lock, flags); + guard(spinlock_irqsave)(&fep->tmreg_lock); /* 1ns counter */ writel(inc << FEC_T_INC_OFFSET, fep->hwp + FEC_ATIME_INC); @@ -332,8 +326,6 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev) /* reset the ns time counter */ timecounter_init(&fep->tc, &fep->cc, 0); - - spin_unlock_irqrestore(&fep->tmreg_lock, flags); } /** @@ -352,7 +344,6 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev) static int fec_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) { s32 ppb = scaled_ppm_to_ppb(scaled_ppm); - unsigned long flags; int neg_adj = 0; u32 i, tmp; u32 corr_inc, corr_period; @@ -397,7 +388,7 @@ static int fec_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) else corr_ns = fep->ptp_inc + corr_inc; - spin_lock_irqsave(&fep->tmreg_lock, flags); + guard(spinlock_irqsave)(&fep->tmreg_lock); tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK; tmp |= corr_ns << FEC_T_INC_CORR_OFFSET; @@ -407,8 +398,6 @@ static int fec_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) /* dummy read to update the timer. */ timecounter_read(&fep->tc); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); - return 0; } @@ -423,11 +412,9 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta) { struct fec_enet_private *fep = container_of(ptp, struct fec_enet_private, ptp_caps); - unsigned long flags; - spin_lock_irqsave(&fep->tmreg_lock, flags); + guard(spinlock_irqsave)(&fep->tmreg_lock); timecounter_adjtime(&fep->tc, delta); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); return 0; } @@ -445,18 +432,15 @@ static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts) struct fec_enet_private *fep = container_of(ptp, struct fec_enet_private, ptp_caps); u64 ns; - unsigned long flags; - mutex_lock(&fep->ptp_clk_mutex); - /* Check the ptp clock */ - if (!fep->ptp_clk_on) { - mutex_unlock(&fep->ptp_clk_mutex); - return -EINVAL; + scoped_guard(mutex, &fep->ptp_clk_mutex) { + /* Check the ptp clock */ + if (!fep->ptp_clk_on) + return -EINVAL; + + scoped_guard(spinlock_irqsave, &fep->tmreg_lock) + ns = timecounter_read(&fep->tc); } - spin_lock_irqsave(&fep->tmreg_lock, flags); - ns = timecounter_read(&fep->tc); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); - mutex_unlock(&fep->ptp_clk_mutex); *ts = ns_to_timespec64(ns); @@ -478,15 +462,12 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp, container_of(ptp, struct fec_enet_private, ptp_caps); u64 ns; - unsigned long flags; u32 counter; - mutex_lock(&fep->ptp_clk_mutex); + guard(mutex)(&fep->ptp_clk_mutex); /* Check the ptp clock */ - if (!fep->ptp_clk_on) { - mutex_unlock(&fep->ptp_clk_mutex); + if (!fep->ptp_clk_on) return -EINVAL; - } ns = timespec64_to_ns(ts); /* Get the timer value based on timestamp. @@ -494,21 +475,18 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp, */ counter = ns & fep->cc.mask; - spin_lock_irqsave(&fep->tmreg_lock, flags); - writel(counter, fep->hwp + FEC_ATIME); - timecounter_init(&fep->tc, &fep->cc, ns); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); - mutex_unlock(&fep->ptp_clk_mutex); + scoped_guard(spinlock_irqsave, &fep->tmreg_lock) { + writel(counter, fep->hwp + FEC_ATIME); + timecounter_init(&fep->tc, &fep->cc, ns); + } + return 0; } static int fec_ptp_pps_disable(struct fec_enet_private *fep, uint channel) { - unsigned long flags; - - spin_lock_irqsave(&fep->tmreg_lock, flags); + guard(spinlock_irqsave)(&fep->tmreg_lock); writel(0, fep->hwp + FEC_TCSR(channel)); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); return 0; } @@ -528,7 +506,6 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp, ktime_t timeout; struct timespec64 start_time, period; u64 curr_time, delta, period_ns; - unsigned long flags; int ret = 0; if (rq->type == PTP_CLK_REQ_PPS) { @@ -563,17 +540,17 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp, start_time.tv_nsec = rq->perout.start.nsec; fep->perout_stime = timespec64_to_ns(&start_time); - mutex_lock(&fep->ptp_clk_mutex); - if (!fep->ptp_clk_on) { - dev_err(&fep->pdev->dev, "Error: PTP clock is closed!\n"); - mutex_unlock(&fep->ptp_clk_mutex); - return -EOPNOTSUPP; + scoped_guard(mutex, &fep->ptp_clk_mutex) { + if (!fep->ptp_clk_on) { + dev_err(&fep->pdev->dev, + "Error: PTP clock is closed!\n"); + return -EOPNOTSUPP; + } + + scoped_guard(spinlock_irqsave, &fep->tmreg_lock) + /* Read current timestamp */ + curr_time = timecounter_read(&fep->tc); } - spin_lock_irqsave(&fep->tmreg_lock, flags); - /* Read current timestamp */ - curr_time = timecounter_read(&fep->tc); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); - mutex_unlock(&fep->ptp_clk_mutex); /* Calculate time difference */ delta = fep->perout_stime - curr_time; @@ -653,15 +630,13 @@ static void fec_time_keep(struct work_struct *work) { struct delayed_work *dwork = to_delayed_work(work); struct fec_enet_private *fep = container_of(dwork, struct fec_enet_private, time_keep); - unsigned long flags; - mutex_lock(&fep->ptp_clk_mutex); - if (fep->ptp_clk_on) { - spin_lock_irqsave(&fep->tmreg_lock, flags); - timecounter_read(&fep->tc); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); + scoped_guard(mutex, &fep->ptp_clk_mutex) { + if (fep->ptp_clk_on) { + scoped_guard(spinlock_irqsave, &fep->tmreg_lock) + timecounter_read(&fep->tc); + } } - mutex_unlock(&fep->ptp_clk_mutex); schedule_delayed_work(&fep->time_keep, HZ); }
The Scope-based resource management mechanism has been introduced into kernel since the commit 54da6a092431 ("locking: Introduce __cleanup() based infrastructure"). The mechanism leverages the 'cleanup' attribute provided by GCC and Clang, which allows resources to be automatically released when they go out of scope. Therefore, convert the fec driver to use guard() and scoped_guard() defined in linux/cleanup.h to automate lock lifetime control in the fec driver. Signed-off-by: Wei Fang <wei.fang@nxp.com> --- V2 changes: 1. Rephrase the commit message 2. Remove unnecessary '{}' if the scope of scoped_guard() is single line --- drivers/net/ethernet/freescale/fec_main.c | 36 ++++---- drivers/net/ethernet/freescale/fec_ptp.c | 101 ++++++++-------------- 2 files changed, 54 insertions(+), 83 deletions(-)