[PATCH net-next v15 0/4] ptp: ocp: add TAP CPLD support for ADVA TimeCard X1

[PATCH net-next v15 0/4] ptp: ocp: add TAP CPLD support for ADVA TimeCard X1

[Sagi Maimon]

The ADVA TimeCard X1 (PCI device 0x0410) carries a Lattice MachXO3 CPLD
on the TMC I2C bus.  This series adds access to it and an update path
through devlink flash.

The card has two I2C controllers.  Linux registers only the one at
0x00150000, normally routed to the config EEPROMs; the one at 0x00120000
belongs to the on-card MicroBlaze and is routed to the TMC or M.2 bus as
its firmware selects.  A request/grant handshake re-routes the host
controller onto the TMC bus, where the PCA9548 and the CPLD behind its
channel 0 live.  Everything here is built on that handshake.

Patches 1 and 2 are pre-existing probe-error-path bugs that were buried
in the feature commit in v13; they are split out with Fixes: tags.  Patch
1 is also a prerequisite: patch 3 adds a mutex that ptp_ocp_detach()
destroys, so devlink has to be unregistered first.  Patch 3 adds the bus
arbitration and the read-only interfaces, patch 4 the flashing.

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

Changes since v14 - all from the automated review of v14:

 - The hand-back never waited.  MBLAZE_GRANTED contains the host's own
   request bits, which the release write clears first, so the poll could
   not iterate and the adapter was unlocked while the controller was
   still on the TMC bus.  It now tests the firmware-owned grant field.
   Measured on the card: the register reads 0x00000012 once the segment
   is handed back, ~670 ms after the release write - so the wait is real
   and a "read 0" test would never have exited.  Patch 3.
 - adva_x1_mblaze_release() and adva_x1_bus_release() return the failure
   instead of only logging it, so an operation over a bus that may still
   be mis-routed fails rather than reporting success.  The adapter is
   still unlocked, since holding the root lock would stall every other
   user with no way out.  Patch 3.
 - fw.cpld is reported for every board that has the part, not only once
   its USERCODE has been read.  devlink resolves the component name from
   ->info_get(), so gating it on the cached identity made a part holding
   a bad image - which answers neither read - impossible to reflash.  The
   value is "unknown" until the read succeeds.  Patch 4.
 - The cached identity is dropped right after ERASE completes rather than
   only after every later check passed, so a failure part-way through no
   longer leaves the pre-flash cpld.id and fw.cpld published.  Patch 4.
 - The one-shot ID bookkeeping moved inside adva_x1_cpld_read_id(), under
   cpld_lock, where a flash that resets it cannot have it overwritten by
   the worker afterwards.  Patch 3.
 - A failing USERCODE read no longer discards the device ID that was just
   read, and the value/validity pair is published with
   smp_store_release()/smp_load_acquire() rather than bare WRITE_ONCE().
   Patch 4.
 - The flash now confirms the part actually entered configuration mode
   (status ENAB) after EN_CFG_TP.  An enable frame that does not latch
   leaves an idle part reporting neither BUSY nor FAILED, the erase and
   page writes are ignored and DONE is still set from the old image, so
   every later check passed and the update was reported successful with
   the flash untouched.  Patch 4.
 - adva_x1_cpld_exit_config() gates the flag on the wait it was already
   doing and retries, instead of recording an exit that may not have
   happened; the flash also retries the exit on entry, so a part left
   latched by an earlier failure is recovered.  Patch 4.
 - An upper bound on the image and a fatal-signal check in the page loop:
   both cpld_lock and the i2c root lock are held for the whole image, so
   the duration was caller-chosen and uninterruptible.  Patch 4.
 - cpld_i2c_adap_nr is serialised against the i2c bus notifier, and the
   lookup runs from the worker rather than once from probe: device_add()
   notifies before linking the device to its parent, so with asynchronous
   probing both writers could miss the same adapter.  A number that fails
   the parent check in adva_x1_bus_claim() is now forgotten so the lookup
   runs again.  Patch 3.
 - A mux channel select reported as failed may still have been ACKed, so
   the deselect runs on that path too rather than handing the mux back
   with a channel possibly open; a failed deselect is no longer silently
   dropped.  Patches 3 and 4.
 - The CPLD component reports a terminal "Flash complete"/"Flash error"
   status like the SPI path, and the pre-refresh DONE check logs and sets
   extack like the post-refresh one.  Patch 4.
 - linux/iopoll.h and linux/firmware.h moved to patch 4, where their
   first users are.  Comments no longer describe an upload path that only
   exists in the later patch, and the documentation no longer claims .jed
   containers are rejected - only the page-multiple length is checked.
 - Documentation no longer says the identification read happens once: a
   successful update re-arms it.

Tested on an ADVA TimeCard X1: a full erase/program/refresh cycle of a
6526-page image through "devlink dev flash ... component fw.cpld",
cpld_status reading done afterwards and across a reboot, and the
rejection paths for a .jed image and an unknown component name.


Sagi Maimon (4):
  ptp: ocp: unregister devlink before detach on probe error
  ptp: ocp: fix dpll cleanup on probe error
  ptp: ocp: add TAP CPLD access for ADVA TimeCard X1
  ptp: ocp: add TAP CPLD flashing via devlink

 Documentation/ABI/testing/sysfs-timecard     |  29 +
 Documentation/networking/devlink/index.rst   |   1 +
 Documentation/networking/devlink/ptp_ocp.rst |  75 ++
 MAINTAINERS                                  |   1 +
 drivers/ptp/ptp_ocp.c                        | 939 ++++++++++++++++++-
 5 files changed, 1039 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/networking/devlink/ptp_ocp.rst


base-commit: 87b80c2f6b05cad9f0ff9136709c62a0f59923e3
-- 
2.47.0

[PATCH net-next v15 1/4] ptp: ocp: unregister devlink before detach on probe error

[Sagi Maimon]

From: Sagi Maimon <maimon.sagi@gmail.com>

devlink_register() runs before the dpll setup in ptp_ocp_probe(), but the
failure paths after it went straight to ptp_ocp_detach() and then
devlink_free(), which opens with ASSERT_DEVLINK_NOT_REGISTERED(devlink).
Any dpll failure during probe therefore freed a devlink instance that was
still registered.

Route those paths through a new out_devlink label that unregisters first,
matching the order ptp_ocp_remove() already uses.

Fixes: 09eeb3aecc6c ("ptp_ocp: implement DPLL ops")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 3d26ec1f7b9e..401b04f12d2b 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -5119,12 +5119,12 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(bp->dpll)) {
 		err = PTR_ERR(bp->dpll);
 		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
-		goto out;
+		goto out_devlink;
 	}
 
 	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
 	if (err)
-		goto out;
+		goto out_devlink;
 
 	for (i = 0; i < OCP_SMA_NUM; i++) {
 		bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE,
@@ -5151,6 +5151,8 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		dpll_pin_put(bp->sma[i].dpll_pin, &bp->sma[i].tracker);
 	}
 	dpll_device_put(bp->dpll, &bp->tracker);
+out_devlink:
+	devlink_unregister(devlink);
 out:
 	ptp_ocp_detach(bp);
 out_disable:
-- 
2.47.0

[PATCH net-next v15 2/4] ptp: ocp: fix dpll cleanup on probe error

[Sagi Maimon]

From: Sagi Maimon <maimon.sagi@gmail.com>

Two problems on the dpll error paths in ptp_ocp_probe().

dpll_device_register() takes a second reference and stores bp as the
registration private data, and only dpll_device_unregister() drops it.
out_dpll dropped bp->tracker but never unregistered, so a dpll_pin_get()
or dpll_pin_register() failure left the device registered and visible to
userspace with a priv pointer into the storage devlink_free() releases a
few lines later, as well as leaking the registration object.

A failing dpll_device_register() jumped to the same label as the failing
dpll_device_get(), so the reference taken by dpll_device_get() was never
dropped either.  Give that case its own label.

Fixes: 09eeb3aecc6c ("ptp_ocp: implement DPLL ops")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 401b04f12d2b..f4299878c27c 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -5124,7 +5124,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
 	if (err)
-		goto out_devlink;
+		goto out_dpll_put;
 
 	for (i = 0; i < OCP_SMA_NUM; i++) {
 		bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE,
@@ -5150,6 +5150,12 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]);
 		dpll_pin_put(bp->sma[i].dpll_pin, &bp->sma[i].tracker);
 	}
+	/* dpll_device_register() takes a second reference and stores bp; only
+	 * dpll_device_unregister() drops it.  Without this the device stays
+	 * visible with a priv pointer into storage devlink_free() will release.
+	 */
+	dpll_device_unregister(bp->dpll, &dpll_ops, bp);
+out_dpll_put:
 	dpll_device_put(bp->dpll, &bp->tracker);
 out_devlink:
 	devlink_unregister(devlink);
-- 
2.47.0

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

[Sagi Maimon]

From: Sagi Maimon <maimon.sagi@gmail.com>

The ADVA TimeCard X1 carries a Lattice MachXO3 CPLD on its TMC I2C bus.
Reaching it means taking that bus from the on-card MicroBlaze first.

The card has two I2C controllers.  Linux registers only the one at
0x00150000, normally routed to the config EEPROMs; the one at 0x00120000
belongs to the MicroBlaze and is routed to the TMC or M.2 bus as its
firmware selects.  A request/grant handshake re-routes the host controller
onto the TMC bus, where the PCA9548 and the CPLD behind its channel 0
live.  An operation takes the i2c core adapter lock for the whole grant
window and uses __i2c_transfer() internally; without that, a concurrent
transfer from ptp_ocp_read_eeprom(), from the nvmem attributes or from the
at24 sysfs files would be issued onto the TMC bus instead of to the
EEPROM.  The hand-back waits for the firmware to drop the grant before the
adapter lock is released, for the same reason.

On top of the arbitration this adds two read-only interfaces: a root-only
cpld_status attribute reporting the flags in the CPLD status register, and
the Lattice device ID as the fixed devlink version cpld.id.  Reading the
ID claims the bus, so it must not sit on DEVLINK_CMD_INFO_GET, which
carries no GENL_ADMIN_PERM; the driver reads it once from its own worker,
as soon as the i2c adapter has registered, and info_get() reports the
cached value.

Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 Documentation/ABI/testing/sysfs-timecard     |  24 +
 Documentation/networking/devlink/index.rst   |   1 +
 Documentation/networking/devlink/ptp_ocp.rst |  35 ++
 MAINTAINERS                                  |   1 +
 drivers/ptp/ptp_ocp.c                        | 514 ++++++++++++++++++-
 5 files changed, 571 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/networking/devlink/ptp_ocp.rst

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index 3ae41b7634ac..e58824e0cdcd 100644
--- a/Documentation/ABI/testing/sysfs-timecard
+++ b/Documentation/ABI/testing/sysfs-timecard
@@ -11,6 +11,30 @@ Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
 Description:	This directory contains the attributes of the Nth timecard
 		registered.
 
+What:		/sys/class/timecard/ocpN/cpld_status
+Date:		July 2026
+Contact:	Sagi Maimon <maimon.sagi@gmail.com>
+Description:	(RO, root only) The flags set in the status register of the
+		TAP CPLD, space separated, or an empty line when none is
+		set.  Only present on ADVA x1 TAP boards (PCI ID
+		0xad5a:0x0410).
+
+		==========  ================================================
+		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
+		==========  ================================================
+
+		For example "busy" while an operation runs, or "busy failed"
+		if a failure is latched while another one is in progress.
+
+		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 once by the driver, shortly after
+		probe, and reported from that cached value as the fixed
+		"cpld.id" version by devlink dev info.
+
 What:		/sys/class/timecard/ocpN/available_clock_sources
 Date:		September 2021
 Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
diff --git a/Documentation/networking/devlink/index.rst b/Documentation/networking/devlink/index.rst
index d4a83fdcff7f..1af780c811ee 100644
--- a/Documentation/networking/devlink/index.rst
+++ b/Documentation/networking/devlink/index.rst
@@ -99,6 +99,7 @@ parameters, info versions, and other features it supports.
    nfp
    octeontx2
    prestera
+   ptp_ocp
    qed
    sfc
    stmmac
diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
new file mode 100644
index 000000000000..3f997cd9f3a3
--- /dev/null
+++ b/Documentation/networking/devlink/ptp_ocp.rst
@@ -0,0 +1,35 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========================
+ptp_ocp devlink support
+========================
+
+This document describes the devlink features implemented by the ``ptp_ocp``
+device driver.
+
+Info versions
+=============
+
+The ``ptp_ocp`` driver reports the following versions
+
+.. list-table:: devlink info versions implemented
+   :widths: 5 5 90
+
+   * - Name
+     - Type
+     - Description
+   * - ``fw``
+     - running
+     - Version of the firmware running on the card.  Reported as ``loader``
+       instead when the card is running the firmware loader.
+   * - ``board.id``
+     - fixed
+     - Board identifier, read from the on-card EEPROM.
+   * - ``cpld.id``
+     - fixed
+     - Lattice device ID (IDCODE) of the TAP CPLD, formatted as ``0x%08x``.
+       Only present on ADVA TimeCard X1 boards, which are the only ones
+       carrying that CPLD.  Reading it claims the shared I2C bus and
+       reprograms the on-card mux, so the driver does that once from its
+       own worker and reports the cached value here; the version is
+       omitted until that read has succeeded.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0e04d92d1b09..113f31434617 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20479,6 +20479,7 @@ OPENCOMPUTE PTP CLOCK DRIVER
 M:	Vadim Fedorenko <vadim.fedorenko@linux.dev>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/networking/devlink/ptp_ocp.rst
 F:	drivers/ptp/ptp_ocp.c
 
 OPENCORES I2C BUS DRIVER
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index f4299878c27c..998ea9c6b381 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -24,6 +24,8 @@
 #include <linux/nvmem-consumer.h>
 #include <linux/crc16.h>
 #include <linux/dpll.h>
+#include <linux/unaligned.h>
+#include <linux/delay.h>
 
 #define PCI_DEVICE_ID_META_TIMECARD		0x0400
 
@@ -85,6 +87,8 @@ struct ptp_ocp_adva_info {
 	u8				signals_nr;
 	u8				freq_in_nr;
 	const struct ocp_attr_group	*attr_groups;
+	/* x1: TAP CPLD present */
+	bool				has_cpld;
 };
 
 #define OCP_CTRL_ENABLE		BIT(0)
@@ -163,7 +167,8 @@ struct gpio_reg {
 	u32	gpio1;
 	u32	__pad0;
 	u32	gpio2;
-	u32	__pad1;
+	/* adva_x1: I2C bus ownership register; reserved on other variants */
+	u32	i2c_bus_ctrl;
 };
 
 struct irig_master_reg {
@@ -416,6 +421,25 @@ struct ptp_ocp {
 	dpll_tracker tracker;
 	int signals_nr;
 	int freq_in_nr;
+	/* adva_x1 CPLD I2C (internal use only) */
+	/* serialises CPLD operations */
+	struct mutex		cpld_lock;
+	/* guards cpld_i2c_adap_nr against the bus notifier */
+	spinlock_t		cpld_adap_lock;
+	/* I2C adapter nr; -1 if absent.  Under cpld_adap_lock */
+	int			cpld_i2c_adap_nr;
+	/* claimed adapter; valid under cpld_lock */
+	struct i2c_adapter	*cpld_adap;
+	/* scratch, write half then read half; valid under cpld_lock */
+	u8			*cpld_buf;
+	/* Lattice device ID; 0 if unread */
+	u32			cpld_id;
+	/* one-shot ID read finished, successfully or not; under cpld_lock */
+	bool			cpld_id_tried;
+	/* failed ID read attempts so far; under cpld_lock */
+	unsigned int		cpld_id_attempts;
+	/* x1 TAP CPLD present */
+	bool			has_cpld;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -1273,6 +1297,7 @@ static struct ocp_resource ocp_adva_x1_resource[] = {
 			.signals_nr   = 4,
 			.freq_in_nr   = 4,
 			.attr_groups  = adva_timecard_x1_groups,
+			.has_cpld     = true,
 		},
 	},
 	{ }
@@ -2159,6 +2184,7 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 	const char *fw_image;
 	char buf[32];
 	int err;
+	u32 id;
 
 	fw_image = bp->fw_loader ? "loader" : "fw";
 	sprintf(buf, "%d.%d", bp->fw_tag, bp->fw_version);
@@ -2166,6 +2192,18 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 	if (err)
 		return err;
 
+	/* Read by the worker, not here: this command is unprivileged and
+	 * reading the ID claims the I2C bus.  0 means unread - skip it
+	 * rather than fail, devlink discards the whole reply on error.
+	 */
+	id = READ_ONCE(bp->cpld_id);
+	if (bp->has_cpld && id) {
+		sprintf(buf, "0x%08x", id);
+		err = devlink_info_version_fixed_put(req, "cpld.id", buf);
+		if (err)
+			return err;
+	}
+
 	if (!bp->has_eeprom_data) {
 		ptp_ocp_read_eeprom(bp);
 		if (!bp->has_eeprom_data)
@@ -3199,6 +3237,8 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 		return err;
 	ptp_ocp_sma_init(bp);
 
+	bp->has_cpld = info->has_cpld;
+
 	return ptp_ocp_init_clock(bp, &info->servo);
 }
 
@@ -4226,6 +4266,434 @@ static const struct ocp_attr_group art_timecard_groups[] = {
 	{ },
 };
 
+/*
+ * adva_x1 TAP CPLD (Lattice LCMXO3LF-2100) helpers.
+ *
+ * The card has two I2C controllers; Linux registers only 0x00150000.
+ * The i2c_bus_ctrl handshake re-routes what each is wired to:
+ *
+ *   grant 0:  0x150000 -> EEPROMs     0x120000 -> TMC or M.2, MB's choice
+ *   grant 1:  0x150000 -> TMC bus     0x120000 -> M.2
+ *
+ * The PCA9548 at 0x74 and the CPLD at 0x40 behind its channel 0 are on the
+ * TMC bus, reachable only while the grant is held.  The EEPROMs are not
+ * behind the adapter for that window, so a CPLD operation holds cpld_lock
+ * and the adapter lock to keep the EEPROM and nvmem paths off it.
+ */
+
+#define ADVA_MUX_ADDR     0x74
+#define ADVA_CPLD_ADDR    0x40
+#define ADVA_MUX_CHANNEL  0
+
+/* Longest command or reply; bounds each half of the per-claim scratch. */
+#define ADVA_CPLD_XFER_MAX 32
+
+/* i2c_bus_ctrl carries a host-written request half and a firmware-written
+ * grant half.  MBLAZE_GRANTED is what a granted register reads while our
+ * request still stands; the release write clears the request half, so from
+ * then on only the grant field can be tested.  Measured on an ADVA
+ * TimeCard X1: the register reads 0x00000012 once the firmware has handed
+ * the segment back, about 670 ms after the release write, so the low half
+ * is not zero then and only the grant field is a valid test.
+ */
+#define MBLAZE_REQUEST    0x0000aaaaU
+#define MBLAZE_GRANTED    0x5555aaaaU
+#define MBLAZE_RELEASE    0x55550000U
+#define MBLAZE_GRANT_MASK 0xffff0000U
+#define MBLAZE_GRANT      0x55550000U
+#define MBLAZE_RETRIES    200
+#define MBLAZE_RETRY_US   10000
+
+/* Lattice LCMXO3LF ISC command codes */
+#define CPLD_CMD_READ_ID      0xE0000000UL
+#define CPLD_CMD_READ_STATUS  0x3C000000UL
+#define CPLD_ID_MAX_ATTEMPTS  10     /* one per sync_work tick */
+
+/* Status register bit positions (Lattice LCMXO3LF datasheet) */
+#define CPLD_STATUS_DONE   BIT(8)
+#define CPLD_STATUS_BUSY   BIT(12)
+#define CPLD_STATUS_FAILED BIT(13)
+
+/*
+ * Issue one I2C transaction on the TMC bus: @cmd if not negative, then
+ * @wlen bytes of @wdata (NULL sends zeros), then @rlen bytes read back.
+ * The message is built in the scratch buffer from adva_x1_bus_claim(), so
+ * a run of transfers costs one allocation, not one each.  I2C_M_DMA_SAFE
+ * is not set: the two halves are not separately aligned, so let the core
+ * bounce.  Caller holds the claim, hence __i2c_transfer().
+ */
+static int adva_x1_i2c_xfer(struct ptp_ocp *bp, u8 addr, int cmd,
+			    const void *wdata, u8 wlen,
+			    void *rdata, u8 rlen)
+{
+	u8 *wbuf, *rbuf;
+	struct i2c_adapter *adap;
+	struct i2c_msg msgs[2];
+	int nmsgs = 0, ret;
+	u16 hdr = cmd >= 0;
+
+	lockdep_assert_held(&bp->cpld_lock);
+
+	adap = bp->cpld_adap;
+	if (!adap || !bp->cpld_buf)
+		return -ENODEV;
+
+	if (hdr + wlen > ADVA_CPLD_XFER_MAX || rlen > ADVA_CPLD_XFER_MAX)
+		return -EINVAL;
+
+	wbuf = bp->cpld_buf;
+	rbuf = bp->cpld_buf + ADVA_CPLD_XFER_MAX;
+
+	if (hdr + wlen) {
+		if (hdr)
+			wbuf[0] = cmd;
+		if (wdata)
+			memcpy(wbuf + hdr, wdata, wlen);
+		else
+			memset(wbuf + hdr, 0, wlen);
+		msgs[nmsgs++] = (struct i2c_msg){
+			.addr  = addr,
+			.flags = 0,
+			.len   = hdr + wlen,
+			.buf   = wbuf,
+		};
+	}
+	if (rlen) {
+		msgs[nmsgs++] = (struct i2c_msg){
+			.addr  = addr,
+			.flags = I2C_M_RD,
+			.len   = rlen,
+			.buf   = rbuf,
+		};
+	}
+
+	ret = __i2c_transfer(adap, msgs, nmsgs);
+	if (ret != nmsgs)
+		return (ret < 0) ? ret : -EIO;
+
+	if (rdata && rlen)
+		memcpy(rdata, rbuf, rlen);
+
+	return 0;
+}
+
+/* Hand the segment back, and wait for the firmware to drop the grant before
+ * the caller unlocks the adapter: a transfer queued behind us would
+ * otherwise be clocked out while the controller is still on the TMC bus,
+ * which is what holding the adapter lock across the grant exists to stop.
+ */
+static int adva_x1_mblaze_release(struct ptp_ocp *bp)
+{
+	u32 val = 0;
+	int i;
+
+	if (!bp->pps_select)
+		return 0;
+
+	iowrite32(MBLAZE_RELEASE, &bp->pps_select->i2c_bus_ctrl);
+	/* Flush the posted write, as the acquire path does. */
+	ioread32(&bp->pps_select->i2c_bus_ctrl);
+
+	for (i = 0; i < MBLAZE_RETRIES; i++) {
+		val = ioread32(&bp->pps_select->i2c_bus_ctrl);
+		if ((val & MBLAZE_GRANT_MASK) != MBLAZE_GRANT)
+			return 0;
+		usleep_range(MBLAZE_RETRY_US, MBLAZE_RETRY_US + 1000);
+	}
+
+	dev_err(&bp->pdev->dev,
+		"TMC bus still granted after release, i2c_bus_ctrl 0x%08x\n",
+		val);
+
+	return -ETIMEDOUT;
+}
+
+/* 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)
+{
+	u32 val;
+	int i;
+
+	if (!bp->pps_select)
+		return -ENODEV;
+
+	/* Drop a request left by a caller that died mid-sequence; cpld_lock
+	 * keeps live ones out.  The read back only flushes the posted write.
+	 */
+	iowrite32(0, &bp->pps_select->i2c_bus_ctrl);
+	ioread32(&bp->pps_select->i2c_bus_ctrl);
+
+	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;
+	}
+
+	/* 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;
+}
+
+/* Route the host controller back to the EEPROMs and release the adapter.
+ * Safe after a failed claim: it also clears a request that was never granted.
+ *
+ * The adapter is unlocked even when the hand-back timed out: keeping the
+ * root lock would stall every other user of the controller for good, with
+ * no way to recover it.  The error is returned instead, so the operation
+ * that held the claim fails rather than reporting success over a bus that
+ * may still be routed to the TMC segment.
+ */
+static int adva_x1_bus_release(struct ptp_ocp *bp)
+{
+	struct i2c_adapter *adap = bp->cpld_adap;
+	int err;
+
+	if (!adap)
+		return 0;
+
+	err = adva_x1_mblaze_release(bp);
+	bp->cpld_adap = NULL;
+	kfree(bp->cpld_buf);
+	bp->cpld_buf = NULL;
+	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+	i2c_put_adapter(adap);
+
+	return err;
+}
+
+static int adva_x1_i2c_adap_match(struct device *dev, const void *data)
+{
+	return !!i2c_verify_adapter(dev);
+}
+
+/*
+ * Cache the adapter the CPLD hangs off.  ptp_ocp_i2c_notifier_call() misses
+ * it when the i2c controller driver is already loaded: the adapter is then
+ * added from inside platform_device_register(), before bp->i2c_ctrl is set.
+ * Only direct children are searched, so a mux adapter cannot match.
+ *
+ * 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.
+ */
+static void adva_x1_cache_i2c_adap(struct ptp_ocp *bp)
+{
+	struct device *child;
+
+	if (!bp->has_cpld || !bp->i2c_ctrl ||
+	    READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)
+		return;
+
+	child = device_find_child(&bp->i2c_ctrl->dev, NULL,
+				  adva_x1_i2c_adap_match);
+	if (!child)
+		return;
+
+	/* Re-test under the lock so this does not overwrite a number the
+	 * notifier published meanwhile.  It cannot tell that apart from a
+	 * number the notifier has just invalidated, so an adapter removed in
+	 * this window can still be cached; adva_x1_bus_claim() rejects it on
+	 * the parent check and forgets it, and the next tick looks again.
+	 */
+	scoped_guard(spinlock, &bp->cpld_adap_lock) {
+		if (bp->cpld_i2c_adap_nr < 0)
+			bp->cpld_i2c_adap_nr = i2c_verify_adapter(child)->nr;
+	}
+
+	put_device(child);
+}
+
+/*
+ * 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.
+ */
+static int adva_x1_bus_claim(struct ptp_ocp *bp)
+{
+	struct i2c_adapter *adap;
+	int ret;
+
+	lockdep_assert_held(&bp->cpld_lock);
+
+	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));
+	if (!adap)
+		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
+	 * a number that is not: otherwise every later claim fails the same
+	 * 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;
+		}
+		i2c_put_adapter(adap);
+		return -ENODEV;
+	}
+
+	/* One scratch buffer per claim rather than per transfer: a claim can
+	 * cover many transfers.
+	 */
+	bp->cpld_buf = kzalloc(2 * ADVA_CPLD_XFER_MAX, GFP_KERNEL);
+	if (!bp->cpld_buf) {
+		i2c_put_adapter(adap);
+		return -ENOMEM;
+	}
+
+	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+	bp->cpld_adap = adap;
+
+	ret = adva_x1_mblaze_acquire(bp);
+	if (ret)
+		adva_x1_bus_release(bp);	/* keeps the acquire error */
+
+	return ret;
+}
+
+/* Select a mux channel, or deselect all with ch < 0 - the power-on state.
+ * The mux is on the TMC bus, so what it is left set to never affects the
+ * EEPROM paths.
+ */
+static int adva_x1_mux_select(struct ptp_ocp *bp, int ch)
+{
+	u8 val = (ch >= 0) ? BIT(ch) : 0;
+
+	return adva_x1_i2c_xfer(bp, ADVA_MUX_ADDR, val, NULL, 0, NULL, 0);
+}
+
+/*
+ * Send a 4-byte command and read back without an intermediate STOP: two
+ * messages in one transfer is the Lattice write -> repeated START -> read,
+ * so no protocol-mangling flag is needed.
+ */
+static int adva_x1_cpld_cmd_read(struct ptp_ocp *bp,
+				 u32 cmd_be, u8 *out, u8 out_len)
+{
+	__be32 cmd = cpu_to_be32(cmd_be);
+
+	return adva_x1_i2c_xfer(bp, ADVA_CPLD_ADDR, -1, &cmd, 4, out, out_len);
+}
+
+static int adva_x1_cpld_read_status(struct ptp_ocp *bp, u32 *status)
+{
+	u8 buf[4];
+	int ret;
+
+	ret = adva_x1_cpld_cmd_read(bp, CPLD_CMD_READ_STATUS, buf, 4);
+	if (ret)
+		return ret;
+	*status = get_unaligned_be32(buf);
+	return 0;
+}
+
+/*
+ * Read the Lattice device ID into bp->cpld_id.  Done once, off the
+ * unprivileged devlink path, which reports the cached value.  -EBUSY means
+ * cpld_lock is held, so the caller can retry rather than wait behind a long
+ * CPLD operation.
+ */
+static int adva_x1_cpld_read_id(struct ptp_ocp *bp)
+{
+	u8 data[4];
+	int err, ret;
+
+	if (!mutex_trylock(&bp->cpld_lock))
+		return -EBUSY;
+
+	ret = adva_x1_bus_claim(bp);
+	if (ret)
+		goto out;
+	/* A select reported as failed may still have been ACKed, so the
+	 * deselect runs either way rather than handing the mux back with a
+	 * channel possibly left open.
+	 */
+	ret = adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
+	if (ret)
+		goto deselect;
+	ret = adva_x1_cpld_cmd_read(bp, CPLD_CMD_READ_ID, data, 4);
+	if (!ret)
+		WRITE_ONCE(bp->cpld_id, get_unaligned_be32(data));
+deselect:
+	err = adva_x1_mux_select(bp, -1);
+	if (!ret)
+		ret = err;
+	err = adva_x1_bus_release(bp);
+	if (!ret)
+		ret = err;
+out:
+	/* Under cpld_lock, so an operation that resets this bookkeeping
+	 * while holding the lock cannot have it overwritten afterwards by a
+	 * worker that had already finished reading.
+	 */
+	if (!ret || ++bp->cpld_id_attempts >= CPLD_ID_MAX_ATTEMPTS)
+		bp->cpld_id_tried = true;
+	mutex_unlock(&bp->cpld_lock);
+	if (ret)
+		dev_dbg(&bp->pdev->dev,
+			"CPLD device ID unavailable: %d\n", ret);
+
+	return ret;
+}
+
+/* cpld_status - show the flags set in the TAP CPLD status register. */
+static ssize_t
+cpld_status_show(struct device *dev, struct device_attribute *attr,
+		 char *buf)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(dev);
+	int count = 0;
+	u32 st = 0;
+	int err, ret;
+
+	/* A CPLD operation can hold cpld_lock a long time; stay killable. */
+	ret = mutex_lock_interruptible(&bp->cpld_lock);
+	if (ret)
+		return ret;
+
+	ret = adva_x1_bus_claim(bp);
+	if (ret)
+		goto out;
+	ret = adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
+	if (ret)
+		goto deselect;
+	ret = adva_x1_cpld_read_status(bp, &st);
+deselect:
+	err = adva_x1_mux_select(bp, -1);
+	if (!ret)
+		ret = err;
+	err = adva_x1_bus_release(bp);
+	if (!ret)
+		ret = err;
+out:
+	mutex_unlock(&bp->cpld_lock);
+	if (ret)
+		return ret;
+
+	/* Flags: the names of the bits that are set, nothing when none is,
+	 * rather than a value per bit in one file.
+	 */
+	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 ? " " : "");
+
+	return count + sysfs_emit_at(buf, count, "\n");
+}
+static DEVICE_ATTR_ADMIN_RO(cpld_status);
+
 static struct attribute *adva_timecard_attrs[] = {
 	&dev_attr_serialnum.attr,
 	&dev_attr_gnss_sync.attr,
@@ -4274,6 +4742,7 @@ static struct attribute *adva_timecard_x1_attrs[] = {
 	&dev_attr_ts_window_adjust.attr,
 	&dev_attr_utc_tai_offset.attr,
 	&dev_attr_tod_correction.attr,
+	&dev_attr_cpld_status.attr,
 	NULL,
 };
 
@@ -4904,6 +5373,7 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 		clk_hw_unregister_fixed_rate(bp->i2c_clk);
 	if (bp->n_irqs)
 		pci_free_irq_vectors(bp->pdev);
+	mutex_destroy(&bp->cpld_lock);
 	device_unregister(&bp->dev);
 }
 
@@ -5052,6 +5522,17 @@ 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.
+	 */
+	adva_x1_cache_i2c_adap(bp);
+	if (bp->has_cpld && !READ_ONCE(bp->cpld_id_tried) &&
+	    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);
 }
 
@@ -5080,6 +5561,15 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (err)
 		goto out_disable;
 
+	/* Before the first error path that calls ptp_ocp_detach(), so
+	 * mutex_destroy() runs on an initialised mutex, and before
+	 * ptp_ocp_register_resources(), so this sentinel cannot overwrite an
+	 * adapter number the i2c notifier stores during registration.
+	 */
+	mutex_init(&bp->cpld_lock);
+	spin_lock_init(&bp->cpld_adap_lock);
+	bp->cpld_i2c_adap_nr = -1;
+
 	INIT_DELAYED_WORK(&bp->sync_work, ptp_ocp_sync_work);
 
 	/* compat mode.
@@ -5158,6 +5648,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 out_dpll_put:
 	dpll_device_put(bp->dpll, &bp->tracker);
 out_devlink:
+	/* detach() destroys cpld_lock; no devlink op may still reach it */
 	devlink_unregister(devlink);
 out:
 	ptp_ocp_detach(bp);
@@ -5204,6 +5695,7 @@ ptp_ocp_i2c_notifier_call(struct notifier_block *nb,
 			  unsigned long action, void *data)
 {
 	struct device *dev, *child = data;
+	struct i2c_adapter *adap;
 	struct ptp_ocp *bp;
 	bool add;
 
@@ -5227,11 +5719,25 @@ ptp_ocp_i2c_notifier_call(struct notifier_block *nb,
 
 found:
 	bp = dev_get_drvdata(dev);
-	if (add)
+	adap = i2c_verify_adapter(child);
+	if (add) {
 		ptp_ocp_symlink(bp, child, "i2c");
-	else
+		/* Only the card's own controller: a mux child added on it
+		 * reaches ptp_ocp through the same walk.  Numbers are
+		 * recycled, so the claim re-checks the parent itself.
+		 */
+		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;
+		}
+	} else {
+		/* invalidate before free, but only for the cached adapter */
+		scoped_guard(spinlock, &bp->cpld_adap_lock) {
+			if (bp->cpld_i2c_adap_nr == adap->nr)
+				bp->cpld_i2c_adap_nr = -1;
+		}
 		sysfs_remove_link(&bp->dev.kobj, "i2c");
-
+	}
 	return 0;
 }
 
-- 
2.47.0

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

[Sagi Maimon]

From: Sagi Maimon <maimon.sagi@gmail.com>

The Lattice MachXO3 CPLD on the ADVA TimeCard X1 is programmed over I2C
using in-system programming (ISP).  Build on the TMC bus arbitration added
previously and expose the update path as a devlink flash component, next
to the card flash the driver already writes through devlink.

The component is named fw.cpld and reported as a running version, so
->info_get() both publishes the USERCODE of the image currently in the
part and lets the core accept the name:

  devlink dev flash pci/0000:02:00.0 file adva-cpld.bin component fw.cpld

Flashing acquires the bus, erases the configuration flash, programs the
image page by page and activates it with the MachXO3 REFRESH command,
reporting progress with the standard devlink notifications.  The CPLD is
required to come back with DONE set, not busy and no error code before the
update is called successful, as drivers/fpga/machxo2-spi.c does.

The whole sequence runs under cpld_lock and the i2c adapter lock, so an
EEPROM read blocks for as long as programming takes; the alternative is
reading the TMC bus instead.  Flashing is privileged:
DEVLINK_CMD_FLASH_UPDATE carries GENL_ADMIN_PERM.

Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 Documentation/ABI/testing/sysfs-timecard     |  11 +-
 Documentation/networking/devlink/ptp_ocp.rst |  48 +-
 drivers/ptp/ptp_ocp.c                        | 441 ++++++++++++++++++-
 3 files changed, 479 insertions(+), 21 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index e58824e0cdcd..4ba45c6ee4a3 100644
--- a/Documentation/ABI/testing/sysfs-timecard
+++ b/Documentation/ABI/testing/sysfs-timecard
@@ -31,9 +31,14 @@ 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 once by the driver, shortly after
-		probe, 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 again after a successful CPLD update, and reported from
+		that cached value as the fixed "cpld.id" version by
+		devlink dev info.
+
+		New CPLD firmware is programmed with devlink dev flash,
+		selecting the "fw.cpld" component; see
+		Documentation/networking/devlink/ptp_ocp.rst.
 
 What:		/sys/class/timecard/ocpN/available_clock_sources
 Date:		September 2021
diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
index 3f997cd9f3a3..f94b759d9cd6 100644
--- a/Documentation/networking/devlink/ptp_ocp.rst
+++ b/Documentation/networking/devlink/ptp_ocp.rst
@@ -5,7 +5,7 @@ ptp_ocp devlink support
 ========================
 
 This document describes the devlink features implemented by the ``ptp_ocp``
-device driver.
+device driver: the info versions it reports and its flash update support.
 
 Info versions
 =============
@@ -30,6 +30,46 @@ The ``ptp_ocp`` driver reports the following versions
      - Lattice device ID (IDCODE) of the TAP CPLD, formatted as ``0x%08x``.
        Only present on ADVA TimeCard X1 boards, which are the only ones
        carrying that CPLD.  Reading it claims the shared I2C bus and
-       reprograms the on-card mux, so the driver does that once from its
-       own worker and reports the cached value here; the version is
-       omitted until that read has succeeded.
+       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.
+   * - ``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.
+
+Flash update
+============
+
+The driver implements ``devlink dev flash`` for two separate targets,
+selected with the component name.
+
+.. list-table:: Flash components
+   :widths: 20 80
+
+   * - Component
+     - Description
+   * - (none)
+     - The card's own flash, written through the SPI controller the driver
+       exposes.  The card runs the new image after its next reset.
+   * - ``fw.cpld``
+     - 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.
+
+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
+with the standard devlink status notifications.
+
+Example::
+
+    $ devlink dev flash pci/0000:02:00.0 file adva-cpld.bin component fw.cpld
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 998ea9c6b381..2802989e8494 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -20,12 +20,14 @@
 #include <linux/spi/altera.h>
 #include <net/devlink.h>
 #include <linux/i2c.h>
+#include <linux/iopoll.h>
 #include <linux/mtd/mtd.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/crc16.h>
 #include <linux/dpll.h>
 #include <linux/unaligned.h>
 #include <linux/delay.h>
+#include <linux/firmware.h>
 
 #define PCI_DEVICE_ID_META_TIMECARD		0x0400
 
@@ -434,12 +436,18 @@ struct ptp_ocp {
 	u8			*cpld_buf;
 	/* Lattice device ID; 0 if unread */
 	u32			cpld_id;
+	/* USERCODE of the image in the part; valid once cpld_usercode_ok */
+	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 */
 	bool			cpld_id_tried;
 	/* failed ID read attempts so far; under cpld_lock */
 	unsigned int		cpld_id_attempts;
 	/* x1 TAP CPLD present */
 	bool			has_cpld;
+	/* EN_CFG_TP issued but not yet REFRESH'd */
+	bool			cpld_in_config_mode;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -476,6 +484,13 @@ static int ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r);
 static const struct ocp_sma_op ocp_adva_sma_op;
 static const struct ocp_sma_op ocp_adva_x1_sma_op;
 
+/* Flash component naming the CPLD image, as reported by ->info_get(). */
+#define ADVA_CPLD_COMPONENT	"fw.cpld"
+
+static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
+			      const struct firmware *fw,
+			      struct netlink_ext_ack *extack);
+
 static const struct ocp_attr_group fb_timecard_groups[];
 
 static const struct ocp_attr_group art_timecard_groups[];
@@ -2158,6 +2173,21 @@ ptp_ocp_devlink_flash_update(struct devlink *devlink,
 	const char *msg;
 	int err;
 
+	if (params->component) {
+		if (!bp->has_cpld ||
+		    strcmp(params->component, ADVA_CPLD_COMPONENT)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "unsupported flash component");
+			return -EOPNOTSUPP;
+		}
+
+		err = adva_x1_cpld_flash(bp, devlink, params->fw, extack);
+		msg = err ? "Flash error" : "Flash complete";
+		devlink_flash_update_status_notify(devlink, msg,
+						   ADVA_CPLD_COMPONENT, 0, 0);
+		return err;
+	}
+
 	dev = ptp_ocp_find_flash(bp);
 	if (!dev) {
 		dev_err(&bp->pdev->dev, "Can't find Flash SPI adapter\n");
@@ -2180,6 +2210,8 @@ static int
 ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 			 struct netlink_ext_ack *extack)
 {
+	enum devlink_info_version_type ver_type =
+					DEVLINK_INFO_VERSION_TYPE_COMPONENT;
 	struct ptp_ocp *bp = devlink_priv(devlink);
 	const char *fw_image;
 	char buf[32];
@@ -2192,14 +2224,35 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 	if (err)
 		return err;
 
-	/* Read by the worker, not here: this command is unprivileged and
-	 * reading the ID claims the I2C bus.  0 means unread - skip it
-	 * rather than fail, devlink discards the whole reply on error.
-	 */
-	id = READ_ONCE(bp->cpld_id);
-	if (bp->has_cpld && id) {
-		sprintf(buf, "0x%08x", id);
-		err = devlink_info_version_fixed_put(req, "cpld.id", buf);
+	if (bp->has_cpld) {
+		/* Read by the worker, not here: this command is unprivileged
+		 * and reading the ID claims the I2C bus.  0 means unread -
+		 * skip it rather than fail, devlink discards the whole reply
+		 * on error.
+		 */
+		id = READ_ONCE(bp->cpld_id);
+		if (id) {
+			sprintf(buf, "0x%08x", id);
+			err = devlink_info_version_fixed_put(req, "cpld.id",
+							     buf);
+			if (err)
+				return err;
+		}
+
+		/* 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
+		 * 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.
+		 */
+		if (smp_load_acquire(&bp->cpld_usercode_ok))
+			sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
+		else
+			strscpy(buf, "unknown", sizeof(buf));
+		err = devlink_info_version_running_put_ext(req, "fw.cpld", buf,
+							   ver_type);
 		if (err)
 			return err;
 	}
@@ -4306,13 +4359,36 @@ static const struct ocp_attr_group art_timecard_groups[] = {
 
 /* Lattice LCMXO3LF ISC command codes */
 #define CPLD_CMD_READ_ID      0xE0000000UL
+/* The USERCODE lives in the configuration flash that ERASE wipes, and the
+ * page writes carry it: after programming an image whose JEDEC UH field is
+ * 0x00000004 the part reads back 0x00000004, not an erased value.  So there
+ * is no separate ISC_PROGRAM_USERCODE step here.
+ */
+#define CPLD_CMD_READ_USERCODE 0xC0000000UL
 #define CPLD_CMD_READ_STATUS  0x3C000000UL
+#define CPLD_CMD_EN_CFG_TP    0x74   /* enable config, transparent mode */
+#define CPLD_CMD_DIS_CFG      0x26
+#define CPLD_CMD_ERASE        0x0E
+#define CPLD_CMD_RESET_ADDR   0x46
+#define CPLD_CMD_WRITE_PAGE   0x70
+#define CPLD_CMD_SET_DONE     0x5E
+#define CPLD_CMD_REFRESH      0x79
+#define CPLD_PAGE_SIZE        16
+#define CPLD_POLL_US          10000  /* status poll interval while busy */
+/* 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)
+#define CPLD_EXIT_CFG_TRIES   3
+#define CPLD_ERASE_MS         15000  /* config sector erase, datasheet max */
 #define CPLD_ID_MAX_ATTEMPTS  10     /* one per sync_work tick */
 
 /* Status register bit positions (Lattice LCMXO3LF datasheet) */
 #define CPLD_STATUS_DONE   BIT(8)
+#define CPLD_STATUS_ENAB   BIT(9)
 #define CPLD_STATUS_BUSY   BIT(12)
 #define CPLD_STATUS_FAILED BIT(13)
+#define CPLD_STATUS_ERR    GENMASK(25, 23)
 
 /*
  * Issue one I2C transaction on the TMC bus: @cmd if not negative, then
@@ -4571,6 +4647,54 @@ static int adva_x1_mux_select(struct ptp_ocp *bp, int ch)
 	return adva_x1_i2c_xfer(bp, ADVA_MUX_ADDR, val, NULL, 0, NULL, 0);
 }
 
+/*
+ * Argument bytes that follow an ISC opcode.  Returns NULL with @nargs set
+ * when the arguments are all zero: adva_x1_i2c_xfer() zeroes the buffer.
+ *
+ * EN_CFG_TP, DIS_CFG and REFRESH take two operand bytes here where the SPI
+ * drivers in drivers/fpga send three.  These are the frames the part on this
+ * board answers: the sequence below has programmed and refreshed it
+ * successfully over I2C, so the counts are kept as validated rather than
+ * aligned with the SPI framing.
+ */
+static const u8 *adva_x1_cpld_args(u8 cmd, u8 *nargs)
+{
+	static const u8 en_cfg_tp[] = { 0x08, 0x00 };
+	/* cfg sector only */
+	static const u8 erase_cfg[] = { 0x04, 0x00, 0x00 };
+
+	switch (cmd) {
+	case CPLD_CMD_EN_CFG_TP:
+		*nargs = sizeof(en_cfg_tp);
+		return en_cfg_tp;
+	case CPLD_CMD_ERASE:
+		*nargs = sizeof(erase_cfg);
+		return erase_cfg;
+	case CPLD_CMD_RESET_ADDR:
+	case CPLD_CMD_SET_DONE:
+		*nargs = 3;
+		return NULL;
+	case CPLD_CMD_DIS_CFG:
+	case CPLD_CMD_REFRESH:
+		*nargs = 2;
+		return NULL;
+	default:
+		*nargs = 0;
+		return NULL;
+	}
+}
+
+/* Send an ISC command with the fixed arguments that belong to it. */
+static int adva_x1_cpld_write(struct ptp_ocp *bp, u8 cmd)
+{
+	const u8 *args;
+	u8 nargs;
+
+	args = adva_x1_cpld_args(cmd, &nargs);
+
+	return adva_x1_i2c_xfer(bp, ADVA_CPLD_ADDR, cmd, args, nargs, NULL, 0);
+}
+
 /*
  * Send a 4-byte command and read back without an intermediate STOP: two
  * messages in one transfer is the Lattice write -> repeated START -> read,
@@ -4596,11 +4720,86 @@ static int adva_x1_cpld_read_status(struct ptp_ocp *bp, u32 *status)
 	return 0;
 }
 
+/* Poll the status register until the CPLD goes idle, or @max_ms elapses.
+ * The deadline is on wall time, so the I2C transactions count against it,
+ * and the status is read once more after it expires before giving up.
+ */
+static int adva_x1_cpld_wait_ready(struct ptp_ocp *bp, unsigned int max_ms)
+{
+	u32 status = 0;
+	int err = 0, ret;
+
+	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;
+	/* Keep the transport errno so it reaches userspace as-is; -EIO is
+	 * reserved for the CPLD itself reporting FAILED.
+	 */
+	if (err)
+		return err;
+	if (status & CPLD_STATUS_FAILED)
+		return -EIO;
+
+	return 0;
+}
+
+/* Wait for BUSY to clear.  Unlike adva_x1_cpld_wait_ready() a latched FAILED
+ * status is not an error here: this is used on the exit path, where an ISC
+ * command issued while the part is still erasing or programming may not
+ * latch, so the operation has to be waited out whatever its outcome.
+ */
+static int adva_x1_cpld_wait_idle(struct ptp_ocp *bp, unsigned int max_ms)
+{
+	u32 status = 0;
+	int err = 0, ret;
+
+	ret = read_poll_timeout(adva_x1_cpld_read_status, err,
+				err || !(status & CPLD_STATUS_BUSY),
+				CPLD_POLL_US, max_ms * USEC_PER_MSEC, false,
+				bp, &status);
+	if (ret)
+		return ret;
+
+	return err;
+}
+
+/* Leave transparent configuration mode.
+ *
+ * A DIS_CFG issued while the part is still busy may not latch, so the wait
+ * has to succeed before the write is believed - an ACKed write after a
+ * timed-out wait says nothing about whether the part left the mode.  Retry
+ * a few times rather than leaving it latched on one bad attempt; there is
+ * no other caller that would try again.
+ */
+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);
+		if (!err) {
+			bp->cpld_in_config_mode = false;
+			return;
+		}
+	}
+
+	dev_err(&bp->pdev->dev, "CPLD left in configuration mode: %d\n", err);
+}
+
 /*
- * Read the Lattice device ID into bp->cpld_id.  Done once, off the
- * unprivileged devlink path, which reports the cached value.  -EBUSY means
- * cpld_lock is held, so the caller can retry rather than wait behind a long
- * CPLD operation.
+ * Read the Lattice device ID and the image USERCODE under one bus claim.
+ * Done once, off the unprivileged devlink path, which reports the cached
+ * values.  -EBUSY means cpld_lock is held, so the caller can retry rather
+ * than wait behind a long CPLD operation.
  */
 static int adva_x1_cpld_read_id(struct ptp_ocp *bp)
 {
@@ -4621,8 +4820,21 @@ static int adva_x1_cpld_read_id(struct ptp_ocp *bp)
 	if (ret)
 		goto deselect;
 	ret = adva_x1_cpld_cmd_read(bp, CPLD_CMD_READ_ID, data, 4);
-	if (!ret)
-		WRITE_ONCE(bp->cpld_id, get_unaligned_be32(data));
+	if (ret)
+		goto deselect;
+	/* Publish the ID before the USERCODE is attempted: a part that does
+	 * not answer 0xC0 must not cost us the device ID as well.
+	 */
+	WRITE_ONCE(bp->cpld_id, get_unaligned_be32(data));
+
+	ret = adva_x1_cpld_cmd_read(bp, CPLD_CMD_READ_USERCODE, data, 4);
+	if (ret)
+		goto deselect;
+	WRITE_ONCE(bp->cpld_usercode, get_unaligned_be32(data));
+	/* Pairs with the acquire in ptp_ocp_devlink_info_get(): the value has
+	 * to be visible before the flag that declares it valid.
+	 */
+	smp_store_release(&bp->cpld_usercode_ok, true);
 deselect:
 	err = adva_x1_mux_select(bp, -1);
 	if (!ret)
@@ -4694,6 +4906,207 @@ cpld_status_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_ADMIN_RO(cpld_status);
 
+/*
+ * Program the CPLD configuration flash from @fw and activate it.  Selected
+ * by the "fw.cpld" flash component; the SPI flash keeps the default path.
+ * Runs under cpld_lock with the i2c adapter lock held, so an EEPROM read
+ * blocks for as long as programming takes.
+ */
+static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
+			      const struct firmware *fw,
+			      struct netlink_ext_ack *extack)
+{
+	size_t offset;
+	int err, ret;
+	u32 st;
+
+	if (!fw->size || fw->size % CPLD_PAGE_SIZE) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "image must be a whole number of 16-byte pages");
+		return -EINVAL;
+	}
+	if (fw->size > CPLD_MAX_IMAGE_SZ) {
+		NL_SET_ERR_MSG_MOD(extack, "image too large for this part");
+		return -EINVAL;
+	}
+
+	mutex_lock(&bp->cpld_lock);
+
+	err = adva_x1_bus_claim(bp);
+	if (err)
+		goto unlock;
+	/* A select reported as failed may still have been ACKed, so hand the
+	 * mux back deselected either way.
+	 */
+	err = adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
+	if (err)
+		goto deselect;
+
+	/* A previous flash may have failed to leave configuration mode.  Now
+	 * that the bus is claimed again, retry the exit before re-entering;
+	 * a no-op when the flag is already clear.
+	 */
+	adva_x1_cpld_exit_config(bp);
+
+	/* Set before EN_CFG_TP, not after: the CPLD may have entered config
+	 * mode even if the write errors or the wait times out, and only this
+	 * 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;
+
+	err = adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP);
+	if (!err)
+		err = adva_x1_cpld_wait_ready(bp, 5000);
+	if (err)
+		goto exit_config;
+
+	/* Confirm the part really entered configuration mode.  An enable
+	 * frame that did not latch leaves an idle part reporting neither
+	 * BUSY nor FAILED, the erase and page writes are ignored, and DONE
+	 * is still set from the old image - so every later check passes and
+	 * the update would be reported successful with the flash untouched.
+	 */
+	err = adva_x1_cpld_read_status(bp, &st);
+	if (err)
+		goto exit_config;
+	if (!(st & CPLD_STATUS_ENAB)) {
+		dev_err(&bp->pdev->dev,
+			"CPLD did not enter configuration mode, status 0x%08x\n",
+			st);
+		NL_SET_ERR_MSG_MOD(extack,
+				   "CPLD did not enter configuration mode");
+		err = -EIO;
+		goto exit_config;
+	}
+
+	devlink_flash_update_status_notify(devlink, "Erasing",
+					   ADVA_CPLD_COMPONENT, 0, 0);
+	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;
+
+	/* 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;
+
+	err = adva_x1_cpld_write(bp, CPLD_CMD_RESET_ADDR);
+	if (err)
+		goto exit_config;
+
+	for (offset = 0; offset < fw->size; offset += CPLD_PAGE_SIZE) {
+		u8 args[3 + CPLD_PAGE_SIZE] = { 0x00, 0x00, 0x01 };
+
+		/* 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
+		 * left unconfigured, which is recoverable: fw.cpld stays
+		 * advertised so the image can be written again.
+		 */
+		if (fatal_signal_pending(current)) {
+			err = -EINTR;
+			goto exit_config;
+		}
+
+		memcpy(&args[3], fw->data + offset, CPLD_PAGE_SIZE);
+		err = adva_x1_i2c_xfer(bp, ADVA_CPLD_ADDR, CPLD_CMD_WRITE_PAGE,
+				       args, sizeof(args), NULL, 0);
+		if (!err)
+			err = adva_x1_cpld_wait_ready(bp, 100);
+		if (err)
+			goto exit_config;
+
+		if (!(offset % (CPLD_PAGE_SIZE * 64)))
+			devlink_flash_update_status_notify(devlink,
+							   "Programming",
+							   ADVA_CPLD_COMPONENT,
+							   offset, fw->size);
+	}
+	devlink_flash_update_status_notify(devlink, "Programming",
+					   ADVA_CPLD_COMPONENT,
+					   fw->size, fw->size);
+
+	err = adva_x1_cpld_write(bp, CPLD_CMD_SET_DONE);
+	if (!err)
+		err = adva_x1_cpld_wait_ready(bp, 1000);
+	if (err)
+		goto exit_config;
+
+	err = adva_x1_cpld_read_status(bp, &st);
+	if (err)
+		goto exit_config;
+	if (!(st & CPLD_STATUS_DONE)) {
+		dev_err(&bp->pdev->dev,
+			"CPLD SET_DONE left status 0x%08x\n", st);
+		NL_SET_ERR_MSG_MOD(extack, "CPLD did not accept the image");
+		err = -EIO;
+		goto exit_config;
+	}
+
+	devlink_flash_update_status_notify(devlink, "Activating",
+					   ADVA_CPLD_COMPONENT, 0, 0);
+	err = adva_x1_cpld_write(bp, CPLD_CMD_REFRESH);
+	if (err)
+		goto exit_config;
+
+	/* REFRESH reboots the CPLD out of configuration mode, so the exit
+	 * path must not send DIS_CFG afterwards even if a check below fails.
+	 */
+	bp->cpld_in_config_mode = false;
+
+	/* The new image is already running, so a segment that is not back
+	 * yet must not be reported as a failed update: retry the reselect
+	 * rather than sampling the mux once at a fixed delay.
+	 */
+	msleep(1500);
+	err = read_poll_timeout(adva_x1_mux_select, err, !err, CPLD_POLL_US,
+				3000 * USEC_PER_MSEC, false,
+				bp, ADVA_MUX_CHANNEL);
+	if (err)
+		goto deselect;
+
+	err = adva_x1_cpld_wait_ready(bp, 3000);
+	if (err)
+		goto deselect;
+
+	/* 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.
+	 */
+	err = adva_x1_cpld_read_status(bp, &st);
+	if (err)
+		goto deselect;
+	if (!(st & CPLD_STATUS_DONE) || (st & CPLD_STATUS_BUSY) ||
+	    (st & CPLD_STATUS_ERR)) {
+		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");
+		err = -EIO;
+		goto deselect;
+	}
+
+exit_config:
+	adva_x1_cpld_exit_config(bp);
+deselect:
+	ret = adva_x1_mux_select(bp, -1);
+	if (!err)
+		err = ret;
+	ret = adva_x1_bus_release(bp);
+	if (!err)
+		err = ret;
+unlock:
+	mutex_unlock(&bp->cpld_lock);
+
+	return err;
+}
+
 static struct attribute *adva_timecard_attrs[] = {
 	&dev_attr_serialnum.attr,
 	&dev_attr_gnss_sync.attr,
-- 
2.47.0

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

[netdev-bot+sashiko]

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

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

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_bus_release() drops the i2c root-adapter lock and
  forgets the claim even when adva_x1_mblaze_release() returned…
- [Medium] The one-shot CPLD ID read is executed from
  ptp_ocp_sync_work(), the driver's 1 Hz in-sync status poller.
- [Low] On an acquire timeout the MicroBlaze hand-back handshake is
  executed twice: adva_x1_mblaze_acquire() already writes MBLAZE_RELEASE…
- [Low] adva_x1_bus_claim() returns -ENODEV when i2c_get_adapter() finds
  nothing for the cached number, without clearing bp->cpld_i2c_adap_nr.
- [Low] The new struct ptp_ocp member comments overstate the locking:
  cpld_i2c_adap_nr is documented as 'Under cpld_adap_lock' and…

--

ptp: ocp: add TAP CPLD access for ADVA TimeCard X1

This adds a request/grant handshake that re-routes the registered x1 I2C
controller from the config EEPROMs onto the TMC bus, so the PCA9548 and the
Lattice CPLD behind its channel 0 can be reached.  On top of that it adds a
root-only cpld_status attribute and reports the Lattice device ID as the
fixed devlink version cpld.id, read once from the driver's sync worker.

A few questions about the new arbitration and its bookkeeping below.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index f4299878c27ce..998ea9c6b3817 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -416,6 +421,25 @@ struct ptp_ocp {
>  	dpll_tracker tracker;
>  	int signals_nr;
>  	int freq_in_nr;
> +	/* adva_x1 CPLD I2C (internal use only) */
> +	/* serialises CPLD operations */
> +	struct mutex		cpld_lock;
> +	/* guards cpld_i2c_adap_nr against the bus notifier */
> +	spinlock_t		cpld_adap_lock;
> +	/* I2C adapter nr; -1 if absent.  Under cpld_adap_lock */
> +	int			cpld_i2c_adap_nr;

[Severity: Low]
This isn't a bug, but do these member comments match the code?  The field
is documented as "Under cpld_adap_lock", yet all three readers take no
lock:

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

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

	ptp_ocp_sync_work()
		if (bp->has_cpld && !READ_ONCE(bp->cpld_id_tried) &&
		    READ_ONCE(bp->cpld_i2c_adap_nr) >= 0)

The same applies to cpld_id_tried, documented as "under cpld_lock" and read
lockless in ptp_ocp_sync_work().  Would it be clearer to describe the
actual design, that writes are serialised while readers may observe a stale
value and re-validate it on the parent check?

> +	/* claimed adapter; valid under cpld_lock */
> +	struct i2c_adapter	*cpld_adap;
> +	/* scratch, write half then read half; valid under cpld_lock */
> +	u8			*cpld_buf;
> +	/* Lattice device ID; 0 if unread */
> +	u32			cpld_id;
> +	/* one-shot ID read finished, successfully or not; under cpld_lock */
> +	bool			cpld_id_tried;
> +	/* failed ID read attempts so far; under cpld_lock */
> +	unsigned int		cpld_id_attempts;
> +	/* x1 TAP CPLD present */
> +	bool			has_cpld;
>  };

[ ... ]

> @@ -4226,6 +4266,434 @@ static const struct ocp_attr_group art_timecard_groups[] = {

[ ... ]

> +/* Hand the segment back, and wait for the firmware to drop the grant before
> + * the caller unlocks the adapter: a transfer queued behind us would
> + * otherwise be clocked out while the controller is still on the TMC bus,
> + * which is what holding the adapter lock across the grant exists to stop.
> + */
> +static int adva_x1_mblaze_release(struct ptp_ocp *bp)
> +{
> +	u32 val = 0;
> +	int i;
> +
> +	if (!bp->pps_select)
> +		return 0;
> +
> +	iowrite32(MBLAZE_RELEASE, &bp->pps_select->i2c_bus_ctrl);
> +	/* Flush the posted write, as the acquire path does. */
> +	ioread32(&bp->pps_select->i2c_bus_ctrl);
> +
> +	for (i = 0; i < MBLAZE_RETRIES; i++) {
> +		val = ioread32(&bp->pps_select->i2c_bus_ctrl);
> +		if ((val & MBLAZE_GRANT_MASK) != MBLAZE_GRANT)
> +			return 0;
> +		usleep_range(MBLAZE_RETRY_US, MBLAZE_RETRY_US + 1000);
> +	}
> +
> +	dev_err(&bp->pdev->dev,
> +		"TMC bus still granted after release, i2c_bus_ctrl 0x%08x\n",
> +		val);
> +
> +	return -ETIMEDOUT;
> +}
> +
> +/* 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)
> +{
> +	u32 val;
> +	int i;
> +
> +	if (!bp->pps_select)
> +		return -ENODEV;
> +
> +	/* Drop a request left by a caller that died mid-sequence; cpld_lock
> +	 * keeps live ones out.  The read back only flushes the posted write.
> +	 */
> +	iowrite32(0, &bp->pps_select->i2c_bus_ctrl);
> +	ioread32(&bp->pps_select->i2c_bus_ctrl);
> +
> +	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;
> +	}
> +
> +	/* 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]
Is the hand-back run twice for a single failed acquire?  This path already
writes MBLAZE_RELEASE and runs the full poll loop, and then
adva_x1_bus_claim() calls it again for the same error:

	ret = adva_x1_mblaze_acquire(bp);
	if (ret)
		adva_x1_bus_release(bp);	/* keeps the acquire error */

Both run with the i2c root-adapter lock held, and each loop is
MBLAZE_RETRIES x MBLAZE_RETRY_US, so when the firmware keeps the grant
asserted a single failed claim can hold the shared adapter lock for roughly
half again as long and log "TMC bus still granted after release" twice.
Is one of the two hand-backs superfluous?

> +
> +/* Route the host controller back to the EEPROMs and release the adapter.
> + * Safe after a failed claim: it also clears a request that was never granted.
> + *
> + * The adapter is unlocked even when the hand-back timed out: keeping the
> + * root lock would stall every other user of the controller for good, with
> + * no way to recover it.  The error is returned instead, so the operation
> + * that held the claim fails rather than reporting success over a bus that
> + * may still be routed to the TMC segment.
> + */
> +static int adva_x1_bus_release(struct ptp_ocp *bp)
> +{
> +	struct i2c_adapter *adap = bp->cpld_adap;
> +	int err;
> +
> +	if (!adap)
> +		return 0;
> +
> +	err = adva_x1_mblaze_release(bp);
> +	bp->cpld_adap = NULL;
> +	kfree(bp->cpld_buf);
> +	bp->cpld_buf = NULL;
> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +	i2c_put_adapter(adap);
> +
> +	return err;
> +}

[Severity: High]
Can this leave the shared controller routed to the TMC segment with nothing
holding later users off it?

When adva_x1_mblaze_release() returns -ETIMEDOUT the grant field still
reads MBLAZE_GRANT, so the firmware has not taken the segment back.  This
function nevertheless drops the claim and unlocks unconditionally.  For a
plain (non-mux) adapter both lock levels are the same rt_mutex:

drivers/i2c/i2c-core-base.c
	static void i2c_adapter_unlock_bus(struct i2c_adapter *adapter,
					   unsigned int flags)
	{
		rt_mutex_unlock(&adapter->bus_lock);
	}

so every waiter resumes immediately, and only the errno reaches the CPLD
operation that held the claim.  Nothing latches the "routing unknown"
state, re-drives the hand-back, or gates the next transfer.

The commit message says a transfer issued in that window "would be issued
onto the TMC bus instead of to the EEPROM".  ptp_ocp_read_eeprom() then
caches whatever came back with no validation:

		ret = nvmem_device_read(nvmem, map->off, map->len,
					BP_MAP_ENTRY_ADDR(bp, map));
		if (ret != map->len)
			goto fail;
	}

	bp->has_eeprom_data = true;

and that becomes bp->serial / bp->board_id, republished over the
unprivileged DEVLINK_CMD_INFO_GET path.  An nvmem or at24 write in the same
window would be delivered to whatever answers 0x50/0x58 on the TMC bus.

The comment argues that keeping the lock forever is worse, which is fair,
but should a sticky flag (or a retried hand-back) fence later transfers
until the grant is confirmed dropped, instead of only returning the errno
to the CPLD caller?

[ ... ]

> +/*
> + * 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.
> + */
> +static int adva_x1_bus_claim(struct ptp_ocp *bp)
> +{
> +	struct i2c_adapter *adap;
> +	int ret;
> +
> +	lockdep_assert_held(&bp->cpld_lock);
> +
> +	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));
> +	if (!adap)
> +		return -ENODEV;

[Severity: Low]
Should this path also forget the cached number?  When the lookup fails,
bp->cpld_i2c_adap_nr stays >= 0, so adva_x1_cache_i2c_adap() bails out at
its first test and never re-resolves the adapter, while every
ptp_ocp_sync_work() tick burns one retry until cpld_id_tried is set for
good in adva_x1_cpld_read_id():

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

A stale number can be published in the first place because
adva_x1_cache_i2c_adap() only re-tests cpld_i2c_adap_nr < 0 after
device_find_child() has returned, so a delete notification for that adapter
in the same window still ends up cached.

And a re-added adapter updates the number but resets neither flag:

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

so cpld.id is then absent for the lifetime of the binding.  The comment
above the parent check below states that a bad number is rejected "on the
parent check and forgets it, and the next tick looks again" - does that hold
when i2c_get_adapter() returns NULL rather than a mismatched adapter?

> +
> +	/* The number is freed before the notifier clears it, so it can
> +	 * already be another adapter's.  Check this is still ours, and drop
> +	 * a number that is not: otherwise every later claim fails the same
> +	 * 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;
> +		}
> +		i2c_put_adapter(adap);
> +		return -ENODEV;
> +	}

[ ... ]

> @@ -5052,6 +5522,17 @@ 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.
> +	 */
> +	adva_x1_cache_i2c_adap(bp);
> +	if (bp->has_cpld && !READ_ONCE(bp->cpld_id_tried) &&
> +	    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);
>  }

[Severity: Medium]
Is the 1 Hz sync poller the right place for a claim that can block for
seconds?  adva_x1_mblaze_acquire() sleeps MBLAZE_RETRIES x MBLAZE_RETRY_US
(200 x 10 ms), and on the timeout path the hand-back poll loop runs too, so
one failed attempt can occupy this work item for several seconds, repeated
once per tick until CPLD_ID_MAX_ATTEMPTS is reached.

Does that delay the bp->sync update and the dpll change notification by the
same amount?  ptp_ocp_remove() and .shutdown start with
cancel_delayed_work_sync(&bp->sync_work), so unbind and reboot would block
for the duration as well.

Once cpld_id_attempts reaches 10, cpld_id_tried stays set for the lifetime
of the binding and cpld.id is silently absent, with both new interfaces
read-only and no way to retrigger the read.  Would it be worth mentioning
these effects in the commit message, or moving the read to its own work
item?

[ ... ]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 6 · Low: 3

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] ptp_ocp_devlink_info_get() publishes the literal string
  "unknown" as the devlink *running version* value for the fw.cpld…
- [Medium] adva_x1_cpld_flash() invalidates the cached CPLD identity
  (WRITE_ONCE(bp->cpld_id, 0), WRITE_ONCE(bp->cpld_usercode_ok, false),…
- [Medium] In adva_x1_cpld_flash() the first status wait after EN_CFG_TP
  uses adva_x1_cpld_wait_ready() (drivers/ptp/ptp_ocp.c:4958-4962),…
- [Medium] adva_x1_cpld_flash() invalidates the cached CPLD identity only
  after *both* the ERASE write and its completion wait succeed…
- [Medium] adva_x1_cpld_flash() holds bp->cpld_lock (mutex_lock() at
  drivers/ptp/ptp_ocp.c:4933) and the I2C root adapter lock (taken by…
- [Medium] The driver clears its configuration-mode bookkeeping from an
  I2C ACK alone, and its post-REFRESH success predicate cannot detect a…
- [Low] Documentation/networking/devlink/ptp_ocp.rst states for fw.cpld:
  "The only check the driver makes is that its length is a non-zero…
- [Low] The periodic progress notification in adva_x1_cpld_flash()'s page
  loop is emitted *after* the page at fw->data + offset has been written…
- [Low] adva_x1_cpld_flash() resets the ID-read bookkeeping with plain
  stores (`bp->cpld_id_tried = false; bp->cpld_id_attempts = 0;` at…

--

ptp: ocp: add TAP CPLD flashing via devlink

This adds a second devlink flash target to ptp_ocp: the "fw.cpld"
component programs the Lattice MachXO3 configuration flash on ADVA
TimeCard X1 boards over I2C and activates it with a REFRESH.  ->info_get()
now also reports the CPLD USERCODE as a running version, which is what
lets the core accept the component name.

A few questions about the new code and the documentation added with it.

> diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
> index e58824e0cdcdf..4ba45c6ee4a36 100644
> --- a/Documentation/ABI/testing/sysfs-timecard
> +++ b/Documentation/ABI/testing/sysfs-timecard
> @@ -31,9 +31,14 @@ 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 once by the driver, shortly after
> -		probe, 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 again after a successful CPLD update, and reported from
> +		that cached value as the fixed "cpld.id" version by
> +		devlink dev info.

[Severity: Medium]
Does the code match "again after a successful CPLD update"?

adva_x1_cpld_flash() drops the cached identity as soon as the erase wait
returns, on every attempt that gets that far:

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

Any later failure - a page write error, the new -EINTR abort, SET_DONE or
REFRESH failing - still returns an error, but cpld.id is already gone from
devlink dev info.

Recovery then depends on adva_x1_cpld_read_id() succeeding within
CPLD_ID_MAX_ATTEMPTS sync_work ticks, after which cpld_id_tried latches
and the worker stops trying.

Since the Lattice IDCODE cannot change when the configuration flash is
erased, should cpld.id survive a failed update, or should the documentation
say the identity is dropped on any attempt that reaches the erase?

> +
> +		New CPLD firmware is programmed with devlink dev flash,
> +		selecting the "fw.cpld" component; see
> +		Documentation/networking/devlink/ptp_ocp.rst.
>  
>  What:		/sys/class/timecard/ocpN/available_clock_sources
>  Date:		September 2021
> diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
> index 3f997cd9f3a3f..f94b759d9cd65 100644
> --- a/Documentation/networking/devlink/ptp_ocp.rst
> +++ b/Documentation/networking/devlink/ptp_ocp.rst

[ ... ]

> @@ -30,6 +30,46 @@ The ``ptp_ocp`` driver reports the following versions
>       - Lattice device ID (IDCODE) of the TAP CPLD, formatted as ``0x%08x``.
>         Only present on ADVA TimeCard X1 boards, which are the only ones
>         carrying that CPLD.  Reading it claims the shared I2C bus and
> -       reprograms the on-card mux, so the driver does that once from its
> -       own worker and reports the cached value here; the version is
> -       omitted until that read has succeeded.
> +       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.
> +   * - ``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.

[ ... ]

> +   * - ``fw.cpld``
> +     - 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.

[Severity: Low]
Is "the only check the driver makes" accurate?  adva_x1_cpld_flash() also
rejects large images:

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

with CPLD_MAX_IMAGE_SZ being 256 KiB.

Should the failure semantics be documented here as well?  After the erase
the configuration flash is blank, so any mid-sequence failure - including
the new fatal_signal_pending() abort, whose own comment says "The part is
left unconfigured" - leaves the CPLD unconfigured until a valid image is
written again.

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

[ ... ]

> @@ -2192,14 +2224,35 @@ 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
> +		 * 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.
> +		 */
> +		if (smp_load_acquire(&bp->cpld_usercode_ok))
> +			sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
> +		else
> +			strscpy(buf, "unknown", sizeof(buf));
> +		err = devlink_info_version_running_put_ext(req, "fw.cpld", buf,
> +							   ver_type);

[Severity: Medium]
Should the string "unknown" be published as a devlink running version
value?

The changelog says ->info_get() "publishes the USERCODE of the image
currently in the part", and the .rst entry added by this patch says
fw.cpld is the "USERCODE ... formatted as ``0x%08x``.  Read together with
``cpld.id`` and reported the same way" - and cpld.id is skipped entirely
while unread.

The window is not narrow: every devlink dev info between probe and the
first successful worker read, and permanently after a flash that erased
but did not finish, since adva_x1_cpld_flash() clears cpld_usercode_ok and
the worker gives up after CPLD_ID_MAX_ATTEMPTS.

Does naming the component actually need a value?  In
devlink_info_version_put() the callback runs before the empty-value
early-out:

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 passing "" would still register the name for
devlink_flash_component_get() while emitting no version attribute, which
is the behaviour the .rst describes.

[ ... ]

> @@ -4596,11 +4720,86 @@ static int adva_x1_cpld_read_status(struct ptp_ocp *bp, u32 *status)
>  	return 0;
>  }
>  
> +/* Poll the status register until the CPLD goes idle, or @max_ms elapses.
> + * The deadline is on wall time, so the I2C transactions count against it,
> + * and the status is read once more after it expires before giving up.
> + */
> +static int adva_x1_cpld_wait_ready(struct ptp_ocp *bp, unsigned int max_ms)
> +{
> +	u32 status = 0;
> +	int err = 0, ret;
> +
> +	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;
> +	/* Keep the transport errno so it reaches userspace as-is; -EIO is
> +	 * reserved for the CPLD itself reporting FAILED.
> +	 */
> +	if (err)
> +		return err;
> +	if (status & CPLD_STATUS_FAILED)
> +		return -EIO;
> +
> +	return 0;
> +}

[ ... ]

> +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);
> +		if (!err) {
> +			bp->cpld_in_config_mode = false;
> +			return;
> +		}
> +	}
> +
> +	dev_err(&bp->pdev->dev, "CPLD left in configuration mode: %d\n", err);
> +}

[ ... ]

> @@ -4694,6 +4906,207 @@ cpld_status_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_ADMIN_RO(cpld_status);
>  
> +/*
> + * Program the CPLD configuration flash from @fw and activate it.  Selected
> + * by the "fw.cpld" flash component; the SPI flash keeps the default path.
> + * Runs under cpld_lock with the i2c adapter lock held, so an EEPROM read
> + * blocks for as long as programming takes.
> + */
> +static int adva_x1_cpld_flash(struct ptp_ocp *bp, struct devlink *devlink,
> +			      const struct firmware *fw,
> +			      struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	mutex_lock(&bp->cpld_lock);
> +
> +	err = adva_x1_bus_claim(bp);
> +	if (err)
> +		goto unlock;

[Severity: Medium]
Can this trip the hung-task detector for other users of the adapter?

adva_x1_bus_claim() takes the root adapter lock with
i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER), which is an uninterruptible,
non-killable acquisition for everyone else, and it is held here together
with cpld_lock for the whole cycle:

  EN_CFG wait			5 s
  ERASE wait			CPLD_ERASE_MS (15 s)
  page loop			100 ms per 16-byte page, up to
				CPLD_MAX_IMAGE_SZ / 16 = 16384 pages
  SET_DONE wait			1 s
  msleep(1500) + mux poll	1.5 s + 3 s
  post-REFRESH wait		3 s
  failure path			CPLD_EXIT_CFG_TRIES * CPLD_ERASE_MS (45 s)

Meanwhile the at24/nvmem EEPROM read reached from ptp_ocp_read_eeprom()
(also called from the unprivileged ptp_ocp_devlink_info_get() path) and any
other client of that adapter waits in i2c_lock_bus() and cannot be killed.

The changelog does say "an EEPROM read blocks for as long as programming
takes", but with the worst case above that exceeds the 120 s hung-task
threshold.  Would releasing the bus between phases, or bounding the total
hold, be preferable to the size cap whose comment claims it "Bounds how
long a claim can hold the i2c root lock"?

The new fatal_signal_pending() check only helps the flashing task, not the
waiters - is that the intent?

[ ... ]

> +	err = adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP);
> +	if (!err)
> +		err = adva_x1_cpld_wait_ready(bp, 5000);
> +	if (err)
> +		goto exit_config;

[Severity: Medium]
Is adva_x1_cpld_wait_ready() the right wait for the enable step?

It turns a set CPLD_STATUS_FAILED into -EIO, and the driver's own comment
above adva_x1_cpld_wait_idle() says FAILED is latched across operations
("an ISC command issued while the part is still erasing or programming may
not latch, so the operation has to be waited out whatever its outcome").

Nothing in the driver clears it: the failure path
adva_x1_cpld_exit_config() only sends DIS_CFG.

So if FAILED survives a DIS_CFG plus a fresh EN_CFG_TP, does every later
devlink dev flash ... component fw.cpld return -EIO here, before reaching
the ERASE/REFRESH that would put the part back into a defined state?  That
would contradict the comment in the page loop saying the state "is
recoverable: fw.cpld stays advertised so the image can be written again".

For comparison, machxo2_write_init() in drivers/fpga/machxo2-spi.c tests
FAIL only after ISC_ENABLE and ISC_ERASE, and machxo2_cleanup() recovers
with ISC_ERASE followed by LSC_REFRESH.

[ ... ]

> +	devlink_flash_update_status_notify(devlink, "Erasing",
> +					   ADVA_CPLD_COMPONENT, 0, 0);
> +	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;

[Severity: Medium]
Does this leave stale identity published when the erase is under way but
the wait fails?

If adva_x1_cpld_write(CPLD_CMD_ERASE) is ACKed, the erase is running in
hardware.  If adva_x1_cpld_wait_ready() then returns -ETIMEDOUT, -EIO for
a latched FAILED, or a transport errno, this goto skips the invalidation
below entirely.

bp->cpld_id keeps its old value, cpld_usercode_ok stays true, and
cpld_id_tried stays true from the probe-time read, so the
!READ_ONCE(bp->cpld_id_tried) gate in ptp_ocp_sync_work() never re-reads.
ptp_ocp_devlink_info_get() then keeps publishing cpld.id and an fw.cpld
USERCODE for an image that is no longer in the part, for the rest of the
binding.

Would invalidating before issuing the ERASE (or right after the write
returns 0) match the comment below, "stop reporting its identity even if
the rest of the sequence fails"?

> +
> +	/* 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;

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

ptp_ocp_sync_work() reads the flag without cpld_lock:

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

so the writer holds cpld_lock and the reader holds nothing, which is a
plain-store versus marked-load pair that KCSAN would report.

The comment here says "Written under cpld_lock, which
adva_x1_cpld_read_id() also holds across its own bookkeeping, so the worker
cannot resurrect any of it", and the struct field comment says
"under cpld_lock", but neither covers the worker's unlocked gate.  The
effect looks benign, since the racing read_id() attempt just gets -EBUSY
from mutex_trylock().

[ ... ]

> +		if (!err)
> +			err = adva_x1_cpld_wait_ready(bp, 100);
> +		if (err)
> +			goto exit_config;
> +
> +		if (!(offset % (CPLD_PAGE_SIZE * 64)))
> +			devlink_flash_update_status_notify(devlink,
> +							   "Programming",
> +							   ADVA_CPLD_COMPONENT,
> +							   offset, fw->size);

[Severity: Low]
Should this report offset + CPLD_PAGE_SIZE?

The page at fw->data + offset has already been written and waited for at
this point, so the first notification says 0 bytes done and each periodic
one is a page behind.  Moving the notify above the page write would work
too.

[ ... ]

> +	devlink_flash_update_status_notify(devlink, "Activating",
> +					   ADVA_CPLD_COMPONENT, 0, 0);
> +	err = adva_x1_cpld_write(bp, CPLD_CMD_REFRESH);
> +	if (err)
> +		goto exit_config;
> +
> +	/* REFRESH reboots the CPLD out of configuration mode, so the exit
> +	 * path must not send DIS_CFG afterwards even if a check below fails.
> +	 */
> +	bp->cpld_in_config_mode = false;

[Severity: Medium]
Can an ACKed but unlatched REFRESH be reported as a successful update?

The I2C ACK alone clears cpld_in_config_mode here, and the final check
after the reselect only looks at:

	if (!(st & CPLD_STATUS_DONE) || (st & CPLD_STATUS_BUSY) ||
	    (st & CPLD_STATUS_ERR)) {

Those three conditions are already satisfied by the state left by SET_DONE,
so if the REFRESH frame does not latch, the part stays in configuration
mode running the old image and devlink dev flash still returns 0 - while
the .rst says "the new image runs immediately".

CPLD_STATUS_ENAB is defined by this patch and tested after EN_CFG_TP, but
never after REFRESH.  Would checking ENAB here (and retrying REFRESH, as
machxo2_write_complete() does up to MACHXO2_MAX_REFRESH_LOOP times) be
worthwhile?

The same question applies to adva_x1_cpld_exit_config(), which clears
cpld_in_config_mode as soon as the DIS_CFG write is ACKed without reading
ENAB back.  Since the recovery call at the start of adva_x1_cpld_flash() is
gated on that flag, does one false-positive exit make a part stuck in
configuration mode unreachable by that recovery?

> +
> +	/* The new image is already running, so a segment that is not back
> +	 * yet must not be reported as a failed update: retry the reselect
> +	 * rather than sampling the mux once at a fixed delay.
> +	 */
> +	msleep(1500);

[ ... ]

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