Messages in this thread | | | Date | Thu, 22 Jul 2021 19:45:52 -0700 | From | hemantk@codeauro ... | Subject | Re: [PATCH] net: qrtr: mhi: synchronize qrtr and mhi preparation |
| |
On 2021-07-22 12:50, Bhaumik Bhatt wrote: > On 2021-07-22 12:04 PM, Bhaumik Bhatt wrote: >> On 2021-07-21 03:27 PM, hemantk@codeaurora.org wrote: >>> On 2021-07-21 11:07, Bhaumik Bhatt wrote: >>>> On 2021-07-21 10:52 AM, hemantk@codeaurora.org wrote: >>>>> On 2021-07-20 18:42, Bhaumik Bhatt wrote: >>>>>> A dl callback can be received anytime after >>>>>> mhi_prepare_for_transfer >>>>>> has been called. There is a window where the callback may happen >>>>>> before the probe initializes the qrtr_mhi_dev state. Move the >>>>>> mhi_prepare_for_transfer call after the registering the endpoint. >>>>>> >>>>>> Once moved, 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: a2e2cc0dbb11 ("net: qrtr: Start MHI channels during init") >>>>>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >>>>>> --- >>>>>> net/qrtr/mhi.c | 20 +++++++++++++++----- >>>>>> 1 file changed, 15 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c >>>>>> index 29b4fa3..22b0395 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 ready; >>>>>> }; >>>>>> >>>>>> /* From MHI to QRTR */ >>>>>> @@ -50,6 +51,10 @@ static int qcom_mhi_qrtr_send(struct >>>>>> qrtr_endpoint >>>>>> *ep, struct sk_buff *skb) >>>>>> struct qrtr_mhi_dev *qdev = container_of(ep, struct >>>>>> qrtr_mhi_dev, ep); >>>>>> int rc; >>>>>> >>>>>> + rc = wait_for_completion_interruptible(&qdev->ready); >>>>>> + if (rc) >>>>>> + goto free_skb; >>>>>> + >>>>>> if (skb->sk) >>>>>> sock_hold(skb->sk); >>>>>> >>>>>> @@ -78,11 +83,6 @@ static int qcom_mhi_qrtr_probe(struct >>>>>> mhi_device *mhi_dev, >>>>>> struct qrtr_mhi_dev *qdev; >>>>>> int rc; >>>>>> >>>>>> - /* start channels */ >>>>>> - rc = mhi_prepare_for_transfer(mhi_dev, >>>>>> MHI_CH_INBOUND_ALLOC_BUFS); >>>>>> - if (rc) >>>>>> - return rc; >>>>>> - >>>>>> qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL); >>>>>> if (!qdev) >>>>>> return -ENOMEM; >>>>> would it be good to init completion variable here (call >>>>> init_completion) ? >>>> You mean just before setting qdev->mhi_dev? I don't see why that >>>> would >>>> make a difference >>>> mainly because the qcom_mhi_qrtr_send() will only happen after >>>> endpoint is >>>> registered and DL xfer cb will also only come in after we have >>>> prepared the >>>> channels and completed ready with dev_data already set. >>> looks like qcom_mhi_qrtr_send is not going to get called directly. i >>> was thinking >>> what if this api is called before init_completion() returns. if it is >>> only possible >>> through ep.xmit call back only, can you move it right above >>> qdev->ep.xmit = qcom_mhi_qrtr_send; ? >>>> >> Ah. OK. I see your point. I will do that and upload a v2. >> > On second thought, this is not required because the ep.xmit() will not > be called > until the qrtr_endpoint_register() is done. > > So this version should be fine IMO. Thanks for checking that, Bhaumik.
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
| |