Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries

[Greg KH <gregkh@linuxfoundation.org>]

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