Hi Mani,
On Wed, Dec 17, 2025 at 6:17 PM Manivannan Sadhasivam via B4 Relay
<[email protected]> wrote:
>
> From: Manivannan Sadhasivam <[email protected]>
>
> 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: [email protected]
> 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 <[email protected]>
> Closes:
> https://lore.kernel.org/linux-arm-msm/[email protected]
> Suggested-by: Chris Lew <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> 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. 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
