Message-ID: <pm5amorzm2bvjrvswd4phwiic7wsmitxtzwdvtwqflepxkukjf@esikj32tds7o>
Date: Thu, 18 Dec 2025 21:54:07 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Loic Poulain <loic.poulain@....qualcomm.com>
Cc: manivannan.sadhasivam@....qualcomm.com, 
	Jeff Hugo <jeff.hugo@....qualcomm.com>, Carl Vanderlip <carl.vanderlip@....qualcomm.com>, 
	Oded Gabbay <ogabbay@...nel.org>, Jeff Johnson <jjohnson@...nel.org>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Maxim Kochetkov <fido_max@...ox.ru>, 
	linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	mhi@...ts.linux.dev, linux-wireless@...r.kernel.org, ath11k@...ts.infradead.org, 
	ath12k@...ts.infradead.org, netdev@...r.kernel.org, Bjorn Andersson <andersson@...nel.org>, 
	Johan Hovold <johan@...nel.org>, Chris Lew <quic_clew@...cinc.com>, stable@...r.kernel.org
Subject: 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
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@...nel.org> wrote:
> >
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@....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@...r.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@...nel.org>
> > Closes: https://lore.kernel.org/linux-arm-msm/ZyTtVdkCCES0lkl4@hovoldconsulting.com
> > Suggested-by: Chris Lew <quic_clew@...cinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....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

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

Powered by blists - more mailing lists