Re: [PATCH v21 3/4] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] i3c: hub: p3h2x4x: uninitialized P3H2X4X_TP_NET_CON_CONF breaks
  port isolation
- [High] i3c: hub: p3h2x4x: parent DAA reconciler clobbers DT address
  reservations
- [High] i3c: hub: p3h2x4x: broken SMBus functionality in
  p3h2x4x_tp_smbus_funcs()
- [High] i3c: hub: p3h2x4x: ports disconnected during probe in
  p3h2x4x_tp_i3c_algo()
- [Medium] i3c: hub: p3h2x4x: combined transfer check in
  p3h2x4x_tp_i2c_xfer()
--

commit 9562a1f0e3515b7b354e8a193339c5c2e1630c85
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>

i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality

This commit adds the I3C hub driver for the NXP P3H2x4x family, supporting
downstream target ports configured as virtual I3C buses or SMBus adapters.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> new file mode 100644
> index 0000000000000..a070be32ffed3
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> @@ -0,0 +1,162 @@

[ ... ]

> +static void p3h2x4x_hub_enable_port(struct i3c_master_controller *controller)
> +{
> +	struct tp_bus *bus = p3h2x4x_bus_from_controller(controller);
> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = bus->p3h2x4x_i3c_hub;
> +
> +	if (p3h2x4x_i3c_hub->hub_config.tp_config[bus->tp_port].always_enable)
> +		return;
> +
> +	regmap_set_bits(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP_NET_CON_CONF, bus->tp_mask);
> +}
> +
> +static void p3h2x4x_hub_disable_port(struct i3c_master_controller *controller)
> +{
> +	struct tp_bus *bus = p3h2x4x_bus_from_controller(controller);
> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = bus->p3h2x4x_i3c_hub;
> +
> +	if (p3h2x4x_i3c_hub->hub_config.tp_config[bus->tp_port].always_enable)
> +		return;
> +
> +	regmap_clear_bits(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP_NET_CON_CONF, bus->tp_mask);
> +}

[Severity: High]
Does leaving P3H2X4X_TP_NET_CON_CONF uninitialized before the registration
loop break per-port isolation?

p3h2x4x_hub_enable_port() and p3h2x4x_hub_disable_port() only set and clear
bus->tp_mask, and neither p3h2x4x_configure_tp() nor p3h2x4x_tp_i3c_algo()
clears P3H2X4X_TP_NET_CON_CONF beforehand or on teardown.  If any bits are
already set in P3H2X4X_TP_NET_CON_CONF (for example on driver rebind, or on
P3H2441/P3H2841 where target ports default to connected at power-on),
multiple ports stay bridged onto the internal network during
i3c_master_register_fwnode(), exposing other ports' devices to SETDASA and
ENTDAA meant for a single port.

[ ... ]

> +	for (tp = 0; tp < p3h2x4x->num_target_ports; tp++) {
> +		if (!p3h2x4x_hub->tp_bus[tp].of_node ||
> +		    p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C)
> +			continue;
> +
> +		struct i3c_hub_controller *hub_controller =
> +				&p3h2x4x_hub->tp_bus[tp].hub_controller;
> +		struct i3c_master_controller *controller = &hub_controller->controller;
> +
> +		hub_controller->parent = parent;
> +		hub_controller->hub = p3h2x4x_hub->hub;
> +
> +		dev_set_drvdata(&controller->dev, hub_controller);
> +
> +		ret = i3c_hub_reserve_parent_addrslots_from_dt(hub_controller,
> +							       p3h2x4x_hub->tp_bus[tp].of_node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (tp = 0; tp < p3h2x4x->num_target_ports; tp++) {
> +		if (!p3h2x4x_hub->tp_bus[tp].of_node ||
> +		    p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C)
> +			continue;
> +
> +		struct i3c_master_controller *controller =
> +				&p3h2x4x_hub->tp_bus[tp].hub_controller.controller;
> +
> +		ret = i3c_master_register_fwnode(controller,
> +						 p3h2x4x_hub->dev,
> +						 of_fwnode_handle(p3h2x4x_hub->tp_bus[tp].of_node),
> +						 &i3c_hub_master_ops,
> +						 false);

[Severity: High]
Won't i3c_master_reconcile_dyn_addrs() on the parent bus clobber the
reservations made by i3c_hub_reserve_parent_addrslots_from_dt()?

i3c_hub_reserve_parent_addrslots_from_dt() marks each child's
assigned-address on parent->bus as I3C_ADDR_SLOT_I3C_DEV without a matching
descriptor in parent->bus.devs.i3c.  When the first port runs
i3c_master_register_fwnode() -> ... -> i3c_hub_do_daa() ->
i3c_master_do_daa(parent), i3c_master_reconcile_dyn_addrs(parent) resets
every I3C_ADDR_SLOT_I3C_DEV slot not in parent->bus.devs.i3c to
I3C_ADDR_SLOT_FREE and probes it:

drivers/i3c/master.c:i3c_master_reconcile_dyn_addrs() {
    ...
		if (status != I3C_ADDR_SLOT_I3C_DEV || test_bit(addr, dev_dyn_addrs))
			continue;
		i3c_bus_set_addr_slot_status(&master->bus, addr, I3C_ADDR_SLOT_FREE);
		/* Try to add the device, but probe to see if it is really present */
		__i3c_master_add_i3c_dev_locked(master, addr, true);
    ...
}

For later ports that are still disconnected, the probe sees no device and
leaves the slot I3C_ADDR_SLOT_FREE, wiping out their reservations.  For
devices on the connected port that already got their address via SETDASA on
controller->bus, the probe succeeds and registers a duplicate i3c_device on
parent->bus.

> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(p3h2x4x_hub->dev,
> +					       p3h2x4x_unregister_i3c_master,
> +					       controller);
> +		if (ret)
> +			return ret;
> +
> +		ntwk_mask |= p3h2x4x_hub->tp_bus[tp].tp_mask;
> +		p3h2x4x_hub->tp_bus[tp].is_registered = true;
> +	}
> +
> +	/*
> +	 * Take the routing lock used by the hub callbacks so they cannot
> +	 * observe always_enable before the route is programmed.
> +	 */
> +	mutex_lock(&p3h2x4x_hub->hub->lock);
> +
> +	ret = regmap_write(p3h2x4x_hub->regmap,
> +			   P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
> +	if (!ret) {
> +		for (tp = 0; tp < p3h2x4x->num_target_ports; tp++) {
> +			if (!(ntwk_mask & p3h2x4x_hub->tp_bus[tp].tp_mask))
> +				continue;
> +
> +			p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
> +		}
> +	}
> +	mutex_unlock(&p3h2x4x_hub->hub->lock);

[Severity: High]
Does keeping always_enable false until all ports are registered leave target
ports disconnected while downstream device drivers probe?

CPU0                                  kworker (parent->wq)
p3h2x4x_tp_i3c_algo()
  i3c_master_register_fwnode()
    ...
      i3c_hub_do_daa()
        i3c_master_do_daa(parent)
          queue_work(..., &parent->reg_work)
        i3c_hub_disable_port(controller)
          regmap_clear_bits(..., tp_mask)
                                      i3c_master_reg_work_fn()
                                        device_add(&i3cdev->dev)
                                          <target driver probe>
                                            i3c_device_do_priv_xfers()
                                              <-- port is disconnected
  /* next port's i3c_master_register_fwnode() */
  regmap_write(..., P3H2X4X_TP_NET_CON_CONF, ntwk_mask)

Devices discovered via ENTDAA are queued on parent->reg_work and use
parent->ops directly without calling p3h2x4x_hub_enable_port(), so their
probes fail if they run before P3H2X4X_TP_NET_CON_CONF is written at the
end of p3h2x4x_tp_i3c_algo().  Likewise, for statically described devices on
controller->bus that call i3c_device_enable_ibi() during
i3c_master_register_fwnode(), i3c_hub_enable_ibi() disconnects the port on
exit, dropping IBIs until all remaining ports finish registering.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> new file mode 100644
> index 0000000000000..247d0f4883b7c
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> @@ -0,0 +1,352 @@

[ ... ]

> +static s32 p3h2x4x_tp_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	int ret_sum = 0, ret, msg_count;
> +	u8 rw;
> +
> +	struct tp_bus *bus = i2c_get_adapdata(adap);
> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = bus->p3h2x4x_i3c_hub;
> +
> +	guard(mutex)(&p3h2x4x_i3c_hub->etx_mutex);
> +	guard(mutex)(&bus->port_mutex);
> +
> +	for (msg_count = 0; msg_count < num; msg_count++) {
> +		rw = (msgs[msg_count].flags & I2C_M_RD) ? 1 : 0;
> +		if (!rw) {
> +			/* If a write message is immediately followed by a read message to
> +			 * the same address,  consider combining them into a single transaction.
> +			 */
> +			if (msg_count + 1 < num &&
> +			    msgs[msg_count].addr == msgs[msg_count + 1].addr &&
> +			    (msgs[msg_count + 1].flags & I2C_M_RD)) {
> +				if (msgs[msg_count].len + msgs[msg_count + 1].len >
> +				    P3H2X4X_SMBUS_PAYLOAD_SIZE)
> +					return -EINVAL;

[Severity: Medium]
Should this fall back to sending the write and read as separate transactions
(or set max_comb_1st_msg_len and max_comb_2nd_msg_len in
p3h2x4x_tp_i2c_quirks) instead of returning -EINVAL?

Because p3h2x4x_tp_i2c_quirks advertises max_read_len = 84 and
max_write_len = 84 with no combined limit, callers such as regmap-i2c issue
a 1- or 2-byte register write followed by an 84-byte read; both messages
pass i2c_check_for_quirks(), but their sum exceeds
P3H2X4X_SMBUS_PAYLOAD_SIZE (84) and fails here with -EINVAL.

> +
> +				rw = 2;
> +				msg_count += 1;
> +				ret_sum += 1;
> +			}
> +		}
> +
> +		ret = p3h2x4x_tp_i2c_xfer_msg(p3h2x4x_i3c_hub,
> +					      msgs,
> +					      bus->tp_port,
> +					      (rw == 2) ? (msg_count - 1) : msg_count,
> +					       rw);
> +		if (ret)
> +			return ret;
> +
> +		ret_sum++;
> +	}
> +	return ret_sum;
> +}
> +
> +static u32 p3h2x4x_tp_smbus_funcs(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BLOCK_DATA;
> +}

[Severity: High]
Does advertising I2C_FUNC_SMBUS_BLOCK_DATA without handling I2C_M_RECV_LEN
in p3h2x4x_tp_i2c_xfer_msg() corrupt SMBus block reads and leak stack memory?

Because p3h2x4x_tp_i2c_algorithm has no .smbus_xfer callback,
i2c_smbus_read_block_data() and I2C_SMBUS_BLOCK_DATA read ioctls go through
i2c_smbus_xfer_emulated(), which sets msg[1].flags |= I2C_M_RECV_LEN and
msg[1].len = 1.  p3h2x4x_tp_i2c_xfer_msg() ignores I2C_M_RECV_LEN and reads
only 1 byte (the block length N) into msg[1].buf[0], after which
i2c_smbus_xfer_emulated() copies N unwritten bytes from msg[1].buf[1..N] to
the caller (zeroes from the bounce buffer, or uninitialized stack bytes from
msgbuf1 if kzalloc() in i2c_smbus_try_get_dmabuf() fails).

Also, omitting I2C_FUNC_SMBUS_EMUL leaves standard byte, word, and I2C-block
SMBus operations unadvertised, preventing SMBus and hwmon drivers from using
the adapter.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112124437.3367252-1-lakshay.piplani@nxp.com?part=3

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c