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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a89f6fe7866fe..2d0022a4f85c3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -707,7 +707,15 @@ static int ravb_dmac_init(struct net_device *ndev)
>  		return error;
>  
>  	/* Setting the control will start the AVB-DMAC process. */
> -	return ravb_set_opmode(ndev, CCC_OPC_OPERATION);
> +	error = ravb_set_opmode(ndev, CCC_OPC_OPERATION);
> +	if (error)
> +		return error;
> +
> +	/* Initialise PTP Clock driver */
> +	if (info->gptp)
> +		ravb_ptp_init(ndev, priv->pdev);
> +
> +	return 0;
>  }
>  
>  static void ravb_get_tx_tstamp(struct net_device *ndev)
> @@ -1115,6 +1123,10 @@ static int ravb_stop_dma(struct net_device *ndev)
>  			netdev_err(ndev, "failed to stop AXI BUS\n");
>  	}
>  
> +	/* Stop PTP Clock driver */
> +	if (info->gptp)
> +		ravb_ptp_stop(ndev);
> +

[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.

>  	/* Stop AVB-DMAC process */
>  	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
>  }
> @@ -1719,9 +1731,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
>  
>  	if (netif_running(ndev)) {
>  		netif_device_detach(ndev);
> -		/* Stop PTP Clock driver */
> -		if (info->gptp)
> -			ravb_ptp_stop(ndev);
> +
>  		/* Wait for DMA stopping */
>  		error = ravb_stop_dma(ndev);
>  		if (error) {

[ ... ]

> @@ -1961,7 +1967,7 @@ static int ravb_open(struct net_device *ndev)
>  	ravb_emac_init(ndev);
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> +	if (info->ccc_gac)
>  		ravb_ptp_init(ndev, priv->pdev);
>  
>  	/* PHY control start */
> @@ -1974,9 +1980,6 @@ static int ravb_open(struct net_device *ndev)
>  	return 0;
>  
>  out_ptp_stop:
> -	/* Stop PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> -		ravb_ptp_stop(ndev);
>  	ravb_stop_dma(ndev);

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

[ ... ]

> @@ -2020,10 +2023,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  
>  	netif_tx_stop_all_queues(ndev);
>  
> -	/* Stop PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_stop(ndev);
> -
>  	/* Wait for DMA stopping */
>  	if (ravb_stop_dma(ndev)) {
>  		/* If ravb_stop_dma() fails, the hardware is still operating

[ ... ]

> @@ -2056,10 +2055,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	ravb_emac_init(ndev);
>  
>  out:
> -	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev, priv->pdev);
> -
>  	netif_tx_start_all_queues(ndev);
>  
>  out_unlock:
> @@ -2374,7 +2369,7 @@ static int ravb_close(struct net_device *ndev)
>  	}
>  
>  	/* Stop PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> +	if (info->ccc_gac)
>  		ravb_ptp_stop(ndev);
>  
>  	/* Set the config mode to stop the AVB-DMAC's processes */

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