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

From: Vladimir Oltean
Date: Wed Nov 30 2022 - 20:05:08 EST


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