Re: [PATCH] net: qrtr: mhi: synchronize qrtr and mhi preparation

hemantk
2021-07-23T02:46:00Z

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