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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver ignores the return value of `pm_runtime_resume_and_get()`, risking unpowered hardware accesses and unbalanced PM usage counters.
- [High] The `_ptp_rcar_gen4_settime()` function briefly configures the shared timer with 0 offsets while it is enabled, exposing erroneous timestamps to all sharing Ethernet devices.
- [Medium] The hardware timer is left enabled and running if `ptp_clock_register()` fails during probe.
- [Medium] Passing an `unsigned long` clock rate to `div_s64()` can cause a sign truncation bug if the clock rate is sufficiently high.
--

--- Patch [2]: [PATCH net-next v6 2/3] ptp: Add driver for R-Car Gen4 ---
commit 447a3dd2732c6247d6f684bcaa7abcce33006d04
Author: Niklas Söderlund <[email protected]>

ptp: Add driver for R-Car Gen4

Add driver for the gPTP timer found on R-Car Gen4 devices. The timer is
system-wide and shared by different Ethernet devices on each Gen4
platform.

> diff --git a/drivers/ptp/ptp_rcar_gen4.c b/drivers/ptp/ptp_rcar_gen4.c
> new file mode 100644
> index 0000000000000..e82b6c5abc306
> --- /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);

[Severity: High]
Could this expose a window where the hardware runs with zeroed offsets?

The timer is enabled by writing to PTPTMEC_REG before the actual time offsets
are written to the PTPTOVC* registers. Since this timer is shared across the
system, could this brief window result in bogus timestamps being generated for
connected Ethernet devices?

Should the timer be enabled after the new offsets are written?

> +}

[ ... ]

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

[ ... ]

> +	/* 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]
Is it safe to pass the unsigned long rate to div_s64()?

Since div_s64() expects an s32 divisor, could this cause a sign truncation
regression if the clock rate exceeds INT_MAX (2.147 GHz), resulting in a
negative default_addend calculation?

Should this use div64_ul() or div_u64() instead to prevent unintended
truncation?

> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_resume_and_get(dev);

[Severity: High]
Is it safe to ignore the return value of pm_runtime_resume_and_get()?

If the device fails to power on and returns a negative error code, the driver
proceeds to access hardware via the iowrite32() calls below, which could lead
to an asynchronous external abort and system crash on ARM platforms.

Additionally, if it fails, the usage counter isn't incremented. Would the
unconditional pm_runtime_put_sync() calls in the error path below and in
ptp_rcar_gen4_remove() cause a PM usage counter underflow?

> +
> +	iowrite32(priv->default_addend, priv->base + PTPTIVC0_REG);
> +	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]
If ptp_clock_register() fails, does this error path leave the hardware timer
running?

The timer is started just above this check by writing to PTPTMEC_REG, but
unlike ptp_rcar_gen4_remove(), this path doesn't write to PTPTMDC_REG to
disable it. Could this leak hardware resources and prevent the PM domain from
cleanly suspending?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2