* [PATCH v4 1/1] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1
@ 2026-07-08 6:56 Sagi Maimon
2026-07-15 11:15 ` Simon Horman
0 siblings, 1 reply; 2+ messages in thread
From: Sagi Maimon @ 2026-07-08 6:56 UTC (permalink / raw)
To: jonathan.lemon, vadim.fedorenko, richardcochran, andrew+netdev,
davem, edumazet, kuba, pabeni
Cc: linux-kernel, netdev, Sagi Maimon
The ADVA TimeCard X1 (PCI device 0x0410) uses a Lattice MachXO3 CPLD
that is programmed over I2C using in-system programming (ISP).
The CPLD is connected to a secondary I2C bus shared with the onboard
MicroBlaze soft CPU. Add support for taking ownership of this bus and
exposing the required interfaces through sysfs, allowing userspace tools
to perform CPLD programming.
To limit the scope of this functionality, sysfs-based I2C access is
restricted to the ADVA TimeCard X1 variant and only for the two I2C
slave addresses used during ISP (0x40 CPLD, 0x74 mux).
Add two sysfs attributes under /sys/class/timecard/ocpN/ (x1 only):
i2c_bus_ctrl - arbitrate the shared I2C bus from the MicroBlaze via
a three-step read/write/poll handshake
cpld_i2c_xfer - binary passthrough for I2C transactions to the CPLD
and its PCA9548 mux; one atomic request per write()
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
Addressed comments from:
- Paolo Abeni : https://lore.kernel.org/all/20260707091926.88033-1-pabeni@redhat.com/
Changes since v3:
Changes since v3:
- Make the new ABI X1-specific. The base ADVA TimeCard variant does
not expose cpld_i2c_xfer; the new i2c_bus_ctrl and cpld_i2c_xfer
entries are documented as X1-only in Documentation/ABI/testing/
sysfs-timecard.
- Rename struct gpio_reg.__pad1 to i2c_bus_ctrl and document that the
field at offset 0xC in pps_select is a live register on adva_x1 and
reserved on the other variants.
- Document i2c_bus_ctrl as the MicroBlaze I2C bus arbitration register
and cpld_i2c_xfer as the X1-only binary passthrough ABI, including
the 4-byte request header, address allowlist, NOSTART flag rules,
status byte encoding, and partial-read behavior.
- Guard tap_i2c_lock initialisation and teardown so they run only on
the adva_x1 path; the base ADVA variant does not use the CPLD I2C
state.
- Replace the per-call device_for_each_child() adapter lookup with a
cached adapter number set by the existing I2C bus notifier. Use
i2c_get_adapter()/i2c_put_adapter() in cpld_i2c_write() so the
adapter is reference-counted and safe against concurrent unbind.
Remove the now-unused helper.
- Reject non-zero file offsets in cpld_i2c_write() and honour the
offset in cpld_i2c_read() so the bin_attribute follows the usual
partial-read/write rules.
- Reject TAP_I2C_FLAG_NOSTART when write_len == 0; that flag is only
valid for the read phase of a combined write+read transfer.
- Replace the stack read buffer with i2c_get_dma_safe_msg_buf() /
i2c_put_dma_safe_msg_buf() so the I2C message buffer is safe for
adapters that use DMA.
- Serialize the full transfer+publish path with tap_i2c_lock so
concurrent writers cannot overwrite each other's response in
tap_i2c_rsp.
- Add tap_i2c_errno_to_byte() so the one-byte status field does not
truncate large errno values to a wrong value or false success.
Documentation/ABI/testing/sysfs-timecard | 78 ++++++++
drivers/ptp/ptp_ocp.c | 240 ++++++++++++++++++++++-
2 files changed, 313 insertions(+), 5 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index 3ae41b7634ac..c033d2f90d7e 100644
--- a/Documentation/ABI/testing/sysfs-timecard
+++ b/Documentation/ABI/testing/sysfs-timecard
@@ -291,3 +291,81 @@ Description: (RW) The DCF and IRIG output signals are in UTC, while the
these formats.
The offset may be changed by writing an unsigned integer.
+
+What: /sys/class/timecard/ocpN/i2c_bus_ctrl
+Date: July 2026
+Contact: Jonathan Lemon <jonathan.lemon@gmail.com>
+Description: (RW) MicroBlaze I2C bus arbitration register used on
+ ADVA TimeCard X1 boards.
+
+ This file is a userspace ABI and must remain backward
+ compatible.
+
+ The register controls ownership of the I2C bus shared between
+ the host and the on-board MicroBlaze soft CPU.
+
+ Read 0x00000000 to confirm the bus is free. Write 0x0000ffff
+ to request ownership, then poll reads until 0xffffffff is
+ returned. Write 0x00000000 to release the bus.
+
+ Values are read and written as hexadecimal strings, for
+ example 0x0000ffff.
+
+
+What: /sys/class/timecard/ocpN/cpld_i2c_xfer
+Date: July 2026
+Contact: Jonathan Lemon <jonathan.lemon@gmail.com>
+Description: (RW, binary) I2C passthrough endpoint for ADVA TimeCard X1
+ TAP CPLD transactions.
+
+ This file is a userspace ABI and must remain backward
+ compatible.
+
+ The binary attribute is present only on ADVA TimeCard X1
+ boards. Each write() supplies one complete request and the
+ file offset must be zero.
+
+ Write request layout (byte offsets):
+
+ +--------+----------+-----------+-------+----------------+
+ | Byte 0 | Byte 1 | Byte 2 | Byte3 | Bytes 4.. |
+ +--------+----------+-----------+-------+----------------+
+ | addr | write_len| read_len | flags | write_data[] |
+ +--------+----------+-----------+-------+----------------+
+
+ Header length is 4 bytes.
+
+ Request constraints:
+
+ - addr allowlist is exactly 0x40 (CPLD) and 0x74 (mux)
+ - write_len maximum is 67 bytes
+ - read_len maximum is 20 bytes
+ - at least one of write_len/read_len must be non-zero
+ - request write size is 4 + write_len bytes
+
+ flags field:
+
+ bit 0 (0x01): apply I2C_M_NOSTART to the read segment.
+ Valid only when write_len > 0 and read_len > 0.
+ all other bits: reserved for future use.
+
+ Read response layout (byte offsets):
+
+ +--------+----------------+
+ | Byte 0 | Bytes 1.. |
+ +--------+----------------+
+ | status | read_data[] |
+ +--------+----------------+
+
+ status byte semantics:
+
+ - 0x00 means transfer succeeded
+ - non-zero means errno value encoded as a positive byte
+ - errno values that do not fit in one byte are reported as
+ EIO
+
+ On success, response length is 1 + read_len bytes.
+ On failure, response length is 1 byte (status only).
+
+ read() supports partial reads at non-zero offsets in the usual
+ bin_attribute manner.
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 35e911f1ad78..8df9feb030aa 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -163,7 +163,8 @@ struct gpio_reg {
u32 gpio1;
u32 __pad0;
u32 gpio2;
- u32 __pad1;
+ /* adva_x1: I2C bus ownership register; reserved on other variants */
+ u32 i2c_bus_ctrl;
};
struct irig_master_reg {
@@ -416,6 +417,11 @@ struct ptp_ocp {
dpll_tracker tracker;
int signals_nr;
int freq_in_nr;
+ /* cpld_i2c_xfer sysfs (adva_x1) */
+ struct mutex tap_i2c_lock;
+ int tap_i2c_adap_nr; /* adapter nr; -1 if absent */
+ u8 tap_i2c_rsp[21]; /* [status, read_data...] */
+ size_t tap_i2c_rsp_len;
};
#define OCP_REQ_TIMESTAMP BIT(0)
@@ -3188,6 +3194,12 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
ptp_ocp_nmea_out_init(bp);
ptp_ocp_signal_init(bp);
+ /* tap_i2c is x1-only; the base adva variant has no CPLD I2C path. */
+ if (info->attr_groups == adva_timecard_x1_groups) {
+ mutex_init(&bp->tap_i2c_lock);
+ bp->tap_i2c_adap_nr = -1;
+ }
+
err = ptp_ocp_attr_group_add(bp, info->attr_groups);
if (err)
return err;
@@ -4224,6 +4236,210 @@ static const struct ocp_attr_group art_timecard_groups[] = {
{ },
};
+/*
+ * i2c_bus_ctrl exposes the MicroBlaze I2C bus arbitration register.
+ *
+ * The shared bus requires a three-step handshake before use:
+ * read 0x00000000 (free) -> write 0x0000ffff (request) ->
+ * poll until 0xffffffff (MicroBlaze confirms release).
+ *
+ * The poll is a PCIe non-posted read, which also flushes the preceding
+ * posted write to the FPGA, so no separate kernel read-back is needed.
+ */
+static ssize_t
+i2c_bus_ctrl_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct ptp_ocp *bp = dev_get_drvdata(dev);
+
+ if (!bp->pps_select)
+ return -ENODEV;
+ return sysfs_emit(buf, "0x%08x\n",
+ ioread32(&bp->pps_select->i2c_bus_ctrl));
+}
+
+static ssize_t
+i2c_bus_ctrl_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ptp_ocp *bp = dev_get_drvdata(dev);
+ u32 val;
+
+ if (!bp->pps_select)
+ return -ENODEV;
+ if (kstrtou32(buf, 0, &val))
+ return -EINVAL;
+ iowrite32(val, &bp->pps_select->i2c_bus_ctrl);
+ return count;
+}
+
+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.
+ */
+#define TAP_I2C_ALLOWED_ADDRS_NUM 2
+static const u8 tap_i2c_allowed_addrs[TAP_I2C_ALLOWED_ADDRS_NUM] = {
+ 0x40, /* CPLD */
+ 0x74, /* mux */
+};
+
+/*
+ * tap_i2c_errno_to_byte - encode a kernel errno as a one-byte status.
+ *
+ * Errnos > 255 (e.g. ENOTSUPP=524) or multiples of 256 would truncate
+ * to a wrong or zero value with a plain (u8) cast. Map those to EIO.
+ */
+static u8 tap_i2c_errno_to_byte(int err)
+{
+ int val = (err < 0) ? -err : EIO;
+
+ return (val > 0 && val <= 0xFF) ? (u8)val : EIO;
+}
+
+#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 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;
+ struct i2c_msg msgs[2];
+ u8 *rdbuf = NULL;
+ int nmsgs, ret, i;
+
+ /* Each write is one atomic request; non-zero offset means a
+ * mid-buffer pwrite() which would misparse the header.
+ */
+ if (off != 0)
+ return -EINVAL;
+ if (count < TAP_I2C_REQ_HDR_LEN || count > TAP_I2C_REQ_HDR_LEN + TAP_I2C_MAX_WRITE_LEN)
+ return -EINVAL;
+
+ 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;
+ /* I2C_M_NOSTART suppresses the repeated START between write and read
+ * segments; it has no meaning on a first-and-only message.
+ */
+ if ((flags & TAP_I2C_FLAG_NOSTART) && write_len == 0)
+ return -EINVAL;
+
+ /* i2c_get_adapter() takes a reference under core_lock; safe against
+ * concurrent adapter unbind.
+ */
+ adap = i2c_get_adapter(READ_ONCE(bp->tap_i2c_adap_nr));
+ if (!adap)
+ return -ENODEV;
+
+ 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;
+ /* Allocate a DMA-safe bounce buffer if needed; see
+ * Documentation/i2c/dma-considerations.rst.
+ */
+ rdbuf = i2c_get_dma_safe_msg_buf(&msgs[nmsgs], 1);
+ if (!rdbuf) {
+ i2c_put_adapter(adap);
+ return -ENOMEM;
+ }
+ msgs[nmsgs].buf = rdbuf;
+ nmsgs++;
+ }
+
+ /* Serialise transfer+publish so concurrent writers cannot overwrite
+ * each other's response in tap_i2c_rsp.
+ */
+ mutex_lock(&bp->tap_i2c_lock);
+ ret = i2c_transfer(adap, msgs, nmsgs);
+ 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] = tap_i2c_errno_to_byte(ret);
+ bp->tap_i2c_rsp_len = 1;
+ ret = (ret < 0) ? ret : -EIO;
+ }
+ mutex_unlock(&bp->tap_i2c_lock);
+ /* Free bounce buffer after data is copied; put adapter reference. */
+ if (read_len > 0)
+ i2c_put_dma_safe_msg_buf(rdbuf, &msgs[nmsgs - 1], ret > 0);
+ i2c_put_adapter(adap);
+
+ return ret;
+}
+
+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;
+
+ mutex_lock(&bp->tap_i2c_lock);
+ if (off >= bp->tap_i2c_rsp_len) {
+ ret = 0;
+ } else {
+ ret = min(count, bp->tap_i2c_rsp_len - (size_t)off);
+ memcpy(buf, bp->tap_i2c_rsp + off, ret);
+ }
+ mutex_unlock(&bp->tap_i2c_lock);
+ return ret;
+}
+
+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,
+};
+
static struct attribute *adva_timecard_attrs[] = {
&dev_attr_serialnum.attr,
&dev_attr_gnss_sync.attr,
@@ -4272,11 +4488,18 @@ static struct attribute *adva_timecard_x1_attrs[] = {
&dev_attr_ts_window_adjust.attr,
&dev_attr_utc_tai_offset.attr,
&dev_attr_tod_correction.attr,
+ &dev_attr_i2c_bus_ctrl.attr,
+ NULL,
+};
+
+static const struct bin_attribute *const bin_adva_x1_timecard_attrs[] = {
+ &tap_i2c_bin_attr,
NULL,
};
static const struct attribute_group adva_timecard_x1_group = {
- .attrs = adva_timecard_x1_attrs,
+ .attrs = adva_timecard_x1_attrs,
+ .bin_attrs = bin_adva_x1_timecard_attrs,
};
static const struct ocp_attr_group adva_timecard_x1_groups[] = {
@@ -4902,6 +5125,8 @@ ptp_ocp_detach(struct ptp_ocp *bp)
clk_hw_unregister_fixed_rate(bp->i2c_clk);
if (bp->n_irqs)
pci_free_irq_vectors(bp->pdev);
+ if (bp->tap_i2c_adap_nr != 0) /* initialised only on adva_x1 */
+ mutex_destroy(&bp->tap_i2c_lock);
device_unregister(&bp->dev);
}
@@ -5217,11 +5442,16 @@ ptp_ocp_i2c_notifier_call(struct notifier_block *nb,
found:
bp = dev_get_drvdata(dev);
- if (add)
+ if (add) {
ptp_ocp_symlink(bp, child, "i2c");
- else
+ /* Cache adapter number; cpld_i2c_xfer uses i2c_get_adapter()
+ * for a reference-counted, unbind-safe lookup.
+ */
+ WRITE_ONCE(bp->tap_i2c_adap_nr, i2c_verify_adapter(child)->nr);
+ } else {
+ WRITE_ONCE(bp->tap_i2c_adap_nr, -1); /* invalidate before free */
sysfs_remove_link(&bp->dev.kobj, "i2c");
-
+ }
return 0;
}
--
2.47.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v4 1/1] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1
2026-07-08 6:56 [PATCH v4 1/1] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1 Sagi Maimon
@ 2026-07-15 11:15 ` Simon Horman
0 siblings, 0 replies; 2+ messages in thread
From: Simon Horman @ 2026-07-15 11:15 UTC (permalink / raw)
To: maimon.sagi
Cc: Simon Horman, jonathan.lemon, vadim.fedorenko, richardcochran,
andrew+netdev, davem, edumazet, kuba, pabeni, linux-kernel,
netdev
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 X1
This commit adds sysfs interfaces on the ADVA TimeCard X1 (PCI 0x0410) to
program a Lattice MachXO3 CPLD over a shared I2C bus. It exposes an
i2c_bus_ctrl attribute for arbitrating the bus against the on-board
MicroBlaze, and a cpld_i2c_xfer binary attribute that acts as an I2C
passthrough restricted to slave addresses 0x40 and 0x74.
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 35e911f1ad78..8df9feb030aa 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
[ ... ]
> @@ -416,6 +417,11 @@ struct ptp_ocp {
> dpll_tracker tracker;
> int signals_nr;
> int freq_in_nr;
> + /* cpld_i2c_xfer sysfs (adva_x1) */
> + struct mutex tap_i2c_lock;
> + int tap_i2c_adap_nr; /* adapter nr; -1 if absent */
> + u8 tap_i2c_rsp[21]; /* [status, read_data...] */
> + size_t tap_i2c_rsp_len;
> };
[ ... ]
> @@ -3188,6 +3194,12 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
> ptp_ocp_nmea_out_init(bp);
> ptp_ocp_signal_init(bp);
>
> + /* tap_i2c is x1-only; the base adva variant has no CPLD I2C path. */
> + if (info->attr_groups == adva_timecard_x1_groups) {
> + mutex_init(&bp->tap_i2c_lock);
> + bp->tap_i2c_adap_nr = -1;
> + }
> +
[Critical]
Could this initialization order leave cpld_i2c_xfer non-functional in the
common configuration where xiic-i2c is already loaded when ptp_ocp probes?
The adva_x1 resource table places OCP_I2C_RESOURCE(i2c_ctrl) before the
ptp_ocp_adva_board_init entry, and ptp_ocp_register_resources iterates the
table sequentially. If xiic-i2c is built-in or preloaded,
platform_device_register_resndata() inside ptp_ocp_register_i2c() will run
the xiic-i2c probe synchronously:
ptp_ocp_register_i2c()
platform_device_register_resndata()
xiic-i2c probe -> i2c_add_adapter() -> device_add()
BUS_NOTIFY_ADD_DEVICE -> ptp_ocp_i2c_notifier_call()
WRITE_ONCE(bp->tap_i2c_adap_nr, ...->nr);
Control then returns to ptp_ocp_register_resources() which invokes
ptp_ocp_adva_board_init(), and the x1 branch above overwrites the just
cached number with -1.
No further BUS_NOTIFY_ADD_DEVICE fires for the same adapter, so
tap_i2c_adap_nr stays -1 for the device lifetime and every cpld_i2c_xfer
write() falls through i2c_get_adapter(-1) returning NULL and reports
-ENODEV.
Should tap_i2c_adap_nr be initialised earlier (before resource
registration) so the notifier's write is not clobbered?
> err = ptp_ocp_attr_group_add(bp, info->attr_groups);
> if (err)
> return err;
[ ... ]
> @@ -4224,6 +4236,210 @@ static const struct ocp_attr_group art_timecard_groups[] = {
[ ... ]
> +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)
> +{
[ ... ]
> + 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;
> + /* Allocate a DMA-safe bounce buffer if needed; see
> + * Documentation/i2c/dma-considerations.rst.
> + */
> + rdbuf = i2c_get_dma_safe_msg_buf(&msgs[nmsgs], 1);
> + if (!rdbuf) {
> + i2c_put_adapter(adap);
> + return -ENOMEM;
> + }
> + msgs[nmsgs].buf = rdbuf;
> + nmsgs++;
> + }
[High]
Can this leak the bounce buffer returned by i2c_get_dma_safe_msg_buf()?
Because I2C_M_DMA_SAFE is never set on this msg, i2c_get_dma_safe_msg_buf()
always returns a fresh kzalloc'd buffer. The code then aliases msg->buf
to that buffer:
rdbuf = i2c_get_dma_safe_msg_buf(&msgs[nmsgs], 1);
...
msgs[nmsgs].buf = rdbuf;
Later i2c_put_dma_safe_msg_buf(rdbuf, ..., ret > 0) is called, but that
helper begins with:
void i2c_put_dma_safe_msg_buf(u8 *buf, struct i2c_msg *msg, bool xferred)
{
if (!buf || buf == msg->buf)
return;
...
kfree(buf);
}
Since buf == msg->buf here, it returns without kfree().
In-tree callers (for example i2c-at91-master.c, mxs_i2c_xfer_msg,
mtk_i2c_do_transfer) keep msg->buf pointing at the caller's original
buffer and hold the bounce pointer in a separate driver variable so
buf != msg->buf at release time.
Would using a separate driver-owned pointer for the bounce buffer, or
setting msgs[nmsgs].flags |= I2C_M_DMA_SAFE and freeing with plain
kfree(), avoid the leak?
> +
> + /* Serialise transfer+publish so concurrent writers cannot overwrite
> + * each other's response in tap_i2c_rsp.
> + */
> + mutex_lock(&bp->tap_i2c_lock);
[ ... ]
> @@ -4902,6 +5125,8 @@ ptp_ocp_detach(struct ptp_ocp *bp)
> clk_hw_unregister_fixed_rate(bp->i2c_clk);
> if (bp->n_irqs)
> pci_free_irq_vectors(bp->pdev);
> + if (bp->tap_i2c_adap_nr != 0) /* initialised only on adva_x1 */
> + mutex_destroy(&bp->tap_i2c_lock);
> device_unregister(&bp->dev);
> }
[High]
Is this sentinel test reliable for deciding whether the mutex was
initialised?
mutex_init(&bp->tap_i2c_lock) only runs on adva_x1 in
ptp_ocp_adva_board_init(), but ptp_ocp_i2c_notifier_call() has no variant
filter — it walks up dev->parent looking for any driver named
KBUILD_MODNAME, matching fb, art and base adva just the same:
found:
bp = dev_get_drvdata(dev);
if (add) {
...
WRITE_ONCE(bp->tap_i2c_adap_nr, i2c_verify_adapter(child)->nr);
} else {
WRITE_ONCE(bp->tap_i2c_adap_nr, -1);
...
}
Every non-x1 board that has an OCP_I2C_RESOURCE will end up with
tap_i2c_adap_nr set to -1 (or any adapter number != 0) by the notifier,
so mutex_destroy(&bp->tap_i2c_lock) is then invoked on a never-initialised
mutex. With CONFIG_DEBUG_MUTEXES=y this runs DEBUG_LOCKS_WARN_ON() over
uninitialised memory, and under PREEMPT_RT mutex_destroy becomes a real
function that reads rt_mutex fields.
There is also the reverse case: adapter nr 0 is a legitimate I2C bus
number, so a genuine adva_x1 whose adapter happens to be 0 would skip
mutex_destroy even though the mutex was initialised.
Would a dedicated boolean flag set alongside mutex_init(), or gating on
info->attr_groups == adva_timecard_x1_groups explicitly, be a safer
condition?
> @@ -5217,11 +5442,16 @@ ptp_ocp_i2c_notifier_call(struct notifier_block *nb,
>
> found:
> bp = dev_get_drvdata(dev);
> - if (add)
> + if (add) {
> ptp_ocp_symlink(bp, child, "i2c");
> - else
> + /* Cache adapter number; cpld_i2c_xfer uses i2c_get_adapter()
> + * for a reference-counted, unbind-safe lookup.
> + */
> + WRITE_ONCE(bp->tap_i2c_adap_nr, i2c_verify_adapter(child)->nr);
> + } else {
> + WRITE_ONCE(bp->tap_i2c_adap_nr, -1); /* invalidate before free */
> sysfs_remove_link(&bp->dev.kobj, "i2c");
> -
> + }
> return 0;
> }
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-07-15 11:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-08 6:56 [PATCH v4 1/1] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1 Sagi Maimon
2026-07-15 11:15 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox