Re: [PATCH net-next v5 2/3] ptp: Add driver for R-Car Gen4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing return value check for pm_runtime_get_sync() before MMIO access.
- [Medium] Type mismatch and potential truncation in default_addend calculation.
- [Medium] Timekeeping precision loss during adjtime due to read-modify-write cycle.
- [Medium] Missing hardware disable in the ptp_clock_register() error path.
--

--- Patch [2]: [PATCH net-next v5 2/3] ptp: Add driver for R-Car Gen4 ---
commit 157f9604feea3029de3329fc9af614f925220b8a
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

ptp: Add driver for R-Car Gen4

This commit adds a driver for the gPTP timer found on R-Car Gen4 devices.
The timer is system-wide and shared by Ethernet devices, but it is gated by
different clocks depending on the specific SoC hardware.

> diff --git a/drivers/ptp/ptp_rcar_gen4.c b/drivers/ptp/ptp_rcar_gen4.c
> new file mode 100644
> index 0000000000000..8072d5f2b72dd
> --- /dev/null
> +++ b/drivers/ptp/ptp_rcar_gen4.c

[ ... ]

> +static void _ptp_rcar_gen4_settime(struct ptp_clock_info *ptp,
> +				   const struct timespec64 *ts)
> +{
> +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	iowrite32(1, priv->base + PTPTMDC_REG);
> +	iowrite32(0, priv->base + PTPTOVC20_REG);
> +	iowrite32(0, priv->base + PTPTOVC10_REG);
> +	iowrite32(0, priv->base + PTPTOVC00_REG);
> +	iowrite32(1, priv->base + PTPTMEC_REG);
> +	iowrite32(ts->tv_sec >> 32, priv->base + PTPTOVC20_REG);
> +	iowrite32(ts->tv_sec, priv->base + PTPTOVC10_REG);
> +	iowrite32(ts->tv_nsec, priv->base + PTPTOVC00_REG);
> +}

[ ... ]

> +static int ptp_rcar_gen4_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> +	struct timespec64 ts;
> +	unsigned long flags;
> +	s64 now;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	_ptp_rcar_gen4_gettime(ptp, &ts);
> +	now = ktime_to_ns(timespec64_to_ktime(ts));
> +	ts = ns_to_timespec64(now + delta);
> +	_ptp_rcar_gen4_settime(ptp, &ts);

[Severity: Medium]
Does this read-modify-write cycle introduce jitter and systematic time drift?

Every time ptp_rcar_gen4_adjtime() is called, it halts the hardware timer
via PTPTMDC_REG in _ptp_rcar_gen4_settime(). 

Any time elapsed during this software execution and the fractional
sub-nanosecond progress currently stored in the hardware accumulator might be
permanently lost.

> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return 0;
> +}

[ ... ]

> +static int ptp_rcar_gen4_probe(struct platform_device *pdev)
> +{
> +	struct ptp_rcar_gen4_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	unsigned long rate;

[ ... ]

> +	/* Default timer increment in ns.
> +	 * bit[31:27] - integer
> +	 * bit[26:0]  - decimal
> +	 * increment[ns] = perid[ns] * 2^27 => (1ns * 2^27) / rate[hz]
> +	 */
> +
> +	priv->default_addend = div_s64(1000000000LL << 27, rate);

[Severity: Medium]
Could this calculation result in type truncation and overflow?

The div_s64() function expects an s32 divisor, but it is being passed an
unsigned long 'rate'. If the clock rate exceeds 2.14 GHz, it overflows
s32 and evaluates as negative.

Additionally, the 64-bit result is passed directly to the 32-bit iowrite32()
shortly after this. If the clock rate is below ~31.25 MHz, the result
would exceed 32 bits and silently truncate. Would it be safer to use
div64_ul() and validate the bounds before writing to the register?

> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +
> +	iowrite32(priv->default_addend, priv->base + PTPTIVC0_REG);

[Severity: High]
Should the return value of pm_runtime_get_sync() be checked before proceeding
to access hardware?

If the device fails to power on or the clock fails to enable, ignoring the
negative error code and writing to PTPTIVC0_REG could cause an asynchronous
external abort, potentially leading to a kernel panic.

> +	iowrite32(1, priv->base + PTPTMEC_REG);
> +
> +	priv->clock = ptp_clock_register(&priv->info, dev);
> +	if (IS_ERR(priv->clock)) {
> +		pm_runtime_put_sync(dev);
> +		pm_runtime_disable(dev);
> +		return PTR_ERR(priv->clock);

[Severity: Medium]
Is a hardware disable step missing in this error path?

If ptp_clock_register() fails, the driver cleans up the PM state but fails
to write the disable command to PTPTMDC_REG. The timer might be left running
indefinitely.

The normal driver removal path explicitly disables the timer before
suspending PM:

ptp_rcar_gen4_remove() {
    ...
    iowrite32(1, priv->base + PTPTMDC_REG);
    pm_runtime_put_sync(dev);
}

> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721132936.1331318-1-niklas.soderlund+renesas@ragnatech.se?part=2