Re: [PATCH net-next v15 3/4] ptp: ocp: add TAP CPLD access for ADVA TimeCard X1

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

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

All five are fair and all five are fixed.  The series is already applied
to net-next, so this is answered with an incremental series rather than a
respin; no "pw-bot: cr".

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

> [High] adva_x1_bus_release() drops the i2c root-adapter lock and forgets
> the claim even when adva_x1_mblaze_release() returned -ETIMEDOUT

The consequence you spell out is the part I had not thought through:
ptp_ocp_read_eeprom() stores what it reads without validating it, and
those values go out over the unprivileged info path.

Unlocking unconditionally stays - keeping the root lock after the firmware
has stopped answering would wedge every other user of the controller with
no way back - but the driver now records that the routing is unknown and
refuses to cache an EEPROM read while it is, so it cannot publish TMC bus
contents as the serial and board id.  A later claim that the firmware
grants clears it.

This does not fence the at24 and nvmem sysfs paths.  Those do not go
through the driver and I do not see a way to gate them from here; the fix
is limited to not publishing the result myself.

> [Medium] The one-shot CPLD ID read is executed from ptp_ocp_sync_work(),
> the driver's 1 Hz in-sync status poller.

Agreed, including the unbind and shutdown consequence - ptp_ocp_remove()
is also the .shutdown handler.  The read has its own delayed work now,
queued only on boards with the part and rescheduled only until the
one-shot read settles.

> [Low] On an acquire timeout the MicroBlaze hand-back handshake is
> executed twice

Fixed.  The acquire path no longer does its own hand-back, since the claim
already calls the release path for that error.

> [Low] adva_x1_bus_claim() returns -ENODEV when i2c_get_adapter() finds
> nothing for the cached number, without clearing bp->cpld_i2c_adap_nr.

Correct, and the comment there claiming the number is forgotten was only
true for the mismatched-adapter case.  Both paths go through one helper
now.  A newly cached adapter also re-arms the one-shot read, which
otherwise stayed latched for the rest of the binding.

> [Low] The new struct ptp_ocp member comments overstate the locking

Yes.  The design is that the writers are serialised and the readers
re-validate what they got, which is not what the comments said.  Reworded,
and the cpld_id_tried stores are marked to match its unlocked readers.

The series is tested on an ADVA TimeCard X1.  None of it changes what the
driver puts on the I2C wire - the ISP command sequence, the frame contents
and the arbitration timing are untouched.

Thanks for the review,
Sagi