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

[Niklas Söderlund]

Hello Jakub,

The LLM feedback (except the one for the already existing theoretical 
error in the error path which I will not try to fix in this already big 
series) is based on the LLM hallucination on the commit message. I will 
try to rewrite it so please the LLM.

On 2026-09-14 18:54:33 -0700, Jakub Kicinski wrote:
> 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?

-- 
Kind Regards,
Niklas Söderlund