Re: [PATCH v2 04/10] ptp: qcom: Add PTP driver for the Qualcomm TSC hardware

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] ptp: qcom: torn 64-bit counter read in qcom_tsc_read_time() on
  rollover
- [High] ptp: qcom: period truncated to 0 for small negative ppb in
  adjfine()
- [Medium] ptp: qcom: ahb_clk and devres leak in
  qcom_tsc_enable_clocks() on error
- [Medium] ptp: qcom: uninitialized clock at probe and skipped config in
  qcom_tsc_settime()
--

commit 9c266c0baa9449c0c5ca7b1a7e003c4cbd0cb88b
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

ptp: qcom: Add PTP driver for the Qualcomm TSC hardware

This commit adds a PTP Hardware Clock driver for the Qualcomm Timestamp
Counter (TSC) hardware on SA8775P, QDU1000, and Nord SoCs.

> diff --git a/drivers/ptp/ptp_qcom_tsc.c b/drivers/ptp/ptp_qcom_tsc.c
> new file mode 100644
> index 0000000000000..82ddbdd63389d
> --- /dev/null
> +++ b/drivers/ptp/ptp_qcom_tsc.c
> @@ -0,0 +1,598 @@

[ ... ]

> +static int qcom_tsc_read_time(struct qcom_tsc *tsc, struct timespec64 *ts)
> +{
> +	const struct qcom_tsc_regs *regs = tsc->soc->reg_layout;
> +	u32 hi, lo;
> +
> +	if (!qcom_tsc_is_enabled(tsc))
> +		return -EINVAL;
> +
> +	hi = readl_relaxed(tsc->base + regs->read_cntcv_hi);
> +	lo = readl_relaxed(tsc->base + regs->read_cntcv_lo);

[Severity: High]
Can read_cntcv_lo roll over between reading read_cntcv_hi and
read_cntcv_lo?  If the lower word wraps between the two 32-bit reads, the
old upper word is paired with the post-rollover lower word, returning a
timestamp that jumps backward by 1 second (when rollover is true) or ~4.295
seconds (when rollover is false).  Because qcom_tsc_settime() and
qcom_tsc_update_offset() call qcom_tsc_read_time() to compute the delta or
new counter value written back to the hardware, a torn read during
settime64() or adjtime() also permanently shifts the hardware clock.
Re-reading read_cntcv_hi in a loop until two consecutive upper-word reads
match would avoid this.

[ ... ]

> +static int qcom_tsc_enable_clocks(struct qcom_tsc *tsc)
> +{
> +	int ret;
> +
> +	if (tsc->clocks_enabled)
> +		return 0;
> +
> +	ret = clk_prepare_enable(tsc->ahb_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(tsc->dev, qcom_tsc_clk_disable, tsc->ahb_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(tsc->cntr_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(tsc->dev, qcom_tsc_clk_disable, tsc->cntr_clk);
> +	if (ret)
> +		return ret;
> +
> +	tsc->clocks_enabled = true;
> +
> +	return 0;
> +}

[Severity: Medium]
If enabling tsc->cntr_clk or registering its devm action fails here, does
tsc->ahb_clk remain enabled with tsc->clocks_enabled still false?  Because
qcom_tsc_enable_clocks() runs from the runtime .settime64 callback rather
than probe, returning an error does not unwind devres actions on tsc->dev,
and each subsequent qcom_tsc_settime() call will prepare_enable
tsc->ahb_clk and queue another devres action again.

[ ... ]

> +static int qcom_tsc_drift_correction(struct qcom_tsc *tsc, long scaled_ppm)
> +{
> +	const struct qcom_tsc_soc_data *soc = tsc->soc;
> +	const struct qcom_tsc_regs *regs = soc->reg_layout;
> +	long ppb = scaled_ppm_to_ppb(scaled_ppm);
> +	u64 period, incval = 0;
> +	u32 regval = 0, subperiod;
> +
> +	if (ppb == 0)
> +		return 0;
> +
> +	if (ppb < 0) {
> +		ppb = -ppb;
> +		regval &= ~DRIFT_JUMP_SWALLOW;
> +
> +		period = div_u64(ppb, soc->cntr_res);
> +		if (period >= DRIFT_MAX_RES)
> +			period = DRIFT_MAX_RES;
> +	} else {
> +		regval |= DRIFT_JUMP_SWALLOW;
> +
> +		period = div_u64(ppb, soc->cntr_res);
> +		if (period >= DRIFT_MAX_RES) {
> +			period = DRIFT_MAX_RES;
> +			incval = soc->cntr_res * 2;
> +		} else if (ppb < soc->cntr_res) {
> +			incval = soc->cntr_res + ppb;
> +			period = 1;
> +		} else {
> +			incval = soc->cntr_res * 2;
> +		}
> +	}

[Severity: High]
Does the negative ppb branch truncate period to 0 when 0 < -ppb <
soc->cntr_res (for example, -63 <= ppb <= -1 on SA8775P where cntr_res is
64)?  Unlike the positive branch, which clamps period to 1 and adjusts
incval when ppb < soc->cntr_res, the negative branch leaves period = 0 and
incval = 0 and triggers drift_correct_cmd with a zero period, dropping fine
negative frequency adjustments during PTP synchronization.

[ ... ]

> +static int qcom_ptp_tsc_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	/* Both clocks are enabled on first use, when the counter is set */
> +	tsc->ahb_clk = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(tsc->ahb_clk))
> +		return PTR_ERR(tsc->ahb_clk);
> +
> +	tsc->cntr_clk = devm_clk_get(&pdev->dev, "cntr");
> +	if (IS_ERR(tsc->cntr_clk))
> +		return PTR_ERR(tsc->cntr_clk);
> +
> +	tsc->ptp_info = qcom_ptp_info;
> +
> +	tsc->ptp = ptp_clock_register(&tsc->ptp_info, &pdev->dev);

[Severity: Medium]
Should the clocks and hardware counter be initialized before
ptp_clock_register() exposes the PHC?  Because tsc->clocks_enabled starts
as false and the counter is only enabled in qcom_tsc_settime(), any
gettime64(), adjtime(), adjfine(), or virtual clock creation
(n_vclocks_store() -> ptp_vclock_register()) fails with -EINVAL until
userspace calls clock_settime().  In addition, if BIT(0) in control_cntcr
is already set at probe (from firmware or a prior driver bind/unbind, since
qcom_ptp_tsc_remove() does not disable the counter), the first
qcom_tsc_settime() call sees qcom_tsc_is_enabled(tsc) == true and takes the
qcom_tsc_update_offset() path without programming PULSE_WIDTH_MASK or
ROLLOVER_VAL:

drivers/ptp/ptp_qcom_tsc.c:qcom_tsc_settime() {
    ...
	if (qcom_tsc_is_enabled(tsc)) {
		qcom_tsc_read_time(tsc, &tod);
		ret = qcom_tsc_update_offset(tsc, timespec64_sub(*ts, tod));
		mutex_unlock(&tsc->lock);
		return ret;
	}

	/* Configure and enable the TSC counter */
	regval = readl_relaxed(tsc->base + regs->control_cntcr);
	regval &= ~PULSE_WIDTH_MASK;
	regval |= FIELD_PREP(PULSE_WIDTH_MASK, tsc->soc->pulse_width);
	writel_relaxed(regval, tsc->base + regs->control_cntcr);

	if (tsc->soc->rollover)
		writel_relaxed(ROLLOVER_VAL, tsc->base + regs->rollover_val);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-tscss-v2-0-d5f575010427@oss.qualcomm.com?part=4