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

Greg KH
2024-12-20T15:15:45Z

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