Re: [PATCH v10 0/6] Re-introduce TX FIFO resize for larger EP bursting

From: Pavel Hofman <pavel.hofman@ivitera.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Ferry Toth <fntoth@gmail.com>, Felipe Balbi <balbi@kernel.org>,
	Wesley Cheng <wcheng@codeaurora.org>,
	gregkh@linuxfoundation.org, robh+dt@kernel.org,
	agross@kernel.org, bjorn.andersson@linaro.org,
	frowand.list@gmail.com, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, jackp@codeaurora.org,
	heikki.krogerus@linux.intel.com,
	Ruslan Bilovol <ruslan.bilovol@gmail.com>
Subject: Re: [PATCH v10 0/6] Re-introduce TX FIFO resize for larger EP bursting
Date: Mon, 23 Aug 2021 17:34:57 +0200	[thread overview]
Message-ID: <d1fb0ad5-e304-8864-a2e4-42d5f652a6a7@ivitera.com> (raw)
In-Reply-To: <CAHp75VeZBLgf8YhEjdOV1Hva_dJh_=VHRGyVb=r44yh-9n+F4A@mail.gmail.com>

Dne 23. 08. 21 v 17:21 Andy Shevchenko napsal(a):
> On Mon, Aug 23, 2021 at 5:59 PM Pavel Hofman <pavel.hofman@ivitera.com> wrote:
>> Dne 22. 08. 21 v 21:43 Ferry Toth napsal(a):
>>> Op 19-08-2021 om 23:04 schreef Pavel Hofman:
>>>> Dne 19. 08. 21 v 22:10 Ferry Toth napsal(a):
>>>>> Op 19-08-2021 om 09:51 schreef Pavel Hofman:
>>>>>> Dne 18. 08. 21 v 21:07 Ferry Toth napsal(a):
>>>>>>> Op 18-08-2021 om 00:00 schreef Ferry Toth:
> 
> ...
> 
>>>>>>> So, where do we go from here?
>>>>>>
>>>>>> I know the patches have been tested on dwc2 (by me and others).  I
>>>>>> do not know if Ruslan or Jerome tested them on dwc3 but probably
>>>>>> not. Ruslan has talked about RPi (my case too) and BeagleboneBlack,
>>>>>> both with dwc2. Perhaps the dwc2 behaves a bit differently than dwc3?
>>>>>>
>>>>>> The patches add a new EP-IN for async feedback. I am sorry I have
>>>>>> not followed your long thread (it started as unrelated to uac). Does
>>>>>> the problem appear with f_uac1 or f_uac2? Please how have you
>>>>>> reached the above problem?
>>>>>
>>>>> I'm sorry too. I first believed the issue was related to the patch
>>>>> mentioned in the subject line.
>>>>>
>>>>> The problem appaers with f_uac2. I bost Edison_Arduino board in host
>>>>> mode (there is a switch allowing to select host/device mode). When
>>>>> flipping the switch to device mode udev run a script:
>>>>> But as I am using configfs (excerpt follows) and just disabling the
>>>>> last 2 line resolves the issue, I'm guessing uac2 is the issue. Or
>>>>> exceeding the available resources.
>>>>>
>>>>> # Create directory structure
>>>>> mkdir "${GADGET_BASE_DIR}"
>>>>> cd "${GADGET_BASE_DIR}"
>>>>> mkdir -p configs/c.1/strings/0x409
>>>>> mkdir -p strings/0x409
>>>>>
>>>>> # Serial device
>>>>> mkdir functions/gser.usb0
>>>>> ln -s functions/gser.usb0 configs/c.1/
>>>>> ###
>>>>>
>>>>> # Ethernet device
>>>>> mkdir functions/eem.usb0
>>>>> echo "${DEV_ETH_ADDR}" > functions/eem.usb0/dev_addr
>>>>> echo "${HOST_ETH_ADDR}" > functions/eem.usb0/host_addr
>>>>> ln -s functions/eem.usb0 configs/c.1/
>>>>>
>>>>> # Mass Storage device
>>>>> mkdir functions/mass_storage.usb0
>>>>> echo 1 > functions/mass_storage.usb0/stall
>>>>> echo 0 > functions/mass_storage.usb0/lun.0/cdrom
>>>>> echo 0 > functions/mass_storage.usb0/lun.0/ro
>>>>> echo 0 > functions/mass_storage.usb0/lun.0/nofua
>>>>> echo "${USBDISK}" > functions/mass_storage.usb0/lun.0/file
>>>>> ln -s functions/mass_storage.usb0 configs/c.1/
>>>>>
>>>>> # UAC2 device
>>>>> mkdir functions/uac2.usb0
>>>>> ln -s functions/uac2.usb0 configs/c.1
>>>>> ....
>>>>
>>>> As you say, could perhaps the reason be that the extra EP-IN added in
>>>> those patches (previously 1, now 2 with the default config you use)
>>>> exceeds your EP-IN max count or available fifos somehow?  You have a
>>>> number of functions initialized. If you change the load order of the
>>>> functions, do you get the error later with a different function? Just
>>>> guessing...
>>>>
>>>> You should be able to switch the default async EP-OUT (which
>>>> configures the new feedback EP-IN ) to adaptive EP-OUT (which requires
>>>> no feedback EP) with c_sync=8 parameter of f_uac2.
>>>>
>>>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/usb/gadget/function/f_uac2.c#L47
>>>>
>>>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/usb/gadget/function/f_uac2.c#L830
>>>>
>>>> https://elixir.bootlin.com/linux/v5.14-rc6/source/include/uapi/linux/usb/ch9.h#L453
>>>>
>>>> Does that fix the problem?
>>>
>>> Not sure how to do that. Do you mean the module should have a parameter
>>> called c_sync? `modinfo` list no parameters for usb_f_uac2.
>>
>> Those are configfs params, not available in modinfo.
>>
>> I checked and the value is "adaptive"
>> https://elixir.bootlin.com/linux/v5.14-rc7/source/drivers/usb/gadget/function/f_uac2.c#L1312
> 
>> In your configfs script:
> 
> Kernel shouldn't crash with any available set of configuration
> parameters, right? So, even if the parameter would fix the crash the
> series is buggy and has to be reverted in my opinion.

Sure, no problem with reverting. I am just trying to start up some 
troubleshooting. A resource exhaustion was mentioned here, that's why I 
suggested a way to test the patch with the original number of endpoints 
allocated. I do not get any crashes on my setup which uses fewer gadget 
functions. That's why I asked what happens if the functions load order 
is changed. I am afraid this thread is so complex that the actual 
problem has been burried in the history.

Again, I am not the author of the patches, my USB knowledge is way 
unsufficient for that. I am trying to make them work as they are 
important and make the existing audio gadget actually usable.

Thanks,

Pavel.