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

[netdev-bot+sashiko@kernel.org]

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