Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Silent truncation of TX payloads > 16 bytes when a transfer requests an ACK or involves an RX response, leading to hardware desynchronization. --
commit 13f2cd4029598c96fbe1cf82c539c77f33326afa Author: Marek Vasut <[email protected]> drm/rcar-du: dsi: Implement DSI command TX using AXI memory access Currently, the driver supports register access mode for DSI command transfer, which limits both TX and RX to only 16 Bytes long packets. Implement support for DSI command TX using AXI memory access mode, which extends the packet transfer length up to 128 Bytes long in LP and 1024 Bytes long in HS. > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c > b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c [ ... ] > @@ -977,6 +981,7 @@ static ssize_t rcar_mipi_dsi_host_tx_transfer(struct > mipi_dsi_host *host, > bool is_rx_xfer) > { > const bool is_tx_long = mipi_dsi_packet_format_is_long(msg->type); > + const bool is_tx_axi = !is_rx_xfer && is_tx_long && (msg->tx_len > 16); [Severity: High] If a DSI panel driver initiates a transfer with a payload > 16 bytes and requests an ACK (by setting MIPI_DSI_MSG_REQ_ACK in msg->flags), is_rx_xfer evaluates to true. Does this unintentionally force the driver to bypass AXI mode and use register mode, regardless of tx_len? > struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host); > struct mipi_dsi_packet packet; [ ... ] > @@ -1011,7 +1021,10 @@ static ssize_t rcar_mipi_dsi_host_tx_transfer(struct > mipi_dsi_host *host, > TXCMPHDR_DATA1(packet.header[2]) | > TXCMPHDR_DATA0(packet.header[1])); > > - if (is_tx_long) { > + if (is_tx_axi) { > + memcpy(dsi->cmd_axi_cpu, packet.payload, > + min(msg->tx_len, 1024)); > + } else if (is_tx_long) { > memcpy(payload, packet.payload, > min(msg->tx_len, sizeof(payload))); [Severity: High] When AXI mode is bypassed for a long write (because is_rx_xfer is true), the payload is silently truncated to 16 bytes here. Since the hardware header will still expect the full transfer size, can this starve the hardware FIFO, leading to garbage data transmission and a DSI link timeout? [ ... ] > @@ -1162,10 +1175,16 @@ static ssize_t rcar_mipi_dsi_host_transfer(struct > mipi_dsi_host *host, > struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host); > int ret; > > - if (msg->tx_len > 16 || msg->rx_len > 16) { > - /* ToDo: Implement Memory on AXI bus command mode. */ > + if (msg->tx_len > 1024 || msg->rx_len > 16) { > + /* ToDo: Implement Memory on AXI bus RX command mode. */ > dev_warn(dsi->dev, > - "Register-based command mode supports only up to 16 > Bytes long payload\n"); > + "Command mode supports only up to 1024B long TX and > 16B long RX payload\n"); > + return -EOPNOTSUPP; > + } [Severity: High] This overarching validation check permits transfers with tx_len > 16 to proceed even if is_rx_xfer is true. Since AXI mode isn't supported for RX transfers, and long writes requiring an ACK are forced to bypass AXI mode, should this block tx_len > 16 when is_rx_xfer is true to prevent silent truncation? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
