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

[Vladimir Oltean]

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