Re: [PATCH net-next v3 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] The probe function leaks runtime PM references and leaves the device active if `ptp_clock_register()` fails.
- [High] The return value of `pm_runtime_get_sync()` is unchecked, which can lead to a synchronous external abort if powering the device fails.
- [High] The driver divides by the unchecked result of `clk_get_rate()`, risking a division by zero panic and potential type overflow.
- [Medium] The PTP clock is registered and exposed to user-space before the underlying hardware timer is enabled.
--

commit 523fe97d56330aedad6aa7c19dcf2bc206269fe3
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
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..ab0be2431be8d
> --- /dev/null
> +++ b/drivers/ptp/ptp_rcar_gen4.c
[ ... ]
> +static int ptp_rcar_gen4_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	priv->info = ptp_rcar_gen4_info;
> +
> +	/* 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, clk_get_rate(priv->clk));

[Severity: High]
Could this lead to a division by zero if clk_get_rate() returns 0?

Also, since div_s64() takes an s32 divisor, would an unsigned long clock rate
overflow to a negative value for rates above 2.14 GHz?  Should this verify
the rate is non-zero and use div64_ul() instead?

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

[Severity: High]
Does this need to check the return value of pm_runtime_get_sync()?

If the PM domain fails to power on and returns a negative error code,
unconditionally proceeding to write to the MMIO registers might cause a
synchronous external abort.

> +
> +	priv->clock = ptp_clock_register(&priv->info, dev);
> +	if (IS_ERR(priv->clock))
> +		return PTR_ERR(priv->clock);

[Severity: High]
If ptp_clock_register() fails, does this leave the PM usage counter
unbalanced?  It looks like this error path returns without calling
pm_runtime_put_sync() or pm_runtime_disable() to clean up the state
initialized earlier in ptp_rcar_gen4_probe().

[Severity: Medium]
Is there a race condition here?

By calling ptp_clock_register() before enabling the hardware timer on the
next line, is it possible for a fast user-space process to execute a gettime
or settime request on disabled hardware?

> +
> +	iowrite32(1, priv->base + PTPTMEC_REG);
> +
> +	return 0;
> +}

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