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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing error handling for `pm_runtime_get_sync()` leads to a potential kernel crash and PM usage counter leak.
- [High] Lack of system suspend/resume callbacks causes the hardware timer to lose its state after system sleep.
- [High] Missing bounds validation for `priv->default_addend` allows silent integer truncation for low clock rates.
- [High] Hardware reset coordination flaw allows Ethernet drivers to asynchronously wipe the PTP hardware state.
- [Medium] The hardware timer is left enabled if `ptp_clock_register()` fails during probe.
--

commit 92b19ef008b7527d155231279eeb173dccb9a8de
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
> --- /dev/null
> +++ b/drivers/ptp/ptp_rcar_gen4.c

[ ... ]

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

[Severity: High]
The commit message mentions this gPTP timer shares its reset property with
the system's ethernet devices (like RTSN on V4H).

What happens when the ethernet driver asserts this shared reset line, such
as during network interface up/down cycles?

Since this driver doesn't manage the shared reset line or coordinate state
restoration, it seems the ethernet driver's actions might asynchronously
clear the PTP hardware state (resetting the increment and disabling the
timer) and silently break system time synchronization.

> +	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);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +
> +	iowrite32(priv->default_addend, priv->base + PTPTIVC0_REG);

[Severity: High]
Can priv->default_addend exceed the 32-bit limit of iowrite32() here?

The default addend is calculated as a 64-bit value using div_s64(). If the
clock rate is less than ~31.25 MHz (such as a standard 25 MHz Ethernet
reference clock), the resulting value will be larger than UINT_MAX.

Since iowrite32() expects a 32-bit integer, won't this silently truncate
the upper bits and cause the timer to run at a severely skewed rate?

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

If the device fails to power up, the subsequent iowrite32() could cause a
synchronous external abort or bus error when accessing the powered-down IP
block, leading to a kernel panic.

Also, if pm_runtime_get_sync() fails, does this leak the runtime PM usage
counter, since there is no matching pm_runtime_put_noidle() on the error
path before returning?

> +	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, the driver aborts probe and cleans up the
software PM state.

Should it also disable the hardware timer by calling iowrite32(1,
priv->base + PTPTMDC_REG) here? Otherwise, the hardware is left active and
running while the driver unloads.

> +
> +	return 0;
> +}

[ ... ]

> +static struct platform_driver ptp_rcar_gen4_driver = {
> +	.driver = {
> +		.name = "ptp-rcar-gen4",
> +		.of_match_table = ptp_rcar_gen4_of_match,
> +	},
> +	.probe    = ptp_rcar_gen4_probe,
> +	.remove   = ptp_rcar_gen4_remove,
> +};

[Severity: High]
Does this driver need to handle system suspend/resume events?

When the system enters a sleep state like S3, the platform power domain
typically powers down the device, clearing hardware registers like
PTPTIVC0_REG and PTPTMEC_REG.

Without dev_pm_ops suspend and resume callbacks to restore the timer
increment and re-enable the timer, will the PTP hardware remain permanently
uninitialized and broken after a system sleep cycle?

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