Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Dec 2022 02:52:54 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Arun Ramadoss <arun.ramadoss@microchip.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        woojung.huh@microchip.com, UNGLinuxDriver@microchip.com,
        andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, linux@armlinux.org.uk,
        Tristram.Ha@microchip.com, richardcochran@gmail.com,
        ceggers@arri.de
Subject: Re: [Patch net-next v1 03/12] net: dsa: microchip: ptp: add 4 bytes
 in tail tag when ptp enabled
Message-ID: <20221201005254.lcwwtscmdu6scnpv@skbuf>
References: <20221128103227.23171-1-arun.ramadoss@microchip.com>
 <20221128103227.23171-1-arun.ramadoss@microchip.com>
 <20221128103227.23171-4-arun.ramadoss@microchip.com>
 <20221128103227.23171-4-arun.ramadoss@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221128103227.23171-4-arun.ramadoss@microchip.com>
 <20221128103227.23171-4-arun.ramadoss@microchip.com>
Precedence: bulk

On Mon, Nov 28, 2022 at 04:02:18PM +0530, Arun Ramadoss wrote:
> If PTP is enabled in the hardware, then 4 bytes are added in the tail
> tag. When PTP is enabled and 4 bytes are not added then messages are
> corrupted.

Comment in the code please. Also, please spell it out explicitly that
the tail tag size changes for all TX packets, PTP or not, if PTP
timestamping is enabled. Your phrasing can be unclear and the reader may
think that only PTP packets require a larger tail tag.

> 
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index cd20f39a565f..4c5b35a7883c 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -105,7 +105,6 @@ struct ksz_port {
>  	u8 num;
>  #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
>  	u8 hwts_tx_en;
> -	bool hwts_rx_en;

>  #endif
>  };
>  
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index a41418c6adf6..184aa57a8489 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -54,7 +66,7 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
>  
>  	config.tx_type = dev->ports[port].hwts_tx_en;
>  
> -	if (dev->ports[port].hwts_rx_en)
> +	if (tagger_data->hwtstamp_get_state(ds))

Let's be clear, hwtstamp_get_state() deals with TX timestamping, and
config.rx_filter deals with RX timestamping. Don't mix the two.
Using custom programs like testptp, you can enable RX timestamping but
not TX timestamping, or the other way around. You don't want the driver
to get confused.

>  		config.rx_filter = HWTSTAMP_FILTER_ALL;

Can the switch provide RX timestamps for all kinds of Ethernet packets,
not just PTP? If not, then report just what it can timestamp.

>  	else
>  		config.rx_filter = HWTSTAMP_FILTER_NONE;
>  int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index 0f6ae143afc9..828af38f0598 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -4,6 +4,7 @@
>   * Copyright (c) 2017 Microchip Technology
>   */
>  
> +#include <linux/dsa/ksz_common.h>
>  #include <linux/etherdevice.h>
>  #include <linux/list.h>
>  #include <net/dsa.h>
> @@ -16,9 +17,66 @@
>  #define LAN937X_NAME "lan937x"
>  
>  /* Typically only one byte is used for tail tag. */
> +#define KSZ_PTP_TAG_LEN			4
>  #define KSZ_EGRESS_TAG_LEN		1
>  #define KSZ_INGRESS_TAG_LEN		1
>  
> +#define KSZ_HWTS_EN  0
> +
> +struct ksz_tagger_private {
> +	struct ksz_tagger_data data; /* Must be first */
> +	unsigned long state;
> +};
> +
> +static struct ksz_tagger_private *
> +ksz_tagger_private(struct dsa_switch *ds)
> +{
> +	return ds->tagger_data;
> +}
> +
> +static bool ksz_hwtstamp_get_state(struct dsa_switch *ds)
> +{
> +	struct ksz_tagger_private *priv = ksz_tagger_private(ds);
> +
> +	return test_bit(KSZ_HWTS_EN, &priv->state);
> +}

As discussed, I don't really think there exists a case for hwtstamp_get_state().
Don't abuse the tagger-owned storage.

> +
> +static void ksz_hwtstamp_set_state(struct dsa_switch *ds, bool on)
> +{
> +	struct ksz_tagger_private *priv = ksz_tagger_private(ds);
> +
> +	if (on)
> +		set_bit(KSZ_HWTS_EN, &priv->state);
> +	else
> +		clear_bit(KSZ_HWTS_EN, &priv->state);
> +}
> +
> +static void ksz_disconnect(struct dsa_switch *ds)
> +{
> +	struct ksz_tagger_private *priv = ds->tagger_data;
> +
> +	kfree(priv);
> +	ds->tagger_data = NULL;
> +}
> +
> +static int ksz_connect(struct dsa_switch *ds)
> +{
> +	struct ksz_tagger_data *tagger_data;
> +	struct ksz_tagger_private *priv;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	/* Export functions for switch driver use */
> +	tagger_data = &priv->data;
> +	tagger_data->hwtstamp_get_state = ksz_hwtstamp_get_state;
> +	tagger_data->hwtstamp_set_state = ksz_hwtstamp_set_state;
> +	ds->tagger_data = priv;
> +
> +	return 0;
> +}
> +
>  static struct sk_buff *ksz_common_rcv(struct sk_buff *skb,
>  				      struct net_device *dev,
>  				      unsigned int port, unsigned int len)
> @@ -91,10 +149,11 @@ DSA_TAG_DRIVER(ksz8795_netdev_ops);
>  MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
>  
>  /*
> - * For Ingress (Host -> KSZ9477), 2 bytes are added before FCS.
> + * For Ingress (Host -> KSZ9477), 2/6 bytes are added before FCS.
>   * ---------------------------------------------------------------------------
> - * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
> + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|ts(4bytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
>   * ---------------------------------------------------------------------------
> + * ts   : time stamp (Present only if PTP is enabled in the Hardware)
>   * tag0 : Prioritization (not used now)
>   * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
>   *
> @@ -113,6 +172,19 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
>  #define KSZ9477_TAIL_TAG_OVERRIDE	BIT(9)
>  #define KSZ9477_TAIL_TAG_LOOKUP		BIT(10)
>  
> +/* Time stamp tag is only inserted if PTP is enabled in hardware. */

Stronger. Time stamp tag *needs* to be inserted if PTP is enabled in hardware.
Regardless of whether this is a PTP frame or not.

I think you don't think this is confusing. But it is confusing.
2 years from now, when this patch gets submitted again for being merged,
I don't want to ask the same questions again.

> +static void ksz_xmit_timestamp(struct dsa_port *dp, struct sk_buff *skb)
> +{
> +	struct ksz_tagger_private *priv;
> +
> +	priv = ksz_tagger_private(dp->ds);
> +
> +	if (!test_bit(KSZ_HWTS_EN, &priv->state))
> +		return;
> +
> +	put_unaligned_be32(0, skb_put(skb, KSZ_PTP_TAG_LEN));
> +}