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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new drivers/ptp/ptp_rcar_gen4.c is a second, standalone
  driver for a gPTP block that rtsn (V4H) and rswitch (S4) already map…
- [Medium] ptp_rcar_gen4_probe() calls pm_runtime_resume_and_get(dev) and
  throws away the result.
- [Low] ptp_rcar_gen4_probe() programs the increment and starts the timer
  (iowrite32(1, PTPTMEC_REG), line 182) before ptp_clock_register().

--

ptp: Add driver for R-Car Gen4

This commit adds a standalone PTP clock driver for the gPTP timer found
on R-Car Gen4 SoCs (S4, V4H and V4M). The timer is described as
system-wide and shared by the Ethernet devices. On S4 and V4H it shares
its clock and reset with RSWITCH or RTSN respectively.

> 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
> @@ -0,0 +1,225 @@

[ ... ]

> +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);

[Severity: Medium]
How is this driver meant to coexist with the existing users of the same
gPTP block on S4 and V4H?

On V4H, tsn0 in r8a779g0.dtsi already lists this register range as its
"gptp" resource:

arch/arm64/boot/dts/renesas/r8a779g0.dtsi:
	reg = <0 0xe6460000 0 0x7000>,
	      <0 0xe6449000 0 0x500>;
	reg-names = "tsnes", "gptp";

rtsn_probe() requests and maps that range:

drivers/net/ethernet/renesas/rtsn.c:rtsn_probe() {
    ...
	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gptp");
    ...
	ptpaddr = devm_ioremap_resource(&pdev->dev, res);
    ...
	priv->ptp_priv = rcar_gen4_ptp_alloc(pdev, ptpaddr);
    ...
}

The binding example for this driver uses the same reg = <0xe6449000
0x500>, and so does the r8a779g0 gptp node added later in this series.
devm_ioremap_resource() and devm_platform_ioremap_resource() both call
request_mem_region().

If a board enables both nodes, wouldn't the second driver to probe fail
with -EBUSY? That would lose either the TSN port or the new PHC. The
later gptp node is added disabled, so no in-tree board hits this yet.

On S4, rswitch_probe() maps its whole register region and reaches the
gPTP inside it:

drivers/net/ethernet/renesas/rswitch_main.c:rswitch_probe() {
    ...
	priv->addr = devm_ioremap_resource(&pdev->dev, res);
    ...
	priv->ptp_priv =
		rcar_gen4_ptp_alloc(pdev, priv->addr + RSWITCH_GPTP_OFFSET_S4);
    ...
}

An S4 gptp node looks like it would collide in the same way.

This driver is also very close to the existing
drivers/net/ethernet/renesas/rcar_gen4_ptp.c, which rtsn and rswitch
already use. Both have the same PTPTMEC, PTPTMDC, PTPTIVC0, PTPTOVC*
and PTPGPTPTM* register map, and the same adjfine, gettime, settime
and adjtime logic. The commit message doesn't mention that code, and
neither consumer is converted.

The Kconfig help says the clock "can then be used by Gen4 Ethernet
drivers". In-tree, only ravb consumes an external timer, through the
"ptp-timer" phandle and ptp_clock_index_by_of_node(). rtsn and rswitch
drive the block privately, so the shared-timer setup described in the
commit message doesn't seem reachable on S4 and V4H.

Could the commit message say how the two implementations relate? Or
should rtsn and rswitch be converted to use this PHC?

[ ... ]

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

[Severity: Medium]
Should the return value of pm_runtime_resume_and_get() be checked here?

pm_runtime_resume_and_get() calls pm_runtime_get_active(dev, 0), which
drops the usage reference itself when the resume fails:

include/linux/pm_runtime.h:pm_runtime_get_active() {
	...
	ret = __pm_runtime_resume(dev, RPM_GET_PUT | rpmflags);
	if (ret < 0) {
		pm_runtime_put_noidle(dev);
		return ret;
	}
	...
}

If the clock enable or power domain power-on fails, probe still writes
PTPTIVC0 and PTPTMEC to a module that isn't clocked. It then registers
a PHC whose gettime64, settime64, adjfine and adjtime callbacks access
the same unpowered registers. Userspace can reach those callbacks
through clock_gettime() and the PTP ioctls.

The ptp_clock_register() error path below and ptp_rcar_gen4_remove()
both call pm_runtime_put_sync() for a reference that was never taken.
That ends in rpm_drop_usage_count(), which warns "Runtime PM usage
count underflow!" and returns -EINVAL.

The v6 changelog says pm_runtime_resume_and_get() was chosen on
purpose. Was the usual check and bail-out after it left out by
accident?

> +
> +	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: Low]
If ptp_clock_register() fails, does this leave the timer running?

The timer is started with iowrite32(1, priv->base + PTPTMEC_REG) before
registration. This error path never writes PTPTMDC_REG, but
ptp_rcar_gen4_remove() does:

	iowrite32(1, priv->base + PTPTMDC_REG);

On S4 and V4H the module clock is shared with RSWITCH or RTSN.
Dropping this device's runtime PM reference therefore may not gate
the clock. The counter would keep running with this driver's increment
and no software owner.

rcar_gen4_ptp_register() avoids this by setting PTPTMEC only after
ptp_clock_register() succeeds. Could the same ordering be used here,
or could PTPTMDC be written on this error path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110080727.2377040-1-niklas.soderlund%2Brenesas%40ragnatech.se