Re: [Patch net-next v1 04/12] net: dsa: microchip: ptp: Manipulating absolute time using ptp hw clock

[<Arun.Ramadoss@microchip.com>]

Hi Vladimir,
Thanks for the review comment.

On Thu, 2022-12-01 at 03:04 +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:19PM +0530, Arun Ramadoss wrote:
> > diff --git a/drivers/net/dsa/microchip/ksz_ptp.c
> > b/drivers/net/dsa/microchip/ksz_ptp.c
> > index 184aa57a8489..415522ef4ce9 100644
> > --- a/drivers/net/dsa/microchip/ksz_ptp.c
> > +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> > @@ -200,6 +209,12 @@ static int ksz_ptp_settime(struct
> > ptp_clock_info *ptp,
> >               goto error_return;
> > 
> >       ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME,
> > PTP_LOAD_TIME);
> > +     if (ret)
> > +             goto error_return;
> > +
> > +     spin_lock_bh(&ptp_data->clock_lock);
> 
> Why disable bottom halves? Where is the bottom half that this races
> with?

The interrupts are added in the following patches in the series. During
the deferred packet timestamping, partial timestamps are reconstructed
to absolute time using the ptp_data->clock_time in the bottom halves.
So we need this spin_lock_bh.

> 
> > +     ptp_data->clock_time = *ts;
> > +     spin_unlock_bh(&ptp_data->clock_lock);
> > 
> >  error_return:
> >       mutex_unlock(&ptp_data->lock);
> >  }
> > 
> > +/*  Function is pointer to the do_aux_work in the ptp_clock
> > capability */
> > +static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > +{
> > +     struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> > +     struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> > +     struct timespec64 ts;
> > +
> > +     mutex_lock(&ptp_data->lock);
> > +     _ksz_ptp_gettime(dev, &ts);
> > +     mutex_unlock(&ptp_data->lock);
> 
> Why don't you call ksz_ptp_gettime(ptp, &ts) directly?

I will use ksz_ptp_gettime directly.

> 
> > +
> > +     spin_lock_bh(&ptp_data->clock_lock);
> > +     ptp_data->clock_time = ts;
> > +     spin_unlock_bh(&ptp_data->clock_lock);
> > +
> > +     return HZ;  /* reschedule in 1 second */
> > +}
> > +
> >  static int ksz_ptp_start_clock(struct ksz_device *dev)
> >  {
> > -     return ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > PTP_CLK_ENABLE);
> > +     struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> > +     int ret;
> > +
> > +     ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > PTP_CLK_ENABLE);
> > +     if (ret)
> > +             return ret;
> > +
> > +     spin_lock_bh(&ptp_data->clock_lock);
> > +     ptp_data->clock_time.tv_sec = 0;
> > +     ptp_data->clock_time.tv_nsec = 0;
> > +     spin_unlock_bh(&ptp_data->clock_lock);
> 
> Does ksz_ptp_start_clock() race with anything? The PTP clock has not
> even been registered by the time this has been called. This is
> literally
> an example of the "spin_lock_init(); spin_lock();" antipattern.

Yes, this function is called before PTP clock registeration. I will
remove the spin_lock for here.

> 
> > +
> > +     return 0;
> >  }