Messages in this thread Patch in this message | | | From | <> | Subject | Re: [RFC Patch net-next v2 3/8] net: dsa: microchip: Initial hardware time stamping support | Date | Wed, 23 Nov 2022 13:57:47 +0000 |
| |
Hi Vladimir, Thanks for the comment.
On Tue, 2022-11-22 at 01:13 +0200, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Mon, Nov 21, 2022 at 09:11:45PM +0530, Arun Ramadoss wrote: > > +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? >
I took this configuration template routine from other driver. Can I replace above snippet with
tagger_data->hwtstamp_set_state(dev->ds, rx_on); ret = ksz_ptp_enable_mode(dev, rx_on); if (ret) return ret;
> 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; > > +}; > > + > >
| |