Re: [PATCH v3 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 2 potential issue(s) to consider:
- [High] The I3C transfer buffer shares a CPU cacheline with the device structure and other DMA mappings, causing unsafe DMA operations and potential memory corruption.
- [Low] Unmet Kconfig dependency warning triggered because `SENSORS_SBTSI` unconditionally selects `AMD_SBTSI` without inheriting its dependencies.
--

commit 6d735915bcef7f51c2d83398a0f1592836ede997
Author: Prathima <Prathima.Lk@amd.com>

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

This patch extends the SB-TSI driver to support both I2C and I3C bus
interfaces. It registers both I2C and I3C drivers using
module_i3c_i2c_driver() and updates the Kconfig dependency from I2C to
I3C_OR_I2C.

> diff --git a/drivers/misc/amd-sbi/Kconfig b/drivers/misc/amd-sbi/Kconfig
> index 512251690e0e9..1a96b71f85069 100644
> --- a/drivers/misc/amd-sbi/Kconfig
> +++ b/drivers/misc/amd-sbi/Kconfig
> @@ -23,13 +23,13 @@ config AMD_SBRMI_HWMON
>  
>  config AMD_SBTSI
>  	tristate "AMD side band TSI support"
> -	depends on I2C
> +	depends on I3C_OR_I2C

[Severity: Low]
Could this change cause unmet direct dependencies warnings if another Kconfig
symbol selects AMD_SBTSI without inheriting this new dependency?

For example, if SENSORS_SBTSI selects AMD_SBTSI, it would need to have
compatible dependencies to prevent build failures on unsupported
architectures or when I3C/I2C is disabled.

>  	depends on ARM || ARM64 || COMPILE_TEST
>  	select AUXILIARY_BUS

[ ... ]

> diff --git a/drivers/misc/amd-sbi/tsi-core.c b/drivers/misc/amd-sbi/tsi-core.c
> index 6ef1831515bbb..9278d06d8e5f6 100644
> --- a/drivers/misc/amd-sbi/tsi-core.c
> +++ b/drivers/misc/amd-sbi/tsi-core.c

[ ... ]

> @@ -23,8 +33,54 @@ 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 i3c_xfer xfers[2] = { };
> +	u8 *buf = sbtsi_i3c_buf(data);
> +	int ret;
> +
> +	buf[0] = reg;
> +	/* Add Register data to read/write */
> +	xfers[0].rnw = false;
> +	xfers[0].len = 1;
> +	xfers[0].data.out = &buf[0];
> +
> +	xfers[1].rnw = true;
> +	xfers[1].len = 1;
> +	xfers[1].data.in = &buf[1];

[Severity: High]
Does this code risk memory corruption or unsafe DMA operations?

If the underlying I3C host controller processes these transfers using DMA,
mapping &buf[1] as DMA_FROM_DEVICE requires a cache invalidation upon
unmap.

This invalidation could silently destroy concurrent CPU writes to
variables sharing the same cacheline, such as the kref in struct
sbtsi_data.

Additionally, passing adjacent 1-byte buffers for overlapping
DMA_TO_DEVICE and DMA_FROM_DEVICE mappings in a single transaction
violates the Linux DMA API.

> +
> +	ret = i3c_device_do_xfers(data->i3cdev, xfers, 2, 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..7dde040caf308
> --- /dev/null
> +++ b/drivers/misc/amd-sbi/tsi-core.h
> @@ -0,0 +1,23 @@

[ ... ]

> +struct sbtsi_i3c_priv {
> +	struct sbtsi_data data;
> +	u8 buf[2];

[Severity: High]
Is it safe to define this buffer without explicit cacheline alignment?

Without ____cacheline_aligned, buf shares a CPU cacheline with adjacent
fields in struct sbtsi_data. As this buffer is used in I3C transfers in
sbtsi_i3c_read() and sbtsi_i3c_write(), this could lead to cacheline
sharing issues during DMA.

> +};
> +
> +#endif /* _LINUX_TSI_CORE_H_ */

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