Messages in this thread | | | From | <> | Subject | Re: [Patch net-next v1 04/12] net: dsa: microchip: ptp: Manipulating absolute time using ptp hw clock | Date | Fri, 2 Dec 2022 09:40:17 +0000 |
| |
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; > > }
| |