Re: [PATCH v10] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ptp: ocp: add CPLD ISP support for ADVA TimeCard X1

This adds Lattice MachXO3 CPLD in-system-programming support for the ADVA
TimeCard X1 over the card's I2C bus, arbitrated with the on-card MicroBlaze
firmware.  Two new read-only sysfs attributes (cpld_device_id, cpld_status)
are added along with firmware-upload callbacks for reflashing the CPLD.

A few questions about the new code below.

First, on the commit message:

    The CPLD is connected to a secondary I2C bus shared with the onboard
    MicroBlaze soft CPU.

    All CPLD operations are serialized and coordinated with the MicroBlaze
    firmware to ensure exclusive access to the shared I2C bus.

[Severity: Low]
Which bus is the "secondary" one here?  ocp_adva_x1_resource[] registers
only one I2C controller (OCP_I2C_RESOURCE(i2c_ctrl), xiic-i2c at
0x00150000), and that same adapter carries the on-card EEPROMs:

	{ I2C_BOARD_INFO("24c02", 0x50) },
	{ I2C_BOARD_INFO("24mac402", 0x58), .platform_data = "mac" },

adva_x1_i2c_xfer() then uses bp->cpld_i2c_adap_nr, which the notifier fills
in from that single adapter.

If it is the same adapter, is the "exclusive access" claim complete?  The
pre-existing EEPROM traffic (serialnum_show() -> ptp_ocp_read_eeprom(), and
the nvmem read paths) does not acquire MicroBlaze ownership, and it is not
aware that the CPLD paths leave the PCA9548 with all channels deselected
via adva_x1_mux_select(bp, -1).

> diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
> index 3ae41b7634ac9..f7c9955acb0a9 100644
> --- a/Documentation/ABI/testing/sysfs-timecard
> +++ b/Documentation/ABI/testing/sysfs-timecard
> @@ -11,6 +11,40 @@ Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
>  Description:	This directory contains the attributes of the Nth timecard
>  		registered.
>  
> +What:		/sys/class/timecard/ocpN/cpld_device_id
> +Date:		July 2026
> +Contact:	Sagi Maimon <maimon.sagi@gmail.com>
> +Description:	(RO) The 32-bit Lattice device ID of the TAP CPLD, reported as
> +		a hex string, e.g. "0xe12bc043".
> +
> +		Only present on ADVA x1 TAP boards (PCI ID 0xad5a:0x0410).
> +		The Lattice LCMXO3LF-210 reports 0xe12bc043.

[Severity: Low]
Which of the two documented IDCODEs is the right one?  This file says the
LCMXO3LF-210 reports 0xe12bc043 (twice, including the example output),
while the comment above cpld_device_id_show() in the same patch says:

	 * Returns the 32-bit ID as a hex string, e.g. "0x612bc043\n".
	 * Lattice LCMXO3LF-210 reports 0x612BC043.

Userspace that validates the attribute against the documented constant can
only match one of them.  The leading 0xe looks like it came from the
CPLD_CMD_READ_ID opcode 0xE0000000.

[ ... ]

> +		The driver acquires the MicroBlaze I2C bus internally; no bus
> +		arbitration is required from userspace.
> +
> +		To program new CPLD firmware use the standard kernel
> +		firmware-upload interface registered at:
> +		/sys/class/firmware/adva-cpld/
> +

[Severity: Medium]
Is this path documented unconditionally on purpose?  PTP_1588_CLOCK_OCP in
drivers/ptp/Kconfig selects NET_DEVLINK, CRC16 and DPLL, but not FW_UPLOAD,
and this patch does not touch any Kconfig file.

With CONFIG_FW_UPLOAD=n, firmware_upload_register() is the inline stub in
include/linux/firmware.h that returns ERR_PTR(-EINVAL), so
ptp_ocp_adva_board_init() only prints the dev_warn() and
/sys/class/firmware/adva-cpld/ never appears, while the upload callbacks
become dead code.  Should PTP_1588_CLOCK_OCP select FW_UPLOAD?

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 35e911f1ad78c..cb259413042b8 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -416,6 +421,12 @@ struct ptp_ocp {
>  	dpll_tracker tracker;
>  	int signals_nr;
>  	int freq_in_nr;
> +	/* adva_x1 CPLD I2C (internal use only) */
> +	struct mutex		cpld_lock;            /* serialises CPLD operations */
> +	int			cpld_i2c_adap_nr;     /* I2C adapter nr; -1 if absent */
> +	struct fw_upload	*cpld_fw_upload;      /* firmware upload handle; NULL if absent */
> +	bool			cpld_cancel;          /* cancellation requested */
> +	bool			cpld_in_config_mode;  /* EN_CFG_TP issued but not yet REFRESH'd */
>  };

[ ... ]

> @@ -3197,6 +3211,20 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
>  		return err;
>  	ptp_ocp_sma_init(bp);
>  
> +	if (info->has_cpld) {
> +		struct fw_upload *fwl;
> +
> +		fwl = firmware_upload_register(THIS_MODULE, &bp->pdev->dev,
> +					       "adva-cpld",
> +					       &adva_cpld_upload_ops, bp);
> +		if (IS_ERR(fwl))
> +			dev_warn(&bp->pdev->dev,
> +				 "CPLD firmware upload unavailable: %pe\n",
> +				 fwl);
> +		else
> +			bp->cpld_fw_upload = fwl;
> +	}
> +

[Severity: Medium]
What happens with two X1 cards in the same host?  The name passed here is
the fixed string "adva-cpld", and firmware_upload_register() ->
fw_create_instance() does:

	dev_set_name(f_dev, "%s", fw_name);
	f_dev->parent = device;
	f_dev->class = &firmware_class;

so /sys/class/firmware/adva-cpld can only be created once and the second
device_add() should fail with -EEXIST.  Since the failure is only a
dev_warn() here, the second card loses the documented update path silently.
Every other interface of this driver is per-card (ocpN); would an
instance-qualified name be better here?

>  	return ptp_ocp_init_clock(bp, &info->servo);
>  }
>  
> @@ -4224,6 +4252,442 @@ static const struct ocp_attr_group art_timecard_groups[] = {
>  	{ },
>  };
>  

[ ... ]

> +/*
> + * adva_x1_i2c_xfer() - issue a single I2C transaction on the CPLD bus.
> + *
> + * All buffers are heap-allocated internally to guarantee DMA safety for
> + * the Xilinx I2C controller.  Caller must hold bp->cpld_lock.
> + */
> +static int adva_x1_i2c_xfer(struct ptp_ocp *bp,
> +			    u8 addr, const void *wdata, u8 wlen,
> +			    void *rdata, u8 rlen, bool nostart)
> +{

[Severity: Low]
This isn't a bug, but the "Caller must hold bp->cpld_lock" contract is not
expressed to the tools.  adva_cpld_prepare() returns with the mutex held
and adva_cpld_cleanup() unlocks a mutex it never took, and since
mutex_lock()/mutex_unlock() carry __acquires()/__releases() annotations,
sparse reports a context imbalance for both.  Would
__acquires(&bp->cpld_lock)/__releases(&bp->cpld_lock) plus
lockdep_assert_held(&bp->cpld_lock) in adva_x1_i2c_xfer(),
adva_cpld_write() and adva_cpld_poll_complete() make the contract
enforceable?

[ ... ]

> +	if (rlen) {
> +		rbuf = kzalloc(rlen, GFP_KERNEL);
> +		if (!rbuf) {
> +			ret = -ENOMEM;
> +			goto put;
> +		}
> +		msgs[nmsgs++] = (struct i2c_msg){
> +			.addr  = addr,
> +			.flags = I2C_M_RD | I2C_M_DMA_SAFE |
> +				 (nostart ? I2C_M_NOSTART : 0),
> +			.len   = rlen,
> +			.buf   = rbuf,
> +		};
> +	}

[Severity: Low]
Is I2C_M_NOSTART the flag that was intended here?  The only caller that
passes nostart=true is adva_x1_cpld_cmd_read(), whose comment says:

	 * Send a 4-byte command then read data back without an intermediate STOP
	 * (Lattice combined write→repeated-START→read).

I2C_M_NOSTART asks the adapter to skip the (re)START and to not resend the
slave address with the R/W bit, which is the opposite of a repeated-START
read, and it requires the adapter to advertise I2C_FUNC_NOSTART.  The
xiic-i2c controller this driver instantiates advertises only:

	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;

and never looks at I2C_M_NOSTART, and the core does not reject unsupported
protocol-mangling flags, so the flag is silently dropped and the transfer
becomes the plain write + repeated-START read that the ISC protocol wants.
On an adapter that does honour the flag (i2c-algo-bit, i2c-gpio, tegra,
brcmstb, pxa, bcm-kona) the read would return bus garbage as an ISC status
word, and i2c-bcm-iproc rejects it outright.  Two messages with no
NOSTART flag express the intended sequence; alternatively, should this
check i2c_check_functionality(adap, I2C_FUNC_NOSTART)?

[ ... ]

> +/* Acquire the shared I2C bus from the MicroBlaze firmware. */
> +static int adva_x1_mblaze_acquire(struct ptp_ocp *bp)
> +{
> +	u32 val;
> +	int i;
> +
> +	if (!bp->pps_select)
> +		return -ENODEV;
> +
> +	/* Release any stale grant left by a previous crashed caller. */
> +	iowrite32(0, &bp->pps_select->i2c_bus_ctrl);
> +	val = ioread32(&bp->pps_select->i2c_bus_ctrl);
> +	if (val != 0)
> +		return -EBUSY;

[Severity: Medium]
Can this -EBUSY check ever mean anything?  The register is overwritten with
0 before it is read, so the read only observes the value this function just
wrote, and a non-zero result means the MicroBlaze wrote something in
between rather than "bus busy".

Two consequences worth confirming:

The comment says the write clears a stale grant from a crashed caller, but
the code cannot distinguish that from a grant the MicroBlaze is currently
relying on, since it never inspects the register first.

Back-to-back reads of cpld_status/cpld_device_id can spuriously return
-EBUSY if the MicroBlaze has not yet consumed the MBLAZE_RELEASE written
by adva_x1_mblaze_release(), which is fire-and-forget with no
acknowledgement.

bp->cpld_lock only serialises the host side, so what provides exclusion
against the MicroBlaze in this window?

> +
> +	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;
> +	}
> +	return -ETIMEDOUT;
> +}

[Severity: Medium]
Should the pending MBLAZE_REQUEST be withdrawn on this timeout path?  The
request stays in i2c_bus_ctrl when -ETIMEDOUT is returned, and none of the
three callers releases it:

cpld_device_id_show() and cpld_status_show():

	ret = adva_x1_mblaze_acquire(bp);
	if (ret)
		goto out;		/* skips the release: label */

adva_cpld_prepare():

	if (adva_x1_mblaze_acquire(bp)) {
		ret = FW_UPLOAD_ERR_TIMEOUT;
		goto err_unlock;	/* skips err_release */
	}

So adva_x1_mblaze_release() is never called and the MicroBlaze still sees a
host request after the host gave up.  If it later grants it, it believes the
host owns the segment while nothing on the host side will ever send
MBLAZE_RELEASE.  The only recovery is the unconditional iowrite32(0) at the
top of some later acquire, which writes 0 rather than MBLAZE_RELEASE and
depends on another CPLD operation being requested at all.

[ ... ]

> +static int adva_x1_cpld_wait_ready(struct ptp_ocp *bp, unsigned int max_ms)
> +{
> +	unsigned int elapsed = 0;
> +	u32 status;
> +
> +	while (elapsed < max_ms) {
> +		if (adva_x1_cpld_read_status(bp, &status))
> +			return -EIO;
> +		if (status & CPLD_STATUS_FAILED)
> +			return -EIO;
> +		if (!(status & CPLD_STATUS_BUSY))
> +			return 0;
> +		usleep_range(100000, 101000);
> +		elapsed += 100;
> +	}
> +	return -ETIMEDOUT;
> +}

[Severity: High]
Does this loop poll one time too few?  The sleep and the elapsed update are
the last statements of the iteration, so the status is never re-read after
the final sleep and the loop performs floor(max_ms / 100) reads.

With the 100 ms budget used per page in adva_cpld_write() that means
exactly one read:

	elapsed = 0
	adva_x1_cpld_read_status()	-> BUSY set
	usleep_range(100000, 101000)
	elapsed = 100
	while (elapsed < 100)		-> false
	return -ETIMEDOUT;

adva_cpld_write() turns that into FW_UPLOAD_ERR_HW_ERROR and
fw_upload_main() aborts the upload, but adva_cpld_prepare() has already run
CPLD_CMD_ERASE, so the configuration flash is left erased and only
partially programmed with DONE clear.  A page program that legitimately
asserts BUSY for tPROG at that single poll would fail on the very first
page.

The same off-by-one shortens the 5000/15000/1000/3000 ms budgets by one
poll, and elapsed does not account for the time spent in the I2C status
transaction itself.  Should the status be re-checked after the last sleep?

[Jakub] Please use standard iopoll.h helpers or gate the timeout on a
jiffies-based deadline, not just loop count. And check before return
like the AI suggests..

> +
> +/*
> + * cpld_device_id - show the Lattice device ID of the TAP CPLD.
> + *
> + * Returns the 32-bit ID as a hex string, e.g. "0x612bc043\n".
> + * Lattice LCMXO3LF-210 reports 0x612BC043.
> + */
> +static ssize_t
> +cpld_device_id_show(struct device *dev, struct device_attribute *attr,
> +		    char *buf)
> +{
> +	struct ptp_ocp *bp = dev_get_drvdata(dev);
> +	u8 data[4];
> +	u32 id = 0;
> +	int ret;
> +
> +	mutex_lock(&bp->cpld_lock);

[Severity: Medium]
Can this block uninterruptibly for the whole duration of an upload?
bp->cpld_lock is taken in adva_cpld_prepare() and only dropped in
adva_cpld_cleanup(), covering the 15000 ms erase wait, one page write plus
wait per 16 bytes of image, SET_DONE (1000 ms), msleep(1500) and the
3000 ms post-REFRESH wait.

Both show() functions use plain mutex_lock(), so a "cat
/sys/class/timecard/ocpN/cpld_status" sits in TASK_UNINTERRUPTIBLE and is
unkillable for that whole time, which can also trip the hung-task detector.
kernfs drains in-flight attribute readers, so that stuck reader also stalls
ptp_ocp_attr_group_del() and device removal.  Would
mutex_lock_interruptible(), or a trylock returning -EBUSY while an upload
is in progress, be preferable here?

[Jakub] you seem to be doing multiple things in one patch, please make this
a patch series and separate the FW flashing form the rest. 

[ ... ]

> +static DEVICE_ATTR_RO(cpld_device_id);

[Severity: Medium]
Should these two attributes be world readable?  DEVICE_ATTR_RO() gives mode
0444 for both cpld_device_id and cpld_status, and they are added
unconditionally to adva_timecard_x1_attrs[].  A read is not a passive
register access: it takes bp->cpld_lock, overwrites the shared
host/MicroBlaze arbitration register with iowrite32(0, ...) before checking
ownership, sleeps up to MBLAZE_RETRIES * ~10 ms (about 2.2 s) inside
adva_x1_mblaze_acquire() while holding the mutex, and reprograms the shared
PCA9548 (select channel 0, then deselect all channels).

So any local user can loop on cpld_status and repeatedly clobber the
arbitration state the card firmware relies on, change mux state other I2C
clients on the same adapter depend on, and block uninterruptibly for the
length of a root-initiated upload.  Would 0400/0440 be more appropriate?

[Jakub] why is this not reported in devlink info? If it's the SKU/device id
of the CPLD it should be a "fixed" version?
I'll stop reviewing now, rest of the AI comments sent without checking...

[ ... ]

> +static enum fw_upload_err
> +adva_cpld_prepare(struct fw_upload *fwl, const u8 *data, u32 size)
> +{

[ ... ]

> +	if (!size || size % CPLD_PAGE_SIZE)
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	bp->cpld_cancel		= false;
> +	bp->cpld_in_config_mode	= false;
> +
> +	mutex_lock(&bp->cpld_lock);

[Severity: Medium]
Can this reset discard a cancel request that has already arrived?
fw_upload_start() sets progress = FW_UPLOAD_PROG_RECEIVING and queues the
work before dropping fwlp->lock, and cancel_store() /
firmware_upload_unregister() only refuse to call ops->cancel() when
progress == FW_UPLOAD_PROG_IDLE:

	cancel_store()/firmware_upload_unregister()
	    adva_cpld_cancel()
	        bp->cpld_cancel = true;

	worker
	    adva_cpld_prepare()
	        bp->cpld_cancel = false;	/* request lost */

In the unregister path that matters: firmware_upload_unregister() calls
cancel() and then flush_work(), so with the request lost, unbind/PCI remove
waits for the full erase (up to 15 s) plus every 16-byte page program.

Two related points: bp->cpld_cancel is a plain bool shared between the
worker and the cancelling task with no READ_ONCE/WRITE_ONCE, atomic or
lock, and it is written here outside bp->cpld_lock; and it is never checked
inside adva_x1_cpld_wait_ready(), so a cancel arriving during the 15 s
erase or a page wait is not observed at all.

[ ... ]

> +	if (adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP, en_args, 2) ||
> +	    adva_x1_cpld_wait_ready(bp, 5000)) {
> +		ret = FW_UPLOAD_ERR_HW_ERROR;
> +		goto err_deselect;
> +	}
> +	bp->cpld_in_config_mode = true;

[Severity: Medium]
Is the flag set too late here?  The short-circuit means
adva_x1_cpld_wait_ready() only runs when the EN_CFG_TP write already
succeeded, so on a wait failure (BUSY timeout, FAILED bit, or an I2C error
in the status read) the CPLD has accepted enable-configuration while
bp->cpld_in_config_mode is still false.

err_deselect is guarded by that flag:

	err_deselect:
		if (bp->cpld_in_config_mode) {
			adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG, dis_args, 2);

so CPLD_CMD_DIS_CFG is never sent, the mux is deselected, the bus is handed
back and the CPLD stays in ISC transparent configuration mode.
fw_upload_main() does not call cleanup() when prepare() fails, so there is
no second chance to send DIS_CFG, and the next upload attempt issues
EN_CFG_TP to an already-enabled device.

[ ... ]

> +static enum fw_upload_err
> +adva_cpld_write(struct fw_upload *fwl, const u8 *data,
> +		u32 offset, u32 size, u32 *written)
> +{

[ ... ]

> +	if (adva_x1_cpld_write(bp, CPLD_CMD_WRITE_PAGE,
> +			       page_args, 3 + CPLD_PAGE_SIZE) ||
> +	    adva_x1_cpld_wait_ready(bp, 100))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	*written = CPLD_PAGE_SIZE;
> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +static enum fw_upload_err
> +adva_cpld_poll_complete(struct fw_upload *fwl)
> +{

[ ... ]

> +	if (adva_x1_cpld_write(bp, CPLD_CMD_REFRESH, ref_args, 2))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	/* CPLD reboots after REFRESH; re-select mux once it comes back up */
> +	msleep(1500);
> +	adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
> +	if (adva_x1_cpld_wait_ready(bp, 3000))
> +		return FW_UPLOAD_ERR_TIMEOUT;
> +
> +	bp->cpld_in_config_mode = false;
> +	return FW_UPLOAD_ERR_NONE;
> +}

[Severity: Medium]
Can this report a failure for an update that actually succeeded?  The
adva_x1_mux_select() return value is discarded, so if the segment is not
responsive at exactly the 1500 ms mark the mux write is NAKed silently and
the following READ_STATUS goes to a bus with no channel enabled:

	adva_x1_cpld_wait_ready(bp, 3000)
	    adva_x1_cpld_read_status()	-> I2C error
	    return -EIO
	return FW_UPLOAD_ERR_TIMEOUT;

At that point DONE was already verified set and REFRESH was accepted, so
the new image is running, yet userspace sees a failed update and is likely
to erase and reprogram again.  Would a bounded retry of the post-reset mux
reselect help here?

Also, bp->cpld_in_config_mode is still true on this return path, so
adva_cpld_cleanup() then sends DIS_CFG to a device that already left
configuration mode.

[ ... ]

> @@ -4902,6 +5368,11 @@ 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);
> +	if (bp->cpld_fw_upload) {
> +		firmware_upload_unregister(bp->cpld_fw_upload);
> +		bp->cpld_fw_upload = NULL;
> +	}
> +	mutex_destroy(&bp->cpld_lock);
>  	device_unregister(&bp->dev);
>  }

[Severity: Low]
This isn't a use-after-free: platform_device_unregister(bp->i2c_ctrl) ->
xiic_i2c_remove() -> i2c_del_adapter() does

	init_completion(&adap->dev_released);
	device_unregister(&adap->dev);
	wait_for_completion(&adap->dev_released);

so the adapter cannot go away while adva_x1_i2c_xfer() holds the
i2c_get_adapter() reference, and later lookups just return NULL so the
helper returns -ENODEV.

Would it still be cleaner to move firmware_upload_unregister() above
platform_device_unregister(bp->i2c_ctrl) and pci_free_irq_vectors()?  As
written, an in-flight upload aborts mid-programming with -ENODEV instead of
being cancelled and flushed first, and i2c_del_adapter() blocks for the
duration of whatever transfer is in flight.

>  
> @@ -5217,11 +5699,16 @@ ptp_ocp_i2c_notifier_call(struct notifier_block *nb,
>  
>  found:
>  	bp = dev_get_drvdata(dev);
> -	if (add)
> +	if (add) {
>  		ptp_ocp_symlink(bp, child, "i2c");
> -	else
> +		/* Cache adapter nr; used by cpld_device_id/cpld_status/cpld_program
> +		 * for reference-counted unbind-safe adapter access.
> +		 */

[Severity: Low]
What is cpld_program?  adva_timecard_x1_attrs[] only gains
&dev_attr_cpld_device_id.attr and &dev_attr_cpld_status.attr, and
programming goes through the fw_upload_ops callbacks, so no such attribute
or function exists in the tree.  Looks like a leftover from an earlier
revision.

> +		WRITE_ONCE(bp->cpld_i2c_adap_nr, i2c_verify_adapter(child)->nr);
> +	} else {
> +		WRITE_ONCE(bp->cpld_i2c_adap_nr, -1); /* invalidate before free */
>  		sysfs_remove_link(&bp->dev.kobj, "i2c");
> -
> +	}
>  	return 0;
>  }