Re: [PATCH net-next v7 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] pm: unchecked pm_runtime_resume_and_get() return in ptp_rcar_gen4_probe()
- [High] pm: missing return value check for pm_runtime_resume_and_get()
- [Medium] ptp: missing hardware timer disable on ptp_clock_register() error path
- [Medium] ptp: potential time tearing in gettime due to unlatched sequential reads
- [Medium] ptp: torn hardware state exposed during _ptp_rcar_gen4_settime()
--

commit 3db5fb281fb9f619fc7eb84cff8bc9d6876c1eab
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..e82b6c5abc306
> --- /dev/null
> +++ b/drivers/ptp/ptp_rcar_gen4.c

[ ... ]

> +static void _ptp_rcar_gen4_gettime(struct ptp_clock_info *ptp,
> +				   struct timespec64 *ts)
> +{
> +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	ts->tv_nsec = ioread32(priv->base + PTPGPTPTM00_REG);
> +	ts->tv_sec = ioread32(priv->base + PTPGPTPTM10_REG) |
> +		((s64)ioread32(priv->base + PTPGPTPTM20_REG) << 32);
> +}

[Severity: Medium]
Can these sequential reads result in a torn time read?

If the time advances across a second boundary between the read of
PTPGPTPTM00_REG and the subsequent reads of PTPGPTPTM10_REG and
PTPGPTPTM20_REG, the returned timestamp could be incorrect by a full second
unless there is a hardware latch or a software retry loop.

Since this is also used in ptp_rcar_gen4_adjtime(), a torn read there would
corrupt the hardware clock state by writing the torn value back.

[ ... ]

> +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: Medium]
Does re-enabling the timer before the new time offsets are fully written
create a race window?

Between the write to PTPTMEC_REG (which enables the timer) and the final write
to PTPTOVC00_REG, the timer is actively running with a partially set offset.
Could concurrent hardware timestamping events observe an intermediate torn
state during this window?

[ ... ]

> +static int ptp_rcar_gen4_probe(struct platform_device *pdev)
> +{
> +	struct ptp_rcar_gen4_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	unsigned long rate;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);
> +
> +	rate = clk_get_rate(priv->clk);
> +	if (!rate)
> +		return -ENODEV;
> +
> +	spin_lock_init(&priv->lock);
> +
> +	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, rate);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_resume_and_get(dev);
> +
> +	iowrite32(priv->default_addend, priv->base + PTPTIVC0_REG);
> +	iowrite32(1, priv->base + PTPTMEC_REG);

[Severity: High]
If pm_runtime_resume_and_get() fails (e.g., due to power domain or clock
initialization failure), the device could remain suspended.
Could the subsequent iowrite32() calls then access unclocked or powered-off
hardware, resulting in a synchronous external abort or silent register
corruption?

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

[Severity: High]
If the previous pm_runtime_resume_and_get() had failed, wouldn't this
unconditional pm_runtime_put_sync() cause the PM usage counter to underflow?

[Severity: Medium]
If ptp_clock_register() fails, the error path cuts power, but doesn't it
miss disabling the hardware timer logically?

In ptp_rcar_gen4_remove(), the hardware timer is explicitly disabled by
writing to PTPTMDC_REG before cutting power:

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

Could suspending the PM power domain while the IP block's internal timer is
still logically enabled lead to hardware state corruption or prevent a clean
restart upon module reload?

> +		pm_runtime_disable(dev);
> +		return PTR_ERR(priv->clock);
> +	}
> +
> +	return 0;
> +}

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