Re: [PATCH net-next v4 07/10] net: ethernet: ravb: Add callback for gPTP probe

[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: Add callback for gPTP probe

This adds a probe callback to struct ravb_gptp_info so each RAVB generation
can probe its gPTP timer clock its own way, and moves the GTI computation
into those per-generation callbacks. The gptp_ref_clk hardware flag is
dropped and RZ/V2M gets its own ravb_rzv2m_ptp_info with a probe callback
that fetches the "gptp" clock.

Two questions below about the gPTP callback error paths, both reached
through ravb_open().

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 9e9b1c4fbe956..1092e8ada188b 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c

[ ... ]

> @@ -2704,6 +2705,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
>  }
>  
>  static const struct ravb_gptp_info ravb_gen3_ptp_info = {
> +	.probe = ravb_gen2_ptp_probe,
>  	.set_config_mode = ravb_gen3_ptp_set_config_mode,
>  	.ndev_open = ravb_ptp_init,
>  	.ndev_close = ravb_ptp_stop,
>  };

[Severity: High]
Gen3 wires PTP registration to .ndev_open and teardown only to
.ndev_close, leaving .dmac_stop unset. In ravb_open() the clock is
registered before the PHY is started, and both failures share one label:

	/* Initialise PTP Clock driver */
	if (info->ptp && info->ptp->ndev_open) {
		error = info->ptp->ndev_open(ndev);
		if (error)
			goto out_ptp_stop;
	}

	/* PHY control start */
	error = ravb_phy_start(ndev);
	if (error)
		goto out_ptp_stop;
...
out_ptp_stop:
	ravb_stop_dma(ndev);

The only PTP teardown reachable from out_ptp_stop is in ravb_stop_dma():

	/* Stop PTP Clock driver */
	if (info->ptp && info->ptp->dmac_stop)
		info->ptp->dmac_stop(ndev);

Since ravb_gen3_ptp_info has no .dmac_stop, does a ravb_phy_start()
failure leave the ptp_clock registered by ravb_ptp_init() unregistered?

A failed ndo_open is not followed by ndo_stop, so ravb_ptp_stop() and its
ptp_clock_unregister() would not run at all. A later successful open calls
ravb_ptp_init() again and overwrites priv->ptp.clock, and on unbind
free_netdev() frees the ravb_private holding priv->ptp.info that the still
registered clock points at.

The baseline error label did this explicitly:

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

That call was removed earlier in this series by "net: ethernet: ravb:
Simplify gPTP start and stop", which states "There are no functional
change". Should that claim be revisited, or should out_ptp_stop also invoke
the ndev_close callback?

The same shape is still present at the end of the series (c7fa563):
ravb_gen3_ptp_info keeps .ndev_open = ravb_ptp_init with no .dmac_stop, and
out_ptp_stop still only calls ravb_stop_dma(). Gen4 is not affected at the
end of the series because ravb_gen4_ptp_info has no .ndev_open.

> @@ -2763,6 +2765,24 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
>  	.magic_pkt = 1,
>  };
>  
> +static int ravb_rzv2m_ptp_probe(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	priv->gptp_clk = devm_clk_get(&priv->pdev->dev, "gptp");
> +	if (IS_ERR(priv->gptp_clk))
> +		return PTR_ERR(priv->gptp_clk);
> +
> +	return ravb_compute_gti(ndev, priv->gptp_clk);
> +}
> +
> +static const struct ravb_gptp_info ravb_rzv2m_ptp_info = {
> +	.probe = ravb_rzv2m_ptp_probe,
> +	.set_config_mode = ravb_gen2_ptp_set_config_mode,
> +	.dmac_start = ravb_ptp_init,
> +	.dmac_stop = ravb_ptp_stop,
> +};

[Severity: Medium]
This isn't a bug introduced by this patch, but the .dmac_start =
ravb_ptp_init hook used here (and by ravb_gen2_ptp_info) can fail after the
DMA rings have been allocated. Can that leak the rings?

ravb_dmac_init() allocates first and calls the gPTP hook last:

	error = info->dmac_init(ndev);
	if (error)
		return error;
...
	/* Initialise PTP Clock driver */
	if (info->ptp && info->ptp->dmac_start)
		return info->ptp->dmac_start(ndev);

info->dmac_init() is ravb_dmac_init_rcar(), which allocates both queues via
ravb_ring_init() (RX/TX rings, page pools, SKB arrays and coherent
descriptor memory). Since "net: ethernet: ravb: Propagate error from
ptp_clock_register()" earlier in this series, ravb_ptp_init() returns the
ptp_clock_register() error, and ravb_dmac_init() passes it up to
ravb_open():

	/* Device init */
	error = ravb_dmac_init(ndev);
	if (error)
		goto out_set_reset;
...
out_ptp_stop:
	ravb_stop_dma(ndev);
out_set_reset:
	ravb_set_opmode(ndev, CCC_OPC_RESET);

Neither label calls ravb_ring_free(), and a failed ndo_open is not paired
with ndo_stop, so the rings allocated by ravb_dmac_init_rcar() stay
allocated. A later successful open overwrites priv->rx_ring[],
priv->tx_ring[] and priv->rx_pool[], and ravb_remove() does not free them
either.

The missing ravb_ring_free() on the ravb_open() error labels predates this
series: in the baseline ravb_dmac_init() could already fail at
ravb_set_opmode(CCC_OPC_OPERATION) after ring allocation, and a
ravb_phy_start() failure already returned through out_ptp_stop without
freeing the rings. Should the ravb_open() failure paths call
ravb_ring_free() for both queues?