Re: [PATCH] bus: mhi: ep: Fix chained transfer handling in read path
From: Sumit Kumar
Date: Fri Aug 22 2025 - 04:50:54 EST
On 7/17/2025 10:44 PM, Manivannan Sadhasivam wrote:
On Thu, Jul 17, 2025 at 10:18:54PM GMT, Akhil Vinod wrote:
On 7/16/2025 12:10 PM, Manivannan Sadhasivam wrote:
On Wed, Jul 09, 2025 at 04:03:17PM GMT, Sumit Kumar wrote:
From: Sumit Kumar <sumk@xxxxxxxxxxxxxxxx>
The current implementation of mhi_ep_read_channel, in case of chained
transactions, assumes the End of Transfer(EOT) bit is received with the
doorbell. As a result, it may incorrectly advance mhi_chan->rd_offset
beyond wr_offset during host-to-device transfers when EOT has not yet
arrived. This can lead to access of unmapped host memory, causing
IOMMU faults and processing of stale TREs.
This change modifies the loop condition to ensure rd_offset remains behind
wr_offset, allowing the function to process only valid TREs up to the
current write pointer. This prevents premature reads and ensures safe
traversal of chained TREs.
Fixes: 5301258899773 ("bus: mhi: ep: Add support for reading from the host")
Cc: stable@xxxxxxxxxxxxxxx
Co-developed-by: Akhil Vinod <akhvin@xxxxxxxxxxxxxxxx>
Signed-off-by: Akhil Vinod <akhvin@xxxxxxxxxxxxxxxx>
Signed-off-by: Sumit Kumar <sumk@xxxxxxxxxxxxxxxx>
---
drivers/bus/mhi/ep/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index b3eafcf2a2c50d95e3efd3afb27038ecf55552a5..2e134f44952d1070c62c24aeca9effc7fd325860 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -468,7 +468,7 @@ static int mhi_ep_read_channel(struct mhi_ep_cntrl *mhi_cntrl,
mhi_chan->rd_offset = (mhi_chan->rd_offset + 1) % ring->ring_size;
}
- } while (buf_left && !tr_done);
+ } while (buf_left && !tr_done && mhi_chan->rd_offset != ring->wr_offset);
You should use mhi_ep_queue_is_empty() for checking the available elements to
process. And with this check in place, the existing check in
mhi_ep_process_ch_ring() becomes redundant.
- Mani
Yes, agreed that the check can be replaced with the mhi_ep_queue_is_empty, but the existing
check in mhi_ep_process_ch_ring() is still necessary because there can be a case where
there are multiple chained transactions in the ring.
Example: The ring at the time mhi_ep_read_channel is executing may look like:
chained | chained | EOT#1 | chained | chained | EOT#2
If we remove the check from mhi_ep_process_ch_ring, we bail out of the first transaction itself
and the remaining packets won't be processed. mhi_ep_read_channel in its current form is designed
for a single MHI packet only.
Then you should ignore the EOT flag by removing '!tr_done' check and just check
for buf_left and mhi_ep_process_ch_ring(). Having the same check in caller and
callee doesn't make sense.
- Mani
Agreed, we can remove the tr_done check from the while loop, then all
the elements of the ring will be processed in read_channel.
Additionally, the purpose of buf_left is to process a TRE if
DEFAULT_MTU_SIZE of endpoint is less than host, but the buf_left will
become 0 after processing a part of TRE and will not process the
remaining data.
Therefore will remove the buf_left too from read_channel otherwise it
will exit the loop after processing one TRE or just a part of it.
- Sumit