Possible usb_request leak in the function dwc2_gadget_complete_isoc_request_ddma

Possible usb_request leak in the function dwc2_gadget_complete_isoc_request_ddma

[Zengtao (B)]

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.

Looking forward your reply.

Thank you. 

Regards
Zengtao 

Re: Possible usb_request leak in the function dwc2_gadget_complete_isoc_request_ddma

[Minas Harutyunyan]

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.

Thanks,
Minas

> Looking forward your reply.
> 
> Thank you.
> 
> Regards
> Zengtao
> --
> 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_majordomo-2Dinfo.html&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=vbKt9jwY5FRUysFlZg1CB6HKRyFACygkwZBO0mvSQDc&s=7rLKr3g3UyOZT22GAer-w5wDtv-Bb5awlncAJQ1OICM&e=
> 

答复: Possible usb_request leak in the function dwc2_gadget_complete_isoc_request_ddma

[Zengtao (B)]

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

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

[Minas Harutyunyan]

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

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

[Minas Harutyunyan]

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.


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

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

[Zengtao (B)]

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

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

[Minas Harutyunyan]

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