On Fri, Dec 20, 2024 at 07:02:06PM +0530, Selvarasu Ganesan wrote: > > On 12/20/2024 5:54 PM, Greg KH wrote: > > On Wed, Dec 18, 2024 at 03:51:50PM +0530, Selvarasu Ganesan wrote: > >> On 12/18/2024 11:01 AM, Greg KH wrote: > >>> On Sun, Dec 08, 2024 at 08:53:20PM +0530, Selvarasu Ganesan wrote: > >>>> The current implementation sets the wMaxPacketSize of bulk in/out > >>>> endpoints to 1024 bytes at the end of the f_midi_bind function. However, > >>>> in cases where there is a failure in the first midi bind attempt, > >>>> consider rebinding. > >>> What considers rebinding? Your change does not modify that. > >> Hi Greg, > >> Thanks for your review comments. > >> > >> > >> Here the term "rebind" in this context refers to attempting to bind the > >> MIDI function a second time in certain scenarios. > >> The situations where rebinding is considered include: > >> > >> * When there is a failure in the first UDC write attempt, which may be > >> caused by other functions bind along with MIDI > >> * Runtime composition change : Example : MIDI,ADB to MIDI. Or MIDI to > >> MIDI,ADB > >> > >> The issue arises during the second time the "f_midi_bind" function is > >> called. The problem lies in the fact that the size of > >> "bulk_in_desc.wMaxPacketSize" is set to 1024 during the first call, > >> which exceeds the hardware capability of the dwc3 TX/RX FIFO > >> (ep->maxpacket_limit = 512). > > Ok, but then why not properly reset ALL of the options/values when a > > failure happens, not just this one when the initialization happens > > again? Odds are you might be missing the change of something else here > > as well, right? > Are you suggesting that we reset the entire value of > usb_endpoint_descriptor before call usb_ep_autoconfig? If so, Sorry I am > not clear on your reasoning for wanting to reset all options/values. > After all, all values will be overwritten > afterusb_ep_autoconfig.Additionally, the wMaxPacketSize is the only > value being checked during the EP claim process (usb_ep_autoconfig), and > it has caused issues where claiming wMaxPacketSize is grater than > ep->maxpacket_limit. Then fix up that value on failure, if things fail you should reset it back to a "known good state", right? And what's wrong with resetting all of the values anyway, wouldn't that be the correct thing to do? > > Also, cleaning up from an error is a better thing to do than forcing > > something to be set all the time when you don't have anything gone > > wrong. > As I previously mentioned, this is a general approach to set > wMaxPacketSize before claiming the endpoint. This is because the > usb_ep_autoconfig treats endpoint descriptors as if they were full > speed. Following the same pattern as other function drivers, that > approach allows us to claim the EP with using a full-speed descriptor. > We can use the same approach here instead of resetting wMaxPacketSize > every time. > > The following provided code is used to claim an EP with a full-speed > bulk descriptor in MIDI. Its also working solution. But, We thinking > that it may unnecessarily complicate the code as it only utilizes the > full descriptor for obtaining the EP address here. What you think shall > we go with below approach instead of rest wMaxPacketSize before call > usb_ep_autoconfig? I don't know, what do you think is best to do? You are the one having problems and will need to fix any bugs that your changes will cause :) thanks, greg k-h
