[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