Re: [PATCH v4 1/1] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1

[Simon Horman <horms@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.
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;
>  }