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

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jan 16, 2025 at 10:49:24AM +0530, Selvarasu Ganesan wrote:
> 
> On 12/21/2024 11:37 PM, Selvarasu Ganesan wrote:
> >
> > On 12/20/2024 8:45 PM, Greg KH wrote:
> >> 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?
> >
> > Yes, It's back to known good state if we reset wMaxPacketSize. There 
> > is no point to reset all values in the usb endpoint descriptor 
> > structure as all the member of this structure are predefined value 
> > except wMaxPacketSize and bEndpointAddress. The bEndpointAddress is 
> > obtain as part of usb_ep_autoconfig.
> >
> > static struct usb_endpoint_descriptor bulk_out_desc = {
> >         .bLength =              USB_DT_ENDPOINT_AUDIO_SIZE,
> >         .bDescriptorType =      USB_DT_ENDPOINT,
> >         .bEndpointAddress =     USB_DIR_OUT,
> >         .bmAttributes =         USB_ENDPOINT_XFER_BULK,
> > };
> >
> HI Greg,
> 
> Gentle remainder for your further comments or suggestions on this.

Sorry, I don't remember, it was thousands of patches reviewed ago.  If
you feel your submission was correct, and no changes are needed, resend
with an expanded changelog text to help explain things so I don't have
the same questions again.

thanks,

greg k-h