Re: [PATCH net-next 1/9] ptp: ocp: move the CPLD identification read off the sync worker

[netdev-bot+sashiko@kernel.org]

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

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

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] Moving the CPLD identification read into a self-terminating
  delayed work (ptp_ocp_cpld_work()) removes the only mechanism that re-…
- [Medium] The commit message names unbind/reboot blocking as part of the
  problem being fixed ("because ptp_ocp_remove() - which is also the…
- [Low] Comments describing the per-tick re-resolution of the CPLD i2c
  adapter are no longer accurate.
- [Low] Stale comment: `#define CPLD_ID_MAX_ATTEMPTS 10 /* one per
  sync_work tick */` (drivers/ptp/ptp_ocp.c:4386) still attributes the…

--

ptp: ocp: move the CPLD identification read off the sync worker

The one-shot TAP CPLD identification read is moved out of
ptp_ocp_sync_work() into a new delayed work, ptp_ocp_cpld_work(), which is
queued from probe only on boards that carry the part.  The new work
re-arms itself once a second only while bp->cpld_id_tried is clear, and
ptp_ocp_remove() gains a cancel_delayed_work_sync() for it.

A few questions below.

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

[ ... ]

> @@ -5935,18 +5937,32 @@ 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.
> -	 */
> +	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
> +}
> +
> +/*
> + * Resolve the i2c adapter and read the CPLD identification.
> + *
> + * Kept off ptp_ocp_sync_work(): a claim can block for seconds - the
> + * MicroBlaze handshake polls for up to MBLAZE_RETRIES * MBLAZE_RETRY_US and
> + * the hand-back does the same - which would delay the in-sync sampling and
> + * the dpll change notification, and stall the cancel_delayed_work_sync() on
> + * the unbind and shutdown paths for as long.
> + *
> + * Reschedules only while there is something left to do, so a board without
> + * the part, or one whose identification is settled, costs nothing.
> + */
> +static void ptp_ocp_cpld_work(struct work_struct *work)
> +{
> +	struct ptp_ocp *bp = container_of(work, struct ptp_ocp, cpld_work.work);
> +
>  	adva_x1_cache_i2c_adap(bp);

[Severity: Low]

This isn't a bug, but with adva_x1_cache_i2c_adap() now called only from
ptp_ocp_cpld_work(), are its comments about per-tick retries still
accurate?  Its header says:

 * 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.

and its body says an adapter cached in the removal window is rejected by
adva_x1_bus_claim() "and the next tick looks again", while
adva_x1_bus_claim() says forgetting a recycled number "lets the worker look
the adapter up again".

Once bp->cpld_id_tried is set the new worker stops re-arming, so there is
no next tick and only ptp_ocp_i2c_notifier_call() can repopulate the
number - which the same header comment says can miss the adapter.

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

[Severity: Low]

This isn't a bug, but the attempt budget consumed here is still documented
against the old worker:

#define CPLD_ID_MAX_ATTEMPTS  10     /* one per sync_work tick */

The attempts are now one per cpld_work tick.

>  
> -	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
> +	if (!READ_ONCE(bp->cpld_id_tried))
> +		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
> +				   HZ);
>  }

[Severity: High]

Does this leave the post-flash re-read of the CPLD identity without a
worker to perform it?

adva_x1_cpld_read_id() sets the flag on the first success, or after the
attempt budget runs out:

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

so after the probe-time read settles the tail above stops re-arming
cpld_work, and ptp_ocp_probe() is the only other queue site.

adva_x1_cpld_flash() still clears the bookkeeping and leaves the re-read to
a periodic worker:

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

but it queues nothing and does not re-read the ID or USERCODE itself.

Before this patch ptp_ocp_sync_work() requeued itself every HZ for the life
of the binding, so the next tick saw the cleared flag and read the new
image's IDCODE and USERCODE.  With the split, does bp->cpld_id stay 0 and
bp->cpld_usercode_ok stay false until unbind and rebind, so
ptp_ocp_devlink_info_get() permanently skips cpld.id and reports fw.cpld as
"unknown"?

	id = READ_ONCE(bp->cpld_id);
	if (id) {
		sprintf(buf, "0x%08x", id);
	...
	if (smp_load_acquire(&bp->cpld_usercode_ok))
		sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
	else
		strscpy(buf, "unknown", sizeof(buf));

Documentation/networking/devlink/ptp_ocp.rst states:

       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.

Would queueing bp->cpld_work from adva_x1_cpld_flash() after that reset
(queueing an already pending delayed work is a no-op) match the documented
behaviour again, or should the worker keep re-arming while bp->has_cpld?

The same concern applies to the later reset of cpld_id_tried added in
adva_x1_cache_i2c_adap() by "ptp: ocp: forget a CPLD i2c adapter number
that no longer resolves" in this series; at the end of the series the queue
sites for cpld_work are still probe plus the worker's own conditional tail.

[ ... ]

> @@ -6046,6 +6063,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		}
>  	}
>  	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
> +	if (bp->has_cpld)
> +		queue_delayed_work(system_power_efficient_wq, &bp->cpld_work,
> +				   HZ);
>  
>  	return 0;
>  out_dpll:

[ ... ]

> @@ -6080,6 +6100,7 @@ ptp_ocp_remove(struct pci_dev *pdev)
>  	int i;
>  
>  	cancel_delayed_work_sync(&bp->sync_work);
> +	cancel_delayed_work_sync(&bp->cpld_work);

[Severity: Medium]

Is the unbind and shutdown part of the changelog still true with this
cancel in place?  The changelog says:

    While that runs the in-sync sampling and the dpll notification are delayed
    by the same amount, and because ptp_ocp_remove() - which is also the
    .shutdown handler - begins with cancel_delayed_work_sync(&bp->sync_work),
    unbind and reboot block for it too.

and the new function comment repeats it:

 * the dpll change notification, and stall the cancel_delayed_work_sync() on
 * the unbind and shutdown paths for as long.

cancel_delayed_work_sync() waits for an already running callback to return
rather than interrupting it, so if ptp_ocp_cpld_work() is mid-claim the
executing path is:

ptp_ocp_cpld_work()
  adva_x1_cpld_read_id()
    adva_x1_bus_claim()
      adva_x1_mblaze_acquire()

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

That is 200 iterations of a 10 ms sleep, and on timeout
adva_x1_mblaze_release() polls the same amount again, with no teardown
escape check in either loop.  The window in which the blocking read can be
in flight is the same before and after the split, since the read is
attempted once a second until cpld_id_tried is set.

Should the changelog and the comment be narrowed to the 1 Hz in-sync
sampling and dpll notification latency, or would a teardown flag checked
before taking cpld_lock and between MicroBlaze retries let the still
required synchronous cancel return promptly?

>  	for (i = 0; i < OCP_SMA_NUM; i++) {
>  		if (bp->sma[i].dpll_pin) {
>  			dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]);

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