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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch erroneously removes `ravb_ptp_stop()` from `ravb_wol_setup()`, leaving the gPTP hardware active during a WoL-enabled suspend.
--

commit bbd624fcad5eb4869638d985be16666aae0f0ca2
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

This commit removes explicit gPTP stop/init calls from the Wake-on-LAN setup
and restore paths. The commit message states this is redundant as the device
is always closed and re-opened during suspend and resume.

> net: ethernet: ravb: Remove gPTP control from WoL setup and restore
> 
> Since commit a6a85ba36fd0 ("net: ravb: Move PTP initialization in the
> driver's ndo_open API for ccc_gac platorms") the gPTP clock (if
> supported) is stopped and started by opening and closing the ndev.
> 
> 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 this description accurate when Wake-on-LAN is enabled?

Looking at ravb_suspend() in drivers/net/ethernet/renesas/ravb_main.c,
ravb_close() is explicitly bypassed when wol_enabled is true:

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

	ret = ravb_close(ndev);

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
>
> 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]
Since ravb_close() is not called during a WoL-enabled suspend, doesn't removing
ravb_ptp_stop() here leave the gPTP hardware active during suspend?

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

[Severity: High]
If the PTP clock remains active during suspend, won't it be abruptly torn down
by the ravb_close(ndev) call that occurs later in this ravb_wol_restore()
function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=1