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

On Wed, Jul 01, 2026 at 07:21:00AM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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?

Is it true?

>
> > +	}
> [ ... ]
> > +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?

Make sense, please check, look like need use FIELD_GET()

Frank
>
> > +			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/[email protected]?part=7

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c