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
> <[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.
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
--
மணிவண்ணன் சதாசிவம்
