Re: [PATCH net-next v3 01/10] net: ethernet: ravb: Remove gPTP control from WoL setup and restore

[netdev-bot+sashiko]

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

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

- [High] The commit message's stated justification for the removal is
  factually wrong for the suspend leg, and the removal deletes the only…

--

net: ethernet: ravb: Remove gPTP control from WoL setup and restore

This drops the ccc_gac special case that called ravb_ptp_stop() from
ravb_wol_setup() and ravb_ptp_init() from ravb_wol_restore(), on the
grounds that ndo_open/ndo_close already start and stop the gPTP clock.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791e..36173a09e2fdb 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -3182,9 +3182,6 @@ static int ravb_wol_setup(struct net_device *ndev)
>  	/* Enable MagicPacket */
>  	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
>  
> -	if (priv->info->ccc_gac)
> -		ravb_ptp_stop(ndev);
> -
>  	return enable_irq_wake(priv->emac_irq);
>  }

[Severity: High]

Is the ndev really closed on the suspend leg when WoL is enabled?
ravb_suspend() returns immediately after ravb_wol_setup() without ever
calling ravb_close():

drivers/net/ethernet/renesas/ravb_main.c:ravb_suspend() {
	rtnl_lock();
	if (priv->wol_enabled) {
		ret = ravb_wol_setup(ndev);
		rtnl_unlock();
		return ret;
	}

	ret = ravb_close(ndev);
	...
}

The only ravb_close() in the WoL flow is at the end of ravb_wol_restore(),
i.e. on the resume leg. So the commit message statement:

  "As the ndev will always be closed and re-opened when suspending and
  resuming the system."

does not appear to hold for the suspend direction. Should the message be
adjusted, or is the intent to change behaviour here?

With ravb_ptp_stop() gone from ravb_wol_setup(), doesn't the WoL suspend
path lose the gPTP masking and IRQ draining that ravb_ptp_stop() performed?

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_stop() {
	ravb_ptp_disable(ndev);		/* GIC = 0, GIS = 0 */
	ravb_ptp_sync_irqs(ndev);	/* synchronize_irq(ndev->irq), err_irq, mgmt_irq */
	...
}

After this patch ravb_wol_setup() only clears RIC0/RIC2/TIC, narrows
ECSIPR, and synchronizes priv->emac_irq, so gPTP interrupt sources armed by
userspace (extts PTCE, perout PTME) stay enabled and the AVB-DMAC / err /
mgmt IRQ lines are never drained. Do the retained comments in
ravb_wol_setup() still describe the code?

	/* Disable interrupts by clearing the interrupt masks. */
	...
	/* Only allow ECI interrupts */
	synchronize_irq(priv->emac_irq);

Can a gPTP interrupt latched during the sleep then be replayed against a
block that ravb_wol_restore() is resetting? resume_device_irqs() runs in
the noirq phase, before the .resume callback, so:

ravb_interrupt()/ravb_multi_interrupt()
	if (iss & ISS_CGIS)
		ravb_ptp_interrupt(ndev);

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_interrupt() {
	gis &= ravb_read(ndev, GIC);
	...
	if (gis & GIS_PTMF) {
		...
		ravb_ptp_update_compare(priv, perout->target);
	}
	ravb_write(ndev, ~(gis | GIS_RESERVED), GIS);
}

can run while ravb_wol_restore() is driving the block through
CCC_OPC_RESET and config mode:

drivers/net/ethernet/renesas/ravb_main.c:ravb_wol_restore() {
	/* Set reset mode to rearm the WoL logic. */
	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
	...
	error = ravb_set_config_mode(ndev);
	...
}

Neither ravb_set_opmode() nor ravb_set_config_mode() takes priv->lock, so
is there anything serializing that window? Related, does the PHC stay
registered across a sleep in which the block is subsequently reset?

This also looks like it reverses commit b07bc55cbb1c ("net: ravb: Simplify
ravb_suspend()"), which moved ravb_ptp_stop() into ravb_wol_setup()
specifically because the WoL branch bypasses ravb_close(). Checking the end
of this series (up to "net: ethernet: ravb: Add gPTP support for Gen4"),
ravb_wol_setup() still has no gPTP stop, so no later patch restores it.

Would it work to keep the gPTP masking and IRQ draining in
ravb_wol_setup()? Dropping the ravb_ptp_init() from ravb_wol_restore()
below looks fine on its own, since ravb_close() there already stops gPTP.

> @@ -3204,9 +3201,6 @@ static int ravb_wol_restore(struct net_device *ndev)
>  	if (error)
>  		return error;
>  
> -	if (priv->info->ccc_gac)
> -		ravb_ptp_init(ndev, priv->pdev);
> -
>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  	napi_enable(&priv->napi[RAVB_BE]);

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