Received-SPF: pass (google.com: domain of linux-kernel+bounces-171490-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 7 May 2024 16:01:10 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Wei Fang <wei.fang@nxp.com>
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, shenwei.wang@nxp.com, xiaoning.wang@nxp.com,
	richardcochran@gmail.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, imx@lists.linux.dev
Subject: Re: [PATCH net-next] net: fec: Convert fec driver to use lock guards
Message-ID: <e1fa1aa5-4a40-43c5-a6ab-780667254fe9@lunn.ch>
References: <20240507090520.284821-1-wei.fang@nxp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240507090520.284821-1-wei.fang@nxp.com>

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