Re: [PATCH v4 5/8] misc: amd-sbi: Add support for SB-TSI over I3C

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Concurrent hwmon sysfs accesses cause data races on the shared I3C DMA buffers (`priv->tx` and `priv->rx`) due to missing locking.
- [Medium] The I3C transmit buffer `tx` in `struct sbtsi_i3c_priv` is not cacheline aligned, violating DMA mapping requirements.

Pre-existing issues:
- [High] Temperature register reads are performed non-atomically without a device lock, breaking the SB-TSI hardware latching mechanism on concurrent accesses.
--

--- Patch [5]: [PATCH v4 5/8] misc: amd-sbi: Add support for SB-TSI over I3C ---
commit f466305c592ff454f903d27fc9a9ef283f005d66
Author: Prathima <Prathima.Lk@amd.com>

misc: amd-sbi: Add support for SB-TSI over I3C

This commit extends the AMD SB-TSI driver to support both I2C and I3C bus
interfaces. It selects the appropriate transport based on the probed bus type
and registers both drivers using module_i3c_i2c_driver() while maintaining
backward compatibility with existing I2C deployments.

> diff --git a/drivers/misc/amd-sbi/tsi-core.c b/drivers/misc/amd-sbi/tsi-core.c
> index 6ef1831515bbb..1c6f37f26d945 100644
> --- a/drivers/misc/amd-sbi/tsi-core.c
> +++ b/drivers/misc/amd-sbi/tsi-core.c
[ ... ]
> @@ -23,8 +28,56 @@ static int sbtsi_i2c_xfer(struct sbtsi_data *data, u8 reg, u8 *val, bool is_read
>  	return i2c_smbus_write_byte_data(data->client, reg, *val);
>  }
>  
> +/* I3C read transfer function */
> +static int sbtsi_i3c_read(struct sbtsi_data *data, u8 reg, u8 *val)
> +{
> +	struct sbtsi_i3c_priv *priv = to_sbtsi_i3c_priv(data);
> +	struct i3c_xfer xfers[2] = { };
> +	int ret;
> +
> +	priv->tx[0] = reg;

[Severity: High]
Does this sequence need synchronization to protect the shared priv->tx and 
priv->rx buffers?

The hwmon subsystem does not serialize reads and writes to sysfs attributes.
Without a lock protecting these buffers during the bus transfer, concurrent
sysfs reads from multiple threads could race, causing threads to overwrite
the transmit payload or read incorrect response values before or after
i3c_device_do_xfers() completes.

> +
> +	/* Write the register address (DMA_TO_DEVICE). */
> +	xfers[0].rnw = false;
> +	xfers[0].len = 1;
> +	xfers[0].data.out = priv->tx;
[ ... ]
> +/* I3C write transfer function */
> +static int sbtsi_i3c_write(struct sbtsi_data *data, u8 reg, u8 val)
> +{
> +	struct sbtsi_i3c_priv *priv = to_sbtsi_i3c_priv(data);
> +	struct i3c_xfer xfers = {
> +		.rnw = false,
> +		.len = 2,
> +		.data.out = priv->tx,
> +	};
> +
> +	priv->tx[0] = reg;
> +	priv->tx[1] = val;

[Severity: High]
Similarly, could concurrent sysfs writes overwrite priv->tx here before
the i3c_device_do_xfers() call finishes?

> +
> +	return i3c_device_do_xfers(data->i3cdev, &xfers, 1, I3C_SDR);
> +}
> +
> diff --git a/drivers/misc/amd-sbi/tsi-core.h b/drivers/misc/amd-sbi/tsi-core.h
> new file mode 100644
> index 0000000000000..7e8c0e7c3bcfa
> --- /dev/null
> +++ b/drivers/misc/amd-sbi/tsi-core.h
[ ... ]
> +struct sbtsi_i3c_priv {
> +	struct sbtsi_data data;
> +	u8 tx[2];
> +	u8 rx __aligned(ARCH_DMA_MINALIGN);

[Severity: Medium]
Should the tx array also have the __aligned(ARCH_DMA_MINALIGN) attribute?

Without it, tx might share a cacheline with the preceding struct sbtsi_data
fields. When tx is mapped for DMA to device transfers during I3C operations,
this violates the DMA mapping rules preventing cacheline sharing and can
trigger kernel warnings.

> +};

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of a device-level
lock around temperature register reads break the hardware latching mechanism?

Looking at drivers/hwmon/sbtsi_temp.c in sbtsi_temp_read(), the integer and
decimal registers are read sequentially:

    ret = sbtsi_xfer(data, reg1, val1, true);
    if (!ret)
        ret = sbtsi_xfer(data, reg2, val2, true);

If multiple threads concurrently access sysfs, these non-atomic transfers
could interleave on the bus. This might cause one thread to receive a
mismatched latched decimal value triggered by another thread's integer read,
resulting in corrupted temperature reports.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710111642.850022-1-Akshay.Gupta@amd.com?part=5