Messages in this thread | | | Date | Thu, 22 Jul 2021 12:50:32 -0700 | From | Bhaumik Bhatt <> | Subject | Re: [PATCH] net: qrtr: mhi: synchronize qrtr and mhi preparation |
| |
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.
>>>>> @@ -90,12 +90,22 @@ 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->ready); >>>>> >>>> >>>>> >>>>> return 0; >>> >>> Thanks, >>> Bhaumik >>> --- >>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >>> Forum, >>> a Linux Foundation Collaborative Project >> >> Thanks, >> Hemant >> --- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >> Forum, >> a Linux Foundation Collaborative Project > > Thanks, > Bhaumik > --- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > Forum, > a Linux Foundation Collaborative Project
Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
| |