Re: [PATCH v21 4/4] i3c: hub: p3h2x4x: Add SMBus slave mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] i3c: hub: p3h2x4x: empty rx buffer not rejected in
  p3h2x4x_read_smbus_agent_rx_buf()
- [Medium] i3c: hub: p3h2x4x: stale rx flags not cleared in
  p3h2x4x_tp_i2c_reg_slave()
- [Low] i3c: hub: p3h2x4x: missing I2C_CLIENT_TEN check in
  p3h2x4x_tp_i2c_reg_slave()
--

commit c82ca6aaf774f0de048a84b1a2c414e57c266eef
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

i3c: hub: p3h2x4x: Add SMBus slave mode support

This commit adds SMBus slave mode support for the P3H2x4x hub target ports,
handling receive events via I3C IBI and forwarding received payloads to
registered I2C slave clients.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> index 247d0f4883b7c..02b50d1eb938d 100644
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> @@ -3,6 +3,7 @@
>   * Copyright 2025-2026 NXP
>   * This P3H2X4X driver file contain functions for SMBus/I2C virtual Bus creation and read/write.
>   */
> +#include <linux/i3c/device.h>
>  #include <linux/mfd/p3h2840.h>
>  #include <linux/regmap.h>
>  
> @@ -15,6 +16,171 @@ enum p3h2x4x_smbus_desc_idx {
>  	P3H2X4X_DESC_READ_LEN,
>  };
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void p3h2x4x_read_smbus_agent_rx_buf(struct i3c_device *i3cdev, enum p3h2x4x_rcv_buf rfbuf,
> +					    enum p3h2x4x_tp tp)
> +{
> +	struct p3h2x4x *p3h2x4x = i3cdev_get_drvdata(i3cdev);
> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub;
> +	u8 target_buffer_page, flag_clear, temp = 0, i, addr;
> +	u32 packet_len, slave_address;
> +	struct i2c_client *client;
> +	u8 *slave_rx_buffer;
> +	int ret;
> +
> +	if (!p3h2x4x || !p3h2x4x->i3c_hub_priv)
> +		return;
> +
> +	p3h2x4x_i3c_hub = p3h2x4x->i3c_hub_priv;
> +	slave_rx_buffer = p3h2x4x_i3c_hub->slave_rx_buffer;
> +
> +	switch (rfbuf) {
> +	case RCV_BUF_0:
> +		target_buffer_page = P3H2X4X_TARGET_BUFF_0_PAGE;
> +		flag_clear = P3H2X4X_TARGET_BUF_0_RECEIVE;
> +		break;
> +	case RCV_BUF_1:
> +		target_buffer_page = P3H2X4X_TARGET_BUFF_1_PAGE;
> +		flag_clear = P3H2X4X_TARGET_BUF_1_RECEIVE;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	target_buffer_page += P3H2X4X_NO_PAGE_PER_TP * tp;
> +
> +	ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_PAGE_PTR, target_buffer_page);
> +	if (ret)
> +		goto ibi_err;
> +
> +	/* read buffer length */
> +	ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TARGET_BUFF_LENGTH, &packet_len);
> +	if (ret)
> +		goto ibi_err;
> +
> +	if (packet_len)
> +		packet_len = packet_len - 1;

[Severity: Medium]
Should p3h2x4x_read_smbus_agent_rx_buf() jump to ibi_err when packet_len is
0?

P3H2X4X_TARGET_BUFF_LENGTH includes the 1-byte target address, so a valid
zero-payload transfer has length 1 before decrementing.  When
P3H2X4X_TARGET_BUFF_LENGTH reads as 0 (for example, on an aborted transfer
or empty buffer), falling through with packet_len == 0 reads a stale
P3H2X4X_TARGET_BUFF_ADDRESS from a prior transaction and delivers spurious
I2C_SLAVE_WRITE_REQUESTED and I2C_SLAVE_STOP events to the slave client.

[ ... ]

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int p3h2x4x_tp_i2c_reg_slave(struct i2c_client *slave)
> +{
> +	struct tp_bus *bus = i2c_get_adapdata(slave->adapter);
> +	struct p3h2x4x_i3c_hub_dev *hub = bus->p3h2x4x_i3c_hub;
> +	struct p3h2x4x *p3h2x4x = dev_get_drvdata(hub->dev->parent);
> +	int relock_ret = 0;
> +	int ret = 0;
> +
> +	guard(mutex)(&hub->etx_mutex);
> +
> +	if (!hub->i3cdev || !hub->ibi_ready)
> +		return -EOPNOTSUPP;
> +
> +	if (bus->tp_smbus_client)
> +		return -EBUSY;

[Severity: Low]
Should p3h2x4x_tp_i2c_reg_slave() reject 10-bit addressed clients
(slave->flags & I2C_CLIENT_TEN) with -EAFNOSUPPORT?

i2c_slave_register() skips 7-bit address validation when I2C_CLIENT_TEN is
set and delegates the check to reg_slave().  Without it, registration
succeeds and p3h2x4x_read_smbus_agent_rx_buf() truncates client->addr to its
low 7 bits when matching against P3H2X4X_TARGET_BUFF_ADDRESS.

> +
> +	scoped_guard(mutex, &p3h2x4x->protected_reg_lock) {
> +		/* Unlock access to protected registers */
> +		ret = regmap_write(hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> +				   P3H2X4X_REGISTERS_UNLOCK_CODE);
> +		if (ret)
> +			break;
> +
> +		ret = regmap_set_bits(hub->regmap,
> +				      P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
> +				      bus->tp_mask);
> +
> +		/* Lock access to protected registers */
> +		relock_ret = regmap_write(hub->regmap,
> +					  P3H2X4X_DEV_REG_PROTECTION_CODE,
> +					  P3H2X4X_REGISTERS_LOCK_CODE);
> +	}

[Severity: Medium]
Does p3h2x4x_tp_i2c_reg_slave() need to clear BUF_RECEIVED_FLAG_MASK in
P3H2X4X_TP0_SMBUS_AGNT_STS + bus->tp_port before enabling the port IBI?

p3h2x4x_configure_tp() enables P3H2X4X_TP_SMBUS_AGNT_EN at probe time, and
p3h2x4x_tp_i2c_unreg_slave() only masks the port bit in
P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG.  Any SMBus write arriving on the port
while no slave is registered fills the target buffers and sets the receive
and overflow status flags without triggering p3h2x4x_ibi_handler().  Leaving
those flags set across registration either delivers stale payloads to the
new slave client or leaves both target buffers permanently occupied if an
IBI requires a fresh status transition.

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

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