Re: [PATCH net-next v5 4/7] ptp: ocp: Track EEPROM fields independently

[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: Track EEPROM fields independently

This replaces the single has_eeprom_data flag with per-field has_board_id
and has_serial flags, serializes EEPROM discovery under a new eeprom_lock
mutex, and makes ptp_ocp_read_eeprom() take a field mask and return an
error.  devlink now publishes whichever field is available and
serialnum_show() propagates read errors.

No Link: tags in the commit header.

The commit message describes fixing a user-visible defect:

  "Requiring both reads to succeed before publishing either field makes a
  missing serial EEPROM block board profile selection and hides an otherwise
  valid board ID."

[Severity: Low]
Should this carry a Fixes: tag?  The condition described is reachable on
shipping hardware: fb_eeprom_map places board_id on the untagged '24c02'
client and serial on the "mac" tagged '24mac402' client, so one of the two
can be absent while the other works, and the old code gated both
ptp_ocp_devlink_info_get() and serialnum_show() on has_eeprom_data.  The
all-or-nothing reader, the two-entry map with the "mac" tag and the devlink
publication all came in with commit 0cfcdd1ebcfe ("ptp: ocp: add nvmem
interface for accessing eeprom"), which would be the Fixes: target.

If this is instead meant as preparatory work and not stable material (the
patch does change sysfs error semantics), could the changelog say so
explicitly?

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 3d26ec1f7b9ec..31a4c9dfe391c 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -1969,21 +1977,50 @@ ptp_ocp_nvmem_device_put(struct nvmem_device **nvmemp)
>  	*nvmemp = NULL;
>  }
>  
> -static void
> -ptp_ocp_read_eeprom(struct ptp_ocp *bp)
> +static int
> +ptp_ocp_read_eeprom(struct ptp_ocp *bp, unsigned int fields)
>  {
>  	const struct ptp_ocp_eeprom_map *map;
>  	struct nvmem_device *nvmem;
> +	u8 data[OCP_BOARD_ID_LEN];
> +	const char *field_name = "data";
>  	const void *tag;
> -	int ret;
> -
> -	if (!bp->i2c_ctrl)
> -		return;
> +	unsigned int field;
> +	int ret = 0;
>  
>  	tag = NULL;
>  	nvmem = NULL;
> +	mutex_lock(&bp->eeprom_lock);
> +	fields &= OCP_EEPROM_ALL;
> +	if (bp->has_board_id)
> +		fields &= ~OCP_EEPROM_BOARD_ID;
> +	if (bp->has_serial)
> +		fields &= ~OCP_EEPROM_SERIAL;
> +	if (!fields)
> +		goto out;
> +	if (!bp->i2c_ctrl || !bp->eeprom_map) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
>  
>  	for (map = bp->eeprom_map; map->len; map++) {
> +		if (map->bp_offset == offsetof(struct ptp_ocp, board_id)) {
> +			field = OCP_EEPROM_BOARD_ID;
> +			field_name = "board ID";
> +			if (bp->has_board_id)
> +				continue;
> +		} else if (map->bp_offset == offsetof(struct ptp_ocp, serial)) {
> +			field = OCP_EEPROM_SERIAL;
> +			field_name = "serial number";
> +			if (bp->has_serial)
> +				continue;
> +		} else {
> +			continue;
> +		}
> +
> +		if (!(fields & field))
> +			continue;
> +
>  		if (map->tag != tag) {
>  			tag = map->tag;
>  			ptp_ocp_nvmem_device_put(&nvmem);
> @@ -1992,24 +2029,34 @@ ptp_ocp_read_eeprom(struct ptp_ocp *bp)
>  			nvmem = ptp_ocp_nvmem_device_get(bp, tag);
>  			if (IS_ERR(nvmem)) {
>  				ret = PTR_ERR(nvmem);
> -				goto fail;
> +				goto out;
>  			}
>  		}
> -		ret = nvmem_device_read(nvmem, map->off, map->len,
> -					BP_MAP_ENTRY_ADDR(bp, map));
> -		if (ret != map->len)
> -			goto fail;
> +		ret = nvmem_device_read(nvmem, map->off, map->len, data);
> +		if (ret < 0)
> +			goto out;

[Severity: Medium]
Are the two fields really independent in both directions here?  Both failure
paths inside the loop, the ptp_ocp_nvmem_device_get() failure and this
nvmem_device_read() failure, leave the map walk entirely rather than
recording a failure for that one field and moving on to the next requested
entry.

fb_eeprom_map lists the board ID first, and it lives on a different I2C
client than the serial number:

drivers/ptp/ptp_ocp.c:
	static struct ptp_ocp_eeprom_map fb_eeprom_map[] = {
		{ EEPROM_ENTRY(0x43, board_id) },
		{ EEPROM_ENTRY(0x00, serial), .tag = "mac" },
		{ }
	};

So if the '24c02' board-ID EEPROM is missing or unreadable while the
'24mac402' MAC EEPROM is fine, ptp_ocp_devlink_info_get() calls
ptp_ocp_read_eeprom(bp, OCP_EEPROM_ALL), the walk bails on the first entry
and the serial entry is never attempted.  has_serial stays false, so
devlink_info_serial_number_put() is skipped on every subsequent request.

Isn't that the mirror image of the case the changelog says it removes, "Let
devlink report whichever fields are available"?  Would a per-field error
that continues to the next map entry be closer to the intent?

The serialnum sysfs path escapes this only because it asks for
OCP_EEPROM_SERIAL and skips the board-ID entry.

> +
> +		memcpy(BP_MAP_ENTRY_ADDR(bp, map), data, map->len);
> +		if (field == OCP_EEPROM_BOARD_ID) {
> +			/* Publish the field before marking it ready. */
> +			smp_store_release(&bp->has_board_id, true);
> +		} else {
> +			/* Publish the field before marking it ready. */
> +			smp_store_release(&bp->has_serial, true);
> +		}
>  	}
>  
> -	bp->has_eeprom_data = true;
> +	ret = 0;
>  
>  out:
> +	if (ret && ret != -EPROBE_DEFER && !bp->eeprom_error_reported) {
> +		dev_err(&bp->pdev->dev, "failed to read EEPROM %s: %pe\n",
> +			field_name, ERR_PTR(ret));
> +		bp->eeprom_error_reported = true;
> +	}
>  	ptp_ocp_nvmem_device_put(&nvmem);
> -	return;
> -
> -fail:
> -	dev_err(&bp->pdev->dev, "could not read eeprom: %d\n", ret);
> -	goto out;
> +	mutex_unlock(&bp->eeprom_lock);
> +	return ret;
>  }

[ ... ]

> @@ -2166,24 +2214,24 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>  	if (err)
>  		return err;
>  
> -	if (!bp->has_eeprom_data) {
> -		ptp_ocp_read_eeprom(bp);
> -		if (!bp->has_eeprom_data)
> -			return 0;
> -	}
> +	ptp_ocp_read_eeprom(bp, OCP_EEPROM_ALL);

[ ... ]

> @@ -3757,9 +3805,17 @@ static ssize_t
>  serialnum_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct ptp_ocp *bp = dev_get_drvdata(dev);
> +	int ret;
>  
> -	if (!bp->has_eeprom_data)
> -		ptp_ocp_read_eeprom(bp);
> +	/* Pairs with field publication in ptp_ocp_read_eeprom(). */
> +	if (!smp_load_acquire(&bp->has_serial)) {
> +		ret = ptp_ocp_read_eeprom(bp, OCP_EEPROM_SERIAL);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Can this hand -EPROBE_DEFER to userspace?  ptp_ocp_read_eeprom() gets the
nvmem device through ptp_ocp_nvmem_device_get() -> nvmem_device_find() ->
__nvmem_device_get(), and when bus_find_device() finds no matching nvmem
device that returns -EPROBE_DEFER:

drivers/nvmem/core.c:__nvmem_device_get() {
	...
	if (!nvmem)
		return ERR_PTR(-EPROBE_DEFER);
	...
}

ptp_ocp_read_eeprom() stores that with "ret = PTR_ERR(nvmem); goto out;" and
returns it unchanged; the "ret != -EPROBE_DEFER" test at the out: label only
suppresses the dev_err(), not the return value.  serialnum_show() then
returns it directly, so reading
/sys/class/timecard/ocpN/serialnum with the serial EEPROM's nvmem device not
registered (no 24mac402, at24 not bound yet) gives read(2) errno 517.

That is exactly the "serial EEPROM absent" case this patch targets, and the
attribute is world readable.  Would translating internal codes to something
like -ENODEV, -EAGAIN or the -ENODATA already used below be preferable
before crossing the syscall boundary?

> +		/* Pairs with field publication in ptp_ocp_read_eeprom(). */
> +		if (!smp_load_acquire(&bp->has_serial))
> +			return -ENODATA;
> +	}
>  
>  	return sysfs_emit(buf, "%pM\n", bp->serial);
>  }

[ ... ]