Re: [PATCH v2] i3c: master: dw-i3c-master: Add HDR-DDR support to Designware Controller.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] i3c: master: dw: broken HDR-DDR direction check in dw_i3c_master_i3c_xfers()
- [High] i3c: master: dw: missing HDR command code in dw_i3c_master_i3c_xfers()
--

commit c62ca5b32f1bbdaf98b64129620fa93cf49be278
Author: Dineshkumar Parameswaran <Dineshkumar.Parameswaran@synopsys.com>

i3c: master: dw-i3c-master: Add HDR-DDR support to Designware Controller.

This commit adds HDR-DDR private transfer support to the DesignWare I3C
master driver by advertising I3C_HDR_DDR in dw_i3c_master_bus_init() and
configuring COMMAND_PORT_CP and DW_I3C_HDR_DDR_SPEED in
dw_i3c_master_i3c_xfers().

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index d1119f203d1b6..6a95e982f59d7 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c

[ ... ]

> @@ -1003,20 +1007,25 @@ static int dw_i3c_master_i3c_xfers(struct i3c_dev_desc *dev,
>  		if (i3c_xfers[i].rnw) {
>  			cmd->rx_buf = i3c_xfers[i].data.in;
>  			cmd->rx_len = i3c_xfers[i].len;
> -			cmd->cmd_lo = COMMAND_PORT_READ_TRANSFER |
> -				      COMMAND_PORT_SPEED(dev->info.max_read_ds);
> +			cmd->cmd_lo = COMMAND_PORT_READ_TRANSFER;
>  
>  		} else {
>  			cmd->tx_buf = i3c_xfers[i].data.out;
>  			cmd->tx_len = i3c_xfers[i].len;
> -			cmd->cmd_lo =
> -				COMMAND_PORT_SPEED(dev->info.max_write_ds);
>  		}

[Severity: High]
Does checking i3c_xfers[i].rnw directly misclassify HDR-DDR write transfers
with non-zero command codes as reads?

In struct i3c_xfer (include/linux/i3c/device.h), rnw and cmd share an
anonymous union:

struct i3c_xfer {
	union {
		u8 rnw;
		u8 cmd;
	};

In I3C_HDR_DDR mode, callers populate cmd with the 8-bit HDR command word,
where bit 7 (I3C_HDR_CMD_RNW) encodes the transfer direction (0x00..0x7f
for write, 0x80..0xff for read) and bits 6:0 (I3C_HDR_CMD_CODE) hold the
7-bit command code.

Because dw_i3c_master_i3c_xfers() tests if (i3c_xfers[i].rnw) directly in
three places:

	for (i = 0; i < i3c_nxfers; i++) {
		if (i3c_xfers[i].rnw)
			nrxwords += DIV_ROUND_UP(i3c_xfers[i].len, 4);
		else
			ntxwords += DIV_ROUND_UP(i3c_xfers[i].len, 4);
	}

	...

		if (i3c_xfers[i].rnw) {
			cmd->rx_buf = i3c_xfers[i].data.in;
			cmd->rx_len = i3c_xfers[i].len;
			cmd->cmd_lo = COMMAND_PORT_READ_TRANSFER;

		} else {
			cmd->tx_buf = i3c_xfers[i].data.out;
			cmd->tx_len = i3c_xfers[i].len;
		}

	...

		if (i3c_xfers[i].rnw)
			i3c_xfers[i].len = cmd->rx_len;

any HDR-DDR write transfer with a non-zero command code (0x01..0x7f) has
i3c_xfers[i].rnw != 0. As a result, its payload is counted in nrxwords
instead of ntxwords, the command is programmed with
COMMAND_PORT_READ_TRANSFER, nothing is written to the TX FIFO, and
dw_i3c_master_end_xfer_locked() -> dw_i3c_master_read_rx_fifo() overwrites
the caller's const data.out buffer via cmd->rx_buf (which aliases data.out
in union data).

Should the direction check in dw_i3c_master_i3c_xfers() inspect
(i3c_xfers[i].cmd & I3C_HDR_CMD_RNW) when mode == I3C_HDR_DDR, similar to
i3c_hci_rnw() in drivers/i3c/master/mipi-i3c-hci/core.c and
svc_i3c_master_i3c_xfers() in drivers/i3c/master/svc-i3c-master.c?

>  
>  		cmd->cmd_lo |= COMMAND_PORT_TID(i) |
>  			       COMMAND_PORT_DEV_INDEX(data->index) |
>  			       COMMAND_PORT_ROC;
>  
> +		if (mode == I3C_HDR_DDR)
> +			cmd->cmd_lo |= COMMAND_PORT_CP |
> +				COMMAND_PORT_SPEED(DW_I3C_HDR_DDR_SPEED);

[Severity: High]
Where is the 7-bit HDR command code from i3c_xfers[i].cmd programmed into
cmd->cmd_lo?

When mode == I3C_HDR_DDR, setting COMMAND_PORT_CP marks the command field
COMMAND_PORT_CMD (bits 14:7) as valid for the HDR command code, but
COMMAND_PORT_CMD(i3c_xfers[i].cmd & I3C_HDR_CMD_CODE) is never ORed into
cmd->cmd_lo. Since xfer is zero-allocated in dw_i3c_master_alloc_xfer(),
bits 14:7 remain 0x00 and every HDR-DDR transfer sends command code 0x00 on
the bus regardless of i3c_xfers[i].cmd.

Should this also OR in COMMAND_PORT_CMD(i3c_xfers[i].cmd & I3C_HDR_CMD_CODE)
when mode == I3C_HDR_DDR?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101005243.4739-1-parame@synopsys.com?part=1

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