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: <2025011726-hydration-nephew-0d65@gregkh>
Date: Fri, 17 Jan 2025 12:05:19 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Selvarasu Ganesan <selvarasu.g@...sung.com>
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, thiagu.r@...sung.com
Subject: Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded
 issue during MIDI bind retries

On Thu, Jan 16, 2025 at 10:49:24AM +0530, Selvarasu Ganesan wrote:
> 
> 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.

Sorry, I don't remember, it was thousands of patches reviewed ago.  If
you feel your submission was correct, and no changes are needed, resend
with an expanded changelog text to help explain things so I don't have
the same questions again.

thanks,

greg k-h