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

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] In ravb_ptp_init() (drivers/net/ethernet/renesas/ravb_ptp.c),
  the newly moved gPTP timer-interval programming polls the wrong GCCR…
- [Medium] The commit is presented as relocating existing programming
  ("moving the programming to the gPTP initialization function"), but it…

--

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

This moves the gPTP timer interval programming out of ravb_set_gti() in
ravb_main.c and into ravb_ptp_init(), where it is now done under
priv->lock, and takes priv->ptp.default_addend from priv->gti_tiv instead
of reading GTI back from the hardware.

A couple of questions below about the wait that follows the load request,
and about the callers of ravb_ptp_init() other than ravb_open().

[ ... ]

[Severity: Medium]

Before this patch ravb_set_gti() was called only from ravb_open(), so GTI
was programmed once per open.  ravb_ptp_init() has two other callers that
run on a live interface:

  ravb_set_ringparam()
      if (netif_running(ndev)) {
          ...
          if (info->gptp)
              ravb_ptp_init(ndev, priv->pdev);

  ravb_tx_timeout_work()
  out:
      /* Initialise PTP Clock driver */
      if (info->gptp)
          ravb_ptp_init(ndev, priv->pdev);

Both of those paths previously only read the live GTI back into
priv->ptp.default_addend and left the hardware increment alone.  With the
write moved here they now reprogram GTI with the nominal priv->gti_tiv and
assert GCCR_LTI.

Does that mean an ethtool -G ring resize, or a TX watchdog recovery, now
discards whatever frequency correction ravb_ptp_adjfine() had programmed?

  ravb_ptp_adjfine()
      addend = (u32)adjust_by_scaled_ppm(priv->ptp.default_addend,
                                        scaled_ppm);

The commit message describes this as moving the programming, so this extra
effect on the ringparam and tx-timeout paths is not visible from the
changelog.  Could it be mentioned there if it is intentional?

+
+	/* 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);
 	spin_unlock_irqrestore(&priv->lock, flags);

[Severity: Medium]

Does this ravb_wait() actually observe the GTI load?  The fields in
ravb.h look disjoint:

  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,

GCCR_TCR is the timer capture/reset request field, while the increment load
request just asserted is GCCR_LTI.  So the wait returns immediately as long
as no capture or reset request is outstanding, and the load itself is never
confirmed.

The driver's own handling of the same register elsewhere treats LTI as the
load-pending bit:

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

Would ravb_wait(ndev, GCCR, GCCR_LTI, 0) be the condition intended here?
As written, the following read-modify-write

  ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);

can read GCCR with LTI still set and write it back, re-arming a second load
while one is in flight, and the new GTI write is issued without the
LTI-pending check that ravb_ptp_adjfine() makes.

Note that the register sequence itself matches the pre-patch code, where
ravb_set_gti() wrote GTI plus GCCR_LTI and ravb_ptp_init() then did the same
TCR wait and TCSS read-modify-write, so the re-assertion window and the
ignored ravb_wait() return value are not new here.  What is new is the
comment, and the changelog sentence that the move is based on:

    Having split the two apart this can be improved further by moving the
    programming to the gPTP initialization function, as the first action of
    the gPTP init function is to wait for the timer interval programming to
    be acknowledge by the hardware.

Since the wait polls GCCR_TCR and not GCCR_LTI, is that rationale accurate?

Also in the same comment, GIT looks like a typo for GTI, and "acknowledge"
in the changelog should be "acknowledged".

This is still the same code at the end of the series
(941e688417745c4877c88108bd047381e5c6e533 keeps the identical
ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ) under the same comment), and
priv->ptp.default_addend = priv->gti_tiv followed by the GTI write and LTI
request is unchanged there too.

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