Re: [PATCH net-next 6/9] ptp: ocp: report fw.cpld with an empty value until the USERCODE is read

[netdev-bot+sashiko@kernel.org]

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

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

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

- [Medium] After any CPLD flash that reaches the erase step, the fw.cpld
  running version is never published again (and, at this commit, cpld.id…

--

ptp: ocp: report fw.cpld with an empty value until the USERCODE is read

Instead of publishing the literal string "unknown" for the fw.cpld running
version before the CPLD USERCODE has been read, the driver now passes an
empty value so the component name is still registered with devlink while no
version attribute is emitted.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index e10f6b5149c93..4ce86df6e1968 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -2264,15 +2264,20 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>  		/* 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,
> -		 * so it is reported for every board that has the part and not
> +		 * so it is named for every board that has the part and not
>  		 * only once its USERCODE has been read: a part left holding a
>  		 * bad image answers neither, and gating the component on the
>  		 * read would make that state unrecoverable.
> +		 *
> +		 * An empty value still registers the name with the core -
> +		 * devlink_info_version_put() runs its version_cb before the
> +		 * empty-value early-out - while emitting no version attribute,
> +		 * so nothing is published until the USERCODE has been read.
>  		 */
>  		if (smp_load_acquire(&bp->cpld_usercode_ok))
>  			sprintf(buf, "0x%08x", READ_ONCE(bp->cpld_usercode));
>  		else
> -			strscpy(buf, "unknown", sizeof(buf));
> +			buf[0] = '\0';

[Severity: Medium]
The comment says "nothing is published until the USERCODE has been read".
After a flash, can the USERCODE ever be read again?

adva_x1_cpld_flash() invalidates the cached identity before erasing:

	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;

but the only thing that re-reads it is ptp_ocp_cpld_work(), and that worker
re-arms itself only while the one-shot has not settled:

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

adva_x1_cpld_read_id() sets cpld_id_tried on success, so one second after
probe there is no pending cpld_work item left, and clearing the flag in the
flash path has nothing to wake.  The only other queue site for bp->cpld_work
is ptp_ocp_probe(), and no exit path of adva_x1_cpld_flash() queues it.

With no further read, smp_store_release(&bp->cpld_usercode_ok, true) in
adva_x1_cpld_read_id() never runs again, so ptp_ocp_devlink_info_get() takes
the new else branch forever, and devlink_info_version_put() drops it:

net/devlink/dev.c:devlink_info_version_put() {
	if (req->version_cb)
		req->version_cb(version_name, version_type,
				req->version_cb_priv);

	if (!req->msg || !*version_value)
		return 0;
	...
}

Does this mean fw.cpld is silently missing from every subsequent
"devlink dev info" after a successful flash, recoverable only by unbind and
rebind?  At this commit cpld.id is dropped the same way, since
adva_x1_cpld_flash() also clears bp->cpld_id.

For background: before the read was moved off the always-re-armed 1 Hz
ptp_ocp_sync_work(), clearing cpld_id_tried did cause a re-read on the next
tick; the commit that made the read a one-shot did not add a re-arm on the
flash path.  Checking the end of the series (5b9611f), adva_x1_cpld_flash()
still clears cpld_usercode_ok and cpld_id_tried and cpld_work is still
queued only from probe and from the worker itself, so the missing fw.cpld
version persists there; the cpld.id half is addressed by the later commit
"ptp: ocp: drop only the USERCODE when flashing, and drop it before erasing",
which stops clearing cpld_id.

Would queueing bp->cpld_work from the flash path close this?

>  		err = devlink_info_version_running_put_ext(req, "fw.cpld", buf,
>  							   ver_type);
>  		if (err)

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