[PATCH net-next 0/9] ptp: ocp: TAP CPLD follow-up fixes

[PATCH net-next 0/9] ptp: ocp: TAP CPLD follow-up fixes

[Sagi Maimon]

Follow-up to the automated review of the TAP CPLD series, as Jakub asked
for on the v15 posting.  Nothing here changes what the driver puts on the
I2C wire: the ISP command sequence, the frame contents and the arbitration
timing are untouched.

Patches 1-5 answer the review of 3/4.  Patch 1 is the one with real
runtime behaviour to it: the CPLD identification read shared
ptp_ocp_sync_work() with the 1 Hz in-sync poller, and a claim that has to
wait for the MicroBlaze can occupy that work item for seconds, delaying
the dpll change notification and stalling the cancel_delayed_work_sync()
that ptp_ocp_remove() - also the .shutdown handler - starts with.  Patch 2
is the point the review rated High: the hand-back can fail, and the driver
would then cache and publish whatever answered on the TMC segment as the
board serial and id.

Patches 6-9 answer the review of 4/4.  Three of them remove ways the
driver could report success, or keep reporting an identity, when the part
had not done what was asked:

 - a latched FAILED made every later flash of a part that had failed once
   return -EIO at the enable step, before reaching the ERASE and REFRESH
   that would recover it (patch 8);
 - the post-REFRESH predicate was already satisfied by the state SET_DONE
   leaves behind, so a REFRESH that was ACKed but never latched passed it
   with the old image still running (patch 9);
 - the cached identity was dropped after the erase *wait* rather than
   before the erase was issued, and it dropped cpld.id too, which is the
   silicon IDCODE and cannot change (patch 7).

Patch 6 stops publishing the string "unknown" as a devlink running
version.  The review pointed out that devlink_info_version_put() invokes
its version_cb before the empty-value early-out, so an empty value still
registers the component name for devlink_flash_component_get() - which is
what a placeholder was needed for - while emitting no version attribute.

Tested on an ADVA TimeCard X1: the CPLD programs and activates as before
with the whole series applied, so the two checks that decide whether an
operation is believed - ENAB after EN_CFG_TP in patch 8, and ENAB clear
after REFRESH in patch 9 - agree with the part.  Patches 2-4 sit on the
hand-back timeout and adapter-lookup failure paths, which do not trigger
in normal operation; the hand-back measures about 670 ms against its 2 s
budget.

v15: https://lore.kernel.org/netdev/20260916153242.157171-1-sagi.maimon@adtran.com/T/#u


Sagi Maimon (9):
  ptp: ocp: move the CPLD identification read off the sync worker
  ptp: ocp: do not cache EEPROM content after a failed TMC bus hand-back
  ptp: ocp: hand the TMC bus back once on an acquire timeout
  ptp: ocp: forget a CPLD i2c adapter number that no longer resolves
  ptp: ocp: correct the CPLD bookkeeping comments and the flash progress
  ptp: ocp: report fw.cpld with an empty value until the USERCODE is
    read
  ptp: ocp: drop only the USERCODE when flashing, and drop it before
    erasing
  ptp: ocp: tolerate a latched FAILED when entering configuration mode
  ptp: ocp: confirm the CPLD really left configuration mode after
    REFRESH

 Documentation/ABI/testing/sysfs-timecard     |   8 +-
 Documentation/networking/devlink/ptp_ocp.rst |  28 ++-
 drivers/ptp/ptp_ocp.c                        | 188 +++++++++++++++----
 3 files changed, 170 insertions(+), 54 deletions(-)


base-commit: 10cfa109c880092df32e396647b4afdca9be8350
-- 
2.47.0

[PATCH net-next 1/9] ptp: ocp: move the CPLD identification read off the sync worker

[Sagi Maimon]

ptp_ocp_sync_work() samples the in-sync bit once a second and emits the
dpll change notification.  Since the TAP CPLD support it also resolves the
i2c adapter and performs the one-shot CPLD identification read, and that
read can block for seconds: adva_x1_bus_claim() waits for the MicroBlaze
to grant the TMC segment for up to MBLAZE_RETRIES * MBLAZE_RETRY_US, and
the hand-back polls for the grant to drop for as long again.

While that runs the in-sync sampling and the dpll notification are delayed
by the same amount, and because ptp_ocp_remove() - which is also the
.shutdown handler - begins with cancel_delayed_work_sync(&bp->sync_work),
unbind and reboot block for it too.

Give the identification its own delayed work, queued from probe only on
boards that have the part and rescheduled only until the one-shot read is
settled, so a claim that has to wait no longer holds up anything else.

Fixes: 3b815e29966f ("ptp: ocp: add TAP CPLD access for ADVA TimeCard X1")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 2802989e8494..4a58bcc14648 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -402,6 +402,8 @@ struct ptp_ocp {
 	bool			sync;
 	time64_t		gnss_lost;
 	struct delayed_work	sync_work;
+	/* CPLD identification, off the 1 Hz sync poller */
+	struct delayed_work	cpld_work;
 	int			id;
 	int			n_irqs;
 	struct ptp_ocp_serial_port	port[__PORT_COUNT];
@@ -5935,18 +5937,32 @@ ptp_ocp_sync_work(struct work_struct *work)
 
 	bp->sync = sync;
 
-	/* Resolve the adapter here rather than once in probe, where it can
-	 * race the adapter's own registration, and read the ID as soon as it
-	 * turns up.  A claim can fail transiently - the firmware may not
-	 * grant the segment straight after power-up - so adva_x1_cpld_read_id()
-	 * retries a bounded number of times before giving up.
-	 */
+	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
+}
+
+/*
+ * Resolve the i2c adapter and read the CPLD identification.
+ *
+ * Kept off ptp_ocp_sync_work(): a claim can block for seconds - the
+ * MicroBlaze handshake polls for up to MBLAZE_RETRIES * MBLAZE_RETRY_US and
+ * the hand-back does the same - which would delay the in-sync sampling and
+ * the dpll change notification, and stall the cancel_delayed_work_sync() on
+ * the unbind and shutdown paths for as long.
+ *
+ * Reschedules only while there is something left to do, so a board without
+ * the part, or one whose identification is settled, costs nothing.
+ */
+static void ptp_ocp_cpld_work(struct work_struct *work)
+{
+	struct ptp_ocp *bp = container_of(work, struct ptp_ocp, cpld_work.work);
+
 	adva_x1_cache_i2c_adap(bp);
-	if (bp->has_cpld && !READ_ONCE(bp->cpld_id_tried) &&
-	    READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
+	if (READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
 		adva_x1_cpld_read_id(bp);
 
-	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
+	if (!READ_ONCE(bp->cpld_id_tried))
+		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
+				   HZ);
 }
 
 static int
@@ -5984,6 +6000,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	bp->cpld_i2c_adap_nr = -1;
 
 	INIT_DELAYED_WORK(&bp->sync_work, ptp_ocp_sync_work);
+	INIT_DELAYED_WORK(&bp->cpld_work, ptp_ocp_cpld_work);
 
 	/* compat mode.
 	 * Older FPGA firmware only returns 2 irq's.
@@ -6046,6 +6063,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		}
 	}
 	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
+	if (bp->has_cpld)
+		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
+				   HZ);
 
 	return 0;
 out_dpll:
@@ -6080,6 +6100,7 @@ ptp_ocp_remove(struct pci_dev *pdev)
 	int i;
 
 	cancel_delayed_work_sync(&bp->sync_work);
+	cancel_delayed_work_sync(&bp->cpld_work);
 	for (i = 0; i < OCP_SMA_NUM; i++) {
 		if (bp->sma[i].dpll_pin) {
 			dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]);
-- 
2.47.0

[PATCH net-next 2/9] ptp: ocp: do not cache EEPROM content after a failed TMC bus hand-back

[Sagi Maimon]

adva_x1_bus_release() drops the i2c root adapter lock whatever
adva_x1_mblaze_release() returned.  That is deliberate - holding the lock
after the firmware failed to take the segment back would stall every other
user of the controller with no way to recover it - but it means the errno
reaches only the CPLD operation that held the claim, while the next
transfer on that adapter may still be routed to the TMC bus.

ptp_ocp_read_eeprom() is reachable from the unprivileged
DEVLINK_CMD_INFO_GET path and stores what it reads without validating it,
so in that window it can latch whatever answers 0x50/0x58 on the TMC
segment as bp->serial and bp->board_id and then publish them.

Record that the routing is unknown when the hand-back times out and skip
the EEPROM read while it is, rather than caching a value that was never
read from the EEPROMs.  A later claim that the firmware grants proves the
handshake is working again and clears it.

This does not fence the at24 and nvmem sysfs paths, which do not go
through the driver; it only stops the driver publishing the result.

Fixes: 3b815e29966f ("ptp: ocp: add TAP CPLD access for ADVA TimeCard X1")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 4a58bcc14648..510083dc750a 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -448,6 +448,8 @@ struct ptp_ocp {
 	unsigned int		cpld_id_attempts;
 	/* x1 TAP CPLD present */
 	bool			has_cpld;
+	/* the TMC segment was never handed back; routing is unknown */
+	bool			cpld_bus_stuck;
 	/* EN_CFG_TP issued but not yet REFRESH'd */
 	bool			cpld_in_config_mode;
 };
@@ -2022,6 +2024,18 @@ ptp_ocp_read_eeprom(struct ptp_ocp *bp)
 	if (!bp->i2c_ctrl)
 		return;
 
+	/* A hand-back that timed out leaves the controller possibly still
+	 * routed to the TMC segment.  Reading now would latch whatever
+	 * answers 0x50/0x58 there as the serial and board id, and those are
+	 * published over the unprivileged devlink info path, so refuse
+	 * rather than cache something that was never read from the EEPROMs.
+	 */
+	if (READ_ONCE(bp->cpld_bus_stuck)) {
+		dev_dbg(&bp->pdev->dev,
+			"skipping EEPROM read, TMC bus routing unknown\n");
+		return;
+	}
+
 	tag = NULL;
 	nvmem = NULL;
 
@@ -4537,6 +4551,8 @@ static int adva_x1_bus_release(struct ptp_ocp *bp)
 		return 0;
 
 	err = adva_x1_mblaze_release(bp);
+	if (err)
+		WRITE_ONCE(bp->cpld_bus_stuck, true);
 	bp->cpld_adap = NULL;
 	kfree(bp->cpld_buf);
 	bp->cpld_buf = NULL;
@@ -4632,10 +4648,17 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
 	bp->cpld_adap = adap;
 
 	ret = adva_x1_mblaze_acquire(bp);
-	if (ret)
+	if (ret) {
 		adva_x1_bus_release(bp);	/* keeps the acquire error */
+		return ret;
+	}
 
-	return ret;
+	/* The firmware granted the segment, so it is answering the handshake
+	 * again and the routing is known once more.
+	 */
+	WRITE_ONCE(bp->cpld_bus_stuck, false);
+
+	return 0;
 }
 
 /* Select a mux channel, or deselect all with ch < 0 - the power-on state.
-- 
2.47.0

[PATCH net-next 3/9] ptp: ocp: hand the TMC bus back once on an acquire timeout

[Sagi Maimon]

adva_x1_mblaze_acquire() ran the hand-back itself before returning
-ETIMEDOUT, and adva_x1_bus_claim() then called adva_x1_bus_release() for
the same error, which runs it again.

Both write MBLAZE_RELEASE and poll for the grant to drop for up to
MBLAZE_RETRIES * MBLAZE_RETRY_US, and both run with the i2c root adapter
lock held, so a single failed claim could hold the shared controller for
half as long again and log "TMC bus still granted after release" twice for
one failure.

Leave it to the release path, which the claim already calls.

Fixes: 3b815e29966f ("ptp: ocp: add TAP CPLD access for ADVA TimeCard X1")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 510083dc750a..feb61355078a 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -4526,10 +4526,6 @@ static int adva_x1_mblaze_acquire(struct ptp_ocp *bp)
 			return 0;
 	}
 
-	/* Drop the request we gave up on.  Any error from the hand-back is
-	 * subsumed by the -ETIMEDOUT we are already returning.
-	 */
-	adva_x1_mblaze_release(bp);
 	return -ETIMEDOUT;
 }
 
@@ -4647,6 +4643,12 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
 	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
 	bp->cpld_adap = adap;
 
+	/* adva_x1_bus_release() runs the hand-back, so the acquire path does
+	 * not do it itself: both poll for the grant to drop for up to
+	 * MBLAZE_RETRIES * MBLAZE_RETRY_US with the adapter lock held, and
+	 * doing it twice only holds the shared controller for longer and
+	 * logs the same failure twice.
+	 */
 	ret = adva_x1_mblaze_acquire(bp);
 	if (ret) {
 		adva_x1_bus_release(bp);	/* keeps the acquire error */
-- 
2.47.0

[PATCH net-next 4/9] ptp: ocp: forget a CPLD i2c adapter number that no longer resolves

[Sagi Maimon]

adva_x1_bus_claim() returned -ENODEV when i2c_get_adapter() found nothing
for the cached number but left the number in place, so
adva_x1_cache_i2c_adap() kept bailing out on its first test and never
resolved the adapter again.  The comment on the parent check claimed a bad
number is forgotten and looked up again on the next sweep; that only held
for a mismatched adapter, not for one that had gone away.

Factor the invalidation out and use it on both paths.

Re-arm the one-shot identification when a new adapter is cached as well: a
different adapter may answer where the previous one did not, and leaving
cpld_id_tried set kept cpld.id absent for the rest of the binding.

Fixes: 3b815e29966f ("ptp: ocp: add TAP CPLD access for ADVA TimeCard X1")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index feb61355078a..45313143b6f7 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -4574,6 +4574,19 @@ static int adva_x1_i2c_adap_match(struct device *dev, const void *data)
  * both the notifier and a single lookup here can miss the same adapter.
  * Retrying costs a short klist walk per tick until one of them succeeds.
  */
+/*
+ * Forget the cached adapter number so the next sweep resolves it again.
+ * @nr < 0 forgets whatever is cached; otherwise only that number, so a
+ * caller that raced the notifier cannot clear a newer one.
+ */
+static void adva_x1_forget_i2c_adap(struct ptp_ocp *bp, int nr)
+{
+	scoped_guard(spinlock, &bp->cpld_adap_lock) {
+		if (nr < 0 || bp->cpld_i2c_adap_nr == nr)
+			bp->cpld_i2c_adap_nr = -1;
+	}
+}
+
 static void adva_x1_cache_i2c_adap(struct ptp_ocp *bp)
 {
 	struct device *child;
@@ -4598,6 +4611,15 @@ static void adva_x1_cache_i2c_adap(struct ptp_ocp *bp)
 			bp->cpld_i2c_adap_nr = i2c_verify_adapter(child)->nr;
 	}
 
+	/* A different adapter may answer differently, so let the one-shot
+	 * identification run again rather than leaving cpld.id absent for
+	 * the rest of the binding.
+	 */
+	scoped_guard(mutex, &bp->cpld_lock) {
+		bp->cpld_id_tried = false;
+		bp->cpld_id_attempts = 0;
+	}
+
 	put_device(child);
 }
 
@@ -4614,8 +4636,14 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
 	lockdep_assert_held(&bp->cpld_lock);
 
 	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));
-	if (!adap)
+	if (!adap) {
+		/* The adapter behind the cached number is gone.  Forget it,
+		 * or adva_x1_cache_i2c_adap() keeps bailing out on its first
+		 * test and never resolves the adapter again.
+		 */
+		adva_x1_forget_i2c_adap(bp, -1);
 		return -ENODEV;
+	}
 
 	/* The number is freed before the notifier clears it, so it can
 	 * already be another adapter's.  Check this is still ours, and drop
@@ -4623,10 +4651,7 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
 	 * way, where forgetting it lets the worker look the adapter up again.
 	 */
 	if (!bp->i2c_ctrl || adap->dev.parent != &bp->i2c_ctrl->dev) {
-		scoped_guard(spinlock, &bp->cpld_adap_lock) {
-			if (bp->cpld_i2c_adap_nr == adap->nr)
-				bp->cpld_i2c_adap_nr = -1;
-		}
+		adva_x1_forget_i2c_adap(bp, adap->nr);
 		i2c_put_adapter(adap);
 		return -ENODEV;
 	}
-- 
2.47.0

[PATCH net-next 5/9] ptp: ocp: correct the CPLD bookkeeping comments and the flash progress

[Sagi Maimon]

The struct ptp_ocp member comments overstate the locking.
cpld_i2c_adap_nr was documented as "Under cpld_adap_lock" and cpld_id_tried
as "under cpld_lock", but every reader takes neither: the design is that
the writers are serialised while readers may see a stale value and
re-validate it - adva_x1_bus_claim() re-checks the adapter's parent, and a
stale cpld_id_tried only costs one extra attempt.  Describe that instead.

Pair the stores of cpld_id_tried with those unlocked readers using
WRITE_ONCE() rather than plain stores.

The flash progress notification reported the offset of the page that had
just been written rather than the number of bytes written, so it was one
page behind and never reached fw->size from inside the loop.

No functional change beyond the reported progress value.

Fixes: 3b815e29966f ("ptp: ocp: add TAP CPLD access for ADVA TimeCard X1")
Fixes: a4b7c15aa78f ("ptp: ocp: add TAP CPLD flashing via devlink")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 45313143b6f7..e10f6b5149c9 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -428,9 +428,12 @@ struct ptp_ocp {
 	/* adva_x1 CPLD I2C (internal use only) */
 	/* serialises CPLD operations */
 	struct mutex		cpld_lock;
-	/* guards cpld_i2c_adap_nr against the bus notifier */
+	/* serialises the cpld_i2c_adap_nr writers against each other */
 	spinlock_t		cpld_adap_lock;
-	/* I2C adapter nr; -1 if absent.  Under cpld_adap_lock */
+	/* I2C adapter nr, -1 if absent.  Writers hold cpld_adap_lock;
+	 * readers take no lock and re-validate what they got, since the
+	 * number can be recycled - see adva_x1_bus_claim().
+	 */
 	int			cpld_i2c_adap_nr;
 	/* claimed adapter; valid under cpld_lock */
 	struct i2c_adapter	*cpld_adap;
@@ -442,9 +445,12 @@ struct ptp_ocp {
 	u32			cpld_usercode;
 	/* cpld_usercode has been read since the last flash */
 	bool			cpld_usercode_ok;
-	/* one-shot ID read finished, successfully or not; under cpld_lock */
+	/* one-shot ID read finished, successfully or not.  Written under
+	 * cpld_lock; the worker reads it unlocked, where a stale value only
+	 * costs one extra attempt.
+	 */
 	bool			cpld_id_tried;
-	/* failed ID read attempts so far; under cpld_lock */
+	/* failed ID read attempts so far; cpld_lock */
 	unsigned int		cpld_id_attempts;
 	/* x1 TAP CPLD present */
 	bool			has_cpld;
@@ -4616,7 +4622,7 @@ static void adva_x1_cache_i2c_adap(struct ptp_ocp *bp)
 	 * the rest of the binding.
 	 */
 	scoped_guard(mutex, &bp->cpld_lock) {
-		bp->cpld_id_tried = false;
+		WRITE_ONCE(bp->cpld_id_tried, false);
 		bp->cpld_id_attempts = 0;
 	}
 
@@ -4900,7 +4906,7 @@ static int adva_x1_cpld_read_id(struct ptp_ocp *bp)
 	 * worker that had already finished reading.
 	 */
 	if (!ret || ++bp->cpld_id_attempts >= CPLD_ID_MAX_ATTEMPTS)
-		bp->cpld_id_tried = true;
+		WRITE_ONCE(bp->cpld_id_tried, true);
 	mutex_unlock(&bp->cpld_lock);
 	if (ret)
 		dev_dbg(&bp->pdev->dev,
@@ -5047,7 +5053,7 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
 	 */
 	WRITE_ONCE(bp->cpld_id, 0);
 	WRITE_ONCE(bp->cpld_usercode_ok, false);
-	bp->cpld_id_tried = false;
+	WRITE_ONCE(bp->cpld_id_tried, false);
 	bp->cpld_id_attempts = 0;
 
 	err = adva_x1_cpld_write(bp, CPLD_CMD_RESET_ADDR);
@@ -5056,6 +5062,7 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
 
 	for (offset = 0; offset < fw->size; offset += CPLD_PAGE_SIZE) {
 		u8 args[3 + CPLD_PAGE_SIZE] = { 0x00, 0x00, 0x01 };
+		size_t done;
 
 		/* The loop holds cpld_lock and the i2c root lock for the
 		 * whole image, so give a dying task a way out.  The part is
@@ -5075,11 +5082,12 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
 		if (err)
 			goto exit_config;
 
+		done = offset + CPLD_PAGE_SIZE;
 		if (!(offset % (CPLD_PAGE_SIZE * 64)))
 			devlink_flash_update_status_notify(devlink,
 							   "Programming",
 							   ADVA_CPLD_COMPONENT,
-							   offset, fw->size);
+							   done, fw->size);
 	}
 	devlink_flash_update_status_notify(devlink, "Programming",
 					   ADVA_CPLD_COMPONENT,
-- 
2.47.0

[PATCH net-next 6/9] ptp: ocp: report fw.cpld with an empty value until the USERCODE is read

[Sagi Maimon]

The fw.cpld running version was published as the literal string "unknown"
before the USERCODE had been read, which contradicts both the changelog
and the .rst entry describing it as the USERCODE formatted as 0x%08x, and
is not a version anyone can use.

The string was there because naming the component is what lets
"devlink dev flash ... component fw.cpld" through, and a part left holding
a bad image answers neither READ_ID nor READ_USERCODE - gating the
component on the read would make exactly that state unrecoverable.

An empty value gives both: devlink_info_version_put() invokes its
version_cb, which is what devlink_flash_component_get() collects names
with, before returning early on an empty value, so the component stays
flashable while no version attribute is emitted.

Suggested-by: netdev-bot+sashiko@kernel.org
Fixes: a4b7c15aa78f ("ptp: ocp: add TAP CPLD flashing via devlink")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index e10f6b5149c9..4ce86df6e196 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -2264,15 +2264,20 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 		/* The flashable component.  Naming it here is what lets
 		 * "devlink dev flash ... component fw.cpld" through, as the
 		 * core matches the name against the versions reported here,
-		 * so it is reported for every board that has the part and not
+		 * so it is named for every board that has the part and not
 		 * only once its USERCODE has been read: a part left holding a
 		 * bad image answers neither, and gating the component on the
 		 * read would make that state unrecoverable.
+		 *
+		 * An empty value still registers the name with the core -
+		 * devlink_info_version_put() runs its version_cb before the
+		 * empty-value early-out - while emitting no version attribute,
+		 * so nothing is published until the USERCODE has been read.
 		 */
 		if (smp_load_acquire(&bp->cpld_usercode_ok))
 			sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
 		else
-			strscpy(buf, "unknown", sizeof(buf));
+			buf[0] = '\0';
 		err = devlink_info_version_running_put_ext(req, "fw.cpld", buf,
 							   ver_type);
 		if (err)
-- 
2.47.0

[PATCH net-next 7/9] ptp: ocp: drop only the USERCODE when flashing, and drop it before erasing

[Sagi Maimon]

Two problems with where adva_x1_cpld_flash() invalidated the cached CPLD
identity.

It ran after the erase *wait* returned, so an ACKed ERASE whose wait then
failed skipped it: the erase is running in the part at that point, but
cpld.id and the fw.cpld USERCODE kept describing the image that is being
destroyed, and cpld_id_tried stayed set so nothing re-read them for the
rest of the binding.  Do it before the ERASE is issued instead.

It also cleared cpld_id, which is the Lattice IDCODE - a property of the
silicon that erasing the configuration flash cannot change.  Dropping it
on a failed update only hid information that was still correct, and made
recovery depend on a re-read that may not succeed.  Leave it alone.

The documentation said the identification is read "again after a
successful CPLD update", which was never what the code did; describe what
is actually dropped and restored.  While there, note the size check and
the state the part is left in when an update fails part-way.

Fixes: a4b7c15aa78f ("ptp: ocp: add TAP CPLD flashing via devlink")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 Documentation/ABI/testing/sysfs-timecard     |  8 +++---
 Documentation/networking/devlink/ptp_ocp.rst | 28 +++++++++++++-------
 drivers/ptp/ptp_ocp.c                        | 25 ++++++++++-------
 3 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index 4ba45c6ee4a3..569ab668a225 100644
--- a/Documentation/ABI/testing/sysfs-timecard
+++ b/Documentation/ABI/testing/sysfs-timecard
@@ -31,10 +31,10 @@ Description:	(RO, root only) The flags set in the status register of the
 
 		A read arbitrates for the shared I2C bus and reprograms the
 		on-card mux, so it is restricted to root.  The Lattice device
-		ID of the CPLD is read by the driver shortly after probe,
-		and again after a successful CPLD update, and reported from
-		that cached value as the fixed "cpld.id" version by
-		devlink dev info.
+		ID of the CPLD is read by the driver shortly after probe and
+		reported from that cached value as the fixed "cpld.id"
+		version by devlink dev info.  It identifies the silicon, so a
+		CPLD update does not change it.
 
 		New CPLD firmware is programmed with devlink dev flash,
 		selecting the "fw.cpld" component; see
diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
index f94b759d9cd6..249ca63eebf6 100644
--- a/Documentation/networking/devlink/ptp_ocp.rst
+++ b/Documentation/networking/devlink/ptp_ocp.rst
@@ -32,14 +32,17 @@ The ``ptp_ocp`` driver reports the following versions
        carrying that CPLD.  Reading it claims the shared I2C bus and
        reprograms the on-card mux, so the driver does that from its own
        worker and reports the cached value here; the version is omitted
-       until that read has succeeded.  The read is made once per binding
-       and again after a successful CPLD update.
+       until that read has succeeded.  The IDCODE identifies the silicon,
+       so it is not affected by a CPLD update.
    * - ``fw.cpld``
      - running
      - USERCODE of the image programmed into the TAP CPLD, formatted as
        ``0x%08x``.  Read together with ``cpld.id`` and reported the same
-       way.  This is the component name to pass to ``devlink dev flash``
-       to update the CPLD.
+       way; it is dropped when an update erases the part and reported
+       again once the new image has been read back.  This is the
+       component name to pass to ``devlink dev flash`` to update the
+       CPLD, and the name is reported even while the value is not, so a
+       part left holding a bad image can still be reflashed.
 
 Flash update
 ============
@@ -59,12 +62,17 @@ selected with the component name.
      - The configuration flash of the TAP CPLD on ADVA TimeCard X1 boards,
        programmed over I2C with the MachXO3 in-system programming commands
        and activated with a REFRESH, so the new image runs immediately.
-       The image is the raw configuration bitstream.  The only check the
-       driver makes is that its length is a non-zero multiple of the
-       16-byte page size, so a container such as ``.jed`` has to be
-       converted first rather than passed through - one whose length
-       happens to be a multiple of 16 would be programmed as if it were
-       a bitstream.
+       The image is the raw configuration bitstream.  The driver checks
+       only that its length is a non-zero multiple of the 16-byte page
+       size and that it is not larger than the part takes, so a container
+       such as ``.jed`` has to be converted first rather than passed
+       through - one whose length happens to be a multiple of 16 would be
+       programmed as if it were a bitstream.
+
+       The erase clears the configuration flash before the first page is
+       written, so any failure from that point on - including an abort on
+       a fatal signal - leaves the CPLD unconfigured until a valid image
+       is written.  The component stays available for that.
 
 Programming the CPLD claims the shared I2C bus for the whole cycle, so
 reads of the card's EEPROM block until it completes.  Progress is reported
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 4ce86df6e196..0d6d0c02882c 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -5043,6 +5043,21 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
 		goto exit_config;
 	}
 
+	/* Once the erase is issued the image is gone whatever happens next -
+	 * an ACKed ERASE runs in the part even if the wait for it fails - so
+	 * stop reporting the USERCODE before sending it rather than after
+	 * the whole sequence has succeeded.
+	 *
+	 * cpld.id is left alone: it is the Lattice IDCODE, a property of the
+	 * silicon that erasing the configuration flash cannot change, so
+	 * dropping it on a failed update only hid information that was still
+	 * correct.  Written under cpld_lock, which adva_x1_cpld_read_id()
+	 * also holds across its own bookkeeping.
+	 */
+	WRITE_ONCE(bp->cpld_usercode_ok, false);
+	WRITE_ONCE(bp->cpld_id_tried, false);
+	bp->cpld_id_attempts = 0;
+
 	devlink_flash_update_status_notify(devlink, "Erasing",
 					   ADVA_CPLD_COMPONENT, 0, 0);
 	err = adva_x1_cpld_write(bp, CPLD_CMD_ERASE);
@@ -5051,16 +5066,6 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
 	if (err)
 		goto exit_config;
 
-	/* The old image is gone from here on, so stop reporting its
-	 * identity even if the rest of the sequence fails.  Written under
-	 * cpld_lock, which adva_x1_cpld_read_id() also holds across its own
-	 * bookkeeping, so the worker cannot resurrect any of it.
-	 */
-	WRITE_ONCE(bp->cpld_id, 0);
-	WRITE_ONCE(bp->cpld_usercode_ok, false);
-	WRITE_ONCE(bp->cpld_id_tried, false);
-	bp->cpld_id_attempts = 0;
-
 	err = adva_x1_cpld_write(bp, CPLD_CMD_RESET_ADDR);
 	if (err)
 		goto exit_config;
-- 
2.47.0

[PATCH net-next 8/9] ptp: ocp: tolerate a latched FAILED when entering configuration mode

[Sagi Maimon]

The first status wait after EN_CFG_TP used adva_x1_cpld_wait_ready(),
which turns a set CPLD_STATUS_FAILED into -EIO.  FAILED is latched across
operations and nothing in the driver clears it - the failure path only
sends DIS_CFG - so once a part had failed an operation, every later
"devlink dev flash ... component fw.cpld" would return -EIO at the enable
step, before reaching the ERASE and REFRESH that would put the part back
into a defined state.

That contradicts the recovery this driver relies on elsewhere: the page
loop documents an aborted update as recoverable because fw.cpld stays
advertised so the image can be written again.

Use adva_x1_cpld_wait_idle(), which waits the operation out whatever its
outcome, and let the CPLD_STATUS_ENAB check that follows decide whether
the part actually entered configuration mode.  machxo2_write_init() in
drivers/fpga/machxo2-spi.c tests FAIL the same way, after the enable has
completed rather than as a precondition for it.

Fixes: a4b7c15aa78f ("ptp: ocp: add TAP CPLD flashing via devlink")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 0d6d0c02882c..4f2bf54a23c2 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -5018,9 +5018,16 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
 	 */
 	bp->cpld_in_config_mode = true;
 
+	/* wait_idle(), not wait_ready(): FAILED is latched across operations
+	 * and nothing here clears it, so treating it as fatal at the enable
+	 * step would make every later flash of a part that has failed once
+	 * return -EIO before reaching the ERASE and REFRESH that put it back
+	 * into a defined state.  ENAB below is what says the enable worked;
+	 * machxo2-spi.c likewise tests FAIL only after ISC_ENABLE completes.
+	 */
 	err = adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP);
 	if (!err)
-		err = adva_x1_cpld_wait_ready(bp, 5000);
+		err = adva_x1_cpld_wait_idle(bp, 5000);
 	if (err)
 		goto exit_config;
 
-- 
2.47.0

[PATCH net-next 9/9] ptp: ocp: confirm the CPLD really left configuration mode after REFRESH

[Sagi Maimon]

The post-REFRESH check required DONE set, BUSY clear and no error code.
Those three conditions are already satisfied by the state SET_DONE leaves
behind, so they cannot distinguish a REFRESH that rebooted the part from
one whose frame was ACKed but never latched - and the I2C ACK alone was
taken as proof, clearing cpld_in_config_mode.

A part left that way stays in configuration mode running the old image
while "devlink dev flash ... component fw.cpld" reports success, which is
the opposite of what the documentation promises.

Test CPLD_STATUS_ENAB as well: leaving configuration mode is the one
thing only a REFRESH does, so it is what separates the two cases.  Put
cpld_in_config_mode back when ENAB is still set, so the exit path and the
recovery at the start of the next flash can act on it instead of
believing a mode change that never happened.

Fixes: a4b7c15aa78f ("ptp: ocp: add TAP CPLD flashing via devlink")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 4f2bf54a23c2..9c2b7403bfd0 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -5135,6 +5135,9 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
 
 	/* REFRESH reboots the CPLD out of configuration mode, so the exit
 	 * path must not send DIS_CFG afterwards even if a check below fails.
+	 * The ENAB test below confirms it really left; until then assume it
+	 * did, because sending DIS_CFG to a part that has rebooted is what
+	 * this flag exists to avoid.
 	 */
 	bp->cpld_in_config_mode = false;
 
@@ -5156,12 +5159,21 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
 	/* Require DONE set, not busy and no error code, as machxo2-spi.c does
 	 * after a refresh: without it a CRC or preamble error reads back as a
 	 * successful update.
+	 *
+	 * ENAB has to be clear too.  Those three conditions are already met
+	 * by the state SET_DONE leaves behind, so on their own they cannot
+	 * tell a REFRESH that rebooted the part from one whose frame was
+	 * ACKed but never latched - which leaves the part in configuration
+	 * mode still running the old image.  Leaving configuration mode is
+	 * the one thing only a REFRESH does.
 	 */
 	err = adva_x1_cpld_read_status(bp, &st);
 	if (err)
 		goto deselect;
+	if (st & CPLD_STATUS_ENAB)
+		bp->cpld_in_config_mode = true;
 	if (!(st & CPLD_STATUS_DONE) || (st & CPLD_STATUS_BUSY) ||
-	    (st & CPLD_STATUS_ERR)) {
+	    (st & CPLD_STATUS_ERR) || (st & CPLD_STATUS_ENAB)) {
 		dev_err(&bp->pdev->dev,
 			"CPLD refresh left status 0x%08x\n", st);
 		NL_SET_ERR_MSG_MOD(extack, "CPLD did not come back configured");
-- 
2.47.0

Re: [PATCH net-next 1/9] ptp: ocp: move the CPLD identification read off the sync worker

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Moving the CPLD identification read into a self-terminating
  delayed work (ptp_ocp_cpld_work()) removes the only mechanism that re-…
- [Medium] The commit message names unbind/reboot blocking as part of the
  problem being fixed ("because ptp_ocp_remove() - which is also the…
- [Low] Comments describing the per-tick re-resolution of the CPLD i2c
  adapter are no longer accurate.
- [Low] Stale comment: `#define CPLD_ID_MAX_ATTEMPTS 10 /* one per
  sync_work tick */` (drivers/ptp/ptp_ocp.c:4386) still attributes the…

--

ptp: ocp: move the CPLD identification read off the sync worker

The one-shot TAP CPLD identification read is moved out of
ptp_ocp_sync_work() into a new delayed work, ptp_ocp_cpld_work(), which is
queued from probe only on boards that carry the part.  The new work
re-arms itself once a second only while bp->cpld_id_tried is clear, and
ptp_ocp_remove() gains a cancel_delayed_work_sync() for it.

A few questions below.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 2802989e84947..4a58bcc146487 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -5935,18 +5937,32 @@ ptp_ocp_sync_work(struct work_struct *work)
>  
>  	bp->sync = sync;
>  
> -	/* Resolve the adapter here rather than once in probe, where it can
> -	 * race the adapter's own registration, and read the ID as soon as it
> -	 * turns up.  A claim can fail transiently - the firmware may not
> -	 * grant the segment straight after power-up - so adva_x1_cpld_read_id()
> -	 * retries a bounded number of times before giving up.
> -	 */
> +	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
> +}
> +
> +/*
> + * Resolve the i2c adapter and read the CPLD identification.
> + *
> + * Kept off ptp_ocp_sync_work(): a claim can block for seconds - the
> + * MicroBlaze handshake polls for up to MBLAZE_RETRIES * MBLAZE_RETRY_US and
> + * the hand-back does the same - which would delay the in-sync sampling and
> + * the dpll change notification, and stall the cancel_delayed_work_sync() on
> + * the unbind and shutdown paths for as long.
> + *
> + * Reschedules only while there is something left to do, so a board without
> + * the part, or one whose identification is settled, costs nothing.
> + */
> +static void ptp_ocp_cpld_work(struct work_struct *work)
> +{
> +	struct ptp_ocp *bp = container_of(work, struct ptp_ocp, cpld_work.work);
> +
>  	adva_x1_cache_i2c_adap(bp);

[Severity: Low]

This isn't a bug, but with adva_x1_cache_i2c_adap() now called only from
ptp_ocp_cpld_work(), are its comments about per-tick retries still
accurate?  Its header says:

 * Called from the worker rather than once from probe: device_add() notifies
 * before it links the device to its parent, so with asynchronous probing
 * both the notifier and a single lookup here can miss the same adapter.
 * Retrying costs a short klist walk per tick until one of them succeeds.

and its body says an adapter cached in the removal window is rejected by
adva_x1_bus_claim() "and the next tick looks again", while
adva_x1_bus_claim() says forgetting a recycled number "lets the worker look
the adapter up again".

Once bp->cpld_id_tried is set the new worker stops re-arming, so there is
no next tick and only ptp_ocp_i2c_notifier_call() can repopulate the
number - which the same header comment says can miss the adapter.

> -	if (bp->has_cpld && !READ_ONCE(bp->cpld_id_tried) &&
> -	    READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
> +	if (READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
>  		adva_x1_cpld_read_id(bp);

[Severity: Low]

This isn't a bug, but the attempt budget consumed here is still documented
against the old worker:

#define CPLD_ID_MAX_ATTEMPTS  10     /* one per sync_work tick */

The attempts are now one per cpld_work tick.

>  
> -	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
> +	if (!READ_ONCE(bp->cpld_id_tried))
> +		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
> +				   HZ);
>  }

[Severity: High]

Does this leave the post-flash re-read of the CPLD identity without a
worker to perform it?

adva_x1_cpld_read_id() sets the flag on the first success, or after the
attempt budget runs out:

	if (!ret || ++bp->cpld_id_attempts >= CPLD_ID_MAX_ATTEMPTS)
		bp->cpld_id_tried = true;

so after the probe-time read settles the tail above stops re-arming
cpld_work, and ptp_ocp_probe() is the only other queue site.

adva_x1_cpld_flash() still clears the bookkeeping and leaves the re-read to
a periodic worker:

	/* The old image is gone from here on, so stop reporting its
	 * identity even if the rest of the sequence fails.  Written under
	 * cpld_lock, which adva_x1_cpld_read_id() also holds across its own
	 * bookkeeping, so the worker cannot resurrect any of it.
	 */
	WRITE_ONCE(bp->cpld_id, 0);
	WRITE_ONCE(bp->cpld_usercode_ok, false);
	bp->cpld_id_tried = false;
	bp->cpld_id_attempts = 0;

but it queues nothing and does not re-read the ID or USERCODE itself.

Before this patch ptp_ocp_sync_work() requeued itself every HZ for the life
of the binding, so the next tick saw the cleared flag and read the new
image's IDCODE and USERCODE.  With the split, does bp->cpld_id stay 0 and
bp->cpld_usercode_ok stay false until unbind and rebind, so
ptp_ocp_devlink_info_get() permanently skips cpld.id and reports fw.cpld as
"unknown"?

	id = READ_ONCE(bp->cpld_id);
	if (id) {
		sprintf(buf, "0x%08x", id);
	...
	if (smp_load_acquire(&bp->cpld_usercode_ok))
		sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
	else
		strscpy(buf, "unknown", sizeof(buf));

Documentation/networking/devlink/ptp_ocp.rst states:

       worker and reports the cached value here; the version is omitted
       until that read has succeeded.  The read is made once per binding
       and again after a successful CPLD update.

Would queueing bp->cpld_work from adva_x1_cpld_flash() after that reset
(queueing an already pending delayed work is a no-op) match the documented
behaviour again, or should the worker keep re-arming while bp->has_cpld?

The same concern applies to the later reset of cpld_id_tried added in
adva_x1_cache_i2c_adap() by "ptp: ocp: forget a CPLD i2c adapter number
that no longer resolves" in this series; at the end of the series the queue
sites for cpld_work are still probe plus the worker's own conditional tail.

[ ... ]

> @@ -6046,6 +6063,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		}
>  	}
>  	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
> +	if (bp->has_cpld)
> +		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
> +				   HZ);
>  
>  	return 0;
>  out_dpll:

[ ... ]

> @@ -6080,6 +6100,7 @@ ptp_ocp_remove(struct pci_dev *pdev)
>  	int i;
>  
>  	cancel_delayed_work_sync(&bp->sync_work);
> +	cancel_delayed_work_sync(&bp->cpld_work);

[Severity: Medium]

Is the unbind and shutdown part of the changelog still true with this
cancel in place?  The changelog says:

    While that runs the in-sync sampling and the dpll notification are delayed
    by the same amount, and because ptp_ocp_remove() - which is also the
    .shutdown handler - begins with cancel_delayed_work_sync(&bp->sync_work),
    unbind and reboot block for it too.

and the new function comment repeats it:

 * the dpll change notification, and stall the cancel_delayed_work_sync() on
 * the unbind and shutdown paths for as long.

cancel_delayed_work_sync() waits for an already running callback to return
rather than interrupting it, so if ptp_ocp_cpld_work() is mid-claim the
executing path is:

ptp_ocp_cpld_work()
  adva_x1_cpld_read_id()
    adva_x1_bus_claim()
      adva_x1_mblaze_acquire()

	iowrite32(MBLAZE_REQUEST, &bp->pps_select->i2c_bus_ctrl);
	for (i = 0; i < MBLAZE_RETRIES; i++) {
		usleep_range(MBLAZE_RETRY_US, MBLAZE_RETRY_US + 1000);
		val = ioread32(&bp->pps_select->i2c_bus_ctrl);
		if (val == MBLAZE_GRANTED)
			return 0;
	}

That is 200 iterations of a 10 ms sleep, and on timeout
adva_x1_mblaze_release() polls the same amount again, with no teardown
escape check in either loop.  The window in which the blocking read can be
in flight is the same before and after the split, since the read is
attempted once a second until cpld_id_tried is set.

Should the changelog and the comment be narrowed to the 1 Hz in-sync
sampling and dpll notification latency, or would a teardown flag checked
before taking cpld_lock and between MicroBlaze retries let the still
required synchronous cancel return promptly?

>  	for (i = 0; i < OCP_SMA_NUM; i++) {
>  		if (bp->sma[i].dpll_pin) {
>  			dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com

Re: [PATCH net-next 2/9] ptp: ocp: do not cache EEPROM content after a failed TMC bus hand-back

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] TOCTOU: the new `cpld_bus_stuck` gate in `ptp_ocp_read_eeprom()`
  (drivers/ptp/ptp_ocp.c:2033) is a single snapshot taken before any I2C…
- [Medium] `cpld_bus_stuck` is latched on a single hand-back timeout
  (drivers/ptp/ptp_ocp.c:4555) and is cleared in exactly one place:…
- [Medium] The two driver-owned publishers of the cached EEPROM identity
  now disagree about the new "read refused" outcome, and the patch…

--

ptp: ocp: do not cache EEPROM content after a failed TMC bus hand-back

Adds a cpld_bus_stuck flag to struct ptp_ocp which is set when
adva_x1_mblaze_release() fails to get the TMC segment handed back, and
cleared when a later adva_x1_bus_claim() is granted.  While the flag is
set, ptp_ocp_read_eeprom() returns without reading, so the driver does
not cache or publish a serial number and board id it may have read off
the TMC segment.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 4a58bcc146487..510083dc750a5 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -2022,6 +2024,18 @@ ptp_ocp_read_eeprom(struct ptp_ocp *bp)
>  	if (!bp->i2c_ctrl)
>  		return;
>  
> +	/* A hand-back that timed out leaves the controller possibly still
> +	 * routed to the TMC segment.  Reading now would latch whatever
> +	 * answers 0x50/0x58 there as the serial and board id, and those are
> +	 * published over the unprivileged devlink info path, so refuse
> +	 * rather than cache something that was never read from the EEPROMs.
> +	 */
> +	if (READ_ONCE(bp->cpld_bus_stuck)) {
> +		dev_dbg(&bp->pdev->dev,
> +			"skipping EEPROM read, TMC bus routing unknown\n");
> +		return;
> +	}
> +

[Severity: High]
Can this test still let the bad value be cached?  The flag is sampled
once here, before any transfer, and neither caller of
ptp_ocp_read_eeprom() holds bp->cpld_lock, so nothing keeps the answer
valid for the duration of the nvmem_device_read() loop that follows.

What actually keeps EEPROM traffic off a TMC-routed controller is the
i2c root adapter lock, and the comment above adva_x1_bus_claim() says so:

drivers/ptp/ptp_ocp.c:
/*
 * Claim the TMC bus.  The adapter lock is held across the handshake to keep
 * the EEPROM, nvmem and at24 paths off the controller while it is routed
 * away from the EEPROMs; an EEPROM read blocks until the claim is dropped.
 */

So doesn't this ordering remain reachable?

CPU0 ptp_ocp_devlink_info_get() / serialnum_show()
  ptp_ocp_read_eeprom()
    READ_ONCE(bp->cpld_bus_stuck)   /* false, passes the gate */
    nvmem_device_read()             /* blocks on the adapter lock */

CPU1 ptp_ocp_cpld_work() -> adva_x1_cpld_read_id()
  adva_x1_bus_claim()
    i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER)
    adva_x1_mblaze_acquire()        /* granted, routing on TMC */
  adva_x1_bus_release()
    adva_x1_mblaze_release()        /* -ETIMEDOUT */
    WRITE_ONCE(bp->cpld_bus_stuck, true)
    i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER)   /* releases CPU0 */

CPU0 then completes its transfers over a controller the driver has just
declared possibly still routed to the TMC segment, stores the bytes into
bp->serial and bp->board_id through BP_MAP_ENTRY_ADDR() and sets
bp->has_eeprom_data = true.  Since both callers only call in while
!has_eeprom_data, is there anything that can correct that value later?

The same window also exists between two iterations of the map loop, where
the flag can flip to true after the first nvmem_device_read() has already
returned.

Would checking the flag (or the MBLAZE_GRANT bits) under bp->cpld_lock,
or re-checking it before setting has_eeprom_data, close this?

[Severity: Medium]
The two driver-owned publishers of the cached identity now behave
differently on this new "read refused" outcome.
ptp_ocp_devlink_info_get() re-tests the flag and emits nothing:

	if (!bp->has_eeprom_data) {
		ptp_ocp_read_eeprom(bp);
		if (!bp->has_eeprom_data)
			return 0;
	}

serialnum_show() does not:

	if (!bp->has_eeprom_data)
		ptp_ocp_read_eeprom(bp);

	return sysfs_emit(buf, "%pM\n", bp->serial);

So when the new gate skips the read, doesn't /sys/class/timecard/ocpN/serialnum
still print 00:00:00:00:00:00, or bytes left behind by a partially completed
read?  ptp_ocp_read_eeprom() writes each map entry directly into
bp->serial/bp->board_id and its fail: path only logs, leaving the entries
already stored in place while has_eeprom_data stays false.

Documentation/ABI/testing/sysfs-timecard describes the attribute as
"(RO) Provides the serial number of the timecard." with no encoding for an
unknown value.

The unconditional sysfs_emit() itself predates this patch and is unchanged
at the baseline, so this is not a new code path - but with the new outcome
the emitted value is one the driver itself now considers invalid.  Should
serialnum_show() also test has_eeprom_data before emitting, given the
commit aims to stop the driver publishing a value that was never read from
the EEPROMs?

> @@ -4537,6 +4551,8 @@ static int adva_x1_bus_release(struct ptp_ocp *bp)
>  		return 0;
>  
>  	err = adva_x1_mblaze_release(bp);
> +	if (err)
> +		WRITE_ONCE(bp->cpld_bus_stuck, true);
>  	bp->cpld_adap = NULL;
>  	kfree(bp->cpld_buf);
>  	bp->cpld_buf = NULL;

[ ... ]

> @@ -4632,10 +4648,17 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
>  	bp->cpld_adap = adap;
>  
>  	ret = adva_x1_mblaze_acquire(bp);
> -	if (ret)
> +	if (ret) {
>  		adva_x1_bus_release(bp);	/* keeps the acquire error */
> +		return ret;
> +	}
>  
> -	return ret;
> +	/* The firmware granted the segment, so it is answering the handshake
> +	 * again and the routing is known once more.
> +	 */
> +	WRITE_ONCE(bp->cpld_bus_stuck, false);
> +
> +	return 0;
>  }

[Severity: Medium]
Is the flag recoverable in practice?  It is latched on one hand-back
timeout and cleared in exactly one place, after a fully successful claim.

Two cases seem to leave it set forever.

First, a hand-back that demonstrably succeeded does not clear it.  On the
acquire failure path above, adva_x1_bus_release() runs
adva_x1_mblaze_release(), which may return 0 - meaning it observed the
firmware drop the grant and the controller is back on the EEPROM segment -
and then the function returns the acquire error before reaching the
WRITE_ONCE(bp->cpld_bus_stuck, false).

Second, nothing re-validates the hardware indicator.
adva_x1_mblaze_release() answers the question with a single register read:

	val = ioread32(&bp->pps_select->i2c_bus_ctrl);
	if ((val & MBLAZE_GRANT_MASK) != MBLAZE_GRANT)
		return 0;

but ptp_ocp_read_eeprom() consults only the sticky bool.

Claims are issued by ptp_ocp_cpld_work(), which stops rescheduling once
the one-shot identification is settled:

	if (!READ_ONCE(bp->cpld_id_tried))
		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
				   HZ);

and otherwise only by the root-only cpld_status_show() and the privileged
devlink flash.  So after a single transient -ETIMEDOUT with cpld_id_tried
already set, doesn't the driver's EEPROM read stay suppressed for the rest
of the binding, with devlink info silently omitting the serial number and
board id and serialnum printing 00:00:00:00:00:00, and only a dev_dbg() to
explain it?

Relatedly, the commit message says:

    A later claim that the firmware grants proves the
    handshake is working again and clears it.

Given that the only source of later claims is the one-shot worker plus two
privileged paths, is that recovery generally available, and should the
possibility of a permanently suppressed read be mentioned?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com

Re: [PATCH net-next 3/9] ptp: ocp: hand the TMC bus back once on an acquire timeout

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Stale documented post-condition on adva_x1_mblaze_acquire(): the
  patch removes the in-function adva_x1_mblaze_release() hand-back but…

--

ptp: ocp: hand the TMC bus back once on an acquire timeout

The patch drops the adva_x1_mblaze_release() call from the -ETIMEDOUT path
of adva_x1_mblaze_acquire(), leaving the hand-back to adva_x1_bus_release()
which adva_x1_bus_claim() already calls for the same error.  A comment is
added in adva_x1_bus_claim() recording that the acquire path no longer does
the hand-back itself.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 510083dc750a5..feb61355078a2 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -4526,10 +4526,6 @@ static int adva_x1_mblaze_acquire(struct ptp_ocp *bp)
>  			return 0;
>  	}
>  
> -	/* Drop the request we gave up on.  Any error from the hand-back is
> -	 * subsumed by the -ETIMEDOUT we are already returning.
> -	 */
> -	adva_x1_mblaze_release(bp);
>  	return -ETIMEDOUT;
>  }

[Severity: Low]
Should the header comment on adva_x1_mblaze_acquire() be updated along with
this?  It is left untouched just above the function and still promises the
post-condition that this hunk removes:

drivers/ptp/ptp_ocp.c:
    /* Acquire the shared I2C bus from the MicroBlaze firmware.  Returns with no
     * request outstanding on failure, so the firmware is never left granting a
     * segment to a host that has given up waiting for it.
     */
    static int adva_x1_mblaze_acquire(struct ptp_ocp *bp)

After the change, the -ETIMEDOUT return happens with MBLAZE_REQUEST still
programmed:

	iowrite32(MBLAZE_REQUEST, &bp->pps_select->i2c_bus_ctrl);
	for (i = 0; i < MBLAZE_RETRIES; i++) {
		...
	}

	return -ETIMEDOUT;

so the request is outstanding, and the register comment in this file notes
the firmware can take on the order of 670 ms to answer, i.e. the grant may
still be asserted after the helper returns.

There is no misbehaviour in the current tree: adva_x1_bus_claim() is the
only caller, it assigns bp->cpld_adap = adap before calling acquire so the
"if (!adap) return 0;" early-out in adva_x1_bus_release() cannot skip the
hand-back, and it always calls adva_x1_bus_release() on the error.

The concern is only that a future second caller trusting the helper's own
documentation and simply propagating the error would leave the request armed
with no matching MBLAZE_RELEASE after the i2c root adapter lock is dropped.
Could the header comment say instead that the request is left outstanding on
failure and that the caller must run adva_x1_bus_release()?

> @@ -4647,6 +4643,12 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
>  	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>  	bp->cpld_adap = adap;
>  
> +	/* adva_x1_bus_release() runs the hand-back, so the acquire path does
> +	 * not do it itself: both poll for the grant to drop for up to
> +	 * MBLAZE_RETRIES * MBLAZE_RETRY_US with the adapter lock held, and
> +	 * doing it twice only holds the shared controller for longer and
> +	 * logs the same failure twice.
> +	 */
>  	ret = adva_x1_mblaze_acquire(bp);
>  	if (ret) {
>  		adva_x1_bus_release(bp);	/* keeps the acquire error */

This new comment states that the acquire path does not do the hand-back
itself, which is the opposite of what the acquire header still claims, so
the two comments in the tree disagree about the same invariant.  Checking
the end of the series, the acquire header is still unchanged there, so no
later patch reconciles them.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com

Re: [PATCH net-next 4/9] ptp: ocp: forget a CPLD i2c adapter number that no longer resolves

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The re-arm of the one-shot CPLD identification added to
  adva_x1_cache_i2c_adap() (drivers/ptp/ptp_ocp.c:4618-4621) is dead…
- [High] adva_x1_bus_claim() calls the wildcard form
  adva_x1_forget_i2c_adap(bp, -1) on the i2c_get_adapter() failure path…
- [Medium] adva_x1_cache_i2c_adap() now zeroes bp->cpld_id_attempts
  unconditionally (drivers/ptp/ptp_ocp.c:4620), which silently defeats…
- [Medium] adva_x1_cache_i2c_adap() now performs an unconditional,
  uninterruptible blocking acquisition of bp->cpld_lock -…
- [Low] The patch inserts adva_x1_forget_i2c_adap() (and its own
  kerneldoc-style comment) between the pre-existing block comment that…

--

ptp: ocp: forget a CPLD i2c adapter number that no longer resolves

This adds adva_x1_forget_i2c_adap() and calls it both when i2c_get_adapter()
fails for the cached number and when the parent check rejects the adapter.
It also resets bp->cpld_id_tried and bp->cpld_id_attempts in
adva_x1_cache_i2c_adap() after a number is cached.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index feb61355078a2..45313143b6f76 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -4574,6 +4574,19 @@ static int adva_x1_i2c_adap_match(struct device *dev, const void *data)
>   * both the notifier and a single lookup here can miss the same adapter.
>   * Retrying costs a short klist walk per tick until one of them succeeds.
>   */
> +/*
> + * Forget the cached adapter number so the next sweep resolves it again.
> + * @nr < 0 forgets whatever is cached; otherwise only that number, so a
> + * caller that raced the notifier cannot clear a newer one.
> + */
> +static void adva_x1_forget_i2c_adap(struct ptp_ocp *bp, int nr)
> +{
> +	scoped_guard(spinlock, &bp->cpld_adap_lock) {
> +		if (nr < 0 || bp->cpld_i2c_adap_nr == nr)
> +			bp->cpld_i2c_adap_nr = -1;
> +	}
> +}
> +

[Severity: Low]

Was the placement of the new helper intended here?  The pre-existing block
comment that ends with "Retrying costs a short klist walk per tick until one
of them succeeds." describes adva_x1_cache_i2c_adap(), but it now sits
directly above adva_x1_forget_i2c_adap(), which does not walk any klist and
is called from adva_x1_bus_claim() rather than from the worker tick.

adva_x1_cache_i2c_adap() is left with no header comment at all.  Would moving
the helper (with its own comment) above that block, or below
adva_x1_cache_i2c_adap(), keep each comment attached to the function it
describes?

>  static void adva_x1_cache_i2c_adap(struct ptp_ocp *bp)
>  {
>  	struct device *child;
> @@ -4598,6 +4611,15 @@ static void adva_x1_cache_i2c_adap(struct ptp_ocp *bp)
>  			bp->cpld_i2c_adap_nr = i2c_verify_adapter(child)->nr;
>  	}
>  
> +	/* A different adapter may answer differently, so let the one-shot
> +	 * identification run again rather than leaving cpld.id absent for
> +	 * the rest of the binding.
> +	 */
> +	scoped_guard(mutex, &bp->cpld_lock) {
> +		bp->cpld_id_tried = false;
> +		bp->cpld_id_attempts = 0;
> +	}
> +

[Severity: High]

Can this re-arm ever clear a flag that was actually set?

adva_x1_cache_i2c_adap() has a single caller, ptp_ocp_cpld_work(), and that
worker only re-queues itself while the flag is clear:

ptp_ocp_cpld_work() {
	adva_x1_cache_i2c_adap(bp);
	if (READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
		adva_x1_cpld_read_id(bp);

	if (!READ_ONCE(bp->cpld_id_tried))
		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
				   HZ);
}

The only writer that sets it is adva_x1_cpld_read_id(), which is itself only
called from that worker:

	if (!ret || ++bp->cpld_id_attempts >= CPLD_ID_MAX_ATTEMPTS)
		bp->cpld_id_tried = true;

So once cpld_id_tried is true the worker is never queued again, and every
time the new scoped_guard runs, cpld_id_tried is already false.

The commit message says "leaving cpld_id_tried set kept cpld.id absent for
the rest of the binding".  In that state - identification gave up and a
different adapter shows up afterwards - ptp_ocp_i2c_notifier_call() publishes
the new number:

		if (bp->i2c_ctrl && adap->dev.parent == &bp->i2c_ctrl->dev) {
			scoped_guard(spinlock, &bp->cpld_adap_lock)
				bp->cpld_i2c_adap_nr = adap->nr;
		}

but it neither clears cpld_id_tried/cpld_id_attempts nor queues cpld_work,
and the only other queue_delayed_work(&bp->cpld_work) is the one-time call in
probe.  Does cpld.id therefore still stay 0 for the rest of the binding,
leaving the reset here unreachable for the case it is meant to fix?

[Severity: Medium]

Does zeroing bp->cpld_id_attempts here remove the CPLD_ID_MAX_ATTEMPTS bound
enforced in adva_x1_cpld_read_id()?

The reset is unconditional and sits outside the spinlock section that caches
a number, so it also fires when the locked re-test cached nothing (because
the notifier published a number meanwhile), and it never compares the newly
cached number with the one identification already failed on.  The comment
says "A different adapter may answer differently", but an identical re-cache
wipes the budget too.

Combined with the new forget on !adap in adva_x1_bus_claim(), can this repeat
per tick?  i2c_del_adapter() clears the idr entry before it unregisters the
device:

drivers/i2c/i2c-core-base.c:i2c_del_adapter() {
	mutex_lock(&core_lock);
	found = idr_find(&i2c_adapter_idr, adap->nr);
	if (found == adap)
		idr_replace(&i2c_adapter_idr, NULL, adap->nr);
	mutex_unlock(&core_lock);
	...
	device_unregister(&adap->dev);
}

In that window i2c_get_adapter() returns NULL, so the claim forgets the
number, while device_find_child(&bp->i2c_ctrl->dev, ...) still finds the
adapter child, so the next tick re-caches it and resets the counter to 0.
Each of those ticks performs another bus claim with adva_x1_mblaze_acquire()
polling the grant with the i2c root adapter lock held, and cpld_id_tried can
never latch.

If the retry budget is meant to be dropped here, could the changelog say so,
and could the struct field comment describing cpld_id_tried as a "one-shot ID
read finished, successfully or not" be updated to match?

[Severity: Medium]

Should the worker block uninterruptibly on cpld_lock here?  The rest of the
driver avoids exactly that on this path:

adva_x1_cpld_read_id() {
	if (!mutex_trylock(&bp->cpld_lock))
		return -EBUSY;

cpld_status_show() {
	/* A CPLD operation can hold cpld_lock a long time; stay killable. */
	ret = mutex_lock_interruptible(&bp->cpld_lock);

adva_x1_cpld_flash() holds cpld_lock across the whole programming sequence -
a config-sector erase alone waits up to CPLD_ERASE_MS and the page loop can
write CPLD_MAX_IMAGE_SZ / CPLD_PAGE_SIZE pages, each with its own wait - so
can the worker sleep here for tens of seconds or more?

Two knock-on effects: ptp_ocp_remove() does
cancel_delayed_work_sync(&bp->cpld_work) and inherits that wait, and the
child reference from device_find_child() is still held across the new guard
because put_device(child) only runs after it, so
i2c_del_adapter()'s wait_for_completion(&adap->dev_released) waits for the
same period.

Since only i2c_verify_adapter(child)->nr is used, and that happens inside the
earlier spinlock block, could put_device(child) move ahead of this section,
and could the acquisition follow the trylock pattern the worker already uses?

>  	put_device(child);
>  }
>  
> @@ -4614,8 +4636,14 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
>  	lockdep_assert_held(&bp->cpld_lock);
>  
>  	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));
> -	if (!adap)
> +	if (!adap) {
> +		/* The adapter behind the cached number is gone.  Forget it,
> +		 * or adva_x1_cache_i2c_adap() keeps bailing out on its first
> +		 * test and never resolves the adapter again.
> +		 */
> +		adva_x1_forget_i2c_adap(bp, -1);
>  		return -ENODEV;
> +	}

[Severity: High]

Can the wildcard form here discard a number the notifier just published?

The helper's own comment says the nr >= 0 form exists so that "a caller that
raced the notifier cannot clear a newer one", but this call site passes -1,
which forgets whatever is cached.  The number is sampled with READ_ONCE() and
i2c_get_adapter() then sleeps on the i2c core_lock, so the decision to clear
is based on a value read outside the critical section:

CPU0 adva_x1_bus_claim()
	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));  /* sleeps */

CPU1 ptp_ocp_i2c_notifier_call(), BUS_NOTIFY_ADD_DEVICE
		if (bp->i2c_ctrl && adap->dev.parent == &bp->i2c_ctrl->dev) {
			scoped_guard(spinlock, &bp->cpld_adap_lock)
				bp->cpld_i2c_adap_nr = adap->nr;
		}

CPU0 resumes with !adap
		adva_x1_forget_i2c_adap(bp, -1);   /* nr = -1 again */

The notifier only takes cpld_adap_lock, so cpld_lock does not serialise it
against the claim path.  The common case looks like the sampled value already
being -1: cpld_status_show() or adva_x1_cpld_flash() then calls
i2c_get_adapter(-1), gets NULL, and clears whatever was published meanwhile.

Recovery only exists while ptp_ocp_cpld_work() is still re-queued.  Once
cpld_id_tried is set the worker stops, the notifier will not re-announce an
already registered adapter, and cpld_i2c_adap_nr stays -1, so later CPLD
accesses return -ENODEV - the failure mode this commit is closing.

Would passing the number that was actually sampled, and skipping the call
when that sample was already negative, avoid this?

>  
>  	/* The number is freed before the notifier clears it, so it can
>  	 * already be another adapter's.  Check this is still ours, and drop
> @@ -4623,10 +4651,7 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
>  	 * way, where forgetting it lets the worker look the adapter up again.
>  	 */
>  	if (!bp->i2c_ctrl || adap->dev.parent != &bp->i2c_ctrl->dev) {
> -		scoped_guard(spinlock, &bp->cpld_adap_lock) {
> -			if (bp->cpld_i2c_adap_nr == adap->nr)
> -				bp->cpld_i2c_adap_nr = -1;
> -		}
> +		adva_x1_forget_i2c_adap(bp, adap->nr);
>  		i2c_put_adapter(adap);
>  		return -ENODEV;
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com

Re: [PATCH net-next 5/9] ptp: ocp: correct the CPLD bookkeeping comments and the flash progress

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Inconsistent memory-access annotation for cpld_i2c_adap_nr:
  the patch rewrites the struct comment to state that readers of…
- [Medium] The new cpld_id_tried comment claims "the worker reads it
  unlocked, where a stale value only costs one extra attempt", but the…
- [Low] Stale comment naming the wrong work item: CPLD_ID_MAX_ATTEMPTS is
  documented as "/* one per sync_work tick */" (line 4406), but…

--

ptp: ocp: correct the CPLD bookkeeping comments and the flash progress

This rewrites the struct ptp_ocp member comments for cpld_i2c_adap_nr and
cpld_id_tried to describe serialised writers with lock-free readers, and
converts the cpld_id_tried stores to WRITE_ONCE().  It also changes the
CPLD flash progress notification to report bytes written rather than the
offset of the page just written.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 45313143b6f76..e10f6b5149c93 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -428,9 +428,12 @@ struct ptp_ocp {
>  	/* adva_x1 CPLD I2C (internal use only) */
>  	/* serialises CPLD operations */
>  	struct mutex		cpld_lock;
> -	/* guards cpld_i2c_adap_nr against the bus notifier */
> +	/* serialises the cpld_i2c_adap_nr writers against each other */
>  	spinlock_t		cpld_adap_lock;
> -	/* I2C adapter nr; -1 if absent.  Under cpld_adap_lock */
> +	/* I2C adapter nr, -1 if absent.  Writers hold cpld_adap_lock;
> +	 * readers take no lock and re-validate what they got, since the
> +	 * number can be recycled - see adva_x1_bus_claim().
> +	 */
>  	int			cpld_i2c_adap_nr;

[Severity: Medium]
The new comment states that readers of cpld_i2c_adap_nr take no lock, and
the same reasoning is used in this patch to convert the cpld_id_tried stores
to WRITE_ONCE().  Should the cpld_i2c_adap_nr stores get the same treatment?

The three readers are all unlocked and marked:

adva_x1_cache_i2c_adap()
	if (!bp->has_cpld || !bp->i2c_ctrl ||
	    READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
		return;

adva_x1_bus_claim()
	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));

ptp_ocp_cpld_work()
	if (READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
		adva_x1_cpld_read_id(bp);

while every store stays a plain store:

adva_x1_forget_i2c_adap()
	scoped_guard(spinlock, &bp->cpld_adap_lock) {
		if (nr < 0 || bp->cpld_i2c_adap_nr == nr)
			bp->cpld_i2c_adap_nr = -1;
	}

adva_x1_cache_i2c_adap()
	scoped_guard(spinlock, &bp->cpld_adap_lock) {
		if (bp->cpld_i2c_adap_nr < 0)
			bp->cpld_i2c_adap_nr = i2c_verify_adapter(child)->nr;
	}

ptp_ocp_i2c_notifier_call()
	scoped_guard(spinlock, &bp->cpld_adap_lock)
		bp->cpld_i2c_adap_nr = adap->nr;
	...
	scoped_guard(spinlock, &bp->cpld_adap_lock) {
		if (bp->cpld_i2c_adap_nr == adap->nr)
			bp->cpld_i2c_adap_nr = -1;
	}

cpld_adap_lock excludes the other writers only, so an i2c adapter add or
remove running the notifier while ptp_ocp_cpld_work() or
adva_x1_cache_i2c_adap() performs its READ_ONCE() is a plain-store against
marked-load pair that KCSAN reports on the write side.

An aligned int is not expected to tear on supported architectures, and
adva_x1_bus_claim() re-validates via adap->dev.parent, so is the remaining
issue just that the annotation is now inconsistent with the contract the
comment describes?

> @@ -442,9 +445,12 @@ struct ptp_ocp {
>  	u32			cpld_usercode;
>  	/* cpld_usercode has been read since the last flash */
>  	bool			cpld_usercode_ok;
> -	/* one-shot ID read finished, successfully or not; under cpld_lock */
> +	/* one-shot ID read finished, successfully or not.  Written under
> +	 * cpld_lock; the worker reads it unlocked, where a stale value only
> +	 * costs one extra attempt.
> +	 */
>  	bool			cpld_id_tried;
> -	/* failed ID read attempts so far; under cpld_lock */
> +	/* failed ID read attempts so far; cpld_lock */
>  	unsigned int		cpld_id_attempts;

[Severity: Low]
While the annotation of cpld_id_attempts is being updated here, should the
comment on the macro that bounds it be updated too?  It still says:

#define CPLD_ID_MAX_ATTEMPTS  10     /* one per sync_work tick */

cpld_id_attempts is incremented only in adva_x1_cpld_read_id(), whose only
caller is ptp_ocp_cpld_work(), and the comment above that worker says the
opposite in the same file ("Kept off ptp_ocp_sync_work()").
ptp_ocp_sync_work() no longer touches any CPLD state, so the budget is
consumed one per cpld_work tick.

The comment was accurate before the CPLD identification was moved off the
sync worker earlier in this series; the tree now has two comments about the
same counter naming different workers.

[ ... ]

> @@ -5047,7 +5053,7 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
>  	 */
>  	WRITE_ONCE(bp->cpld_id, 0);
>  	WRITE_ONCE(bp->cpld_usercode_ok, false);
> -	bp->cpld_id_tried = false;
> +	WRITE_ONCE(bp->cpld_id_tried, false);
>  	bp->cpld_id_attempts = 0;

[Severity: Medium]
Two questions about cpld_id_tried here.

First, the new struct comment says a stale unlocked read "only costs one
extra attempt".  In the stale-true direction, does it not cost all of the
remaining attempts?  ptp_ocp_cpld_work() re-arms itself only while the flag
reads false:

	if (!READ_ONCE(bp->cpld_id_tried))
		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
				   HZ);

Second, does anything consume this reset after a flash?

adva_x1_cpld_read_id() has a single caller, ptp_ocp_cpld_work(), and
cpld_work is queued from only two places: ptp_ocp_register_resources() and
that self-requeue above.  The worker stops as soon as the ID read succeeds,
roughly a second after probe, and adva_x1_cpld_flash() clears the flags but
never queues cpld_work:

	WRITE_ONCE(bp->cpld_usercode_ok, false);
	WRITE_ONCE(bp->cpld_id_tried, false);
	bp->cpld_id_attempts = 0;

With no live worker left to read the flag, does cpld_usercode_ok stay false
for the rest of the binding, so ptp_ocp_devlink_info_get():

	if (smp_load_acquire(&bp->cpld_usercode_ok))
		sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
	else
		buf[0] = '\0';

reports fw.cpld with an empty value after a successful flash until the
driver is rebound?

At the baseline the identification ran from ptp_ocp_sync_work(), which
requeued unconditionally every HZ, so the same reset did trigger a re-read.
The peer reset in adva_x1_cache_i2c_adap() still works because it runs
inside the live worker.  Would queueing cpld_work after clearing
cpld_id_tried in adva_x1_cpld_flash() restore the re-identification?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com

Re: [PATCH net-next 6/9] ptp: ocp: report fw.cpld with an empty value until the USERCODE is read

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After any CPLD flash that reaches the erase step, the fw.cpld
  running version is never published again (and, at this commit, cpld.id…

--

ptp: ocp: report fw.cpld with an empty value until the USERCODE is read

Instead of publishing the literal string "unknown" for the fw.cpld running
version before the CPLD USERCODE has been read, the driver now passes an
empty value so the component name is still registered with devlink while no
version attribute is emitted.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index e10f6b5149c93..4ce86df6e1968 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -2264,15 +2264,20 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>  		/* The flashable component.  Naming it here is what lets
>  		 * "devlink dev flash ... component fw.cpld" through, as the
>  		 * core matches the name against the versions reported here,
> -		 * so it is reported for every board that has the part and not
> +		 * so it is named for every board that has the part and not
>  		 * only once its USERCODE has been read: a part left holding a
>  		 * bad image answers neither, and gating the component on the
>  		 * read would make that state unrecoverable.
> +		 *
> +		 * An empty value still registers the name with the core -
> +		 * devlink_info_version_put() runs its version_cb before the
> +		 * empty-value early-out - while emitting no version attribute,
> +		 * so nothing is published until the USERCODE has been read.
>  		 */
>  		if (smp_load_acquire(&bp->cpld_usercode_ok))
>  			sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
>  		else
> -			strscpy(buf, "unknown", sizeof(buf));
> +			buf[0] = '\0';

[Severity: Medium]
The comment says "nothing is published until the USERCODE has been read".
After a flash, can the USERCODE ever be read again?

adva_x1_cpld_flash() invalidates the cached identity before erasing:

	WRITE_ONCE(bp->cpld_id, 0);
	WRITE_ONCE(bp->cpld_usercode_ok, false);
	WRITE_ONCE(bp->cpld_id_tried, false);
	bp->cpld_id_attempts = 0;

but the only thing that re-reads it is ptp_ocp_cpld_work(), and that worker
re-arms itself only while the one-shot has not settled:

	if (!READ_ONCE(bp->cpld_id_tried))
		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
				   HZ);

adva_x1_cpld_read_id() sets cpld_id_tried on success, so one second after
probe there is no pending cpld_work item left, and clearing the flag in the
flash path has nothing to wake.  The only other queue site for bp->cpld_work
is ptp_ocp_probe(), and no exit path of adva_x1_cpld_flash() queues it.

With no further read, smp_store_release(&bp->cpld_usercode_ok, true) in
adva_x1_cpld_read_id() never runs again, so ptp_ocp_devlink_info_get() takes
the new else branch forever, and devlink_info_version_put() drops it:

net/devlink/dev.c:devlink_info_version_put() {
	if (req->version_cb)
		req->version_cb(version_name, version_type,
				req->version_cb_priv);

	if (!req->msg || !*version_value)
		return 0;
	...
}

Does this mean fw.cpld is silently missing from every subsequent
"devlink dev info" after a successful flash, recoverable only by unbind and
rebind?  At this commit cpld.id is dropped the same way, since
adva_x1_cpld_flash() also clears bp->cpld_id.

For background: before the read was moved off the always-re-armed 1 Hz
ptp_ocp_sync_work(), clearing cpld_id_tried did cause a re-read on the next
tick; the commit that made the read a one-shot did not add a re-arm on the
flash path.  Checking the end of the series (5b9611f), adva_x1_cpld_flash()
still clears cpld_usercode_ok and cpld_id_tried and cpld_work is still
queued only from probe and from the worker itself, so the missing fw.cpld
version persists there; the cpld.id half is addressed by the later commit
"ptp: ocp: drop only the USERCODE when flashing, and drop it before erasing",
which stops clearing cpld_id.

Would queueing bp->cpld_work from the flash path close this?

>  		err = devlink_info_version_running_put_ext(req, "fw.cpld", buf,
>  							   ver_type);
>  		if (err)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com

Re: [PATCH net-next 7/9] ptp: ocp: drop only the USERCODE when flashing, and drop it before erasing

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] adva_x1_cpld_flash() invalidates the cached CPLD identification
  state (WRITE_ONCE(bp->cpld_usercode_ok, false);…
- [Medium] The patch adds to Documentation/networking/devlink/ptp_ocp.rst
  that the driver "checks only that its length is a non-zero multiple of…
- [Low] The new paragraph in Documentation/networking/devlink/ptp_ocp.rst
  states unconditionally that "The erase clears the configuration flash…
- [Low] The patch adds to the "Info versions" table in
  Documentation/networking/devlink/ptp_ocp.rst that for fw.cpld "the…

--

ptp: ocp: drop only the USERCODE when flashing, and drop it before erasing

This moves the invalidation of the cached CPLD identification in
adva_x1_cpld_flash() to before the ERASE command is issued, stops clearing
cpld_id (the Lattice IDCODE), and rewrites the matching documentation in
Documentation/ABI/testing/sysfs-timecard and
Documentation/networking/devlink/ptp_ocp.rst.

A few questions below about the new invalidation point and the new wording.

[ ... Documentation/ABI/testing/sysfs-timecard hunk snipped ... ]

> diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
> index f94b759d9cd65..249ca63eebf61 100644
> --- a/Documentation/networking/devlink/ptp_ocp.rst
> +++ b/Documentation/networking/devlink/ptp_ocp.rst
> @@ -32,14 +32,17 @@ The ``ptp_ocp`` driver reports the following versions

[ ... ]

>     * - ``fw.cpld``
>       - running
>       - USERCODE of the image programmed into the TAP CPLD, formatted as
>         ``0x%08x``.  Read together with ``cpld.id`` and reported the same
> -       way.  This is the component name to pass to ``devlink dev flash``
> -       to update the CPLD.
> +       way; it is dropped when an update erases the part and reported
> +       again once the new image has been read back.  This is the

[Severity: High]
Which code path performs the read back this sentence describes?

adva_x1_cpld_read_id() is the only writer that sets cpld_usercode_ok back
to true:

drivers/ptp/ptp_ocp.c:adva_x1_cpld_read_id() {
	...
	smp_store_release(&bp->cpld_usercode_ok, true);
	...
}

and its only caller is ptp_ocp_cpld_work(), which requeues itself only from
inside its own run:

drivers/ptp/ptp_ocp.c:ptp_ocp_cpld_work() {
	adva_x1_cache_i2c_adap(bp);
	if (READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
		adva_x1_cpld_read_id(bp);

	if (!READ_ONCE(bp->cpld_id_tried))
		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
				   HZ);
}

The other queue site is ptp_ocp_probe(), once per binding:

	if (bp->has_cpld)
		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
				   HZ);

In the normal case the probe-time read succeeds, adva_x1_cpld_read_id() sets
cpld_id_tried true, and the worker chain stops about a second after probe.
Neither adva_x1_cpld_flash() nor the devlink flash entry point queues
cpld_work again, and ptp_ocp_i2c_notifier_call() only updates
cpld_i2c_adap_nr.

So after any flash that reaches the new invalidation point - including a
fully successful one - does cpld_usercode_ok stay false until unbind/rebind,
making ptp_ocp_devlink_info_get() take the empty-value branch:

drivers/ptp/ptp_ocp.c:ptp_ocp_devlink_info_get() {
	...
		if (smp_load_acquire(&bp->cpld_usercode_ok))
			sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
		else
			buf[0] = '\0';
	...
}

and devlink_info_version_put() then drops the attribute entirely:

net/devlink/dev.c:devlink_info_version_put() {
	...
	if (!req->msg || !*version_value)
		return 0;
	...
}

If that is right, is the fw.cpld running version permanently omitted after a
successful CPLD update, and is the clearing of cpld_id_tried in the flash
path a dead store since nothing re-evaluates the requeue condition?

Would pairing the invalidation with a mod_delayed_work() on bp->cpld_work
after the bus is released (outside cpld_lock, so the worker's lock
acquisition can succeed), or an inline USERCODE re-read at the end of the
flash sequence, match what this documentation now promises?  Otherwise
should the text say the value stays absent until rebind?

> +       component name to pass to ``devlink dev flash`` to update the
> +       CPLD, and the name is reported even while the value is not, so a
> +       part left holding a bad image can still be reflashed.

[Severity: Low]
This isn't a bug, but in a table describing what devlink dev info shows,
is "the name is reported even while the value is not" the right wording?

With an empty value, devlink_info_version_put() hands the name to
req->version_cb and then returns before opening the nest:

net/devlink/dev.c:devlink_info_version_put() {
	...
	if (req->version_cb)
		req->version_cb(version_name, version_type,
				req->version_cb_priv);

	if (!req->msg || !*version_value)
		return 0;
	...
}

so neither DEVLINK_ATTR_INFO_VERSION_NAME nor _VALUE is emitted; the name
only reaches devlink_flash_component_lookup_cb() so that the component stays
selectable for flashing.  The driver's own comment in
ptp_ocp_devlink_info_get() already says "nothing is published".

Could this say the component stays selectable for flashing rather than that
its name is reported?

> @@ -59,12 +62,17 @@ selected with the component name.
>       - The configuration flash of the TAP CPLD on ADVA TimeCard X1 boards,
>         programmed over I2C with the MachXO3 in-system programming commands
>         and activated with a REFRESH, so the new image runs immediately.
> -       The image is the raw configuration bitstream.  The only check the
> -       driver makes is that its length is a non-zero multiple of the
> -       16-byte page size, so a container such as ``.jed`` has to be
> -       converted first rather than passed through - one whose length
> -       happens to be a multiple of 16 would be programmed as if it were
> -       a bitstream.
> +       The image is the raw configuration bitstream.  The driver checks
> +       only that its length is a non-zero multiple of the 16-byte page
> +       size and that it is not larger than the part takes, so a container

[Severity: Medium]
Does "not larger than the part takes" describe the check that is actually
made?  The only other size test in adva_x1_cpld_flash() is:

	if (fw->size > CPLD_MAX_IMAGE_SZ) {
		NL_SET_ERR_MSG_MOD(extack, "image too large for this part");
		return -EINVAL;
	}

and the constant's own comment says it is not a capacity check:

/* Bounds how long a claim can hold the i2c root lock, not image validity:
 * far above any bitstream this part takes.
 */
#define CPLD_MAX_IMAGE_SZ     (256 * 1024)

An image several times larger than the configuration flash, but under
256 KiB and a multiple of 16, passes both tests and is erased and written
page by page.  The tree then carries two contradictory statements about the
same limit.

Should the documentation describe it as an upper bound on how long the I2C
bus is held rather than a part-capacity check (and possibly the extack
string reworded to match)?

> +       such as ``.jed`` has to be converted first rather than passed
> +       through - one whose length happens to be a multiple of 16 would be
> +       programmed as if it were a bitstream.
> +
> +       The erase clears the configuration flash before the first page is
> +       written, so any failure from that point on - including an abort on
> +       a fatal signal - leaves the CPLD unconfigured until a valid image
> +       is written.  The component stays available for that.

[Severity: Low]
Is "any failure from that point on ... leaves the CPLD unconfigured" true at
both ends of the sequence?

After a successful REFRESH the new image is already running:

drivers/ptp/ptp_ocp.c:adva_x1_cpld_flash() {
	...
	err = adva_x1_cpld_write(bp, CPLD_CMD_REFRESH);
	if (err)
		goto exit_config;

	bp->cpld_in_config_mode = false;
	...
}

yet a failure of the mux reselect retry loop, of adva_x1_cpld_wait_ready(),
of the status read, or of the bus release still returns an error with the new
configuration active and DONE set.

At the other end, the driver enters ISP with

#define CPLD_CMD_EN_CFG_TP    0x74   /* enable config, transparent mode */

and erases the configuration sector only:

	/* cfg sector only */
	static const u8 erase_cfg[] = { 0x04, 0x00, 0x00 };

while the failure path calls adva_x1_cpld_exit_config(), which sends DIS_CFG
rather than REFRESH.  On MachXO2/XO3 transparent mode the loaded design keeps
running until a REFRESH or power cycle, so a page-write failure or fatal
signal abort appears to leave the old design running with an invalid boot
image.

Would it be more accurate to say a failure after the erase can leave an
invalid boot image, and that whether the old or the new design is active
depends on whether REFRESH was issued?

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 4ce86df6e1968..0d6d0c02882c1 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -5043,6 +5043,21 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
>  		goto exit_config;
>  	}
>  
> +	/* Once the erase is issued the image is gone whatever happens next -
> +	 * an ACKed ERASE runs in the part even if the wait for it fails - so
> +	 * stop reporting the USERCODE before sending it rather than after
> +	 * the whole sequence has succeeded.
> +	 *
> +	 * cpld.id is left alone: it is the Lattice IDCODE, a property of the
> +	 * silicon that erasing the configuration flash cannot change, so
> +	 * dropping it on a failed update only hid information that was still
> +	 * correct.  Written under cpld_lock, which adva_x1_cpld_read_id()
> +	 * also holds across its own bookkeeping.
> +	 */
> +	WRITE_ONCE(bp->cpld_usercode_ok, false);
> +	WRITE_ONCE(bp->cpld_id_tried, false);
> +	bp->cpld_id_attempts = 0;

[Severity: High]
This is the same concern as above, at the code that raises it.

Moving the invalidation ahead of the ERASE also means it now runs before the
command is even sent.  If the ERASE write is NAKed and the part never acted
on it, a still-correct USERCODE has been dropped - and with the worker
already retired there is no path that reads it again.

Is there a reason not to queue bp->cpld_work (or re-read the USERCODE
inline) once the bus has been released, so that cpld_usercode_ok can become
true again within the same binding?

>  	devlink_flash_update_status_notify(devlink, "Erasing",
>  					   ADVA_CPLD_COMPONENT, 0, 0);
>  	err = adva_x1_cpld_write(bp, CPLD_CMD_ERASE);
> @@ -5051,16 +5066,6 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
>  	if (err)
>  		goto exit_config;
>  
> -	/* The old image is gone from here on, so stop reporting its
> -	 * identity even if the rest of the sequence fails.  Written under
> -	 * cpld_lock, which adva_x1_cpld_read_id() also holds across its own
> -	 * bookkeeping, so the worker cannot resurrect any of it.
> -	 */
> -	WRITE_ONCE(bp->cpld_id, 0);
> -	WRITE_ONCE(bp->cpld_usercode_ok, false);
> -	WRITE_ONCE(bp->cpld_id_tried, false);
> -	bp->cpld_id_attempts = 0;
> -
>  	err = adva_x1_cpld_write(bp, CPLD_CMD_RESET_ADDR);
>  	if (err)
>  		goto exit_config;

Also, the commit message says the documentation should "describe what is
actually dropped and restored".  With no code path restoring
cpld_usercode_ok after a flash, is there anything restored to describe?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com

Re: [PATCH net-next 8/9] ptp: ocp: tolerate a latched FAILED when entering configuration mode

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch relaxes only the EN_CFG_TP wait to
  adva_x1_cpld_wait_idle() (drivers/ptp/ptp_ocp.c:5030) but leaves the…

--

ptp: ocp: tolerate a latched FAILED when entering configuration mode

The first status wait after CPLD_CMD_EN_CFG_TP in adva_x1_cpld_flash() is
changed from adva_x1_cpld_wait_ready() to adva_x1_cpld_wait_idle(), so a
latched CPLD_STATUS_FAILED no longer turns the enable step into -EIO.  The
following CPLD_STATUS_ENAB check is left to decide whether the part entered
configuration mode.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 0d6d0c02882c1..4f2bf54a23c21 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -5018,9 +5018,16 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
>  	 */
>  	bp->cpld_in_config_mode = true;
>  
> +	/* wait_idle(), not wait_ready(): FAILED is latched across operations
> +	 * and nothing here clears it, so treating it as fatal at the enable
> +	 * step would make every later flash of a part that has failed once
> +	 * return -EIO before reaching the ERASE and REFRESH that put it back
> +	 * into a defined state.  ENAB below is what says the enable worked;
> +	 * machxo2-spi.c likewise tests FAIL only after ISC_ENABLE completes.
> +	 */
>  	err = adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP);
>  	if (!err)
> -		err = adva_x1_cpld_wait_ready(bp, 5000);
> +		err = adva_x1_cpld_wait_idle(bp, 5000);
>  	if (err)
>  		goto exit_config;

[Severity: High]

Given the premise that FAILED is latched and nothing in the driver clears
it, does the sequence actually reach the ERASE and REFRESH that the new
comment says put the part back into a defined state?

The next wait in adva_x1_cpld_flash() is still the strict one:

	err = adva_x1_cpld_write(bp, CPLD_CMD_ERASE);
	if (!err)
		err = adva_x1_cpld_wait_ready(bp, CPLD_ERASE_MS);
	if (err)
		goto exit_config;

and adva_x1_cpld_wait_ready() stops polling on FAILED independently of
BUSY, with sleep_before_read == false, so the first status read can
observe the stale bit:

	ret = read_poll_timeout(adva_x1_cpld_read_status, err,
				err || (status & CPLD_STATUS_FAILED) ||
				!(status & CPLD_STATUS_BUSY),
				CPLD_POLL_US, max_ms * USEC_PER_MSEC, false,
				bp, &status);
	if (ret)
		return ret;
	...
	if (status & CPLD_STATUS_FAILED)
		return -EIO;

So on a part with FAILED latched from an earlier aborted update, wouldn't
"devlink dev flash ... component fw.cpld" still return -EIO, just a few
commands later than before?

Is the state the part is left in also worse than before this change?  The
ERASE has been ACKed by then, and this function documents that itself:

	/* Once the erase is issued the image is gone whatever happens next -
	 * an ACKed ERASE runs in the part even if the wait for it fails - so
	 * stop reporting the USERCODE before sending it rather than after
	 * the whole sequence has succeeded.

The goto exit_config path runs adva_x1_cpld_exit_config(), which only waits
BUSY out and sends CPLD_CMD_DIS_CFG, so CPLD_CMD_SET_DONE and
CPLD_CMD_REFRESH are never issued.  Before this patch the same device state
produced a refusal at the enable step with the configuration image intact;
afterwards it erases the configuration flash with DONE cleared and no
refresh.  On this board the CPLD is only reachable through the TMC mux
segment, so does an unconfigured part still have the access path needed to
write a replacement image?

Would the ERASE, page-write (adva_x1_cpld_wait_ready(bp, 100) in the page
loop) and SET_DONE waits need the same treatment, i.e. tolerating a
pre-existing FAILED and testing it only after the operation completes?
That is what the cited machxo2_write_init() in drivers/fpga/machxo2-spi.c
does - it issues ISC_ENABLE and ISC_ERASE, waits with
wait_until_not_busy(), and only then tests FAIL:

	ret = wait_until_not_busy(spi);
	if (ret)
		goto fail;

	get_status(spi, &status);
	if (test_bit(FAIL, &status)) {

The alternative reading, that the part clears FAILED at the start of each
new ISC operation, would make the enable wait harmless either way, but it
also contradicts the "FAILED is latched across operations and nothing in
the driver clears it" premise the commit message and the new comment are
built on.  Which of the two describes the hardware?

Checking the rest of the series, the ERASE wait is unchanged through
5b9611f9 ("ptp: ocp: confirm the CPLD really left configuration mode after
REFRESH"), so this does not appear to be addressed by a later patch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com

Re: [PATCH net-next 9/9] ptp: ocp: confirm the CPLD really left configuration mode after REFRESH

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In adva_x1_cpld_flash() (drivers/ptp/ptp_ocp.c), the new post-
  REFRESH check sets bp->cpld_in_config_mode = true when…

Pre-existing issues:
- [Low] Documentation/ABI/testing/sysfs-timecard (lines 22-27) defines
  the cpld_status flag "done" as "the configuration flash holds a valid…

--

ptp: ocp: confirm the CPLD really left configuration mode after REFRESH

The post-REFRESH status check in adva_x1_cpld_flash() now also requires
CPLD_STATUS_ENAB to be clear, on the grounds that DONE/BUSY/ERR alone cannot
tell a REFRESH that rebooted the part from one that was only ACKed.  When
ENAB is still set, bp->cpld_in_config_mode is restored to true before the
function fails with -EIO.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 4f2bf54a23c21..9c2b7403bfd05 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -5135,6 +5135,9 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
>  
>  	/* REFRESH reboots the CPLD out of configuration mode, so the exit
>  	 * path must not send DIS_CFG afterwards even if a check below fails.
> +	 * The ENAB test below confirms it really left; until then assume it
> +	 * did, because sending DIS_CFG to a part that has rebooted is what
> +	 * this flag exists to avoid.
>  	 */
>  	bp->cpld_in_config_mode = false;
>  
> @@ -5156,12 +5159,21 @@ static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
>  	/* Require DONE set, not busy and no error code, as machxo2-spi.c does
>  	 * after a refresh: without it a CRC or preamble error reads back as a
>  	 * successful update.
> +	 *
> +	 * ENAB has to be clear too.  Those three conditions are already met
> +	 * by the state SET_DONE leaves behind, so on their own they cannot
> +	 * tell a REFRESH that rebooted the part from one whose frame was
> +	 * ACKed but never latched - which leaves the part in configuration
> +	 * mode still running the old image.  Leaving configuration mode is
> +	 * the one thing only a REFRESH does.
>  	 */
>  	err = adva_x1_cpld_read_status(bp, &st);
>  	if (err)
>  		goto deselect;
> +	if (st & CPLD_STATUS_ENAB)
> +		bp->cpld_in_config_mode = true;
>  	if (!(st & CPLD_STATUS_DONE) || (st & CPLD_STATUS_BUSY) ||
> -	    (st & CPLD_STATUS_ERR)) {
> +	    (st & CPLD_STATUS_ERR) || (st & CPLD_STATUS_ENAB)) {
>  		dev_err(&bp->pdev->dev,
>  			"CPLD refresh left status 0x%08x\n", st);
>  		NL_SET_ERR_MSG_MOD(extack, "CPLD did not come back configured");

[Severity: High]
Can the restored flag actually be acted on in this call?  The ENAB-set case
sets bp->cpld_in_config_mode = true and then leaves via goto deselect, but
the labels at the bottom of adva_x1_cpld_flash() are ordered:

	exit_config:
		adva_x1_cpld_exit_config(bp);
	deselect:
		ret = adva_x1_mux_select(bp, -1);
		...
		ret = adva_x1_bus_release(bp);

so jumping to deselect skips adva_x1_cpld_exit_config(), which is the only
code that issues CPLD_CMD_DIS_CFG and the only reader of the flag:

	static void adva_x1_cpld_exit_config(struct ptp_ocp *bp)
	{
		int err, i;

		if (!bp->cpld_in_config_mode)
			return;

		for (i = 0; i < CPLD_EXIT_CFG_TRIES; i++) {
			err = adva_x1_cpld_wait_idle(bp, CPLD_ERASE_MS);
			if (!err)
				err = adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG);
	...

In the exact case this change detects - REFRESH ACKed but not latched, part
still in transparent configuration mode running the old image - the mux
channel, the I2C segment and cpld_lock are all released with no DIS_CFG
attempted, even though the earlier comment in the same function says:

	/* Set before EN_CFG_TP, not after: ...
	 * makes the exit path send DIS_CFG.  A stray DIS_CFG is harmless;
	 * leaving config mode enabled is not.
	 */
	bp->cpld_in_config_mode = true;

The commit message says the flag is put back "so the exit path and the
recovery at the start of the next flash can act on it".  Is the exit path
part reachable here?  The other call site of adva_x1_cpld_exit_config() is
the recovery at the start of the next adva_x1_cpld_flash(), so recovery only
happens if another devlink flash of fw.cpld is issued on the same bound
instance.

Is that deferral durable?  ptp_ocp_remove() does not exit configuration
mode, and a fresh probe zero-initialises bp->cpld_in_config_mode, so after
rmmod, rebind or a PCI reset the knowledge that the part is still latched in
configuration mode is gone.

Would goto exit_config be the right target for the ENAB-set case?
adva_x1_cpld_exit_config() returns immediately when the flag is clear, so the
other post-REFRESH failure paths would be unaffected.  For comparison,
machxo2_write_complete() retries LSC_REFRESH and always runs its cleanup;
here neither a REFRESH retry nor DIS_CFG is attempted.

[Severity: Low]
This isn't a bug introduced by this patch, but the new interpretation of the
status register does not line up with what the sysfs ABI says about the same
register.  Documentation/ABI/testing/sysfs-timecard describes cpld_status as:

		==========  ================================================
		done        the configuration flash holds a valid image and
			    it is active
		busy        an internal operation is in progress
		failed      the last ISC operation failed
		==========  ================================================

and cpld_status_show() emits only those three bits:

	if (st & CPLD_STATUS_DONE)
		count += sysfs_emit_at(buf, count, "done");
	if (st & CPLD_STATUS_BUSY)
		count += sysfs_emit_at(buf, count, "%sbusy", count ? " " : "");
	if (st & CPLD_STATUS_FAILED)
		count += sysfs_emit_at(buf, count, "%sfailed",
				       count ? " " : "");

CPLD_STATUS_ENAB is never surfaced.  After the failure this patch adds (DONE
set, ENAB set), a read of /sys/class/timecard/ocpN/cpld_status reports "done",
which per the documented wording means the image is valid and active.  Should
the ABI text and cpld_status_show() also learn about the configuration-mode
bit so the state being detected here is visible?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922142829.57740-1-maimon.sagi%40gmail.com