lkml.org 
[lkml]   [2024]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2024-05-27 18:19    [W:1.658 / U:0.760 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site