Re: bug? dwc2: insufficient fifo memory

[John Stultz]

On Mon, Apr 17, 2017 at 3:36 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> On Fri, Feb 24, 2017 at 2:46 PM, John Stultz <john.stultz@xxxxxxxxxx> 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