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

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The `fw.cpld` flash component is only reachable while the
  cached CPLD identity is valid.
- [Medium] Documentation/networking/devlink/ptp_ocp.rst states for
  `fw.cpld` that "The image is the raw configuration bitstream, a whole…
- [Medium] `bp->cpld_id` / `bp->cpld_usercode` are invalidated only on
  the fully successful path of `adva_x1_cpld_flash()`…
- [Medium] In `adva_x1_cpld_read_id()` the newly added USERCODE
  transaction failing throws away the device ID that was already read…
- [Medium] `adva_x1_cpld_exit_config()` (drivers/ptp/ptp_ocp.c:4688-4703)
  discards the return value of `adva_x1_cpld_wait_idle(bp,…
- [Medium] `adva_x1_cpld_flash()` invalidates the cached CPLD identity
  (`WRITE_ONCE(bp->cpld_id, 0); WRITE_ONCE(bp->cpld_id_tried, false);…
- [Medium] The ISP sequence in `adva_x1_cpld_flash()` programs the
  configuration flash (EN_CFG_TP 0x74, ERASE 0x0E with operand 0x04 =…
- [Low] Inconsistent devlink error/status reporting between the two
  components of the same `->flash_update()` implementation.
- [Low] The comment on `adva_x1_cpld_wait_idle()`
  (drivers/ptp/ptp_ocp.c:4665) says it waits "for BUSY to clear,…
- [Low] `adva_x1_cpld_read_id()` publishes `cpld_usercode` and then
  `cpld_id` with plain `WRITE_ONCE()` and documents the dependency…

--

ptp: ocp: add TAP CPLD flashing via devlink

This adds a devlink flash component named fw.cpld that programs the Lattice
MachXO3 CPLD on ADVA TimeCard X1 boards over I2C using the in-system
programming commands, then activates the image with REFRESH.  The same
patch starts reporting the image USERCODE as a running version from
->info_get(), which is also what makes the component name acceptable to the
devlink core.  A few questions below about the component gating, the cached
identity and the configuration-mode exit path.

> diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
> index 3f997cd9f3a3..37e9f1ebd1a8 100644
> --- a/Documentation/networking/devlink/ptp_ocp.rst
> +++ b/Documentation/networking/devlink/ptp_ocp.rst

[ ... ]

> @@ -33,3 +33,38 @@ The ``ptp_ocp`` driver reports the following versions
> +   * - ``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, a whole number of
> +       16-byte pages; container formats such as ``.jed`` are rejected.

[Severity: Medium]
Where are container formats such as .jed rejected?  The only validation in
adva_x1_cpld_flash() is a length check:

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

There is no format or container detection anywhere in the function, so a
.jed or .bit file whose byte length happens to be a multiple of 16 passes,
the configuration sector is erased by CPLD_CMD_ERASE and the container bytes
are programmed as configuration frames.  Should the documentation be
narrowed to describe only the length check, or should the code inspect the
image format?

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 98cb26077181..58b90b2b282f 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -2158,6 +2169,17 @@ 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;
> +		}
> +
> +		return adva_x1_cpld_flash(bp, devlink, params->fw, extack);
> +	}
> +
>  	dev = ptp_ocp_find_flash(bp);
>  	if (!dev) {
>  		dev_err(&bp->pdev->dev, "Can't find Flash SPI adapter\n");

[ ... ]

> @@ -2202,6 +2226,16 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>  		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.
> +		 */
> +		sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
> +		err = devlink_info_version_running_put_ext(req, "fw.cpld", buf,
> +							   ver_type);
> +		if (err)
> +			return err;
>  	}

[Severity: Medium]
Is the fw.cpld component reachable when the cached CPLD identity is not
valid?  The version is only emitted inside:

	id = READ_ONCE(bp->cpld_id);
	if (bp->has_cpld && id) {

and the core requires the name to have been reported:

net/devlink/dev.c:devlink_flash_component_get() {
	...
	if (!lookup_ctx.lookup_name_found) {
		NL_SET_ERR_MSG_ATTR(extack, nla_component,
				    "selected component is not supported by this device");
		return -EINVAL;
	}
	...
}

The handler above accepts the component on the weaker condition
bp->has_cpld alone, so the two sides disagree and the -EOPNOTSUPP branch
cannot be reached for a name the core already rejected.

Two states the driver creates itself leave cpld_id at 0:

  - adva_x1_cpld_flash() ends its success path with
    WRITE_ONCE(bp->cpld_id, 0), so a flash issued before ptp_ocp_sync_work()
    manages the next read gets -EINVAL from the core.

  - ptp_ocp_sync_work() abandons the one-shot read after
    CPLD_ID_MAX_ATTEMPTS non-EBUSY failures:

	if (err != -EBUSY &&
	    (!err || ++bp->cpld_id_attempts >= CPLD_ID_MAX_ATTEMPTS))
		WRITE_ONCE(bp->cpld_id_tried, true);

    after which fw.cpld is never advertised again for the life of the
    binding, and the documented flash command is unavailable even though
    adva_x1_cpld_flash() would run.

Should the component be advertised independently of the cached identity, so
that recovering from a bad image does not depend on a successful ID read?

[ ... ]

> @@ -4522,6 +4568,48 @@ static int adva_x1_mux_select(struct ptp_ocp *bp, int ch)
> +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;
> +	}
> +}

[Severity: Medium]
Where does the sequence program the USERCODE?  This switch enumerates every
ISC opcode the driver can emit and there is no ISC_PROGRAM_USERCODE (0xC2)
entry, and adva_x1_cpld_flash() never writes a usercode either.

The USERCODE field lives in the configuration flash that CPLD_CMD_ERASE with
operand { 0x04, 0x00, 0x00 } wipes, and it has its own read opcode
(CPLD_CMD_READ_USERCODE, 0xC0).  If the page writes issued with
CPLD_CMD_WRITE_PAGE do not carry it, then the value read back by
adva_x1_cpld_read_id() after a successful flash is the erased value rather
than the USERCODE of the image just programmed.

That is the value the patch publishes as the fw.cpld running version, and
the commit message describes it as "the USERCODE of the image currently in
the part".  Can you confirm whether the incremental program frames restore
the usercode field on this part?

> @@ -4547,14 +4635,82 @@ static int adva_x1_cpld_read_status(struct ptp_ocp *bp, u32 *status)

[ ... ]

> +/* Wait for BUSY to clear, ignoring a pending cancel.  An ISC command issued
> + * while the part is still erasing or programming may not latch, so an abort
> + * has to wait the operation out before it can leave configuration mode.
> + */
> +static int adva_x1_cpld_wait_idle(struct ptp_ocp *bp, unsigned int max_ms)

[Severity: Low]
This isn't a bug, but what is the "pending cancel" the comment refers to?
There is no cancel, abort or signal check anywhere in the CPLD code; a grep
for cancel, signal_pending and fatal_signal in this file finds only this
comment, an unrelated flash status string and cancel_delayed_work_sync() in
ptp_ocp_remove().

adva_x1_cpld_wait_ready() is equally uninterruptible; the only actual
difference is that it also treats CPLD_STATUS_FAILED as an error, which the
comment does not mention.  Could the comment state that difference instead?

> +/* Leave transparent configuration mode.  cpld_in_config_mode stays set when
> + * DIS_CFG does not get through, so a later cleanup() tries again rather than
> + * recording an exit that never happened.
> + */
> +static void adva_x1_cpld_exit_config(struct ptp_ocp *bp)
> +{
> +	int err;
> +
> +	if (!bp->cpld_in_config_mode)
> +		return;
> +
> +	adva_x1_cpld_wait_idle(bp, CPLD_ERASE_MS);
> +
> +	err = adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG);
> +	if (err)
> +		dev_warn(&bp->pdev->dev,
> +			 "CPLD left in configuration mode: %d\n", err);
> +	else
> +		bp->cpld_in_config_mode = false;
> +}

[Severity: Medium]
Which "later cleanup()" retries the exit?  bp->cpld_in_config_mode is read
only by this function, and its only caller is adva_x1_cpld_flash().
ptp_ocp_remove()/ptp_ocp_detach(), cpld_status_show() and
adva_x1_cpld_read_id() never look at it, so when DIS_CFG returns an error
the flag stays set and nothing ever sends it again - the part can stay
latched in transparent configuration mode across module unload.

The discarded return value looks related: if
adva_x1_cpld_wait_idle(bp, CPLD_ERASE_MS) timed out with the part still
BUSY, then by this function's own reasoning the DIS_CFG opcode probably did
not latch, yet an ACKed write still clears cpld_in_config_mode.  Should the
wait_idle() result gate the flag update, and should the teardown path
attempt the release?

>  /*
> - * Read the Lattice device ID into bp->cpld_id.  Done once, off the
[ ... ]
>  static int adva_x1_cpld_read_id(struct ptp_ocp *bp)
>  {
> +	u32 id, usercode;
>  	u8 data[4];
>  	int ret;
>  
> @@ -4568,8 +4724,19 @@ static int adva_x1_cpld_read_id(struct ptp_ocp *bp)
>  	if (ret)
>  		goto release;
>  	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;
> +	id = get_unaligned_be32(data);
> +
> +	ret = adva_x1_cpld_cmd_read(bp, CPLD_CMD_READ_USERCODE, data, 4);
> +	if (ret)
> +		goto deselect;
> +	usercode = get_unaligned_be32(data);

[Severity: Medium]
Does a failing USERCODE read throw away the device ID that was just read
successfully?  Before this patch a successful READ_ID alone set
bp->cpld_id; now the goto skips both stores, so an I2C NAK or a part that
does not answer the 0xC0 opcode leaves cpld_id at 0.

Since the read is a bounded one-shot, ptp_ocp_sync_work() latches
cpld_id_tried after CPLD_ID_MAX_ATTEMPTS failures and never retries, so a
persistent USERCODE failure permanently removes the pre-existing cpld.id
version as well - and with it the fw.cpld entry that gates flashing, as both
come out of the same if (bp->has_cpld && id) block.

Would publishing the ID before attempting the USERCODE read keep the older
behaviour intact?

> +
> +	/* cpld_id is the validity flag for both, so publish it last. */
> +	WRITE_ONCE(bp->cpld_usercode, usercode);
> +	WRITE_ONCE(bp->cpld_id, id);
> +deselect:

[Severity: Low]
Do these accessors give the ordering the comment relies on?  WRITE_ONCE()
and READ_ONCE() only prevent compiler tearing, they add no store-store or
load-load ordering, and the reader's control dependency on id does not order
the following load:

	id = READ_ONCE(bp->cpld_id);
	if (bp->has_cpld && id) {
		...
		sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));

On a weakly ordered machine can ptp_ocp_devlink_info_get() observe a valid
cpld_id together with a stale or zero cpld_usercode and report fw.cpld as
0x00000000?  smp_store_release()/smp_load_acquire() would express the
publish pattern the comment describes.

> @@ -4627,6 +4794,154 @@ cpld_status_show(struct device *dev, struct device_attribute *attr,

[ ... ]

> +	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)) {
> +		err = -EIO;
> +		goto exit_config;
> +	}

[Severity: Low]
Why does this DONE check report differently from the identical one after
REFRESH?  Here userspace gets a bare -EIO with extack untouched and nothing
in the log, while the post-refresh check sets both:

	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");

Related: the CPLD path emits "Erasing"/"Programming"/"Activating" but never a
terminal notification, whereas the SPI path in
ptp_ocp_devlink_flash_update() finishes with:

	msg = err ? "Flash error" : "Flash complete";
	devlink_flash_update_status_notify(devlink, msg, NULL, 0, 0);

Should the CPLD component report a terminal status too, given the new
documentation says progress "is reported with the standard devlink status
notifications"?

[ ... ]

> +	/* The image on the part changed, so the cached USERCODE has too. */
> +	WRITE_ONCE(bp->cpld_id, 0);
> +	WRITE_ONCE(bp->cpld_id_tried, false);
> +	bp->cpld_id_attempts = 0;
> +
> +exit_config:
> +	adva_x1_cpld_exit_config(bp);
> +deselect:
> +	adva_x1_mux_select(bp, -1);
> +release:
> +	adva_x1_bus_release(bp);
> +unlock:
> +	mutex_unlock(&bp->cpld_lock);

[Severity: Medium]
Should the cache also be invalidated on the failure paths?  This block is
reached only when every post-refresh check passed.  Every failure taken after
CPLD_CMD_ERASE completed - a page write error, SET_DONE failure, DONE not
set, the REFRESH write failing, the mux reselect timing out, or the
post-refresh status check - jumps to exit_config or deselect and skips it.

The boot-time read has already latched cpld_id_tried, and
ptp_ocp_sync_work() gates the re-read on that flag:

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

so ptp_ocp_devlink_info_get() keeps publishing cpld.id and the fw.cpld
running version of an image that has been erased or only partially
rewritten, for the life of the binding.  Would invalidating immediately
after the erase is issued be more accurate than after final verification?

[Severity: Medium]
Can ptp_ocp_sync_work() clobber this invalidation?  These three fields are
written here under cpld_lock, but the worker writes the same fields with no
lock held - adva_x1_cpld_read_id() takes and drops cpld_lock internally, so
the bookkeeping that follows it runs unprotected:

ptp_ocp_sync_work()
	err = adva_x1_cpld_read_id(bp);	/* takes and releases cpld_lock */
	if (err != -EBUSY &&
	    (!err || ++bp->cpld_id_attempts >= CPLD_ID_MAX_ATTEMPTS))
		WRITE_ONCE(bp->cpld_id_tried, true);

Interleaving:

worker		adva_x1_cpld_read_id() succeeds, cpld_lock released,
		preempted before WRITE_ONCE(cpld_id_tried, true)

flash		mutex_lock(&bp->cpld_lock), programs for tens of seconds,
		WRITE_ONCE(bp->cpld_id, 0)
		WRITE_ONCE(bp->cpld_id_tried, false)
		bp->cpld_id_attempts = 0
		mutex_unlock(&bp->cpld_lock)

worker		WRITE_ONCE(bp->cpld_id_tried, true)

That leaves cpld_id == 0 with tried latched, so the worker never re-reads,
info_get() permanently omits cpld.id and fw.cpld, and further
devlink dev flash ... component fw.cpld requests are rejected by
devlink_flash_component_get() until the driver is rebound.  The plain
bp->cpld_id_attempts = 0 store here also races with the worker's
++bp->cpld_id_attempts.  Would moving the tried/attempts bookkeeping inside
adva_x1_cpld_read_id(), under cpld_lock, close this?

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