Re: [PATCH net-next v5 6/7] ptp: ocp: Add R4006 I2C peripheral topology

[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 R4006 I2C peripheral topology

Adds an I2C profile describing the LM75B/SHT3x/ICP10100 sensors and the five
IS32FL3207 RGB LED groups behind the PCA9546 mux on R4006 boards. The board
ID is now read independently of the serial EEPROM and used to select the
profile.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 444b26a5e2a9f..5ae50ad83bdac 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -2496,13 +2575,29 @@ ptp_ocp_i2c_populate_topology(struct ptp_ocp *bp,
>  
>  	if (!READ_ONCE(bp->i2c_root_present))
>  		return 0;
> -	if (!ptp_ocp_i2c_supported(bp) || !bp->i2c_profile)
> +	if (!ptp_ocp_i2c_supported(bp) || !bp->eeprom_map)
>  		return 0;
>  
>  	adapter = ptp_ocp_i2c_root_adapter(i2c_ctrl);
>  	if (!adapter)
>  		return -EAGAIN;
>  
> +	/* Pairs with field publication in ptp_ocp_read_eeprom(). */
> +	if (!smp_load_acquire(&bp->has_board_id)) {
> +		ret = ptp_ocp_read_eeprom(bp, OCP_EEPROM_BOARD_ID);
> +		if (ret)
> +			goto out_put_adapter;
> +	}

[Severity: Medium]
Does relaxing this guard from !bp->i2c_profile to !bp->eeprom_map change
behaviour on boards that are not R4006?

Before this patch nothing ever assigned bp->i2c_profile, so
ptp_ocp_i2c_populate_topology() returned 0 immediately on every board and
the topology/retry machinery never ran. Now every device accepted by
ptp_ocp_i2c_supported() (the Meta and Celestica TimeCard IDs) passes the
guard, because ptp_ocp_fb_board_init() sets:

	bp->eeprom_map = fb_eeprom_map;

so all of them reach the new unconditional board-ID read.

What happens when that read fails rather than returning an unknown board
ID?  ptp_ocp_read_eeprom() returns -EPROBE_DEFER when no nvmem provider
is registered, -EOPNOTSUPP with CONFIG_NVMEM=n via the
nvmem_device_read() stub, or -ENODEV/-EIO on a bus failure.  Note that
PTP_1588_CLOCK_OCP selects neither EEPROM_AT24 nor NVMEM, so a build or
configuration without at24 is an ordinary case.

The error is propagated through goto out_put_adapter to
ptp_ocp_i2c_work(), which hands it to ptp_ocp_i2c_retry():

	if (retries >= OCP_I2C_RETRY_MAX) {
		if (exhausted)
			dev_err(&bp->pdev->dev,
				"I2C topology failed after %d attempts: %pe; "
				"retrying every %d seconds\n",
				OCP_I2C_RETRY_MAX, ERR_PTR(error),
				OCP_I2C_RECOVERY_SECS);
		delay = OCP_I2C_RECOVERY_SECS * HZ;
	}

	/* Preserve a faster rerun queued by an I2C bus notification. */
	queue_delayed_work(system_wq, &bp->i2c_work, delay);

There is no bound on the recovery re-queues, so a board that will never
have a profile ends up logging an "I2C topology failed" error and then
re-arming bp->i2c_work every 30 seconds for the lifetime of the binding.
Would it be preferable to treat a failed board-ID read as "no profile"
and return 0, or to keep the early-out tied to a board that actually has
a profile?

> +	/* Pairs with field publication in ptp_ocp_read_eeprom(). */
> +	if (!smp_load_acquire(&bp->has_board_id)) {
> +		ret = -EAGAIN;
> +		goto out_put_adapter;
> +	}
> +	if (!bp->i2c_profile)
> +		bp->i2c_profile = ptp_ocp_i2c_select_profile(bp);
> +	if (!bp->i2c_profile)
> +		goto out_put_adapter;
> +
>  	ret = ptp_ocp_i2c_init_nodes(bp);
>  	if (ret)
>  		goto out_put_adapter;

Related to the above, the commit message says:

    while unknown board IDs remain untouched.

That holds for board IDs that are read successfully, but could the
changelog also mention that the topology and retry work is now entered on
every supported PCI ID, and what happens when the board-ID EEPROM cannot
be read at all?