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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ad54dc65-5ad5-4122-8105-da8f578fc30c@samsung.com>
Date: Mon, 6 Jan 2025 10:47: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/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,
> };
>
>>>> 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


Hi Greg,

Do you have any suggestions or further comments on this?.

Thanks,
Selva