netdev - Re: [PATCH v2] net: qrtr: mhi: synchronize qrtr and mhi preparation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFEp6-1h01SScjbv_m8rU9DxhEgAFOBT_7U2mQegFZQq_O0y4A@mail.gmail.com>
Date: Thu, 5 Jun 2025 10:24:46 +0200
From: Loic Poulain <loic.poulain@....qualcomm.com>
To: Chris Lew <chris.lew@....qualcomm.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: "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 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()?

Regards,
Loic