| Message ID | 20241104-qrtr_mhi-v1-1-79adf7e3bba5@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: qrtr: mhi: synchronize qrtr and mhi preparation | expand |
On Mon, Nov 04, 2024 at 05:29:37PM -0800, Chris Lew wrote: > From: Bhaumik Bhatt <bbhatt@codeaurora.org> > > 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. Add a wait in the sending path to > ensure the channels are prepared before trying to do a ul transfer. > > 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: Bhaumik Bhatt <bbhatt@codeaurora.org> > Signed-off-by: Chris Lew <quic_clew@quicinc.com> I think we need to have the check in 'mhi_queue()' instead of waiting for the channels in client drivers. Would it be a problem if qrtr returns -EAGAIN from qcom_mhi_qrtr_send() instead of waiting for the channel? - Mani > --- > net/qrtr/mhi.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > index 69f53625a049..5b7268868bbd 100644 > --- a/net/qrtr/mhi.c > +++ b/net/qrtr/mhi.c > @@ -15,6 +15,7 @@ struct qrtr_mhi_dev { > struct qrtr_endpoint ep; > struct mhi_device *mhi_dev; > struct device *dev; > + struct completion prepared; > }; > > /* From MHI to QRTR */ > @@ -53,6 +54,10 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb) > if (skb->sk) > sock_hold(skb->sk); > > + rc = wait_for_completion_interruptible(&qdev->prepared); > + if (rc) > + goto free_skb; > + > rc = skb_linearize(skb); > if (rc) > goto free_skb; > @@ -85,6 +90,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, > qdev->mhi_dev = mhi_dev; > qdev->dev = &mhi_dev->dev; > qdev->ep.xmit = qcom_mhi_qrtr_send; > + init_completion(&qdev->prepared); > > dev_set_drvdata(&mhi_dev->dev, qdev); > rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO); > @@ -97,6 +103,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, > qrtr_endpoint_unregister(&qdev->ep); > return rc; > } > + complete_all(&qdev->prepared); > > dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); > > > --- > base-commit: 1ffec08567f426a1c593e038cadc61bdc38cb467 > change-id: 20241104-qrtr_mhi-dfec353030af > > Best regards, > -- > Chris Lew <quic_clew@quicinc.com> >
On 11/7/2024 3:27 AM, Manivannan Sadhasivam wrote: > On Mon, Nov 04, 2024 at 05:29:37PM -0800, Chris Lew wrote: >> From: Bhaumik Bhatt <bbhatt@codeaurora.org> >> >> 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. Add a wait in the sending path to >> ensure the channels are prepared before trying to do a ul transfer. >> >> 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: Bhaumik Bhatt <bbhatt@codeaurora.org> >> Signed-off-by: Chris Lew <quic_clew@quicinc.com> > > I think we need to have the check in 'mhi_queue()' instead of waiting for the > channels in client drivers. Would it be a problem if qrtr returns -EAGAIN from > qcom_mhi_qrtr_send() instead of waiting for the channel? > The packet would get dropped which usually ends up causing some functional problem down the line. I can add retry handling for EAGAIN in qcom_mhi_qrtr_send(). Downstream we had also seen some issue where we received EAGAIN because the ring buffer was full. I think we saw issues doing a dumb retry so we triggered the retry on getting a ul_callback(). We would need to differentiate between this kind of EAGAIN from a ringbuf full EAGAIN. > - Mani > >> --- >> net/qrtr/mhi.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c >> index 69f53625a049..5b7268868bbd 100644 >> --- a/net/qrtr/mhi.c >> +++ b/net/qrtr/mhi.c >> @@ -15,6 +15,7 @@ struct qrtr_mhi_dev { >> struct qrtr_endpoint ep; >> struct mhi_device *mhi_dev; >> struct device *dev; >> + struct completion prepared; >> }; >> >> /* From MHI to QRTR */ >> @@ -53,6 +54,10 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb) >> if (skb->sk) >> sock_hold(skb->sk); >> >> + rc = wait_for_completion_interruptible(&qdev->prepared); >> + if (rc) >> + goto free_skb; >> + >> rc = skb_linearize(skb); >> if (rc) >> goto free_skb; >> @@ -85,6 +90,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, >> qdev->mhi_dev = mhi_dev; >> qdev->dev = &mhi_dev->dev; >> qdev->ep.xmit = qcom_mhi_qrtr_send; >> + init_completion(&qdev->prepared); >> >> dev_set_drvdata(&mhi_dev->dev, qdev); >> rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO); >> @@ -97,6 +103,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, >> qrtr_endpoint_unregister(&qdev->ep); >> return rc; >> } >> + complete_all(&qdev->prepared); >> >> dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); >> >> >> --- >> base-commit: 1ffec08567f426a1c593e038cadc61bdc38cb467 >> change-id: 20241104-qrtr_mhi-dfec353030af >> >> Best regards, >> -- >> Chris Lew <quic_clew@quicinc.com> >> >
On Mon, Nov 04, 2024 at 05:29:37PM -0800, Chris Lew wrote: > From: Bhaumik Bhatt <bbhatt@codeaurora.org> > > 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. Add a wait in the sending path to > ensure the channels are prepared before trying to do a ul transfer. > > 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: Bhaumik Bhatt <bbhatt@codeaurora.org> > Signed-off-by: Chris Lew <quic_clew@quicinc.com> > @@ -53,6 +54,10 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb) > if (skb->sk) > sock_hold(skb->sk); > > + rc = wait_for_completion_interruptible(&qdev->prepared); > + if (rc) > + goto free_skb; > + > rc = skb_linearize(skb); > if (rc) > goto free_skb; > @@ -85,6 +90,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, > qdev->mhi_dev = mhi_dev; > qdev->dev = &mhi_dev->dev; > qdev->ep.xmit = qcom_mhi_qrtr_send; > + init_completion(&qdev->prepared); > > dev_set_drvdata(&mhi_dev->dev, qdev); > rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO); > @@ -97,6 +103,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, > qrtr_endpoint_unregister(&qdev->ep); > return rc; > } > + complete_all(&qdev->prepared); > > dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); While this probably works, it still looks like a bit of a hack. Why can't you restructure the code so that the channels are fully initialised before you register or enable them instead? Johan
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index 69f53625a049..5b7268868bbd 100644 --- a/net/qrtr/mhi.c +++ b/net/qrtr/mhi.c @@ -15,6 +15,7 @@ struct qrtr_mhi_dev { struct qrtr_endpoint ep; struct mhi_device *mhi_dev; struct device *dev; + struct completion prepared; }; /* From MHI to QRTR */ @@ -53,6 +54,10 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb) if (skb->sk) sock_hold(skb->sk); + rc = wait_for_completion_interruptible(&qdev->prepared); + if (rc) + goto free_skb; + rc = skb_linearize(skb); if (rc) goto free_skb; @@ -85,6 +90,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, qdev->mhi_dev = mhi_dev; qdev->dev = &mhi_dev->dev; qdev->ep.xmit = qcom_mhi_qrtr_send; + init_completion(&qdev->prepared); dev_set_drvdata(&mhi_dev->dev, qdev); rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO); @@ -97,6 +103,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, qrtr_endpoint_unregister(&qdev->ep); return rc; } + complete_all(&qdev->prepared); dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");