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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: ravb: Simplify gPTP start and stop

The patch folds the gptp-flag ravb_ptp_init() and ravb_ptp_stop() calls into
ravb_dmac_init() and ravb_stop_dma(), and drops the info->gptp part of the
two combined gptp/ccc_gac call sites in ravb_open() and ravb_close().  It is
described as having no functional change.

A few questions about the resulting start/stop balance below.

> 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 new stop reachable on all exits from ravb_stop_dma()?  It sits after
three unconditional error returns earlier in the same function:

	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, ravb_close(), ravb_set_ringparam(),
ravb_tx_timeout_work() and ravb_open()'s unwind all called ravb_ptp_stop()
unconditionally before ravb_stop_dma(), so a wait timeout did not affect the
unregister.

ravb_close() treats a ravb_stop_dma() failure as non-fatal:

	/* Set the config mode to stop the AVB-DMAC's processes */
	if (ravb_stop_dma(ndev) < 0)
		netdev_err(ndev,
			   "device will be stopped after h/w processes are done.\n");

and then goes on to free the rings, program CCC_OPC_RESET and drop the
runtime-PM reference.  For .gptp devices (ravb_gen2_hw_info,
ravb_rzv2m_hw_info) does that leave the PHC registered after ndo_stop has
returned success, with the gPTP interrupt enables still set because
ravb_ptp_disable()/ravb_ptp_sync_irqs() in ravb_ptp_stop() were skipped?

A subsequent open then runs ravb_dmac_init() -> ravb_ptp_init(), whose

	WRITE_ONCE(priv->ptp.clock, clock);

overwrites the only handle, while ravb_ptp_stop() only xchg()s the current
pointer.  Since ravb_remove() has no independent PTP unregister and ends in
free_netdev(), does the stranded registration leave ptp_clock_info pointing
into freed netdev private memory, reachable through the surviving
/dev/ptpN?

ravb_ptp_stop() looks idempotent (xchg of priv->ptp.clock), so would doing
the stop before the fallible ravb_wait() calls, or on all exit paths, keep
the old behaviour?

>  	/* 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) {

[Severity: High]
Same question for this call site: with the explicit pre-stop removed, does a
ravb_stop_dma() timeout here return with the interface detached and the Gen2
gPTP clock still registered?

[ ... ]

> @@ -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);
>  out_set_reset:
>  	ravb_set_opmode(ndev, CCC_OPC_RESET);

[Severity: High]
Does this leak the registered PTP clock for info->ccc_gac devices when
ravb_open() fails?

ravb_open() still calls ravb_ptp_init() for info->ccc_gac, but the
counterpart at out_ptp_stop: is gone, and the relocated teardown in
ravb_stop_dma() is gated on:

	if (info->gptp)
		ravb_ptp_stop(ndev);

info->gptp and info->ccc_gac are mutually exclusive (.gptp in
ravb_gen2_hw_info and ravb_rzv2m_hw_info, .ccc_gac in ravb_gen3_hw_info and
ravb_gen4_hw_info), so nothing unregisters the clock on this path.

The failure right after the init is a plain software path, ravb_phy_start()
-> ravb_phy_init() returns -ENOENT when of_phy_connect() fails, or
propagates an of_phy_register_fixed_link() error:

	error = ravb_phy_init(ndev);
	if (error)
		return error;

Since ndo_open failed, ndo_stop/ravb_close() is never called, so the
if (info->ccc_gac) ravb_ptp_stop(ndev); in ravb_close() does not run either.
Does that leave /dev/ptpN live on a block that ravb_open() then puts into
CCC_OPC_RESET and runtime-suspends, so gettime64/settime64/adjfine perform
MMIO with no runtime-PM reference held, and with GIC/GIS enables still set?

And on a later successful open, ravb_ptp_init()'s
WRITE_ONCE(priv->ptp.clock, clock) replaces the handle, so the first
registration can no longer be reached by ravb_ptp_stop().  With
ravb_remove() calling free_netdev() without any PHC unregister, does that
become a use-after-free through the surviving PTP chardev?

Would keeping if (info->ccc_gac) ravb_ptp_stop(ndev); at out_ptp_stop:
restore the balance?

> @@ -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:

[Severity: Medium]
Can TX-timeout recovery now resume traffic with the Gen2 gPTP clock left
unregistered?

In ravb_stop_dma() the new stop runs just before the fallible final
transition:

	/* Stop PTP Clock driver */
	if (info->gptp)
		ravb_ptp_stop(ndev);

	/* Stop AVB-DMAC process */
	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);

If that ravb_set_opmode() times out, ravb_tx_timeout_work() takes the
"hardware is still operating" branch:

	if (ravb_stop_dma(ndev)) {
		...
		ravb_rcv_snd_enable(ndev);
		goto out;
	}
	...
out:
	netif_tx_start_all_queues(ndev);

With the ravb_ptp_init() at out: removed, RX/TX and the TX queues are
re-enabled for .gptp devices while the PHC has been unregistered and the
gPTP interrupts cleared.  Previously this path always re-registered the
clock when resuming operation.  Does hardware timestamping and /dev/ptpN
now stay gone until a full close/open cycle?

[ ... ]