Message-ID: <a1dedf06-e804-4580-a690-25e55312eab8@samsung.com>
Date: Fri, 20 Dec 2024 19:02:06 +0530
From: Selvarasu Ganesan <selvarasu.g@...sung.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: quic_jjohnson@...cinc.com, kees@...nel.org, abdul.rahim@...ahoo.com,
	m.grzeschik@...gutronix.de, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, jh0801.jung@...sung.com,
	dh10.jung@...sung.com, naushad@...sung.com, akash.m5@...sung.com,
	rc93.raju@...sung.com, taehyun.cho@...sung.com, hongpooh.kim@...sung.com,
	eomji.oh@...sung.com, shijie.cai@...sung.com, alim.akhtar@...sung.com,
	stable@...r.kernel.org
Subject: Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded
 issue during MIDI bind retries


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.
>
> 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?

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index 837fcdfa3840..fe12c8d82bf2 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -116,6 +116,22 @@ DECLARE_UAC_AC_HEADER_DESCRIPTOR(1);
  DECLARE_USB_MIDI_OUT_JACK_DESCRIPTOR(1);
  DECLARE_USB_MS_ENDPOINT_DESCRIPTOR(16);

+static struct usb_endpoint_descriptor fs_bulk_in_desc = {
+ .bLength =              USB_DT_ENDPOINT_SIZE,
+ .bDescriptorType =      USB_DT_ENDPOINT,
+
+ .bEndpointAddress =     USB_DIR_IN,
+ .bmAttributes =         USB_ENDPOINT_XFER_BULK,
+};
+
+static struct usb_endpoint_descriptor fs_bulk_out_desc = {
+ .bLength =              USB_DT_ENDPOINT_SIZE,
+ .bDescriptorType =      USB_DT_ENDPOINT,
+
+ .bEndpointAddress =     USB_DIR_OUT,
+ .bmAttributes =         USB_ENDPOINT_XFER_BULK,
+};
+
  /* B.3.1  Standard AC Interface Descriptor */
  static struct usb_interface_descriptor ac_interface_desc = {
.bLength =              USB_DT_INTERFACE_SIZE,
@@ -908,11 +924,11 @@ static int f_midi_bind(struct usb_configuration 
*c, struct usb_function *f)
status = -ENODEV;

/* allocate instance-specific endpoints */
- midi->in_ep = usb_ep_autoconfig(cdev->gadget, &bulk_in_desc);
+ midi->in_ep = usb_ep_autoconfig(cdev->gadget, &fs_bulk_in_desc);
if (!midi->in_ep)
goto fail;

- midi->out_ep = usb_ep_autoconfig(cdev->gadget, &bulk_out_desc);
+ midi->out_ep = usb_ep_autoconfig(cdev->gadget, &fs_bulk_out_desc);
if (!midi->out_ep)
goto fail;

@@ -1006,6 +1022,9 @@ static int f_midi_bind(struct usb_configuration 
*c, struct usb_function *f)
ms_in_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->out_ports);
ms_in_desc.bNumEmbMIDIJack = midi->out_ports;

+ bulk_in_desc.bEndpointAddress = fs_bulk_in_desc.bEndpointAddress;
+ bulk_out_desc.bEndpointAddress = fs_bulk_out_desc.bEndpointAddress;
+
/* ... and add them to the list */
endpoint_descriptor_index = i;
midi_function[i++] = (struct usb_descriptor_header *) &bulk_out_desc;



>
> thanks,
>
> greg k-h
>

Powered by blists - more mailing lists