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

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

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.

Thanks,
Selva

>>>> 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
>
> We agree. Restting wMaxPacketSize is the best solution for this issue, 
> as concluded from our internal review meeting as well.
>
> Thanks,
> Selva