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

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

[Sumit Kumar]

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);
 
 	return 0;
 

---
base-commit: 4c06e63b92038fadb566b652ec3ec04e228931e8
change-id: 20250709-chained_transfer-0b95f8afa487

Best regards,
-- 
Sumit Kumar <quic_sumk@quicinc.com>

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

[Krishna Chaitanya Chundru]

On 7/9/2025 4:03 PM, 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>
Reviewed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.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);
>   
>   	return 0;
>   
> 
> ---
> base-commit: 4c06e63b92038fadb566b652ec3ec04e228931e8
> change-id: 20250709-chained_transfer-0b95f8afa487
> 
> Best regards,

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

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Akhil Vinod]

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.

- Akhil

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

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Sumit Kumar]

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

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

[Manivannan Sadhasivam]

On Fri, Aug 22, 2025 at 02:20:29PM GMT, Sumit Kumar wrote:
> 
> 
> 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.
> 

Right. If we are removing the ring empty check from mhi_ep_process_ch_ring(),
then we do need to remove the buf_left check from mhi_ep_read_channel().

- Mani

-- 
மணிவண்ணன் சதாசிவம்