Messages in this thread | | | Date | Thu, 1 Dec 2022 03:04:57 +0200 | From | Vladimir Oltean <> | Subject | Re: [Patch net-next v1 04/12] net: dsa: microchip: ptp: Manipulating absolute time using ptp hw clock |
| |
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?
> + ptp_data->clock_time = *ts; > + spin_unlock_bh(&ptp_data->clock_lock); > > error_return: > mutex_unlock(&ptp_data->lock); > @@ -254,6 +269,7 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta) > { > struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp); > struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data); > + struct timespec64 delta64 = ns_to_timespec64(delta); > s32 sec, nsec; > u16 data16; > int ret; > @@ -286,15 +302,51 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta) > data16 |= PTP_STEP_DIR; > > ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16); > + if (ret) > + goto error_return; > + > + spin_lock_bh(&ptp_data->clock_lock); > + ptp_data->clock_time = timespec64_add(ptp_data->clock_time, delta64); > + spin_unlock_bh(&ptp_data->clock_lock); > > error_return: > mutex_unlock(&ptp_data->lock); > return ret; > } > > +/* 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?
> + > + 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.
> + > + return 0; > }
| |