Received-SPF: pass (google.com: domain of linux-kernel+bounces-171229-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Suman Ghosh <sumang@marvell.com>
To: Wei Fang <wei.fang@nxp.com>, "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org"
	<kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "shenwei.wang@nxp.com" <shenwei.wang@nxp.com>,
        "xiaoning.wang@nxp.com"
	<xiaoning.wang@nxp.com>,
        "richardcochran@gmail.com"
	<richardcochran@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "imx@lists.linux.dev" <imx@lists.linux.dev>
Subject: RE: [EXTERNAL] [PATCH net-next] net: fec: Convert fec driver to use
 lock guards
Thread-Topic: [EXTERNAL] [PATCH net-next] net: fec: Convert fec driver to use
 lock guards
Thread-Index: AQHaoF9GY7NoN4l0BUeo+37vLMNQnLGLptEg
Date: Tue, 7 May 2024 11:55:23 +0000
Message-ID: 
 <SJ0PR18MB521656F021A8A9BECD0CE105DBE42@SJ0PR18MB5216.namprd18.prod.outlook.com>
References: <20240507090520.284821-1-wei.fang@nxp.com>
In-Reply-To: <20240507090520.284821-1-wei.fang@nxp.com>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?r61w+5aSL4uALALghIgvQ1WBGS8xvmv1UKhwnIQTIFhl4GW5CSUt9P/RJf1h?=
 =?us-ascii?Q?H5JiZYESUvpIz1KHysI95RocJTWw/bO355hf2fOX/fUHzeM+DC1NMjSupnjq?=
 =?us-ascii?Q?mHbS/eo/WaDd1ycpzCsIycV+d9hlvy91ROL43gfl20a2i1J28c02AlONs8Iw?=
 =?us-ascii?Q?zpN4YAwlrTSypfemzPnC4z9i9b9U+8FPP6vgSV2lgNp5+eIwf1J/B/6RsUw1?=
 =?us-ascii?Q?VRUGNIasuV03f7uFQB9d7QCjkhSRDcjdp5STYypfHsHbRB3LlgbMlkfa5sBa?=
 =?us-ascii?Q?RIE5LiOIF1NaSZJgnOfwUYAzCrOrWNxo0uMlH0lAQx4dU+QxM4+q/am9URJT?=
 =?us-ascii?Q?omE6bsJTf5BmOKD5ROZOMEhNK0ltmitwz2SU3dBFwLOi5gjfXcoy9vZYx5mG?=
 =?us-ascii?Q?owO1yw3viBBOu4pzNnuS/IXxqCMwMqsoJe4fDrZ/hVC4nKlt1e0ZAyITTLak?=
 =?us-ascii?Q?aap0yHaEwgC4mn8SFRhHyvvNb4SER0NCwKVo4yPw0pXDhKSX9YskG4BB241l?=
 =?us-ascii?Q?MB134KBUPy40zaEIpVTKNRNEjbjXfPEH/iEw1Kbf4qV15rBe55nBq4DfDblt?=
 =?us-ascii?Q?o6iyx1laqJfjUdg1tRwflsWWGtFas8m5uvcFlt+C8gl48EvLh6n7FGZz8WLH?=
 =?us-ascii?Q?y2HZBuMxidHRI3Oka3fBBqqEdwA8iKUPl2hztFPuh7etwlj9zvFnEpnwHEqo?=
 =?us-ascii?Q?1/OdQotDQx0Ce4HJWQIBTg3VtpJ4EwTuiRIQ+tE5EJtIzjvjc+uiUC2yyL8K?=
 =?us-ascii?Q?yACMAE94xvmOnUf44b/bSRD2K6MOIOEcMII9P0+x/lWUSQLEcFsSPj3KsUj5?=
 =?us-ascii?Q?2FWU9OOPDMEc3TizwgSbZWc8xZP4vSsFgnj+vOOQoqk81YdTVg50HSxr1c0T?=
 =?us-ascii?Q?GF2/ntEalMeiiwqcx3sC9EQXoc+JS6PbYCDRa2BpXaKaT50oCHra+JEt20EJ?=
 =?us-ascii?Q?Lk5KAeicfLWaJeIYpNZvywtCvCxaNYJWL4ja7FsESGq7jDrDohItEnGPL4NZ?=
 =?us-ascii?Q?KhwkVNBrGsGACz+TQbWg4IfUA2h2Hqa5oGxepxWAM+rYanumT13FN9wCujnt?=
 =?us-ascii?Q?H5Z30p71+grickwZ/v6OyDZoBwAOcokJ4dEXB6I0KpwPkeMCdbj62eKTAiUe?=
 =?us-ascii?Q?nD+d2PvuFSTcYVwh2rWW0w8ZKb1vriZv4gi86QtXC7BU8trClsgCHN20eh2l?=
 =?us-ascii?Q?1k4L871UGxLbiFFkRgFujalAzZRT0I+/vkDpr9GJPP9XnxgAgy1ehbOuFIHS?=
 =?us-ascii?Q?ez7GIh5UUHHaSBiS587D7NOTx1E0CsEAfXP5sl6OfjGCrX2FKqkIrsXZtUCm?=
 =?us-ascii?Q?iy5sb5N3Qe1oqZ6j0AmaAh7sbhQc+6I2MZirzkBq1WByX2a/D6HFyrg9BpIN?=
 =?us-ascii?Q?6suZwJdoSX/SfvXwXrUVqhloiOPI+aJPyO0FwIqQwOEafq/yI9ItwAdKAcUK?=
 =?us-ascii?Q?geOpBfb95kqkYXrXcMSY5VO03AY9yfhHYhxkUkPcJL/Z/RvSzDvJaMlPI8jm?=
 =?us-ascii?Q?i29MC3PdKkoCSphXYoI2ctmr6QU29pon0Dgyjm8dhh/GcjUuMoZ4S+ePFFh7?=
 =?us-ascii?Q?JdCvB1UkxMyM5QeYFQjGkbGuzv422ZhrFLJ2NJqP?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR18MB5216.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 88b4171f-bbd4-4ec4-4289-08dc6e8c93ae
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 May 2024 11:55:23.1550
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Sys3QEj1ELckrz6Qi70dDyyi3otoyBRqd1vHsThp7n6AUVs2bmGWFAy072lcomOP2dAVwxpnKbz+8A8XJlVJ2g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR18MB4304
X-Proofpoint-ORIG-GUID: JTU2Jl-SnVxdswLO0zl9sWJ18LGMl8hl
X-Proofpoint-GUID: JTU2Jl-SnVxdswLO0zl9sWJ18LGMl8hl
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-07_06,2024-05-06_02,2023-05-22_02

> 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 =3D timecounter_cyc2time(&fep->tc, ts);
>-	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>+	scoped_guard(spinlock_irqsave, &fep->tmreg_lock) {
>+		ns =3D timecounter_cyc2time(&fep->tc, ts);
>+	}
>
> 	memset(hwtstamps, 0, sizeof(*hwtstamps));
> 	hwtstamps->hwtstamp =3D 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 =3D clk_prepare_enable(fep->clk_ptp);
>-			if (ret) {
>-				mutex_unlock(&fep->ptp_clk_mutex);
>-				goto failed_clk_ptp;
>-			} else {
>-				fep->ptp_clk_on =3D true;
>+			scoped_guard(mutex, &fep->ptp_clk_mutex) {
>+				ret =3D clk_prepare_enable(fep->clk_ptp);
>+				if (ret)
>+					goto failed_clk_ptp;
>+				else
>+					fep->ptp_clk_on =3D true;
> 			}
>-			mutex_unlock(&fep->ptp_clk_mutex);
> 		}
>
> 		ret =3D 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 =3D false;
>-			mutex_unlock(&fep->ptp_clk_mutex);
>+			scoped_guard(mutex, &fep->ptp_clk_mutex) {
>+				clk_disable_unprepare(fep->clk_ptp);
>+				fep->ptp_clk_on =3D 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 =3D 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, "go=
to failed_clk_ref" is getting called after scoped_guard() declaration and y=
ou 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 =3D 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 =3D=3D enable)
>-		return 0;
>-
> 	fep->pps_channel =3D DEFAULT_PPS_CHANNEL;
> 	fep->reload_period =3D PPS_OUPUT_RELOAD_PERIOD;
>
>-	spin_lock_irqsave(&fep->tmreg_lock, flags);
>+	guard(spinlock_irqsave)(&fep->tmreg_lock);
>+
>+	if (fep->pps_enable =3D=3D 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 =3D 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 =3D (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 =3D netdev_priv(ndev);
>-	unsigned long flags;
> 	int inc;
>
> 	inc =3D 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 =3D scaled_ppm_to_ppb(scaled_ppm);
>-	unsigned long flags;
> 	int neg_adj =3D 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 =3D fep->ptp_inc + corr_inc;
>
>-	spin_lock_irqsave(&fep->tmreg_lock, flags);
>+	guard(spinlock_irqsave)(&fep->tmreg_lock);
>
> 	tmp =3D readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
> 	tmp |=3D 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 =3D
> 	    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 =3D
> 	    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 =3D timecounter_read(&fep->tc);
>+		}
> 	}
>-	spin_lock_irqsave(&fep->tmreg_lock, flags);
>-	ns =3D timecounter_read(&fep->tc);
>-	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>-	mutex_unlock(&fep->ptp_clk_mutex);
>
> 	*ts =3D 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 =3D 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 =3D 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 =3D 0;
>
> 	if (rq->type =3D=3D PTP_CLK_REQ_PPS) {
>@@ -563,17 +541,18 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp=
,
> 			start_time.tv_nsec =3D rq->perout.start.nsec;
> 			fep->perout_stime =3D 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 =3D timecounter_read(&fep->tc);
>+				}
> 			}
>-			spin_lock_irqsave(&fep->tmreg_lock, flags);
>-			/* Read current timestamp */
>-			curr_time =3D timecounter_read(&fep->tc);
>-			spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>-			mutex_unlock(&fep->ptp_clk_mutex);
>
> 			/* Calculate time difference */
> 			delta =3D fep->perout_stime - curr_time; @@ -653,15 +632,14 @@
>static void fec_time_keep(struct work_struct *work)  {
> 	struct delayed_work *dwork =3D to_delayed_work(work);
> 	struct fec_enet_private *fep =3D 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
>