Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices

From: Vladimir Oltean <olteanv@gmail.com>
To: Sergey Organov <sorganov@gmail.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Fugang Duan <fugang.duan@nxp.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Richard Cochran <richardcochran@gmail.com>
Subject: Re: [PATCH v2 net] net: fec: fix hardware time stamping by external devices
Date: Sun, 12 Jul 2020 02:19:37 +0300	[thread overview]
Message-ID: <20200711231937.wu2zrm5spn7a6u2o@skbuf> (raw)
In-Reply-To: <20200711120842.2631-1-sorganov@gmail.com>

Hi Sergey,

On Sat, Jul 11, 2020 at 03:08:42PM +0300, Sergey Organov wrote:
> Fix support for external PTP-aware devices such as DSA or PTP PHY:
> 
> Make sure we never time stamp tx packets when hardware time stamping
> is disabled.
> 
> Check for PTP PHY being in use and then pass ioctls related to time
> stamping of Ethernet packets to the PTP PHY rather than handle them
> ourselves. In addition, disable our own hardware time stamping in this
> case.
> 
> Fixes: 6605b73 ("FEC: Add time stamping code and a PTP hardware clock")

Please use a 12-character sha1sum. Try to use the "pretty" format
specifier I gave you in the original thread, it saves you from counting,
and also from people complaining once it gets merged:

https://www.google.com/search?q=stephen+rothwell+%22fixes+tag+needs+some+work%22

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> 
> v2:
>   - Extracted from larger patch series
>   - Description/comments updated according to discussions
>   - Added Fixes: tag
> 
>  drivers/net/ethernet/freescale/fec.h      |  1 +
>  drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++------
>  drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index d8d76da..832a217 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -590,6 +590,7 @@ struct fec_enet_private {
>  void fec_ptp_init(struct platform_device *pdev, int irq_idx);
>  void fec_ptp_stop(struct platform_device *pdev);
>  void fec_ptp_start_cyclecounter(struct net_device *ndev);
> +void fec_ptp_disable_hwts(struct net_device *ndev);
>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
>  int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
>  
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 3982285..cc7fbfc 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1294,8 +1294,13 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>  			ndev->stats.tx_bytes += skb->len;
>  		}
>  
> -		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
> -			fep->bufdesc_ex) {
> +		/* NOTE: SKBTX_IN_PROGRESS being set does not imply it's we who
> +		 * are to time stamp the packet, so we still need to check time
> +		 * stamping enabled flag.
> +		 */
> +		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
> +			     fep->hwts_tx_en) &&
> +		    fep->bufdesc_ex) {
>  			struct skb_shared_hwtstamps shhwtstamps;
>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
>  
> @@ -2723,10 +2728,16 @@ static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
>  		return -ENODEV;
>  
>  	if (fep->bufdesc_ex) {
> -		if (cmd == SIOCSHWTSTAMP)
> -			return fec_ptp_set(ndev, rq);
> -		if (cmd == SIOCGHWTSTAMP)
> -			return fec_ptp_get(ndev, rq);
> +		bool use_fec_hwts = !phy_has_hwtstamp(phydev);

I thought we were in agreement that FEC does not support PHY
timestamping at this point, and this patch would only be fixing DSA
switches (even though PHYs would need this fixed too, when support is
added for them)? I would definitely not introduce support (and
incomplete, at that) for a new feature in a bugfix patch.

But it looks like we aren't.

> +
> +		if (cmd == SIOCSHWTSTAMP) {
> +			if (use_fec_hwts)
> +				return fec_ptp_set(ndev, rq);
> +			fec_ptp_disable_hwts(ndev);
> +		} else if (cmd == SIOCGHWTSTAMP) {
> +			if (use_fec_hwts)
> +				return fec_ptp_get(ndev, rq);
> +		}
>  	}
>  
>  	return phy_mii_ioctl(phydev, rq, cmd);
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 945643c..f8a592c 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -452,6 +452,18 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
>  	return -EOPNOTSUPP;
>  }
>  
> +/**
> + * fec_ptp_disable_hwts - disable hardware time stamping
> + * @ndev: pointer to net_device
> + */
> +void fec_ptp_disable_hwts(struct net_device *ndev)

This is not really needed, is it?
- PHY ability of hwtstamping does not change across the runtime of the
  kernel (or do you have a "special" one where it does?)
- The initial values for hwts_tx_en and hwts_rx_en are already 0
- There is no code path for which it is possible for hwts_tx_en or
  hwts_rx_en to have been non-zero prior to this call making them zero.

It is just "to be sure", in a very non-necessary way.

But nonetheless, it shouldn't be present in this patch either way, due
to the fact that one patch should have one topic only, and the topic of
this patch should be solving a clearly defined bug.

> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +
> +	fep->hwts_tx_en = 0;
> +	fep->hwts_rx_en = 0;
> +}
> +
>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -- 
> 2.10.0.1.g57b01a3
> 

Thanks,
-Vladimir