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

[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: Remove gPTP control from WoL setup and restore

This removes the ccc_gac special case that stopped the gPTP clock in
ravb_wol_setup() and re-initialized it in ravb_wol_restore(), on the
grounds that ravb_open()/ravb_close() already handle gPTP start/stop.
A few questions about the suspend side of that reasoning below.

>     This makes the special case to stop and start it when resuming from WoL
>     redundant. As the ndev will always be closed and re-opened when
>     suspending and resuming the system.

[Severity: High]
Is the statement "the ndev will always be closed and re-opened when
suspending and resuming the system" accurate for the WoL path?  In
ravb_suspend() the WoL case returns before ravb_close() is reached:

	rtnl_lock();
	if (priv->wol_enabled) {
		ret = ravb_wol_setup(ndev);
		rtnl_unlock();
		return ret;
	}

	ret = ravb_close(ndev);

So on a WoL suspend, ravb_close() (and therefore its
"if (info->gptp || info->ccc_gac) ravb_ptp_stop(ndev);") does not run at
all during suspend; it only runs later, on the resume side, from the
ravb_close() call at the end of ravb_wol_restore().  Does that make the
suspend-side removal something other than a redundancy cleanup?

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791..36173a09e2fd 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]
Following on from the commit message question: was this call also doing
gPTP interrupt masking that nothing else on this path does?  ravb_ptp_stop()
does more than unregister the PHC:

drivers/net/ethernet/renesas/ravb_ptp.c:
static void ravb_ptp_disable(struct net_device *ndev)
{
	ravb_write(ndev, 0, GIC);
	ravb_write(ndev, 0, GIS);
}
...
void ravb_ptp_stop(struct net_device *ndev)
{
	...
	ravb_ptp_disable(ndev);
	ravb_ptp_sync_irqs(ndev);

ravb_wol_setup() clears RIC0/RIC2/TIC and writes ECSIPR, and only
synchronizes and wake-enables priv->emac_irq.  It never touches GIC/GIS,
and it does not synchronize ndev->irq/err_irq/mgmt_irq.  The GIC bits are
armed from userspace via ravb_ptp_extts() (GIC_PTCE) and ravb_ptp_perout()
(GIC_PTME) and are cleared only by ravb_ptp_disable().

Since the WoL path also skips pm_runtime_force_suspend() and
reset_control_assert(), the block stays powered while suspended, so a
programmed compare/capture can still fire on the now-suspended non-wake
line and be latched and replayed by resume_device_irqs().  ravb uses
SYSTEM_SLEEP_PM_OPS(ravb_suspend, ravb_resume), so that replay happens
before ravb_resume() runs.

Can the replayed interrupt then reach ravb_ptp_interrupt() while
ravb_wol_restore() is resetting the block?

	ravb_interrupt()/ravb_multi_interrupt()
	  -> ravb_ptp_interrupt()
	       gis &= ravb_read(ndev, GIC);
	       if ((gis & GIS_PTCF) && clock) {
		       ...
		       event.timestamp = ravb_read(ndev, GCPT);
		       ptp_clock_event(clock, &event);

priv->ptp.clock is still non-NULL after this change, so would that push a
timestamp read from a just-reset GCPT to userspace, and RMW GIS/GCCR/GPTC
concurrently with the unlocked ravb_set_opmode()/ravb_set_config_mode() in
ravb_wol_restore()?

Also, does the remaining "/* Only allow ECI interrupts */" comment still
describe the code for ccc_gac parts, and is the PHC (/dev/ptpN) now meant
to stay registered for the whole suspended interval?

The restore-side removal of ravb_ptp_init() does look redundant; would it
make sense to keep the suspend-side masking and synchronization, or to
spell out in the commit message why dropping it is safe?

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

[Severity: Medium]
After this change, is ravb_wol_restore() the only place that drives the
AVB-DMAC through CCC_OPC_RESET while the gPTP PHC is still registered?
The reset and config-mode re-entry happen at the top of the function:

	/* Set reset mode to rearm the WoL logic. */
	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
	if (error)
		return error;

	/* Set AVB config mode. */
	error = ravb_set_config_mode(ndev);

The other opmode transition sites stop the PHC first.  ravb_close():

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

and the ravb_open() error path:

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

The reset drops the GCCR TCSS=ADJGPTP selection and GTI programming that
ravb_ptp_init() установ establishes, and priv->ptp.default_addend /
current_addend still describe the pre-reset hardware; the clock is
unregistered only later, by the ravb_close() at the end of the same
function.  Pre-patch the ccc_gac branch unregistered the PHC in
ravb_wol_setup() and re-created it here, so no registered PHC spanned the
reset.  Should ravb_wol_restore() follow the same ordering as its peers?

>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  	napi_enable(&priv->napi[RAVB_BE]);

[Severity: High]
This isn't a bug introduced by this patch, but while both functions are
being touched: can the NAPI enable/disable counts get out of balance on
the WoL error paths, and hang in napi_disable()?

ravb_wol_setup() disables both NAPI instances and then returns the
enable_irq_wake() result without re-enabling them:

	if (info->nc_queues)
		napi_disable(&priv->napi[RAVB_NC]);
	napi_disable(&priv->napi[RAVB_BE]);
	ravb_write(ndev, ECSIPR_MPDIP, ECSIPR);
	...
	return enable_irq_wake(priv->emac_irq);

ravb_suspend() propagates that error directly.  On the restore side, the
two early returns above skip the napi_enable() calls when ravb_wait()
times out.

Since the WoL suspend path never closes the interface, the netdev stays
netif_running() with NAPI disabled, and a failed suspend callback leaves
dev->power.is_suspended clear so ravb_resume() is skipped.  The next
ravb_close() (from "ip link set dev X down" or unregister_netdev()), or a
retried suspend re-entering ravb_wol_setup(), calls napi_disable() again:

net/core/dev.c:napi_disable_locked() {
	...
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}

		new = val | NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC;
	...
}

A completed disable leaves SCHED|NPSVC set and only napi_enable() clears
them, so does the second disable spin here with no timeout while rtnl and
the netdev instance lock are held?