Re: [PATCH 1/2] net: qrtr: Drop the MHI auto_queue feature for IPCR DL channels

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

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