Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Vladimir Oltean <olteanv@gmail.com>]

Hi Sergey,

On Mon, Jul 06, 2020 at 05:26:12PM +0300, Sergey Organov wrote:
> When external PTP-aware PHY is in use, it's that PHY that is to time
> stamp network packets, and it's that PHY where configuration requests
> of time stamping features are to be routed.
> 
> To achieve these goals:
> 
> 1. Make sure we don't time stamp packets when external PTP PHY is in use
> 
> 2. Make sure we redirect ioctl() related to time stamping of Ethernet
>    packets to connected PTP PHY rather than handle them ourselves
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  1 +
>  drivers/net/ethernet/freescale/fec_main.c | 18 ++++++++++++++----
>  drivers/net/ethernet/freescale/fec_ptp.c  | 12 ++++++++++++
>  3 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index a6cdd5b6..de9f46a 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -595,6 +595,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 2d0d313..995ea2e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1298,7 +1298,11 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>  			ndev->stats.tx_bytes += skb->len;
>  		}
>  
> +		/* It could be external PHY that had set SKBTX_IN_PROGRESS, so
> +		 * we still need to check it's we who are to time stamp
> +		 */
>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
> +		    unlikely(fep->hwts_tx_en) &&

I think this could qualify as a pretty significant fix in its own right,
that should go to stable trees. Right now, this patch appears pretty
easy to overlook.

Is this the same situation as what is being described here for the
gianfar driver?

https://patchwork.ozlabs.org/project/netdev/patch/20191227004435.21692-2-olteanv@gmail.com/

If so, it is interesting because I thought we had agreed that it's only
DSA who suffers from the double-TX-timestamp design issue, not PHYTER.
Not to mention, interesting because FEC + a timestamping DSA switch such
as mv88e6xxx is not unheard of. Hmmm...

>  			fep->bufdesc_ex) {
>  			struct skb_shared_hwtstamps shhwtstamps;
>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
> @@ -2755,10 +2759,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);
> +
> +		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)
> +{
> +	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
> 

Cheers,
-Vladimir