Re: [PATCH] bus: mhi: ep: Fix chained transfer handling in read path

[Sumit Kumar <quic_sumk@quicinc.com>]

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@qti.qualcomm.com>
>>>>
>>>> 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@vger.kernel.org
>>>> Co-developed-by: Akhil Vinod <akhvin@qti.qualcomm.com>
>>>> Signed-off-by: Akhil Vinod <akhvin@qti.qualcomm.com>
>>>> Signed-off-by: Sumit Kumar <sumk@qti.qualcomm.com>
>>>> ---
>>>>    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