From: AI Reviewer <redacted>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
ptp: ocp: add CPLD ISP support for ADVA TimeCard

This change adds a sysfs interface on the adva_x1 ptp_ocp variant to
own a secondary I2C bus and forward arbitrary I2C transactions to a
Lattice MachXO3 CPLD for in-system programming. A new attribute
i2c_bus_ctrl controls bus ownership, and a bin_attr cpld_i2c_xfer
implements a write-request/read-response RPC guarded by an
allowlisted address set.

A few questions on the implementation below.

[Low]
Should tap_i2c_lock be paired with mutex_destroy() on the
ptp_ocp_detach()/ptp_ocp_dev_release() paths? The mutex is initialized
here but never destroyed, which skips the lockdep/DEBUG_MUTEXES
validation that the lock is not held at teardown and breaks the usual
init/destroy symmetry for driver-owned mutexes.

[Low]
The commit message says the interface is available on "ADVA TimeCard
boards", but only adva_timecard_x1_group gets the new attributes;
adva_timecard_group is unchanged. However mutex_init() lives in
ptp_ocp_adva_board_init(), which is called for both the adva and
adva_x1 variants, so the mutex is initialized on non-x1 hardware where
tap_i2c_rsp is never consumed. Should the commit message clarify that
this is x1-specific, or should the feature be exposed on the base
adva variant as well?

[Medium]
Is it safe to repurpose struct gpio_reg's __pad1 field as a live control
register? The struct declares that slot as u32 __pad1, a padding/reserved
member, and pps_select is registered for every board variant via
OCP_MEM_RESOURCE(pps_select). Could a future rearrangement of gpio_reg
padding silently break this, and does offset 0xC of pps_select do
something different (or truly reserved) on non-adva-x1 boards where an
accidental write could produce undefined FPGA behavior? Would renaming
the field (for example to i2c_bus_ctrl) and documenting its per-variant
meaning make the intent clearer?

[Low]
Since this write hands ownership of a shared I2C bus between the host
and the on-board MicroBlaze, does anything guarantee the write has
been posted to the FPGA before userspace triggers the next
cpld_i2c_xfer transaction? There is no read-back, barrier, or
completion polling here, so on architectures with posted PCIe writes
the first ISP transaction after a hand-off could see the old owner
and NACK or produce corrupted data.

+static DEVICE_ATTR_RW(i2c_bus_ctrl);
+
+/*
+ * cpld_i2c_xfer - sysfs binary I2C passthrough for adva_x1 TAP CPLD.
+ *
+ * write: [addr][write_len][read_len][flags][write_data...]
+ *   flags bit 0: I2C_M_NOSTART on the read segment
+ * read:  [status][read_data...]
+ *   status 0 = success, else positive errno
+ *
+ * Only addresses 0x40 (CPLD) and 0x74 (mux) are permitted.
+ */

[Medium]
Both i2c_bus_ctrl and cpld_i2c_xfer become permanent userspace ABI
including a binary encoded request/response protocol (4-byte header,
status byte, allowlist, NOSTART flag bit), but no
Documentation/ABI/testing/sysfs-driver-ptp-ocp entries are added.
Would documenting the byte layout there rather than only in an inline
C source comment help lock it down as UAPI?

[Low]
As a broader design question rather than a bug: since /dev/i2c-N
(i2c-dev) already exposes arbitrary I2C transactions with the
I2C_RDWR ioctl, is a stateful sysfs bin_attr the right vehicle for
this? Related observations for maintainer consideration:

 - i2c_bus_ctrl exposes a raw MMIO register with no semantic model
   (any u32 accepted, unspecified bit meanings).

 - The CPLD/mux allowlist is hard-coded in ptp_ocp.c rather than
   instantiated as proper i2c_client/i2c_mux child devices whose
   lifetime is managed by the I2C core.

 - The driver does not hold a persistent handle to the adapter and
   rediscovers it on every write via device_for_each_child(), which
   is fragile if bind order changes or the parent hosts multiple
   adapter children.

Once this lands as UAPI it is hard to undo, so it seems worth an
explicit look.

+#define TAP_I2C_ALLOWED_ADDRS_NUM  2
+static const u8 tap_i2c_allowed_addrs[TAP_I2C_ALLOWED_ADDRS_NUM] = {
+	0x40, /* CPLD */
+	0x74, /* mux  */
+};
+
+#define TAP_I2C_REQ_HDR_LEN   4
+#define TAP_I2C_MAX_WRITE_LEN 67
+#define TAP_I2C_MAX_READ_LEN  20
+#define TAP_I2C_FLAG_NOSTART  BIT(0)
+
+static int
+ptp_ocp_tap_i2c_find_adapter(struct device *dev, void *data)
+{
+	struct i2c_adapter **adap = data;
+
+	*adap = i2c_verify_adapter(dev);
+	return (*adap) ? 1 : 0;
+}
+
+static ssize_t
+ptp_ocp_cpld_i2c_write(struct file *file, struct kobject *kobj,
+		       const struct bin_attribute *attr,
+		       char *buf, loff_t off, size_t count)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
+	const u8 *req = (const u8 *)buf;
+	u8 addr, write_len, read_len, flags;
+	struct i2c_adapter *adap = NULL;
+	struct i2c_msg msgs[2];
+	u8 rdbuf[TAP_I2C_MAX_READ_LEN];
+	int nmsgs, ret, i;
+
+	if (count < TAP_I2C_REQ_HDR_LEN || count > TAP_I2C_REQ_HDR_LEN + TAP_I2C_MAX_WRITE_LEN)
+		return -EINVAL;

[Medium]
Is off ignored on purpose here? If sysfs delivers a large write split
into multiple chunks, or if userspace uses pwrite() at a non-zero
offset, this handler will interpret the fragment starting at off as a
fresh [addr][write_len][read_len][flags] request and generate
arbitrary I2C traffic. The read side already rejects off > 0, so the
two ends are inconsistent.

+	addr      = req[0];
+	write_len = req[1];
+	read_len  = req[2];
+	flags     = req[3];
+
+	/* Validate */
+	for (i = 0; i < TAP_I2C_ALLOWED_ADDRS_NUM; i++)
+		if (addr == tap_i2c_allowed_addrs[i])
+			break;
+	if (i == TAP_I2C_ALLOWED_ADDRS_NUM)
+		return -EPERM;
+
+	if (write_len > TAP_I2C_MAX_WRITE_LEN)
+		return -EINVAL;
+	if (read_len > TAP_I2C_MAX_READ_LEN)
+		return -EINVAL;
+	if (write_len + TAP_I2C_REQ_HDR_LEN > count)
+		return -EINVAL;
+	if (write_len == 0 && read_len == 0)
+		return -EINVAL;
+
+	if (!bp->i2c_ctrl)
+		return -ENODEV;
+	device_for_each_child(&bp->i2c_ctrl->dev, &adap,
+			      ptp_ocp_tap_i2c_find_adapter);
+	if (!adap)
+		return -ENODEV;

[High]
Can the adapter go away between this lookup and i2c_transfer()?
device_for_each_child() only holds a klist iterator reference for the
duration of the callback and drops it via klist_iter_exit() before
returning, and i2c_verify_adapter() is just a type check that takes no
reference:

    static int
    ptp_ocp_tap_i2c_find_adapter(struct device *dev, void *data)
    {
        struct i2c_adapter **adap = data;

        *adap = i2c_verify_adapter(dev);
        return (*adap) ? 1 : 0;
    }

bp->i2c_ctrl is a platform_device, and the actual I2C adapter is a
child bound by a separate I2C controller driver that can be unbound
via /sys/bus/platform/drivers/<name>/unbind. If that happens between
device_for_each_child() returning and i2c_transfer(adap, ...), the
struct i2c_adapter may already be freed. Would taking an explicit
reference via i2c_get_adapter()/i2c_put_adapter() around the transfer
close this window?

+
+	nmsgs = 0;
+	if (write_len > 0) {
+		msgs[nmsgs].addr  = addr;
+		msgs[nmsgs].flags = 0;
+		msgs[nmsgs].len   = write_len;
+		msgs[nmsgs].buf   = (u8 *)req + TAP_I2C_REQ_HDR_LEN;
+		nmsgs++;
+	}
+	if (read_len > 0) {
+		u16 rd_flags = I2C_M_RD;
+
+		if (flags & TAP_I2C_FLAG_NOSTART)
+			rd_flags |= I2C_M_NOSTART;
+		msgs[nmsgs].addr  = addr;
+		msgs[nmsgs].flags = rd_flags;
+		msgs[nmsgs].len   = read_len;
+		msgs[nmsgs].buf   = rdbuf;
+		nmsgs++;
+	}

[Medium]
When userspace supplies flags = TAP_I2C_FLAG_NOSTART together with
write_len = 0 and read_len > 0, I2C_M_NOSTART ends up on the first (and
only) message. Documentation/i2c/i2c-protocol.rst says:

    If you set the I2C_M_NOSTART variable for the first partial
    message, we do not generate Addr, but we do generate the start
    condition S. This will probably confuse all other clients on your
    bus, so don't try this.

The CPLD sits on a mux-shared secondary bus (0x74), so should this
combination be rejected outright rather than passed through?

[Medium]
rdbuf lives on the kernel stack and is handed directly to
i2c_transfer(). Documentation/i2c/dma-considerations.rst requires
i2c_msg.buf to be DMA-safe when the underlying adapter uses DMA, and
the DMA characteristics of the adapter behind bp->i2c_ctrl are not
known to this code. Would i2c_get_dma_safe_msg_buf() /
i2c_put_dma_safe_msg_buf() be more appropriate here?

+
+	ret = i2c_transfer(adap, msgs, nmsgs);
+
+	mutex_lock(&bp->tap_i2c_lock);
+	if (ret == nmsgs) {
+		bp->tap_i2c_rsp[0] = 0;
+		if (read_len > 0)
+			memcpy(&bp->tap_i2c_rsp[1], rdbuf, read_len);
+		bp->tap_i2c_rsp_len = 1 + read_len;
+		ret = count;
+	} else {
+		bp->tap_i2c_rsp[0]  = (u8)(ret < 0 ? -ret : EIO);
+		bp->tap_i2c_rsp_len = 1;
+		ret = (ret < 0) ? ret : -EIO;
+	}
+	mutex_unlock(&bp->tap_i2c_lock);
+
+	return ret;
+}

[High]
Can two concurrent users of cpld_i2c_xfer observe each other's
responses via bp->tap_i2c_rsp?

The mutex is only held while publishing the result and is released
before the corresponding read() runs. Consider:

    Writer A: i2c_transfer() completes with data X, preempted before
              taking tap_i2c_lock
    Writer B: i2c_transfer() completes with data Y, publishes Y under
              the lock, releases lock
    Writer A: resumes, publishes X under the lock
    Reader B: reads and gets Writer A's X

Even a single well-behaved userspace process performs write() and
read() as separate syscalls with no session identity, so nothing ties
a read() back to the write() that produced it. For a stateful ISP
session with the CPLD, is there a mechanism (open-exclusive, per-fd
buffer, single write/read syscall pair, ioctl-style) that would keep
transactions correlated?

[Medium]
Is the "positive errno" contract in the header comment held by this
encoding?

    bp->tap_i2c_rsp[0]  = (u8)(ret < 0 ? -ret : EIO);

Any errno whose low 8 bits are zero (for example -ENOTSUPP is 524 and
truncates to 12, which happens to be ENOMEM; values like 256 or 512
would truncate to 0 and encode "success") ends up in the shadow buffer
as a completely different or false-success value. For userspace ISP
tools that consult this status byte to decide whether to continue
programming, would returning the errno as a wider field (or capping
the values that can appear) be safer?

+
+static ssize_t
+ptp_ocp_cpld_i2c_read(struct file *file, struct kobject *kobj,
+		      const struct bin_attribute *attr,
+		      char *buf, loff_t off, size_t count)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
+	ssize_t ret;
+
+	if (off > 0)
+		return 0;
+
+	mutex_lock(&bp->tap_i2c_lock);
+	ret = min(count, bp->tap_i2c_rsp_len);
+	memcpy(buf, bp->tap_i2c_rsp, ret);
+	mutex_unlock(&bp->tap_i2c_lock);
+	return ret;
+}

[Medium]
Does this drop response bytes on short reads? off is not honored: if
userspace reads with count < tap_i2c_rsp_len (up to 21 bytes), the
first read returns the leading fragment and advances the file
position, then the next read arrives with off > 0 and this handler
returns 0 (EOF), so the tail of the response is lost. Would the usual
bin_attribute pattern

    memcpy(buf, bp->tap_i2c_rsp + off,
           min(count, bp->tap_i2c_rsp_len - off));

be more appropriate here?

+
+static const struct bin_attribute tap_i2c_bin_attr = {
+	.attr  = { .name = "cpld_i2c_xfer", .mode = 0600 },
+	.write = ptp_ocp_cpld_i2c_write,
+	.read  = ptp_ocp_cpld_i2c_read,
+};
+

[ ... ]
-- 
This is an AI-generated review.