Re: [RFC Patch net-next v2 3/8] net: dsa: microchip: Initial hardware time stamping support

[<Arun.Ramadoss@microchip.com>]

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