Re: [Patch net-next v1 03/12] net: dsa: microchip: ptp: add 4 bytes in tail tag when ptp enabled

[<Arun.Ramadoss@microchip.com>]

Hi Vladimir,
On Thu, 2022-12-01 at 02:52 +0200, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> 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.

I will elaborate the commit description, why the additional 4 bytes are
required.

> 
> > 
> > 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.

Initially I thought like using one variable in tagger_data to control
the whether to add 4 bytes in tail tag or not. And another variable in
ksz_port to check whether rx timestamping enabled or not. 
To avoid using two variables to track the timestamping, I thought
reusing the tagger variable to check rx timestamping as well as PTP
enabled in hardware.

I need to change algorithm such a way that, 
- When either Tx timestamping or Rx timestamping enabled in any one of
the port, enable PTP in hardware and add 4 additional bytes in tail
tag.
- Add hwtstamp_config variable in ksz_port, to set and get the hwtstamp
configuration.

> 
> >               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.

Ok. I will update it.

> 
> >       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 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.

Ok. I will update it.

> 
> 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));
> > +}