Re: [PATCH net-next v5 02/10] net: ethernet: ravb: Move programming of gPTP timer interval

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In ravb_ptp_init() (drivers/net/ethernet/renesas/ravb_ptp.c),
  the patch writes priv->gti_tiv to GTI, requests a timer-increment load…
- [Medium] The commit message presents the change as moving the GTI
  programming out of ravb_open() into ravb_ptp_init() (plus a locking…

Pre-existing issues:
- [Medium] ravb_ptp_init() discards the return value of ravb_wait(ndev,
  GCCR, GCCR_TCR, GCCR_TCR_NOREQ) and unconditionally continues: it…

--

net: ethernet: ravb: Move programming of gPTP timer interval

This removes ravb_set_gti() from ravb_main.c and moves the GTI write plus
the GCCR.LTI load request into ravb_ptp_init(), where it is now done under
priv->lock.  ravb_ptp_init() also stops seeding priv->ptp.default_addend
from the GTI register and uses the cached priv->gti_tiv instead.

A few questions below.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 36173a09e2fdb..a89f6fe7866fe 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1846,20 +1846,6 @@ static int ravb_set_config_mode(struct net_device *ndev)
>  	return error;
>  }
>  
> -static void ravb_set_gti(struct net_device *ndev)
> -{
> -	struct ravb_private *priv = netdev_priv(ndev);
> -	const struct ravb_hw_info *info = priv->info;
> -
> -	if (!(info->gptp || info->ccc_gac))
> -		return;
> -
> -	ravb_write(ndev, priv->gti_tiv, GTI);
> -
> -	/* Request GTI loading */
> -	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> -}
> -

[ ... ]

> @@ -1974,8 +1960,6 @@ static int ravb_open(struct net_device *ndev)
>  
>  	ravb_emac_init(ndev);
>  
> -	ravb_set_gti(ndev);
> -
>  	/* Initialise PTP Clock driver */
>  	if (info->gptp || info->ccc_gac)
>  		ravb_ptp_init(ndev, priv->pdev);

[Severity: Medium]
The commit message describes this as moving the programming out of the RAVB
main logic into the gPTP init function, but ravb_open() is not the only
caller of ravb_ptp_init().  ravb_set_ringparam() and ravb_tx_timeout_work()
also call it, and neither of those ever called ravb_set_gti() before, nor do
they reset the device (ravb_set_ringparam() only stops DMA, and
ravb_tx_timeout_work() can reach its "out:" label even after ravb_stop_dma()
failed):

drivers/net/ethernet/renesas/ravb_main.c:ravb_set_ringparam() {
	...
	/* Initialise PTP Clock driver */
	if (info->gptp)
		ravb_ptp_init(ndev, priv->pdev);

	netif_device_attach(ndev);
	...
}

On those paths GTI still holds whatever ravb_ptp_adjfine() last programmed:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_adjfine() {
	addend = (u32)adjust_by_scaled_ppm(priv->ptp.default_addend, scaled_ppm);
	...
	ravb_write(ndev, addend & GTI_TIV, GTI);
}

So does an "ethtool -G" ring reconfiguration, or a TX watchdog timeout, now
silently reset the PHC frequency back to nominal and discard the frequency
adjustment userspace applied?

The new behaviour may well be the more correct one, since the old code
re-seeded default_addend from an already adjusted register value and later
adjfine() scaling then compounded off a drifted base.  If that is the intent,
could the commit message mention the behaviour change, and should this carry
a Fixes tag?

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 43218bc15b151..ac91a6813368d 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -321,11 +321,20 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
>  
>  	priv->ptp.info = ravb_ptp_info;
>  
> -	priv->ptp.default_addend = ravb_read(ndev, GTI);
> +	priv->ptp.default_addend = priv->gti_tiv;
>  	priv->ptp.current_addend = priv->ptp.default_addend;
>  
>  	spin_lock_irqsave(&priv->lock, flags);
> +
> +	/* Set gPTP Timer Increment Value. */
> +	ravb_write(ndev, priv->ptp.default_addend, GTI);
> +
> +	/* Request GTI loading. */
> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +
> +	/* Wait for GIT loading to complete by checking there are no requests. */
>  	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
> +
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
>  	spin_unlock_irqrestore(&priv->lock, flags);

[Severity: Medium]
Does this ravb_wait() actually observe the load request that was just
issued?  The request is GCCR.LTI, but the poll masks GCCR.TCR, and the two
are disjoint fields in ravb.h:

	GCCR_TCR	= 0x00000003,
	GCCR_TCR_NOREQ	= 0x00000000, /* No request */
	GCCR_TCR_RESET	= 0x00000001, /* gPTP/AVTP presentation timer reset */
	GCCR_TCR_CAPTURE = 0x00000003, /* Capture value set in GCCR.TCSS */
	GCCR_LTO	= 0x00000004,
	GCCR_LTI	= 0x00000008,

and ravb_wait() only tests the mask it is handed:

drivers/net/ethernet/renesas/ravb_main.c:ravb_wait() {
	for (i = 0; i < 10000; i++) {
		if ((ravb_read(ndev, reg) & mask) == value)
			return 0;
		udelay(10);
	}
	return -ETIMEDOUT;
}

TCR is already NOREQ here, so the poll returns on its first read.  The
driver's own convention elsewhere in this file treats LTI as the pending
increment load indicator:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_adjfine() {
	gccr = ravb_read(ndev, GCCR);
	if (gccr & GCCR_LTI) {
		spin_unlock_irqrestore(&priv->lock, flags);
		return -EBUSY;
	}
	ravb_write(ndev, addend & GTI_TIV, GTI);
	ravb_write(ndev, gccr | GCCR_LTI, GCCR);
}

Given that, can the following read-modify-write read GCCR with LTI still
asserted and write that bit back, re-issuing an increment load that was not
intended?

	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);

And can TCSS end up switched to ADJGPTP before the increment value is known
to be latched, with a subsequent ravb_ptp_adjfine() then returning -EBUSY
because it does check GCCR_LTI?

Should the wait poll GCCR.LTI until the hardware clears it instead, and
should the new GTI write mask the value with GTI_TIV the way
ravb_ptp_adjfine() does?

Two related notes on the wording.  The new comment says "Wait for GIT
loading" - GIT should be GTI.  The commit message states:

    "as the first action of the gPTP init function is to wait for the timer
    interval programming to be acknowledge by the hardware"

With the poll looking at TCR rather than LTI, is that acknowledgement
actually happening?  There is also a typo, "acknowledge" should be
"acknowledged".

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but the return
value of ravb_wait() here is discarded, and ravb_ptp_init() returns void, so
GCCR.TCSS is programmed and ptp_clock_register() runs even when the poll
returned -ETIMEDOUT.  ravb_open() then calls
netif_tx_start_all_queues() and returns success with no indication that the
gPTP register sequence did not complete, and ravb_set_ringparam() and
ravb_tx_timeout_work() get no status either.

What makes this worth mentioning here is that the patch moves the GTI write
and the LTI load request into this same unchecked sequence, and newly exposes
that sequence to the ring-parameter and TX-timeout recovery paths.  Would it
make sense to check the ravb_wait() result while the code is being reworked?

For reference, the unchecked ravb_wait() is still unchecked at the end of the
series (02206b03b69a), even after the later patch "net: ethernet: ravb:
Propagate error from ptp_clock_register()" changes ravb_ptp_init() to return
int.

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