Messages in this thread | | | Date | Tue, 22 Nov 2022 01:13:14 +0200 | From | Vladimir Oltean <> | Subject | Re: [RFC Patch net-next v2 3/8] net: dsa: microchip: Initial hardware time stamping support |
| |
On Mon, Nov 21, 2022 at 09:11:45PM +0530, Arun Ramadoss wrote: > +static int ksz_ptp_enable_mode(struct ksz_device *dev, bool enable) > +{ > + u16 data = 0; > + > + /* Enable PTP mode */ > + if (enable) > + data = PTP_ENABLE; > + > + return ksz_rmw16(dev, REG_PTP_MSG_CONF1, PTP_ENABLE, data); > +} > + > +static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port, > + struct hwtstamp_config *config) > +{ > + struct ksz_tagger_data *tagger_data = ksz_tagger_data(dev->ds); > + struct ksz_port *prt = &dev->ports[port]; > + bool rx_on; > + > + /* reserved for future extensions */ > + if (config->flags) > + return -EINVAL; > + > + switch (config->tx_type) { > + case HWTSTAMP_TX_OFF: > + case HWTSTAMP_TX_ONESTEP_P2P: > + prt->hwts_tx_en = config->tx_type; > + break; > + case HWTSTAMP_TX_ON: > + if (!is_lan937x(dev)) > + return -ERANGE; > + > + prt->hwts_tx_en = config->tx_type; > + break; > + default: > + return -ERANGE; > + } > + > + switch (config->rx_filter) { > + case HWTSTAMP_FILTER_NONE: > + rx_on = false; > + break; > + default: > + rx_on = true; > + break; > + } > + > + if (rx_on != tagger_data->hwtstamp_get_state(dev->ds)) { > + int ret; > + > + tagger_data->hwtstamp_set_state(dev->ds, false); > + > + ret = ksz_ptp_enable_mode(dev, rx_on); > + if (ret) > + return ret; > + > + if (rx_on) > + tagger_data->hwtstamp_set_state(dev->ds, true); > + }
What's your excuse which such a horrible code pattern? What will happen so bad with the packet if it's flagged with a TX timestamp request in KSZ_SKB_CB(skb) at the same time as REG_PTP_MSG_CONF1 is written to?
Also, doesn't dev->ports[port].hwts_tx_en serve as a guard against flagging packets for TX timestamps when you shouldn't?
> + > + return 0; > +} > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c > index 37db5156f9a3..6a909a300c13 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> > @@ -18,6 +19,62 @@ > #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); > +} > + > +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); > +}
| |