Message-ID: <CAOdFzcgqiR9qR9KajxBL+CzS5NLBFy4+18L0YfxXU+G=RWEs1A@mail.gmail.com>
Date: Mon, 16 Jun 2025 16:47:14 -0700
From: Chris Lew <chris.lew@....qualcomm.com>
To: Loic Poulain <loic.poulain@....qualcomm.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.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>,
        Hemant Kumar <quic_hemantk@...cinc.com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Manivannan Sadhasivam <mani@...nel.org>, linux-arm-msm@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH v2] net: qrtr: mhi: synchronize qrtr and mhi preparation

On Thu, Jun 5, 2025 at 1:24 AM Loic Poulain
<loic.poulain@....qualcomm.com> wrote:
>
> On Wed, Jun 4, 2025 at 11:05 PM Chris Lew <chris.lew@....qualcomm.com> wrote:
> >
> > The call to qrtr_endpoint_register() was moved before
> > mhi_prepare_for_transfer_autoqueue() to prevent a case where a dl
> > callback can occur before the qrtr endpoint is registered.
> >
> > Now the reverse can happen where qrtr will try to send a packet
> > before the channels are prepared. The correct sequence needs to be
> > prepare the mhi channel, register the qrtr endpoint, queue buffers for
> > receiving dl transfers.
> >
> > Since qrtr will not use mhi_prepare_for_transfer_autoqueue(), qrtr must
> > do the buffer management and requeue the buffers in the dl_callback.
> > Sizing of the buffers will be inherited from the mhi controller
> > settings.
> >
> > 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/
> > Signed-off-by: Chris Lew <chris.lew@....qualcomm.com>
> > ---
> >  net/qrtr/mhi.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 47 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> > index 69f53625a049..5e7476afb6b4 100644
> > --- a/net/qrtr/mhi.c
> > +++ b/net/qrtr/mhi.c
> > @@ -15,6 +15,8 @@ struct qrtr_mhi_dev {
> >         struct qrtr_endpoint ep;
> >         struct mhi_device *mhi_dev;
> >         struct device *dev;
> > +
> > +       size_t dl_buf_len;
> >  };
> >
> >  /* From MHI to QRTR */
> > @@ -24,13 +26,22 @@ 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)
> > +               return;
> > +
> > +       if (mhi_res->transaction_status == -ENOTCONN) {
> > +               devm_kfree(qdev->dev, mhi_res->buf_addr);
> > +               return;
> > +       } else if (mhi_res->transaction_status) {
> >                 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");
> > +
> > +       rc = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, mhi_res->buf_addr, qdev->dl_buf_len, MHI_EOT);
> >  }
> >
> >  /* From QRTR to MHI */
> > @@ -72,6 +83,30 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
> >         return rc;
> >  }
> >
> > +static int qrtr_mhi_queue_rx(struct qrtr_mhi_dev *qdev)
> > +{
> > +       struct mhi_device *mhi_dev = qdev->mhi_dev;
> > +       struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > +       int rc = 0;
> > +       int nr_el;
> > +
> > +       qdev->dl_buf_len = mhi_cntrl->buffer_len;
> > +       nr_el = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
> > +       while (nr_el--) {
> > +               void *buf;
> > +
> > +               buf = devm_kzalloc(qdev->dev, qdev->dl_buf_len, GFP_KERNEL);
> > +               if (!buf) {
> > +                       rc = -ENOMEM;
> > +                       break;
> > +               }
> > +               rc = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, buf, qdev->dl_buf_len, MHI_EOT);
> > +               if (rc)
> > +                       break;
> > +       }
> > +       return rc;
> > +}
> > +
> >  static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
> >                                const struct mhi_device_id *id)
> >  {
> > @@ -87,17 +122,24 @@ 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);
> > +
> > +       /* start channels */
> > +       rc = mhi_prepare_for_transfer(mhi_dev);
> >         if (rc)
> >                 return rc;
> >
> > -       /* start channels */
> > -       rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);
>
> The autoqueue has been introduced to simplify drivers, but if it
> becomes unused, should we simply remove that interface from MHI? Or
> improve it with a autoqueue_prepare() and autoqueue_start()?
>

Yes, I think it is reasonable to remove the autoqueue() interface from
MHI. If another driver comes along and needs something similar we can
revert the commit that removes it.
When I had more cycles I was planning on removing the related code. I
was very late in providing this patch, so I wanted to get this on the
list first.

> Regards,
> Loic

Thanks,
Chris

Powered by blists - more mailing lists