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

[Jakub Kicinski]

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: Move programming of gPTP timer interval

This moves the GTI write and the GCCR.LTI load request out of
ravb_set_gti() in the main driver logic and into ravb_ptp_init(), so the
programming happens under priv->lock.  ravb_ptp_init() now also seeds
priv->ptp.default_addend from priv->gti_tiv instead of reading GTI back
from the hardware.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 36173a09e2fd..a89f6fe7866f 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);
> -}
> -

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 43218bc15b15..157fb78209c7 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. */
>  	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
> +
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);

[Severity: Medium]
Does this ravb_wait() actually observe the GTI load?  The load request is
GCCR.LTI, but the poll masks GCCR.TCR, and those are separate 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,

Setting LTI does not change TCR, and ravb_ptp_tcr_request() both issues and
waits out its TCR request under priv->lock, so no TCR request is outstanding
here.  Doesn't that make the poll return on its first read, leaving the
GTI load unwaited-for?

If so, could the new comment be reworded?  It describes waiting for the
increment load, while the wait is really draining a pending TCR request
before switching GCCR.TCSS.  The comment also spells GTI as "GIT".

The same reasoning applies to the changelog:

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

Since that first action polls TCR rather than LTI, does the stated
justification for the move still hold?  (There is also a typo, "to be
acknowledge" should be "to be acknowledged".)

There is a follow-on question about the ravb_modify() immediately after the
wait.  It is a read-modify-write of GCCR:

drivers/net/ethernet/renesas/ravb_main.c:ravb_modify() {
	ravb_write(ndev, (ravb_read(ndev, reg) & ~clear) | set, reg);
}

If LTI is still asserted at that point, doesn't this read it back and write
it out again, re-issuing the load request while switching the capture
source?  The driver's own runtime update path treats LTI as the pending
indicator and declines to touch the registers in that state:

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

Relatedly, the new code writes GTI without first checking that LTI is
clear.  On the re-init paths that reach ravb_ptp_init() from
ravb_set_ringparam() and ravb_tx_timeout_work() (including the "goto out"
case where ravb_stop_dma() failed and the MAC is still running), can a load
requested by a preceding adjfine() be overwritten while still in flight?
Would polling for (GCCR & GCCR_LTI) == 0 before the GTI write, and again
after requesting the load, be the wait that was intended here?

This behaviour is unchanged at the end of the series (c7fa563cd046 still
carries the same comment and the same GCCR_TCR poll).

[Severity: Medium]
This isn't a bug introduced by this patch, but the return value of that
ravb_wait() is discarded, and it was already unchecked before the move.
ravb_wait() can fail:

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

On -ETIMEDOUT the code still programs GCCR.TCSS_ADJGPTP and registers the
PTP clock, so the capture source can be reprogrammed with a reset/capture
request still outstanding.  Since ravb_ptp_init() returns void at this
revision there is nothing to propagate, so this would be a separate change
rather than something for this patch.

>  	spin_unlock_irqrestore(&priv->lock, flags);
>