| Message ID | 20250604-qrtr_mhi_auto-v2-1-a143433ddaad@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] net: qrtr: mhi: synchronize qrtr and mhi preparation | expand |
On Wed, Jun 4, 2025 at 11:05 PM Chris Lew <chris.lew@oss.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@kernel.org> > Closes: https://lore.kernel.org/linux-arm-msm/ZyTtVdkCCES0lkl4@hovoldconsulting.com/ > Signed-off-by: Chris Lew <chris.lew@oss.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
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); + rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO); if (rc) { - qrtr_endpoint_unregister(&qdev->ep); + mhi_unprepare_from_transfer(mhi_dev); return rc; } + rc = qrtr_mhi_queue_rx(qdev); + if (rc) { + qrtr_endpoint_unregister(&qdev->ep); + mhi_unprepare_from_transfer(mhi_dev); + } + dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); return 0;
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@kernel.org> Closes: https://lore.kernel.org/linux-arm-msm/ZyTtVdkCCES0lkl4@hovoldconsulting.com/ Signed-off-by: Chris Lew <chris.lew@oss.qualcomm.com> --- net/qrtr/mhi.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 5 deletions(-) --- base-commit: f48887a98b78880b7711aca311fbbbcaad6c4e3b change-id: 20250508-qrtr_mhi_auto-3a3567456f95 Best regards,