Re: [PATCH 15/17] i3c: mipi-i3c-hci: Support configurable device NACK retries

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unconditional use of DAT_v1 operations in IBI hooks causes a NULL pointer dereference on controllers using v2 command descriptors.
- [High] I2C client drivers passing stack-allocated buffers will cause memory corruption or DMA mapping failures because the driver fails to use DMA-safe buffer helpers.
--

commit 7e856c46ca9ed0bd422799648ce415e50625c2ea
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: mipi-i3c-hci: Support configurable device NACK retries

This patch implements the .set_dev_nack_retry() master operation using the
DAT_0_DEV_NACK_RETRY_CNT field in Device Address Table entries. It adds helper
operations to manage the retry counts across allocated DAT entries and defaults
the retry count to 1 for consistency across controller versions.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 7a39be64c4e13..57d84d7682d7d 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -555,6 +555,11 @@ static int i3c_hci_i2c_xfers(struct i2c_dev_desc *dev,
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential issue in
i3c_hci_i2c_xfers() regarding DMA mappings.

When the I2C subsystem passes stack-allocated buffers for ordinary I2C
transfers, they are assigned directly to the hardware transfer struct:

    for (i = 0; i < nxfers; i++) {
        xfer[i].data = i2c_xfers[i].buf;
        xfer[i].data_len = i2c_xfers[i].len;

These buffers are later mapped for DMA in dma.c:hci_dma_map_xfer(), which
doesn't check for stack addresses. Does this code need to use the
i2c_get_dma_safe_msg_buf() helper here before delegating to hardware
processing to prevent direct DMA mapping of stack memory and potential
memory corruption?

> +static void i3c_hci_dat_v1_set_curr_nack_retry(struct i3c_hci *hci, unsigned int dat_idx)
> +{
> +	mipi_i3c_hci_dat_v1.set_nack_retry(hci, dat_idx, hci->master.dev_nack_retry_count);
> +}
> +
>  static int i3c_hci_attach_i3c_dev(struct i3c_dev_desc *dev)
>  {

[ ... ]

> @@ -733,6 +740,16 @@ static void i3c_hci_recycle_ibi_slot(struct i3c_dev_desc *dev,
>  	hci->io->recycle_ibi_slot(hci, dev, slot);
>  }
>  

[Severity: High]
This is a pre-existing issue, but I noticed a problem with how IBI hooks are
handled on controllers using v2 command descriptors.

In __i3c_hci_disable_ibi() and i3c_hci_request_ibi(), the core
unconditionally calls DAT_v1 operations:

    mipi_i3c_hci_dat_v1.set_flags(hci, dev_data->dat_idx, DAT_0_SIR_REJECT, 0);

Since controllers using v2 command descriptors do not initialize or allocate
the Device Address Table (hci->DAT), could this result in a NULL pointer
dereference when the v1 operation attempts to read from hci->DAT[dat_idx].w0?

Should there be a v1/v2 descriptor check before calling these operations
during IBI teardown and setup?

> +static int i3c_hci_set_dev_nack_retry(struct i3c_master_controller *m, unsigned int cnt)
> +{
> +	struct i3c_hci *hci = to_i3c_hci(m);
> +
> +	if (hci->cmd != &mipi_i3c_hci_cmd_v1)
> +		return -EOPNOTSUPP;
> +
> +	return mipi_i3c_hci_dat_v1.set_all_nack_retry(hci, cnt);
> +}
> +
>  static const struct i3c_master_controller_ops i3c_hci_ops = {
>  	.bus_init		= i3c_hci_bus_init,
>  	.bus_cleanup		= i3c_hci_bus_cleanup,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914113003.183150-1-adrian.hunter@intel.com?part=15

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c