Messages in this thread | | | From | <> | Subject | Re: [Patch net-next v1 03/12] net: dsa: microchip: ptp: add 4 bytes in tail tag when ptp enabled | Date | Thu, 1 Dec 2022 10:56:07 +0000 |
| |
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)); > > +}
| |