On Wed, Dec 17, 2025 at 09:55:01PM +0100, Loic Poulain wrote:
Hi Mani,
On Wed, Dec 17, 2025 at 6:17 PM Manivannan Sadhasivam via B4 Relay devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org wrote:
From: Manivannan Sadhasivam manivannan.sadhasivam@oss.qualcomm.com
MHI stack offers the 'auto_queue' feature, which allows the MHI stack to auto queue the buffers for the RX path (DL channel). Though this feature simplifies the client driver design, it introduces race between the client drivers and the MHI stack. For instance, with auto_queue, the 'dl_callback' for the DL channel may get called before the client driver is fully probed. This means, by the time the dl_callback gets called, the client driver's structures might not be initialized, leading to NULL ptr dereference.
Currently, the drivers have to workaround this issue by initializing the internal structures before calling mhi_prepare_for_transfer_autoqueue(). But even so, there is a chance that the client driver's internal code path may call the MHI queue APIs before mhi_prepare_for_transfer_autoqueue() is called, leading to similar NULL ptr dereference. This issue has been reported on the Qcom X1E80100 CRD machines affecting boot.
So to properly fix all these races, drop the MHI 'auto_queue' feature altogether and let the client driver (QRTR) manage the RX buffers manually. In the QRTR driver, queue the RX buffers based on the ring length during probe and recycle the buffers in 'dl_callback' once they are consumed. This also warrants removing the setting of 'auto_queue' flag from controller drivers.
Currently, this 'auto_queue' feature is only enabled for IPCR DL channel. So only the QRTR client driver requires the modification.
Cc: stable@vger.kernel.org Fixes: 227fee5fc99e ("bus: mhi: core: Add an API for auto queueing buffers for DL channel") Fixes: 68a838b84eff ("net: qrtr: start MHI channel after endpoit creation") Reported-by: Johan Hovold johan@kernel.org Closes: https://lore.kernel.org/linux-arm-msm/ZyTtVdkCCES0lkl4@hovoldconsulting.com Suggested-by: Chris Lew quic_clew@quicinc.com Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@oss.qualcomm.com
drivers/accel/qaic/mhi_controller.c | 44 ----------------------- drivers/bus/mhi/host/pci_generic.c | 20 ++--------- drivers/net/wireless/ath/ath11k/mhi.c | 4 --- drivers/net/wireless/ath/ath12k/mhi.c | 4 --- net/qrtr/mhi.c | 67 +++++++++++++++++++++++++++++------ 5 files changed, 58 insertions(+), 81 deletions(-)
[...]
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index 69f53625a049..0b4d181ea747 100644 --- a/net/qrtr/mhi.c +++ b/net/qrtr/mhi.c @@ -24,13 +24,25 @@ static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev, struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev); int rc;
if (!qdev || mhi_res->transaction_status)
if (!qdev || (mhi_res->transaction_status && mhi_res->transaction_status != -ENOTCONN)) return;/* Channel got reset. So just free the buffer */if (mhi_res->transaction_status == -ENOTCONN) {devm_kfree(&mhi_dev->dev, mhi_res->buf_addr);return;}rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr, mhi_res->bytes_xferd); if (rc == -EINVAL) dev_err(qdev->dev, "invalid ipcrouter packet\n");/* Done with the buffer, now recycle it for future use */rc = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, mhi_res->buf_addr,mhi_dev->mhi_cntrl->buffer_len, MHI_EOT);if (rc)dev_err(&mhi_dev->dev, "Failed to recycle the buffer: %d\n", rc);}
/* From QRTR to MHI */ @@ -72,6 +84,27 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb) return rc; }
+static int qcom_mhi_qrtr_queue_dl_buffers(struct mhi_device *mhi_dev) +{
void *buf;int ret;while (!mhi_queue_is_full(mhi_dev, DMA_FROM_DEVICE)) {This approach might be a bit racy, since a buffer could complete before the alloc+queue loop finishes. That could e.g lead to recycle error in a concurrent DL callback.
I don't think the race is possible as we just queue the buffers during probe and resume. But I get your point, using mhi_get_free_desc_count() is more straightforward.
- Mani
It might be simpler to just queue the number of descriptors returned by mhi_get_free_desc_count().
buf = devm_kmalloc(&mhi_dev->dev, mhi_dev->mhi_cntrl->buffer_len, GFP_KERNEL);if (!buf)return -ENOMEM;ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, buf, mhi_dev->mhi_cntrl->buffer_len,MHI_EOT);if (ret) {dev_err(&mhi_dev->dev, "Failed to queue buffer: %d\n", ret);return ret;}}return 0;+}
static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id) { @@ -87,20 +120,30 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, qdev->ep.xmit = qcom_mhi_qrtr_send;
dev_set_drvdata(&mhi_dev->dev, qdev);
rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO);if (rc)return rc; /* start channels */rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);if (rc) {qrtr_endpoint_unregister(&qdev->ep);
rc = mhi_prepare_for_transfer(mhi_dev);if (rc) return rc;
}
rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO);if (rc)goto err_unprepare;rc = qcom_mhi_qrtr_queue_dl_buffers(mhi_dev);if (rc)goto err_unregister; dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); return 0;+err_unregister:
qrtr_endpoint_unregister(&qdev->ep);+err_unprepare:
mhi_unprepare_from_transfer(mhi_dev);return rc;}
Regards, Loic