Re: [PATCH net-next v15 4/4] ptp: ocp: add TAP CPLD flashing via devlink

[Sagi Maimon <maimon.sagi@gmail.com>]

On Sun, 20 Sep 2026 15:01:22 +0000 netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 9 potential
> issue(s) to consider.

Five of the six Medium findings are fixed in the follow-up series I am
posting alongside this; the sixth I have answered below rather than
changed.  All three Low ones are fixed.  The series is already applied to
net-next, so this is incremental rather than a respin; no "pw-bot: cr".

  https://lore.kernel.org/netdev/20260922142829.57740-1-maimon.sagi@gmail.com/T/#u

> [Medium] ptp_ocp_devlink_info_get() publishes the literal string
> "unknown" as the devlink *running version* value for the fw.cpld
> component
> (...)
> so passing "" would still register the name for
> devlink_flash_component_get() while emitting no version attribute

Thank you - that is better than what I had, and I had not spotted that
version_cb runs ahead of the empty-value early-out.  The placeholder
existed only because naming the component is what makes it flashable, and
a part left holding a bad image answers neither READ_ID nor
READ_USERCODE, so gating the name on a successful read would have made
exactly that state unrecoverable.  An empty value keeps that property
without publishing a version nobody can use.  Changed, with a
Suggested-by.

> [Medium] adva_x1_cpld_flash() invalidates the cached CPLD identity only
> after *both* the ERASE write and its completion wait succeed

Right.  An ACKed ERASE is running in the part whatever the wait then
returns, so the invalidation has to happen before the ERASE is issued,
not after it is confirmed.  Moved.

> Since the Lattice IDCODE cannot change when the configuration flash is
> erased, should cpld.id survive a failed update

It should, and this is the better half of that finding.  cpld.id was
being dropped along with the USERCODE, so a failed update hid information
that was still correct and made recovery depend on a re-read that might
not succeed.  Only the USERCODE is dropped now.  The ABI and .rst text
claiming the identification is re-read "after a successful CPLD update"
never matched the code either; both now describe what is actually
dropped and restored.

> [Medium] In adva_x1_cpld_flash() the first status wait after EN_CFG_TP
> uses adva_x1_cpld_wait_ready()

This is the most useful finding in the set.  FAILED is latched and
nothing in the driver clears it, so a part that had failed once would
return -EIO at the enable step of every later flash, before reaching the
ERASE and REFRESH that would put it back into a defined state - directly
contradicting the "recoverable, fw.cpld stays advertised so the image can
be written again" comment in the page loop.  The enable step uses
adva_x1_cpld_wait_idle() now, and the ENAB check that follows decides
whether the part entered configuration mode, which is also what
machxo2_write_init() does.

> [Medium] The driver clears its configuration-mode bookkeeping from an
> I2C ACK alone, and its post-REFRESH success predicate cannot detect a
> REFRESH that was ACKed but never latched

Correct, and the reason is exactly the one you give: DONE set, BUSY clear
and no error code are already true of the state SET_DONE leaves behind,
so they cannot separate the two cases.  The check now also requires ENAB
to be clear, since leaving configuration mode is the one thing only a
REFRESH does, and cpld_in_config_mode is put back when it is not, so the
exit path and the recovery at the start of the next flash act on the real
state instead of an ACK.

I have not added the REFRESH retry loop machxo2_write_complete() uses.
With the ENAB test in place a REFRESH that does not latch is now reported
as a failure rather than a success, which is the part that mattered; a
retry would change what goes on the wire for a case I have not been able
to produce on the part.  Happy to add it if you would rather have it.

> [Medium] adva_x1_cpld_flash() holds bp->cpld_lock and the I2C root
> adapter lock (...) can this trip the hung-task detector?

This is the one I have not changed, and I would rather say why than guess
at a fix I cannot verify.

The 100 ms in the page loop is a timeout, not a per-page cost: a real
6526-page image programs in well under a minute, and the hand-back that
follows was measured at about 670 ms against its 2 s budget.  The worst
case you add up is reachable only if the part stops answering at every
step, in which case the sequence is failing anyway.

Releasing the bus between phases is not safe here: the whole point of
holding the root lock across the claim is that the controller is routed
away from the EEPROMs for the duration, so dropping it mid-sequence would
let an EEPROM read land on the TMC segment - the problem patch 2 of the
follow-up exists to prevent.  Bounding the total hold would mean
abandoning a part mid-erase, which is worse than waiting.

Lowering CPLD_MAX_IMAGE_SZ would shrink the arithmetic without changing
the real behaviour, so I have left the cap where it is rather than
pretend it is a fix.  If you would rather see the phases split or the cap
tightened, say so and I will do it.

> [Low] Is "the only check the driver makes" accurate?

No - the size cap arrived after that sentence was written.  Corrected,
and the .rst now also says what state a failure part-way leaves the part
in, and that the component stays available to write a valid image again.

> [Low] Should this report offset + CPLD_PAGE_SIZE?

Yes; it was a page behind and never reached fw->size from inside the
loop.  Fixed.

> [Low] Should these two be WRITE_ONCE() as well?

Yes.  Done, and the struct comments that claimed a lock the readers do
not take are reworded.

The series is tested on an ADVA TimeCard X1: the CPLD still programs and
activates with all of it applied, so the two checks that decide whether an
operation is believed - ENAB after EN_CFG_TP, and ENAB clear after REFRESH
- agree with the part rather than just with my reading of the datasheet.

Thanks for the review - the enable-step and REFRESH findings were real
false-success paths and I would not have found them on my own.

Sagi