答复: Possible usb_request leak in the function dwc2_gadget_complete_isoc_request_ddma

["Zengtao (B)" <prime.zeng@hisilicon.com>]

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, so I think you should not at least depend on the depdma if you
think on descriptor should be processed in the interrupt handler.

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.

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.

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