Messages in this thread | | | From | Suman Ghosh <> | Subject | RE: [EXTERNAL] [PATCH net-next] net: fec: Convert fec driver to use lock guards | Date | Tue, 7 May 2024 11:55:23 +0000 |
| |
> drivers/net/ethernet/freescale/fec_main.c | 37 ++++---- >drivers/net/ethernet/freescale/fec_ptp.c | 104 +++++++++------------- > 2 files changed, 58 insertions(+), 83 deletions(-) > >diff --git a/drivers/net/ethernet/freescale/fec_main.c >b/drivers/net/ethernet/freescale/fec_main.c >index 8bd213da8fb6..5f98c0615115 100644 >--- a/drivers/net/ethernet/freescale/fec_main.c >+++ b/drivers/net/ethernet/freescale/fec_main.c >@@ -1397,12 +1397,11 @@ 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 +2312,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 +2333,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 +2349,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) { [Suman] Hi Wei, I am new to the use of scoped_guard() and have a confusion here. Above, "goto failed_clk_ref" is getting called after scoped_guard() declaration and you are again doing scoped_guard() inside the goto label failed_clk_ref. Why is this double declaration needed? >+ 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..ed64e077a64a 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,16 @@ 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 +463,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 +476,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 +507,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 +541,18 @@ 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 +632,14 @@ >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); } >-- >2.34.1 >
| |