On 6/5/2017 5:32 AM, Minas Harutyunyan wrote:
> On 6/2/2017 10:20 PM, John Stultz wrote:
>> On Mon, Apr 17, 2017 at 3:36 PM, John Stultz <[email protected]> wrote:
>>> On Fri, Feb 24, 2017 at 2:46 PM, John Stultz <[email protected]> wrote:
>>>> Hey John,
>>>> So after the USB tree landed in 4.11-rc, I've been seeing the
>>>> following warning at bootup.
>>>>
>>>> It seems the fifo_mem/total_fifo_size value on hikey is 1920, but I'm
>>>> seeing the addresses zip upward quickly as the txfsz values are all
>>>> 2048. Not exactly sure whats wrong here. Things still seem to work
>>>> properly.
>>>>
>>>> thanks
>>>> -john
>>>>
>>>>
>>>> [ 8.944987] dwc2 f72c0000.usb: bound driver configfs-gadget
>>>> [ 8.956651] insufficient fifo memory
>>>> [ 8.956703] ------------[ cut here ]------------
>>>> [ 8.964906] WARNING: CPU: 7 PID: 1 at drivers/usb/dwc2/gadget.c:330
>>>> dwc2_hsotg_init_fifo+0x1a8/0x1c8
>>>
>>>
>>> Hey John,
>>> So I finally got a bit of time to look deeper into this, and it
>>> seems like this issue was introduced by commit 3c6aea7344c3 ("usb:
>>> dwc2: gadget: Add checking for g-tx-fifo-size parameter"), as that
>>> change added the following snippit:
>>>
>>> if (hsotg->params.g_tx_fifo_size[fifo] < min ||
>>> hsotg->params.g_tx_fifo_size[fifo] > dptxfszn) {
>>> dev_warn(hsotg->dev, "%s: Invalid parameter
>>> g_tx_fifo_size[%d]=%d\n",
>>> __func__, fifo,
>>> hsotg->params.g_tx_fifo_size[fifo]);
>>> hsotg->params.g_tx_fifo_size[fifo] = dptxfszn;
>>> }
>>>
>>> On HiKey, we have g-tx-fifo-size = <128 128 128 128 128 128> in the
>>> dtsi, and the fifo_mem value ends up initialized at 1920.
>>>
>>> Unfortunately, in the above, it sees other entries in the
>>> g_tx_fifo_size[] array are initialized to zero, which then causes them
>>> to be set to the "default" value of dptxfszn which is 2048. So then
>>> later in dwc2_hsotg_init_fifo() the addr value (which adds the
>>> fifo_size array value each step) quickly grows beyond the fifo_mem
>>> value, causing the warning.
>>>
>>> Not sure what the right fix is here? Should the min value be used
>>> instead of the "default" dptxfszn value? Again, I'm not sure I see
>>> any side-effects from this warning, but wanted to try to figure out
>>> how to fix it properly.
>>
>> Hey John, Minas,
>> Wanted to follow up on this again. I'm using a patch that looks as
>> follows (sorry for the copy-paste whitespace damage) in the meantime
>> to work around this issue:
>>
>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>> index 9cd8722..13a911b 100644
>> --- a/drivers/usb/dwc2/params.c
>> +++ b/drivers/usb/dwc2/params.c
>> @@ -475,7 +475,7 @@ static void dwc2_check_param_tx_fifo_sizes(struct
>> dwc2_hsotg *hsotg)
>> dev_warn(hsotg->dev, "%s: Invalid parameter
>> g_tx_fifo_size[%d]=%d\n",
>> __func__, fifo,
>> hsotg->params.g_tx_fifo_size[fifo]);
>> - hsotg->params.g_tx_fifo_size[fifo] = dptxfszn;
>> + hsotg->params.g_tx_fifo_size[fifo] = min;
>> }
>> }
>> }
>>
>> Any suggestions on what a proper fix would look like?
>>
>> thanks
>> -john
>>
>
> Hi John,
>
> The patch series: "[PATCH 0/4] usb: dwc2: gadget: Fix dynamic FIFO
> initialization flow" from Sevak Arakelyan submitted to LKML at 4/26/2017
> should resolve this issue.
>
> Thanks,
> Minas
>
Hi John,
See if this patch helps and let us know.
Unfortunately, the author of this patch (and expert on this part of
the code) has left Synopsys. So it might take us a bit to look into
this.
Regards,
John
On Mon, Jun 5, 2017 at 10:50 AM, John Youn <[email protected]> wrote:
> On 6/5/2017 5:32 AM, Minas Harutyunyan wrote:
>> On 6/2/2017 10:20 PM, John Stultz wrote:
>>> On Mon, Apr 17, 2017 at 3:36 PM, John Stultz <[email protected]> wrote:
>>>> On Fri, Feb 24, 2017 at 2:46 PM, John Stultz <[email protected]> wrote:
>>>>> Hey John,
>>>>> So after the USB tree landed in 4.11-rc, I've been seeing the
>>>>> following warning at bootup.
>>>>>
>>>>> It seems the fifo_mem/total_fifo_size value on hikey is 1920, but I'm
>>>>> seeing the addresses zip upward quickly as the txfsz values are all
>>>>> 2048. Not exactly sure whats wrong here. Things still seem to work
>>>>> properly.
>>>>>
>>>>> thanks
>>>>> -john
>>>>>
>>>>>
>>>>> [ 8.944987] dwc2 f72c0000.usb: bound driver configfs-gadget
>>>>> [ 8.956651] insufficient fifo memory
>>>>> [ 8.956703] ------------[ cut here ]------------
>>>>> [ 8.964906] WARNING: CPU: 7 PID: 1 at drivers/usb/dwc2/gadget.c:330
>>>>> dwc2_hsotg_init_fifo+0x1a8/0x1c8
>>>>
>>>>
>>>> Hey John,
>>>> So I finally got a bit of time to look deeper into this, and it
>>>> seems like this issue was introduced by commit 3c6aea7344c3 ("usb:
>>>> dwc2: gadget: Add checking for g-tx-fifo-size parameter"), as that
>>>> change added the following snippit:
>>>>
>>>> if (hsotg->params.g_tx_fifo_size[fifo] < min ||
>>>> hsotg->params.g_tx_fifo_size[fifo] > dptxfszn) {
>>>> dev_warn(hsotg->dev, "%s: Invalid parameter
>>>> g_tx_fifo_size[%d]=%d\n",
>>>> __func__, fifo,
>>>> hsotg->params.g_tx_fifo_size[fifo]);
>>>> hsotg->params.g_tx_fifo_size[fifo] = dptxfszn;
>>>> }
>>>>
>>>> On HiKey, we have g-tx-fifo-size = <128 128 128 128 128 128> in the
>>>> dtsi, and the fifo_mem value ends up initialized at 1920.
>>>>
>>>> Unfortunately, in the above, it sees other entries in the
>>>> g_tx_fifo_size[] array are initialized to zero, which then causes them
>>>> to be set to the "default" value of dptxfszn which is 2048. So then
>>>> later in dwc2_hsotg_init_fifo() the addr value (which adds the
>>>> fifo_size array value each step) quickly grows beyond the fifo_mem
>>>> value, causing the warning.
>>>>
>>>> Not sure what the right fix is here? Should the min value be used
>>>> instead of the "default" dptxfszn value? Again, I'm not sure I see
>>>> any side-effects from this warning, but wanted to try to figure out
>>>> how to fix it properly.
>>>
>>> Hey John, Minas,
>>> Wanted to follow up on this again. I'm using a patch that looks as
>>> follows (sorry for the copy-paste whitespace damage) in the meantime
>>> to work around this issue:
>>>
>>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>>> index 9cd8722..13a911b 100644
>>> --- a/drivers/usb/dwc2/params.c
>>> +++ b/drivers/usb/dwc2/params.c
>>> @@ -475,7 +475,7 @@ static void dwc2_check_param_tx_fifo_sizes(struct
>>> dwc2_hsotg *hsotg)
>>> dev_warn(hsotg->dev, "%s: Invalid parameter
>>> g_tx_fifo_size[%d]=%d\n",
>>> __func__, fifo,
>>> hsotg->params.g_tx_fifo_size[fifo]);
>>> - hsotg->params.g_tx_fifo_size[fifo] = dptxfszn;
>>> + hsotg->params.g_tx_fifo_size[fifo] = min;
>>> }
>>> }
>>> }
>>>
>>> Any suggestions on what a proper fix would look like?
>>>
>>> thanks
>>> -john
>>>
>>
>> Hi John,
>>
>> The patch series: "[PATCH 0/4] usb: dwc2: gadget: Fix dynamic FIFO
>> initialization flow" from Sevak Arakelyan submitted to LKML at 4/26/2017
>> should resolve this issue.
>>
>> Thanks,
>> Minas
>>
>
> Hi John,
>
> See if this patch helps and let us know.
>
> Unfortunately, the author of this patch (and expert on this part of
> the code) has left Synopsys. So it might take us a bit to look into
> this.
Hey Folks,
I realized I didn't get back to you, as I got pulled out to other
work. I was testing with 4.13-rc, and noticed the same issue, and
figured I should get back to you.
I've applied the 4 patches from the patch series you've pointed me to.
Though with this patchset I still hit the same warning, but this time
from dwc2_hsotg_core_init_disconnected() rather then
dwc2_hsotg_init_fifo().
[ 8.016067] init: processing action (sys.usb.config=adb &&
sys.usb.configfs=1 && sys.usb.ffs.ready=1) from
(/init.usb.configfs.rc:21)
[ 8.029367] dwc2 f72c0000.usb: bound driver configfs-gadget
[ 8.035140] ------------[ cut here ]------------
[ 8.043357] WARNING: CPU: 7 PID: 1 at drivers/usb/dwc2/gadget.c:317
dwc2_hsotg_core_init_disconnected+0x52c/0x560
[ 8.053643] CPU: 7 PID: 1 Comm: init Not tainted
4.13.0-rc1-00038-ga257c7e #3331
[ 8.061042] Hardware name: HiKey Development Board (DT)
[ 8.066285] task: ffffffc005f68000 task.stack: ffffffc005f70000
[ 8.072230] PC is at dwc2_hsotg_core_init_disconnected+0x52c/0x560
[ 8.078415] LR is at dwc2_hsotg_core_init_disconnected+0x52c/0x560
[ 8.084593] pc : [<ffffff80086a65ac>] lr : [<ffffff80086a65ac>]
pstate: 600001c5
[ 8.091987] sp : ffffffc005f73be0
[ 8.095298] x29: ffffffc005f73be0 x28: ffffff8008dc9a18
[ 8.100629] x27: ffffffc074d1907c x26: 000000000000000f
[ 8.105949] x25: 0000000000000007 x24: 000000000000011c
[ 8.111263] x23: 0000000000000580 x22: 0000000000000000
[ 8.116577] x21: 0000000000000007 x20: 0000000008000580
[ 8.121902] x19: ffffffc074d19018 x18: 0000000000000010
[ 8.127229] x17: aaaaaaaaaaaaaaab x16: ffffff80081da680
[ 8.132548] x15: ffffff8088e5b3a7 x14: 0000000000000006
[ 8.137863] x13: ffffff8008e5b3b5 x12: ffffff8008d89118
[ 8.143176] x11: 0000000000000000 x10: ffffffc005f73be0
[ 8.148493] x9 : ffffffc005f73be0 x8 : 79726f6d656d206f
[ 8.153807] x7 : 66696620746e6569 x6 : 0000000000000340
[ 8.159121] x5 : 0000000000000015 x4 : 0000000000000000
[ 8.164439] x3 : 0000000000000002 x2 : 0000000000000002
[ 8.169769] x1 : 0000000000000001 x0 : 0000000000000018
[ 8.175099] Call trace:
[ 8.177552] Exception stack(0xffffffc005f73a10 to 0xffffffc005f73b40)
[ 8.183990] 3a00:
ffffffc074d19018 0000008000000000
[ 8.191829] 3a20: ffffffc005f73be0 ffffff80086a65ac
0000000000000007 000000000000000f
[ 8.199664] 3a40: ffffffc074d1907c ffffff8008dc9a18
ffffffc005f73a90 0000000000000000
[ 8.207512] 3a60: ffffffc005f73be0 ffffffc005f73be0
ffffffc005f73ba0 00000000ffffffc8
[ 8.215362] 3a80: ffffffc005f73ab0 ffffff80081057a4
ffffffc005f73be0 ffffffc005f73be0
[ 8.223210] 3aa0: ffffffc005f73ba0 00000000ffffffc8
0000000000000018 0000000000000001
[ 8.231065] 3ac0: 0000000000000002 0000000000000002
0000000000000000 0000000000000015
[ 8.238919] 3ae0: 0000000000000340 66696620746e6569
79726f6d656d206f ffffffc005f73be0
[ 8.246774] 3b00: ffffffc005f73be0 0000000000000000
ffffff8008d89118 ffffff8008e5b3b5
[ 8.254627] 3b20: 0000000000000006 ffffff8088e5b3a7
ffffff80081da680 aaaaaaaaaaaaaaab
[ 8.262487] [<ffffff80086a65ac>]
dwc2_hsotg_core_init_disconnected+0x52c/0x560
[ 8.269740] [<ffffff80086a7038>] dwc2_hsotg_pullup+0x90/0x100
[ 8.275495] [<ffffff80086cb2a0>] usb_udc_connect_control.isra.0+0x78/0x90
[ 8.282308] [<ffffff80086cb3a4>] udc_bind_to_driver+0xec/0x110
[ 8.288166] [<ffffff80086cc308>] usb_gadget_probe_driver+0xa0/0x150
[ 8.294459] [<ffffff80086ca73c>] gadget_dev_desc_UDC_store+0xe4/0x100
[ 8.300931] [<ffffff8008258af8>] configfs_write_file+0xc0/0x198
[ 8.306881] [<ffffff80081da234>] __vfs_write+0x1c/0x118
[ 8.312133] [<ffffff80081da4d0>] vfs_write+0xa0/0x1b0
[ 8.317201] [<ffffff80081da6c4>] SyS_write+0x44/0xa0
[ 8.322193] [<ffffff8008082f30>] el0_svc_naked+0x24/0x28
[ 8.327508] ---[ end trace c6da4c029bccb75e ]---
The same hack I was using earlier
- hsotg->params.g_tx_fifo_size[fifo] = dptxfszn;
+ hsotg->params.g_tx_fifo_size[fifo] = min;
still avoids the issue.
thanks
-john