RE: [PATCH net-next v3 08/14] net: ethernet: oa_tc6: Remove FCS size in RX frame

[Jerry.Ray]

Hi Rajagopal,

Thanks - confirmed: with patch 08 applied, lan865x just needs the FCS
handling, and s2500 gets it via hw_features |= NETIF_F_RXFCS. 

Two things though: 
(1) I had dropped patch 08 in my tree, hence the separate strip patch.
(2) Patch 08 uses NETIF_F_RXFCS inverted from its kernel meaning
   ("append FCS"): it strips when the bit is in hw_features and checks
   hw_features rather than dev->features, so ethtool -K rx-fcs on never
   actually delivers the FCS, and a consumer must advertise a debug feature
   to get default behavior. The mainline idiom (r8169, rtase, 8139too, mlx4)
   is if (!(dev->features & NETIF_F_RXFCS)) strip; 
   with hw_features |= NETIF_F_RXFCS to allow toggling. Since every OA TC6
   device delivers the FCS, I'd suggest the framework strip unconditionally
   by default using that idiom (patch attached), rather than gating on a
   per-vendor opt-in.

Please consider.

> 
> When MAC-PHY appends FCS to the incoming frame, FCS,
> it is removed from the frame before passing it to the stack.
> 
> Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@xxxxxxxxxx>
> ---
>  drivers/net/ethernet/oa_tc6/oa_tc6.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index d2b05f98765b..de5f1548139f 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> @@ -786,6 +786,9 @@ static void oa_tc6_submit_rx_skb(struct oa_tc6 *tc6)
>         tc6->netdev->stats.rx_packets++;
>         tc6->netdev->stats.rx_bytes += tc6->rx_skb->len;
> 
> +       if ((tc6->netdev->hw_features & NETIF_F_RXFCS) != 0)
> +               skb_trim(tc6->rx_skb, tc6->rx_skb->len - ETH_FCS_LEN);
> +
>         netif_rx(tc6->rx_skb);
> 
>         tc6->rx_skb = NULL;
> --

As below:

diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
index d2b05f98765b..255c7f066032 100644
--- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
@@ -782,6 +782,16 @@ static void oa_tc6_submit_rx_skb(struct oa_tc6 *tc6)
 {
 	oa_tc6_update_ts_in_rx_skb(tc6);
 
+	/* The MAC-PHY delivers each frame with its Ethernet FCS attached.
+	 * Strip it before handing the frame to the stack, unless the user has
+	 * asked to keep it via NETIF_F_RXFCS. Leaving the FCS on is harmless
+	 * for IP traffic but is parsed as a (malformed) suffix TLV by PTP,
+	 * which makes ptp4l reject every message as a "bad message".
+	 */
+	if (!(tc6->netdev->features & NETIF_F_RXFCS) &&
+	    tc6->rx_skb->len > ETH_FCS_LEN)
+		skb_trim(tc6->rx_skb, tc6->rx_skb->len - ETH_FCS_LEN);
+
 	tc6->rx_skb->protocol = eth_type_trans(tc6->rx_skb, tc6->netdev);
 	tc6->netdev->stats.rx_packets++;
 	tc6->netdev->stats.rx_bytes += tc6->rx_skb->len;
--

Regards,
Jerry.