Re: [PATCH net-next v2 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 return value check for pm_runtime_get_sync() before hardware access.
- [High] Potential divide-by-zero kernel panic due to unchecked clk_get_rate().
- [High] The driver lacks system PM callbacks to restore hardware register state after a suspend/resume cycle.
- [Medium] Missing PM runtime cleanup on the ptp_clock_register() error path.
- [Medium] The PTP clock is registered and exposed to userspace before the hardware timer is enabled.
--

commit 0a2d69b0a8fcc58f8e457187381c743ade641986
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

ptp: Add driver for R-Car Gen4

This commit adds a new driver for the gPTP timer found on R-Car Gen4
devices. The clock can be used by Ethernet drivers for PTP time
synchronization.

> 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)
> +{
> +	struct ptp_rcar_gen4_priv *priv;
> +	struct device *dev = &pdev->dev;

[ ... ]

> +	/* 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]
If clk_get_rate() returns 0 because the hardware clock is unconfigured or
fails to determine the rate, does this div_s64() result in a divide-by-zero
panic during the probe sequence?

Would it be better to verify the rate is strictly greater than zero before
performing the division?

> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

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

If the device fails to resume, perhaps due to a power domain failure,
could accessing its registers via iowrite32() cause a synchronous external
abort and lead to a kernel crash?

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

[Severity: Medium]
If ptp_clock_register() fails and the function returns the error directly,
does this leave the runtime PM reference permanently incremented?

It looks like pm_runtime_put_sync() and pm_runtime_disable() are not called
on this error path.

[Severity: Medium]
Is it possible for userspace to interact with the device before the hardware
timer is enabled?

The ptp_clock_register() function exposes the PTP character device, but the
timer is only enabled via PTPTMEC_REG after this succeeds. Could a process
open the device and read invalid time values during this race window?

> +
> +	iowrite32(1, priv->base + PTPTMEC_REG);
> +
> +	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 system PM callbacks to handle suspend and resume?

Without them, if the system enters deep sleep and the SoC power domain
shuts off the device, the internal register state, such as the default
addend and enable bit, might be lost. Would the PTP clock hardware remain
stopped and broken after the system wakes up?

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