Re: [PATCH net-next v3 03/10] net: ethernet: ravb: Simplify gPTP start and stop

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

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

- [High] ravb_open() leaks the registered PTP clock for info->ccc_gac
  devices (R-Car Gen3/Gen4) when ravb_phy_start() fails.
- [High] Folding ravb_ptp_stop() into the tail of ravb_stop_dma()
  (ravb_main.c:1126-1128) makes gPTP teardown conditional on the AVB-…

--

net: ethernet: ravb: Simplify gPTP start and stop

This moves the info->gptp gPTP clock start and stop into ravb_dmac_init()
and ravb_stop_dma() so the gPTP clock follows the AVB-DMAC, and drops the
info->gptp part of the combined "gptp || ccc_gac" conditions in
ravb_open() and ravb_close().  The commit message states "There are no
functional change".  Two behaviour differences below look like they may
not hold up.

[Severity: High]

Is the gPTP stop still guaranteed to run for info->gptp parts?  Placing it
here puts it behind the three ravb_wait() timeouts earlier in
ravb_stop_dma():

	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
	if (error)
		return error;

	error = ravb_wait(ndev, CSR, CSR_TPO0 | CSR_TPO1 | CSR_TPO2 | CSR_TPO3,
			  0);
	if (error)
		return error;
	...
	error = ravb_wait(ndev, CSR, CSR_RPO, 0);
	if (error)
		return error;

Before this patch every caller ran the stop unconditionally before
ravb_stop_dma(), so a wedged DMAC could not skip it.  Now ravb_close()
only logs the failure and keeps going:

	if (ravb_stop_dma(ndev) < 0)
		netdev_err(ndev,
			   "device will be stopped after h/w processes are done.\n");

so on a .gptp = 1 part (ravb_gen2_hw_info, ravb_rzv2m_hw_info) the PHC
stays registered across close.  A later open then goes
ravb_open() -> ravb_dmac_init() -> ravb_ptp_init(), which registers a
second clock and overwrites the pointer without checking:

	clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
	...
	WRITE_ONCE(priv->ptp.clock, clock);

Does that permanently orphan the first registration?  ravb_remove() does
no PTP teardown before free_netdev(), and the orphaned clock's
ptp_clock_info lives in the netdev private area, so its ops would point
at freed memory after unbind.

Skipping ravb_ptp_stop() on those early returns also skips the interrupt
quiescing it performs:

	ravb_ptp_disable(ndev);
	ravb_ptp_sync_irqs(ndev);

ravb_close() then walks and frees priv->ts_skb_list without priv->lock,
while ravb_timestamp_interrupt() -> ravb_get_tx_tstamp() can still be
doing list_del() on the same list.  Can that corrupt the list?

There is also the reverse ordering.  If ravb_stop_dma() fails only in its
final ravb_set_opmode(ndev, CCC_OPC_CONFIG), the ravb_ptp_stop() above has
already unregistered the clock, and ravb_tx_timeout_work() takes the
"goto out" path whose ravb_ptp_init() is removed by this patch (hunk
below), leaving a running interface with no PHC until an administrative
down/up.  Same question for ravb_set_ringparam(): its

		error = ravb_stop_dma(ndev);
		if (error) {
			netdev_err(ndev,
				   "cannot set ringparam! Any AVB processes are still running?\n");
			return error;
		}

now returns with the gPTP clock still registered where previously it had
been stopped first.

[ ... ]

[Severity: High]

Does this leak the registered PTP clock for info->ccc_gac parts?

ravb_open() still registers it just above:

	if (info->ccc_gac)
		ravb_ptp_init(ndev, priv->pdev);

and the next fallible step is ravb_phy_start(), which jumps here.  The
replacement stop folded into ravb_stop_dma() is gated on info->gptp, and
that flag is mutually exclusive with ccc_gac (ravb_gen2_hw_info and
ravb_rzv2m_hw_info set .gptp = 1, ravb_gen3_hw_info and ravb_gen4_hw_info
set .ccc_gac = 1), so nothing unregisters the clock on this path.

ravb_close() is the only remaining ccc_gac ravb_ptp_stop() site, and it is
not reached because ndo_open returned an error.  The device is then put in
CCC_OPC_RESET and runtime suspended while /dev/ptpN stays alive with
ptp_clock_info pointing into the netdev private area.  Each further
"ip link set ethX up" attempt registers another clock and orphans the
previous one via the WRITE_ONCE(priv->ptp.clock, clock) in
ravb_ptp_init(), and ravb_remove() frees the netdev without unregistering
anything.

 out_set_reset:
 	ravb_set_opmode(ndev, CCC_OPC_RESET);

[ ... ]

[ ... ]

Both concerns come down to the same question: since ravb_stop_dma() can
return early, and its callers treat that failure differently, is the
folded start/stop still symmetric on every exit path, or would performing
the stop before the ravb_wait() loops (or leaving it at the call sites)
keep the previous behaviour?

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