Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Tue, 17 May 2022 19:42:54 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Harini Katakam <harini.katakam@xilinx.com>
Cc:     <nicolas.ferre@microchip.com>, <davem@davemloft.net>,
        <richardcochran@gmail.com>, <claudiu.beznea@microchip.com>,
        <pabeni@redhat.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <michal.simek@xilinx.com>,
        <harinikatakamlinux@gmail.com>, <radhey.shyam.pandey@xilinx.com>
Subject: Re: [PATCH 1/3] net: macb: Fix PTP one step sync support
Message-ID: <20220517194254.015e87f3@kernel.org>
In-Reply-To: <20220517073259.23476-2-harini.katakam@xilinx.com>
References: <20220517073259.23476-1-harini.katakam@xilinx.com>
        <20220517073259.23476-2-harini.katakam@xilinx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 17 May 2022 13:02:57 +0530 Harini Katakam wrote:
> PTP one step sync packets cannot have CSUM padding and insertion in
> SW since time stamp is inserted on the fly by HW.
> In addition, ptp4l version 3.0 and above report an error when skb
> timestamps are reported for packets that not processed for TX TS
> after transmission.
> Add a helper to identify PTP one step sync and fix the above two
> errors.
> Also reset ptp OSS bit when one step is not selected.
> 
> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")

Please make sure to CC authors of the patches under fixes.
./scripts/get_maintainer should point them out.

> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>

If this is a fix it needs to be posted as [PATCH net] separately first
so we can get it into stable as well. The trees get merged together
every Thursday, if you're quick you may still make it this week.
Then after trees get merged you should send send the remaining patches.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

> Notes:
> -> Added the macb pad and fcs fixes tag because strictly speaking the PTP support  
> patch precedes the fcs patch in timeline.
> -> FYI, the error observed with setting HW TX timestamp for one step sync packets:  
> ptp4l[405.292]: port 1: unexpected socket error
> 
>  drivers/net/ethernet/cadence/macb_main.c | 54 ++++++++++++++++++++----
>  drivers/net/ethernet/cadence/macb_ptp.c  |  4 +-
>  2 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index e993616308f8..e23a03e8badf 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -37,6 +37,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>
> +#include <linux/ptp_classify.h>

Please keep the includes in alphabetical order.

>  #include "macb.h"
>  
>  /* This structure is only used for MACB on SiFive FU540 devices */
> @@ -98,6 +99,9 @@ struct sifive_fu540_macb_mgmt {
>  
>  #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
>  
> +/* IEEE1588 PTP flag field values  */
> +#define PTP_FLAG_TWOSTEP	0x2

Shouldn't this go into the PTP header?

>  /* DMA buffer descriptor might be different size
>   * depends on hardware configuration:
>   *
> @@ -1122,6 +1126,36 @@ static void macb_tx_error_task(struct work_struct *work)
>  	napi_enable(&queue->napi_tx);
>  }
>  
> +static inline bool ptp_oss(struct sk_buff *skb)

Please spell out then name more oss == open source software.

No inline here, please, let the compiler decide if the function is
small enough. One step timestamp may be a rare use case so inlining
this twice is not necessarily the right choice.

> +{
> +	struct ptp_header *hdr;
> +	unsigned int ptp_class;
> +	u8 msgtype;
> +
> +	/* No need to parse packet if PTP TS is not involved */
> +	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> +		goto not_oss;
> +
> +	/* Identify and return whether PTP one step sync is being processed */
> +	ptp_class = ptp_classify_raw(skb);
> +	if (ptp_class == PTP_CLASS_NONE)
> +		goto not_oss;
> +
> +	hdr = ptp_parse_header(skb, ptp_class);
> +	if (!hdr)
> +		goto not_oss;
> +
> +	if (hdr->flag_field[0] & PTP_FLAG_TWOSTEP)
> +		goto not_oss;
> +
> +	msgtype = ptp_get_msgtype(hdr, ptp_class);
> +	if (msgtype == PTP_MSGTYPE_SYNC)
> +		return true;
> +
> +not_oss:
> +	return false;
> +}
> +
>  static int macb_tx_complete(struct macb_queue *queue, int budget)
>  {
>  	struct macb *bp = queue->bp;
> @@ -1158,13 +1192,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  
>  			/* First, update TX stats if needed */
>  			if (skb) {
> -				if (unlikely(skb_shinfo(skb)->tx_flags &
> -					     SKBTX_HW_TSTAMP) &&
> -				    gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> -					/* skb now belongs to timestamp buffer
> -					 * and will be removed later
> -					 */
> -					tx_skb->skb = NULL;
> +				if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&

ptp_oss already checks if HW_TSTAMP is set.

> +				    !ptp_oss(skb)) {
> +					if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {

Why convert the gem_ptp_do_txstamp check from a && in the condition to
a separate if?

> +						/* skb now belongs to timestamp buffer
> +						 * and will be removed later
> +						 */
> +						tx_skb->skb = NULL;
> +					}
>  				}
>  				netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
>  					    macb_tx_ring_wrap(bp, tail),
> @@ -2063,7 +2098,7 @@ static unsigned int macb_tx_map(struct macb *bp,
>  			ctrl |= MACB_BF(TX_LSO, lso_ctrl);
>  			ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl);
>  			if ((bp->dev->features & NETIF_F_HW_CSUM) &&
> -			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl)
> +			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl && !ptp_oss(skb))
>  				ctrl |= MACB_BIT(TX_NOCRC);
>  		} else
>  			/* Only set MSS/MFS on payload descriptors
> @@ -2159,9 +2194,10 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
>  	struct sk_buff *nskb;
>  	u32 fcs;
>  
> +	/* Not available for GSO and PTP one step sync */

If the functions are named right this comment should not be needed.

>  	if (!(ndev->features & NETIF_F_HW_CSUM) ||
>  	    !((*skb)->ip_summed != CHECKSUM_PARTIAL) ||
> -	    skb_shinfo(*skb)->gso_size)	/* Not available for GSO */
> +	    skb_shinfo(*skb)->gso_size || ptp_oss(*skb))
>  		return 0;
>  
>  	if (padlen <= 0) {
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index fb6b27f46b15..9559c16078f9 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -470,8 +470,10 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
>  	case HWTSTAMP_TX_ONESTEP_SYNC:
>  		if (gem_ptp_set_one_step_sync(bp, 1) != 0)
>  			return -ERANGE;
> -		fallthrough;
> +		tx_bd_control = TSTAMP_ALL_FRAMES;
> +		break;
>  	case HWTSTAMP_TX_ON:
> +		gem_ptp_set_one_step_sync(bp, 0);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	default: