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

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

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.


> 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.

> 
> 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.

> 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
>>>
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordo
>> mo-2Dinfo.html&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_Zqb
>> btSAsD16pvOL2S3XHxQnSzq8kusyI&m=vbKt9jwY5FRUysFlZg1CB6HKRyFACygkw
>> ZBO0mvSQDc&s=7rLKr3g3UyOZT22GAer-w5wDtv-Bb5awlncAJQ1OICM&e=
>>>
> 
>