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,
};
>>> 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
