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).
Let consider the below sequence,
_1. First time f_midi_bind:_
* As per the current codethe size of bulk_in_desc.wMaxPacketSize is 0
in first time call.
* Call usb_ep_autoconfig to match EP and got success as no failure in
the below code as part of usb_ep_autoconfig.
usb_gadget_ep_match_desc()
{
..
..
if (max > ep->maxpacket_limit)// (64 > 512)
return 0;
return 1;// Found Maching EP
}
* EP claim got success and set bulk_in_desc.wMaxPacketSize =1024 at
end of f_midi_bind.
_2. Second time enter into f_midi_bind _
* The size of bulk_in_desc.wMaxPacketSize become 1024 because of above
code.
* Call usb_ep_autoconfig for EP claim and has a failure now in the
below code as part of usb_ep_autoconfig
usb_gadget_ep_match_desc()
{
..
..
if (max > ep->maxpacket_limit)// (1024 > 512)
return 0; // Not found any matchingEP
}
To resolve this issue, our patch sets the default value of
"bulk_in_desc.wMaxPacketSize" to 64 before endpoint claim. This ensures
that the endpoint claim is successful during the second time
"f_midi_bind" is called.
>
>> This scenario may encounter an f_midi_bind issue due
>> to the previous bind setting the bulk endpoint's wMaxPacketSize to 1024
>> bytes, which exceeds the ep->maxpacket_limit where configured TX/RX
>> FIFO's maxpacket size of 512 bytes for IN/OUT endpoints in support HS
>> speed only.
>> This commit addresses this issue by resetting the wMaxPacketSize before
>> endpoint claim.
> resets it to what? Where did the magic numbers come from? How do we
> know this is now full speed and not high speed?
It’s a generic way of setwMaxPacketSize before endpoint claim. Its
because of the usb_ep_autoconfig treats endpoint descriptors as if they
were full speed. This approach follows the same pattern as other
function drivers, which also set the wMaxPacketSize to 64 before
endpoint claim.
The following are the example of how other functions drivers EP claim.
_1. drivers/usb/gadget/function/f_eem.c_
eem_bind ()
{
…
..
/* allocate instance-specific endpoints */
ep = usb_ep_autoconfig(cdev->gadget, &eem_fs_in_desc);
//eem_fs_in_desc.wMaxPacketSize=64
if (!ep)
goto fail;
}
_2. drivers/usb/gadget/function/f_rndis.c_
rndis_bind()
{
...
...
/* allocate instance-specific endpoints */
ep = usb_ep_autoconfig(cdev->gadget, &fs_in_desc);
//fs_in_desc.wMaxPacketSize=64
if (!ep)
goto fail;
rndis->port.in_ep = ep;
}
Anyway the wMaxPacketSize set with 64 byte after complete
usb_ep_autoconfig() as part of below this function ifep->maxpacket_limit
is grater then 64. It's means treats endpoint descriptors as if they
were full speed by default.
struct usb_ep *usb_ep_autoconfig()
{
..
…
if (type == USB_ENDPOINT_XFER_BULK) {
int size = ep->maxpacket_limit;
/* min() doesn't work on bitfields with gcc-3.5 */
if (size > 64)
size = 64;
desc->wMaxPacketSize = cpu_to_le16(size);
}
..
..
}
>> Fixes: 46decc82ffd5 ("usb: gadget: unconditionally allocate hs/ss descriptor in bind operation")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Selvarasu Ganesan <selvarasu.g@xxxxxxxxxxx>
>> ---
>> drivers/usb/gadget/function/f_midi.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index 837fcdfa3840..5caa0e4eb07e 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -907,6 +907,15 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
>>
>> status = -ENODEV;
>>
>> + /*
>> + * Reset wMaxPacketSize with maximum packet size of FS bulk transfer before
>> + * endpoint claim. This ensures that the wMaxPacketSize does not exceed the
>> + * limit during bind retries where configured TX/RX FIFO's maxpacket size
>> + * of 512 bytes for IN/OUT endpoints in support HS speed only.
>> + */
>> + bulk_in_desc.wMaxPacketSize = cpu_to_le16(64);
>> + bulk_out_desc.wMaxPacketSize = cpu_to_le16(64);
> Where did "64" come from? How do we know this is full speed? Later
> on in this function the endpoint sizes are set, why set them here to
> these small values when you do not know the speed?
>
> Or, if it had failed before, reset the values on the failure, not here
> before you start anything up, right?
Explained as part of above ask.
>
> thanks,
>
> greg k-h
>
