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

[Selvarasu Ganesan <selvarasu.g@samsung.com>]

On 1/17/2025 4:35 PM, Greg KH wrote:
> 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

Hi Greg,

I understand. Thanks for your update.

Yes, no changes are needed. I updated new version with expanded 
changelog in below link.

https://lore.kernel.org/all/20250118060134.927-1-selvarasu.g@samsung.com/

Thanks,
Selva
>