Re: [PATCH v13 7/7] i3c: hub: p3h2x4x: Add SMBus slave mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Early error paths in `p3h2x4x_read_smbus_agent_rx_buf` fail to clear the hardware buffer receive flag, resulting in an infinite In-Band Interrupt (IBI) storm.
- [High] Hardware IBI configuration is unconditionally overwritten after exposing I2C adapters, potentially silently disabling slave interrupts.
- [Medium] The `P3H2X4X_TARGET_BUF_CA_TF` case in the IBI handler is unreachable because its value is masked out by the switch expression.
--

commit f1791953bf7e067a7ec560f25d66e9566565ed9b
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.
It allows the hub SMBus slave agent to receive downstream payloads and
report receive events through In-Band Interrupts (IBI). The payloads are
then forwarded to the registered I2C slave client.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> index fd000ff635077..f752cbe35c482 100644
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> @@ -15,6 +15,157 @@ enum p3h2x4x_smbus_desc_idx {
[ ... ]
> +static void p3h2x4x_read_smbus_agent_rx_buf(struct i3c_device *i3cdev, enum p3h2x4x_rcv_buf rfbuf,
> +					    enum p3h2x4x_tp tp, bool is_of)
> +{
> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = i3cdev_get_drvdata(i3cdev);
> +	u8 slave_rx_buffer[P3H2X4X_SMBUS_TARGET_PAYLOAD_SIZE] = { 0 };
> +	u8 target_buffer_page, flag_clear = 0, temp = 0, i, addr;
> +	u32 packet_len, slave_address, ret;
> +	struct i2c_client *client;
[ ... ]
> +	/* 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;
> +
> +	if (packet_len > P3H2X4X_SMBUS_TARGET_PAYLOAD_SIZE) {
> +		dev_err(&i3cdev->dev, "Received message too big for p3h2x4x buffer\n");
> +		goto ibi_err;

[Severity: High]
Since flag_clear is initialized to 0, if an error path like this one jumps
to the ibi_err label early, will it skip setting flag_clear entirely?

If flag_clear remains 0, the cleanup code below won't write to the
P3H2X4X_TP0_SMBUS_AGNT_STS register to clear the hardware interrupt buffer
receive flag. Could this cause the unserviced buffer status to remain active
in the hardware and result in an continuous IBI interrupt storm?

> +	}
[ ... ]
> +ibi_err:
> +	regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_PAGE_PTR, 0x00);
> +
> +	if (flag_clear)
> +		regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + tp, flag_clear);
> +}
[ ... ]
> +void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
> +			 const struct i3c_ibi_payload *payload)
> +{
[ ... ]
> +		ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + i,
> +				  &target_port_status);
> +		if (ret) {
> +			dev_err(&i3cdev->dev, "target port read status failed %d\n", ret);
> +			continue;
> +		}
> +
> +		/* process data receive buffer */
> +		switch (target_port_status & BUF_RECEIVED_FLAG_MASK) {
> +		case P3H2X4X_TARGET_BUF_CA_TF:

[Severity: Medium]
Is this case reachable?

If BUF_RECEIVED_FLAG_MASK is defined as 0xE (GENMASK(3, 1)) and
P3H2X4X_TARGET_BUF_CA_TF is defined as 0x1 (BIT(0)), a bitwise AND with 0xE
will always clear bit 0.

Will the switch expression ever evaluate to 0x1, or does this unintentionally
fall through to the default case which clears all bits by writing
BUF_RECEIVED_FLAG_TF_MASK?

> +			break;
> +		case P3H2X4X_TARGET_BUF_0_RECEIVE:
[ ... ]
> +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;
> +	int ret;
> +
> +	guard(mutex)(&hub->etx_mutex);
> +
> +	if (!hub->i3cdev || !hub->ibi_ready)
> +		return -EOPNOTSUPP;
> +
> +	if (bus->tp_smbus_client)
> +		return -EBUSY;
> +
> +	ret = regmap_set_bits(hub->regmap,
> +			      P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
> +			      bus->tp_mask);
> +	if (ret)
> +		return ret;
> +
> +	bus->tp_smbus_client = slave;
> +	hub->hub_config.tp_config[bus->tp_port].ibi_en = true;
> +
> +	return 0;
> +}

[Severity: High]
When p3h2x4x_tp_smbus_algo() loops over target ports and exposes I2C adapters
via i2c_add_adapter(), this can trigger synchronous driver binding. A slave
driver probing could call i2c_slave_register(), which calls this function to
enable its IBI by setting a bit in P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG.

However, if p3h2x4x_tp_smbus_algo() unconditionally writes
P3H2X4X_IBI_DISABLED (0) to P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG after the adapter
registration loop finishes, will this silently overwrite and clear the
interrupt configurations just set by any concurrently probing slave drivers?

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