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

Arun.Ramadoss
2022-12-02T09:40:39Z

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