Re: 答复: 答复: Possible usb_request leak in the function dwc2_gadget_complete_isoc_request_ddma

[Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>]

Hi Zengtao,

On 3/7/2018 7:38 AM, Zengtao (B) wrote:
> Hi  Minas:
> 
>> -----邮件原件-----
>> 发件人: Minas Harutyunyan [mailto:Minas.Harutyunyan@synopsys.com]
>> 发送时间: 2018年3月2日 18:27
>> 收件人: Zengtao (B) <prime.zeng@hisilicon.com>; Minas Harutyunyan
>> <Minas.Harutyunyan@synopsys.com>; johnyoun@synopsys.com
>> <John.Youn@synopsys.com>
>> 抄送: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
>> linux-kernel@vger.kernel.org
>> 主题: Re: 答复: Possible usb_request leak in the function
>> dwc2_gadget_complete_isoc_request_ddma
>>
>> On 3/2/2018 2:10 PM, Minas Harutyunyan wrote:
>> Re-sending once time more because mail doesn't delivered to linux-usb and
>> linux-kernel mailing list.
>>
>> Hi Zentago,
>>>
>>> On 3/2/2018 5:48 AM, Zengtao (B) wrote:
>>>> Hi Minas:
>>>>
>>>> Thanks for your reply.
>>>>
>>>>> -----邮件原件-----
>>>>> 发件人: Minas Harutyunyan [mailto:Minas.Harutyunyan@synopsys.com]
>>>>> 发送时间: 2018年2月28日 18:27
>>>>> 收件人: Zengtao (B) <prime.zeng@hisilicon.com>;
>> johnyoun@synopsys.com
>>>>> <John.Youn@synopsys.com>
>>>>> 抄送: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
>>>>> linux-kernel@vger.kernel.org
>>>>> 主题: Re: Possible usb_request leak in the function
>>>>> dwc2_gadget_complete_isoc_request_ddma
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 2/28/2018 1:00 PM, Zengtao (B) wrote:
>>>>>> Hi johnyoun:
>>>>>>
>>>>>> I found a suspected bug, and I am writing to confirm with you.
>>>>>>
>>>>>> In the function
>>>>> dwc2_gadget_complete_isoc_request_ddma(drivers/usb/dwc2/gadget.c).
>>>>>> Only the first request from the eq queue is processed while maybe
>>>>>> there are
>>>>> more than one descriptors done by the HW.
>>>>>>
>>>>>> 1. Each usb request is associated with a DMA descriptor, but this
>>>>>> is not reflect in the driver, so when one DMA descriptor is done,
>>>>>> we don't know which usb request is done, but I think if only one
>>>>>> DMA descriptor is
>>>>> done, we can know that the first USB request in eq queue is done,
>>>>> because the HW DMA descriptor and SW usb request are both in sequence.
>>>>>>
>>>>>> 2. In the function dwc2_gadget_complete_isoc_request_ddma, we may
>>>>>> complete more than one DMA descriptor but only the first Usb
>>>>>> request is
>>>>> processed, but in fact, we should all the usb requests associated
>>>>> with all the done DMA descriptors.
>>>>>>
>>>>>> 3. I noticed that each DMA descriptor is configured to report an
>>>>>> interrupt, and if each DMA descriptor generate an interrupt, the
>>>>>> above Flow
>>>>> should be ok, but the interrupts can merge and we have used the
>>>>> depdma to figure out the largest finished DMA descriptor index.
>>>>>>
>>>>>
>>>>> Why you suspect that subsequent interrupts can be merged? Did you
>>>>> see this case? Can you provide a log?
>>>>> Even in case of minimal interval=1, time between 2 subsequent
>>>>> interrupts should be about 125us. It's fully enough to process
>>>>> target descriptor, complete request, enqueue new request and prepare
>> new descriptor.
>>>>>
>>>> Yes, I have seen thas case on my platform.
>>>>
>>>> I think the following should be considered.
>>>>
>>>> 1. The currently driver code checks the depdma register to figure out
>>>> the latest finished descriptor, and even in the interrupt handler
>>>> triggered by one descriptor, the depdma may continue to increase,
>>
>> No, depdma increasing only after fetching descriptor to process.
>> Descriptor fetched by core at follow time:
>> 1. ISOC IN. If descriptor programmed for N uf then descriptor fetched at
>> N-1 uf, performing DMA (buffers data to TxFIFO) and on DMA completion
>> generate XferComplete interrupt if IOC bit set.
>> 2. ISOC OUT. Descriptor fetched only when if RxFIFO not empty, performing DMA
>> (RxFIFO to buffers) and on DMA completion generate XferComplete interrupt if
>> IOC bit set.
>> In both case gap between interrupts is 125us * interval.
>> One exception for ISOC IN. Descriptor can be fetched much earlier than (target
>> uf - 1) if Ignore Frame Number feature enabled in DCTL bit 15.
>> But this feature not implemented in dwc2 driver all. We planning to add this
>> feature in driver later.
>>
>>
> 
> I don't quite understand your description, but it seems this is not an effect handling,
> At least the hardware and software are not running in parallel for descriptors.(hw and sw
> Process the descriptors in parallel with less dependency)
> 
>>>> so I think you should not at least depend on the depdma if you think
>>>> on descriptor should be processed in the interrupt handler.
>>>>
>> This implementation based on core programming flow.
>>
>>>> 2. The Linux system can't assure that the system is available in
>>>> every 125us, for example Some other kernel modules need to disable
>>>> the interrupts or other interrupts handler cost more than 125us, and
>>>> this unfortunately happened on our platform, but that is not an illegal case.
>> I suspect, if the system/platform have more than 125us IRQ latency then it can't
>> be used for ISOCs traffic.
>>
> For the current Dw driver implementation, if the system has more the 125us IRQ latency,
> It will have packets drop for ISCO traffic. But if we don't design the 2 chunks of descriptors
> And don't generate one interrupt for each descriptor, and if will have circle descriptors
> List and generate one interrupt for multiple descriptors, we can tolerant more IRQ latency
>   jitters
> 
>>>>
>>>> 3. The hardware allows each dma descriptor to set it's own ioc bit,
>>>> so we can't just assume That every dma descriptor must set the ioc
>>>> bit to 1. And if we enforce each descriptor to generate an interrupt, then too
>> frequent interrupts(125us) will be really a burden for the system.
>>>>
>> So, you suggesting to not set IOC bit on each descriptor, but only on last one.
>> In this case, as soon as core will process last descriptor (bit L=1) and generate
>> XferComplete (IOC=1), core will return to first descriptor which already
>> processed by core and BNA interrupt will be asserted.
>> Because to process all descriptors, complete all requests, fill descriptors for new
>> request, re-enabling EP, etc. can get more than 125us.
>>
>> Actually, current implementation of ISOC IN/OUT in DDMA based on 2 chunks of
>> descriptors which is not so correct. We prepared patches to fully update ISOC
>> IN/OUT flow for DDMA mode. As soon as we finish testing we will submit to
>> kernel.
>>>
> If you don't mind, I can help to test on our platform, it seems that the new design somewhat
> solves the issue 2 above.
> 
>>>> Thank you.
>>>>
>>>>>>
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-usb"
>>>>>> in the body of a message to majordomo@vger.kernel.org More
>>>>>> majordomo info at
>>>>>>
>>
> 
> 
Please test series of patches "[PATCH 0/3] usb: dwc2: gadget: Update 
ISOC DDMA flow." on your platform and let me know on results.

Thanks,
Minas