| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0b104470017034eaa970ec37a04e8624be9d0d57.camel@microchip.com>
Date: Thu, 1 Dec 2022 10:56:07 +0000
From: <Arun.Ramadoss@...rochip.com>
To: <olteanv@...il.com>
CC: <andrew@...n.ch>, <linux-kernel@...r.kernel.org>,
<UNGLinuxDriver@...rochip.com>, <vivien.didelot@...il.com>,
<linux@...linux.org.uk>, <ceggers@...i.de>,
<Tristram.Ha@...rochip.com>, <f.fainelli@...il.com>,
<kuba@...nel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
<richardcochran@...il.com>, <netdev@...r.kernel.org>,
<Woojung.Huh@...rochip.com>, <davem@...emloft.net>
Subject: Re: [Patch net-next v1 03/12] net: dsa: microchip: ptp: add 4 bytes
in tail tag when ptp enabled
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@...rochip.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));
> > +}
Powered by blists - more mailing lists