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

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hello Jakub,

On 2026-09-14 18:54:36 -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: 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:

Yes. This is the LLM hallucinating.. we write in LTI and pull for 
completion in TCR. I will update the comment to try and address the 
LLM..


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

-- 
Kind Regards,
Niklas Söderlund