[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@xxxxxxxxx>
---
 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