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,
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,
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
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
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
linux-stable-mirror@lists.linaro.org
-
Akhil Vinod -
Krishna Chaitanya Chundru -
Manivannan Sadhasivam -
Sumit Kumar