Re: [PATCH v2 4/6] 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:
- [High] Kconfig dependency uses a non-existent symbol (`I3C_OR_I2C`), causing silent build failures.
- [High] I3C transfer functions pass stack-allocated buffers for DMA transfers, risking memory corruption and panics.
- [Medium] The driver violates I3C subsystem API boundaries by including `<linux/i3c/master.h>` to access opaque internal structures.
--

commit fd51ed18b45ce179c5ee6ea15c970ec2d140c6c7
Author: Prathima <Prathima.Lk@amd.com>

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

Extend the SB-TSI driver to support both I2C and I3C bus interfaces
by selecting the appropriate transport based on the probed bus type.
The driver registers both I2C and I3C drivers using module_i3c_i2c_driver().

> diff --git a/drivers/misc/amd-sbi/Kconfig b/drivers/misc/amd-sbi/Kconfig
> --- 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

Is this a valid Kconfig symbol? It looks like it will evaluate as a single
config symbol named CONFIG_I3C_OR_I2C instead of a logical OR. Could this
cause the driver to be silently unbuildable? Perhaps it should be written
as depends on I3C || I2C?

> diff --git a/drivers/misc/amd-sbi/tsi-core.c b/drivers/misc/amd-sbi/tsi-core.c
> --- a/drivers/misc/amd-sbi/tsi-core.c
> +++ b/drivers/misc/amd-sbi/tsi-core.c
[ ... ]
> +/* I3C read transfer function */
> +static int sbtsi_i3c_read(struct sbtsi_data *data, u8 reg, u8 *val)
> +{
> +	struct i3c_xfer xfers[2] = { };
> +
> +	/* Add Register data to read/write */
> +	xfers[0].rnw = false;
> +	xfers[0].len = 1;
> +	xfers[0].data.out = &reg;
> +
> +	xfers[1].rnw = true;
> +	xfers[1].len = 1;
> +	xfers[1].data.in = val;
> +
> +	return i3c_device_do_xfers(data->i3cdev, xfers, 2, I3C_SDR);
> +}

Does i3c_device_do_xfers() map the buffers for DMA? If so, passing stack
memory like &reg and val could lead to memory corruption or kernel panics
when CONFIG_VMAP_STACK is enabled. Could these buffers be allocated on the
heap, for example by embedding them in the sbtsi_data structure?

> +/* I3C write transfer function */
> +static int sbtsi_i3c_write(struct sbtsi_data *data, u8 reg, u8 val)
> +{
> +	u8 buf[2];
> +	struct i3c_xfer xfers = {
> +		.rnw = false,
> +		.len = 2,
> +		.data.out = buf,
> +	};
> +
> +	buf[0] = reg;
> +	buf[1] = val;
> +
> +	return i3c_device_do_xfers(data->i3cdev, &xfers, 1, I3C_SDR);
> +}

Similarly, does buf[2] on the stack present a DMA mapping issue here?

> diff --git a/drivers/misc/amd-sbi/tsi.c b/drivers/misc/amd-sbi/tsi.c
> --- a/drivers/misc/amd-sbi/tsi.c
> +++ b/drivers/misc/amd-sbi/tsi.c
[ ... ]
> +static int sbtsi_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	struct device *dev = i3cdev_to_dev(i3cdev);
> +	struct sbtsi_data *data;
> +	int err;
> +	u8 val;
> +
> +	/*
> +	 * AMD OOB devices differ on basis of Instance ID,
> +	 * for SBTSI, instance ID is 0.
> +	 * As the device Id match is not on basis of Instance ID,
> +	 * add the below check to probe the SBTSI device only and
> +	 * not other OOB devices.
> +	 */
> +	if (I3C_PID_INSTANCE_ID(i3cdev->desc->info.pid) != 0)
> +		return -ENXIO;

Is accessing i3cdev->desc->info.pid directly safe? The i3c_device struct is
typically treated as opaque to client drivers. Would it be better to use
i3c_device_get_info() to populate a struct i3c_device_info and read the
fields from there to ensure internal locking is respected?

[ ... ]
> +	data->ext_range_mode = FIELD_GET(BIT(SBTSI_CONFIG_EXT_RANGE_SHIFT), val);
> +	data->read_order = FIELD_GET(BIT(SBTSI_CONFIG_READ_ORDER_SHIFT), val);
> +
> +	dev_set_drvdata(dev, data);
> +	return sbtsi_create_hwmon_adev(dev, i3cdev->desc->info.dyn_addr);
> +}

Similarly here, can this direct access to i3cdev->desc->info.dyn_addr be
replaced with the public i3c_device_get_info() API?

> diff --git a/include/linux/misc/tsi.h b/include/linux/misc/tsi.h
> --- a/include/linux/misc/tsi.h
> +++ b/include/linux/misc/tsi.h
> @@ -9,18 +9,26 @@
>  #define _LINUX_TSI_H_
>  
>  #include <linux/i2c.h>
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/master.h>

Does the driver need to include linux/i3c/master.h? Client drivers normally
only include linux/i3c/device.h, and relying on master.h might break the
I3C API boundaries.

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