* [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries [not found] <CGME20241208152338epcas5p4fde427bb4467414417083221067ac7ab@epcas5p4.samsung.com> @ 2024-12-08 15:23 ` Selvarasu Ganesan 2024-12-10 9:53 ` Selvarasu Ganesan 2024-12-18 5:31 ` Greg KH 0 siblings, 2 replies; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-08 15:23 UTC (permalink / raw) To: gregkh, quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel Cc: jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, Selvarasu Ganesan, stable 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. 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. Fixes: 46decc82ffd5 ("usb: gadget: unconditionally allocate hs/ss descriptor in bind operation") Cc: stable@vger.kernel.org Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> --- 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); + /* allocate instance-specific endpoints */ midi->in_ep = usb_ep_autoconfig(cdev->gadget, &bulk_in_desc); if (!midi->in_ep) -- 2.17.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-08 15:23 ` [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries Selvarasu Ganesan @ 2024-12-10 9:53 ` Selvarasu Ganesan 2024-12-10 10:18 ` Greg KH 2024-12-18 5:31 ` Greg KH 1 sibling, 1 reply; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-10 9:53 UTC (permalink / raw) To: gregkh, quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel Cc: jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable Hello Maintainers. Gentle remainder for review. Thanks, Selva On 12/8/2024 8:53 PM, 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. 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. > > Fixes: 46decc82ffd5 ("usb: gadget: unconditionally allocate hs/ss descriptor in bind operation") > Cc: stable@vger.kernel.org > Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> > --- > 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); > + > /* allocate instance-specific endpoints */ > midi->in_ep = usb_ep_autoconfig(cdev->gadget, &bulk_in_desc); > if (!midi->in_ep) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-10 9:53 ` Selvarasu Ganesan @ 2024-12-10 10:18 ` Greg KH 2024-12-10 14:11 ` Selvarasu Ganesan 0 siblings, 1 reply; 22+ messages in thread From: Greg KH @ 2024-12-10 10:18 UTC (permalink / raw) To: Selvarasu Ganesan Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable On Tue, Dec 10, 2024 at 03:23:22PM +0530, Selvarasu Ganesan wrote: > Hello Maintainers. > > Gentle remainder for review. You sent this 2 days ago, right? Please take the time to review other commits on the miailing list if you wish to see your patches get reviewed faster, to help reduce the workload of people reviewing your changes. Otherwise just wait for people to get to it, what is the rush here? thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-10 10:18 ` Greg KH @ 2024-12-10 14:11 ` Selvarasu Ganesan 2024-12-10 14:25 ` Greg KH 0 siblings, 1 reply; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-10 14:11 UTC (permalink / raw) To: Greg KH Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable On 12/10/2024 3:48 PM, Greg KH wrote: > On Tue, Dec 10, 2024 at 03:23:22PM +0530, Selvarasu Ganesan wrote: >> Hello Maintainers. >> >> Gentle remainder for review. > You sent this 2 days ago, right? > > Please take the time to review other commits on the miailing list if you > wish to see your patches get reviewed faster, to help reduce the > workload of people reviewing your changes. > > Otherwise just wait for people to get to it, what is the rush here? > > thanks, > > greg k-h Hi Greg, There is no rush. I understand that the review will take time and I apologize for any inconvenience caused by sending the reminder email. Thanks, Selva ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-10 14:11 ` Selvarasu Ganesan @ 2024-12-10 14:25 ` Greg KH 2024-12-12 12:28 ` Selvarasu Ganesan 0 siblings, 1 reply; 22+ messages in thread From: Greg KH @ 2024-12-10 14:25 UTC (permalink / raw) To: Selvarasu Ganesan Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable On Tue, Dec 10, 2024 at 07:41:53PM +0530, Selvarasu Ganesan wrote: > > On 12/10/2024 3:48 PM, Greg KH wrote: > > On Tue, Dec 10, 2024 at 03:23:22PM +0530, Selvarasu Ganesan wrote: > >> Hello Maintainers. > >> > >> Gentle remainder for review. > > You sent this 2 days ago, right? > > > > Please take the time to review other commits on the miailing list if you > > wish to see your patches get reviewed faster, to help reduce the > > workload of people reviewing your changes. > > > > Otherwise just wait for people to get to it, what is the rush here? > > > > thanks, > > > > greg k-h > > > Hi Greg, > > > There is no rush. I understand that the review will take time and I > apologize for any inconvenience caused by sending the reminder email. Great, during that time, please do some patch reviews of the changes on the mailing list to help us out. thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-10 14:25 ` Greg KH @ 2024-12-12 12:28 ` Selvarasu Ganesan 0 siblings, 0 replies; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-12 12:28 UTC (permalink / raw) To: Greg KH Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable On 12/10/2024 7:55 PM, Greg KH wrote: > On Tue, Dec 10, 2024 at 07:41:53PM +0530, Selvarasu Ganesan wrote: >> On 12/10/2024 3:48 PM, Greg KH wrote: >>> On Tue, Dec 10, 2024 at 03:23:22PM +0530, Selvarasu Ganesan wrote: >>>> Hello Maintainers. >>>> >>>> Gentle remainder for review. >>> You sent this 2 days ago, right? >>> >>> Please take the time to review other commits on the miailing list if you >>> wish to see your patches get reviewed faster, to help reduce the >>> workload of people reviewing your changes. >>> >>> Otherwise just wait for people to get to it, what is the rush here? >>> >>> thanks, >>> >>> greg k-h >> >> Hi Greg, >> >> >> There is no rush. I understand that the review will take time and I >> apologize for any inconvenience caused by sending the reminder email. > Great, during that time, please do some patch reviews of the changes on > the mailing list to help us out. > > thanks, > > greg k-h Hi Greg, Sure, I will try to contribute on reviewing other patches. Thanks, Selva > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-08 15:23 ` [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries Selvarasu Ganesan 2024-12-10 9:53 ` Selvarasu Ganesan @ 2024-12-18 5:31 ` Greg KH 2024-12-18 10:21 ` Selvarasu Ganesan 1 sibling, 1 reply; 22+ messages in thread From: Greg KH @ 2024-12-18 5:31 UTC (permalink / raw) To: Selvarasu Ganesan Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable 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. > 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? > > Fixes: 46decc82ffd5 ("usb: gadget: unconditionally allocate hs/ss descriptor in bind operation") > Cc: stable@vger.kernel.org > Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> > --- > 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? thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-18 5:31 ` Greg KH @ 2024-12-18 10:21 ` Selvarasu Ganesan 2024-12-18 15:51 ` Alan Stern 2024-12-20 12:24 ` Greg KH 0 siblings, 2 replies; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-18 10:21 UTC (permalink / raw) To: Greg KH Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable 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@vger.kernel.org >> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> >> --- >> 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-18 10:21 ` Selvarasu Ganesan @ 2024-12-18 15:51 ` Alan Stern 2024-12-19 4:06 ` Selvarasu Ganesan 2024-12-20 12:24 ` Greg KH 1 sibling, 1 reply; 22+ messages in thread From: Alan Stern @ 2024-12-18 15:51 UTC (permalink / raw) To: Selvarasu Ganesan Cc: Greg KH, quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable On Wed, Dec 18, 2024 at 03:51:50PM +0530, Selvarasu Ganesan wrote: > 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). Is this gadget supposed to be able to run at SuperSpeed? I thought the dwc3 controller supported SuperSpeed operation. The USB-3 spec requires that all SuperSpeed bulk endpoints must have a wMaxPacketSize of 1024 (see Table 9-24 on p.9-25 of the USB-3.1 specification). Alan Stern ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-18 15:51 ` Alan Stern @ 2024-12-19 4:06 ` Selvarasu Ganesan 0 siblings, 0 replies; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-19 4:06 UTC (permalink / raw) To: Alan Stern Cc: Greg KH, quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable On 12/18/2024 9:21 PM, Alan Stern wrote: > On Wed, Dec 18, 2024 at 03:51:50PM +0530, Selvarasu Ganesan wrote: >> 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). > Is this gadget supposed to be able to run at SuperSpeed? I thought the > dwc3 controller supported SuperSpeed operation. > > The USB-3 spec requires that all SuperSpeed bulk endpoints must have a > wMaxPacketSize of 1024 (see Table 9-24 on p.9-25 of the USB-3.1 > specification). > > Alan Stern > Hi Alan, No, In our platform, the DWC3 controller supports up to HighSpeed. While DWC3 is capable of SuperSpeed operation, it is not necessary to operate at the same speed. Moreover, even in our SoC, the DWC3 IP is limited to supporting only USB 2.0 mode (HighSpeed) at the hardware design level. As previously mentioned, there is no need to set the wMaxPacketSize based on the current speed support before claiming the endpoint (before calling usb_ep_autoconfig), as usb_ep_autoconfig treats endpoint descriptors as if they were full-speed by default. For reference, let's see the usb_ep_autoconfig code where the wMaxPacketSize is set to 64 bytes if the ep->maxpacket_limit is greater than 64. As i mentioned earlier in our specific failure scenarios, the code does not reach this point because the wMaxPacketSize is greater than the ep->maxpacket_limit. struct usb_ep *usb_ep_autoconfig() { … … … /* report (variable) full speed bulk maxpacket */ if (type == USB_ENDPOINT_XFER_BULK) { int size = ep->maxpacket_limit; if (size > 64) size = 64; desc->wMaxPacketSize = cpu_to_le16(size); } return ep; } Thanks, Selva ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-18 10:21 ` Selvarasu Ganesan 2024-12-18 15:51 ` Alan Stern @ 2024-12-20 12:24 ` Greg KH 2024-12-20 13:32 ` Selvarasu Ganesan 1 sibling, 1 reply; 22+ messages in thread From: Greg KH @ 2024-12-20 12:24 UTC (permalink / raw) To: Selvarasu Ganesan Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable 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? 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. thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-20 12:24 ` Greg KH @ 2024-12-20 13:32 ` Selvarasu Ganesan 2024-12-20 15:15 ` Greg KH 0 siblings, 1 reply; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-20 13:32 UTC (permalink / raw) To: Greg KH Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable 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 > ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-20 13:32 ` Selvarasu Ganesan @ 2024-12-20 15:15 ` Greg KH 2024-12-21 18:07 ` Selvarasu Ganesan 0 siblings, 1 reply; 22+ messages in thread From: Greg KH @ 2024-12-20 15:15 UTC (permalink / raw) To: Selvarasu Ganesan Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable 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? > > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-20 15:15 ` Greg KH @ 2024-12-21 18:07 ` Selvarasu Ganesan 2025-01-06 5:17 ` Selvarasu Ganesan 2025-01-16 5:19 ` Selvarasu Ganesan 0 siblings, 2 replies; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-21 18:07 UTC (permalink / raw) To: Greg KH Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-21 18:07 ` Selvarasu Ganesan @ 2025-01-06 5:17 ` Selvarasu Ganesan 2025-01-16 5:19 ` Selvarasu Ganesan 1 sibling, 0 replies; 22+ messages in thread From: Selvarasu Ganesan @ 2025-01-06 5:17 UTC (permalink / raw) To: Greg KH Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-21 18:07 ` Selvarasu Ganesan 2025-01-06 5:17 ` Selvarasu Ganesan @ 2025-01-16 5:19 ` Selvarasu Ganesan 2025-01-17 11:05 ` Greg KH 1 sibling, 1 reply; 22+ messages in thread From: Selvarasu Ganesan @ 2025-01-16 5:19 UTC (permalink / raw) To: Greg KH Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable, thiagu.r 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. Thanks, Selva >>>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2025-01-16 5:19 ` Selvarasu Ganesan @ 2025-01-17 11:05 ` Greg KH 2025-01-18 6:09 ` Selvarasu Ganesan 0 siblings, 1 reply; 22+ messages in thread From: Greg KH @ 2025-01-17 11:05 UTC (permalink / raw) To: Selvarasu Ganesan Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable, thiagu.r 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2025-01-17 11:05 ` Greg KH @ 2025-01-18 6:09 ` Selvarasu Ganesan 0 siblings, 0 replies; 22+ messages in thread From: Selvarasu Ganesan @ 2025-01-18 6:09 UTC (permalink / raw) To: Greg KH Cc: quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable, thiagu.r On 1/17/2025 4:35 PM, Greg KH wrote: > 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 Hi Greg, I understand. Thanks for your update. Yes, no changes are needed. I updated new version with expanded changelog in below link. https://lore.kernel.org/all/20250118060134.927-1-selvarasu.g@samsung.com/ Thanks, Selva > ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <CGME20241208151349epcas5p1a94ca45020318f54885072d4987160b3@epcas5p1.samsung.com>]
* [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries [not found] <CGME20241208151349epcas5p1a94ca45020318f54885072d4987160b3@epcas5p1.samsung.com> @ 2024-12-08 15:13 ` Faraz Ata 2024-12-08 15:28 ` Selvarasu Ganesan 0 siblings, 1 reply; 22+ messages in thread From: Faraz Ata @ 2024-12-08 15:13 UTC (permalink / raw) To: gregkh, quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel Cc: jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, Selvarasu Ganesan, stable From: Selvarasu Ganesan <selvarasu.g@samsung.com> 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. 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 Fixes: 46decc82ffd5 ("usb: gadget: unconditionally allocate hs/ss descriptor in bind operation") Cc: stable@vger.kernel.org Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> --- 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); + /* allocate instance-specific endpoints */ midi->in_ep = usb_ep_autoconfig(cdev->gadget, &bulk_in_desc); if (!midi->in_ep) -- 2.17.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-08 15:13 ` Faraz Ata @ 2024-12-08 15:28 ` Selvarasu Ganesan 2024-12-08 15:48 ` Greg KH 0 siblings, 1 reply; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-08 15:28 UTC (permalink / raw) To: Faraz Ata, gregkh, quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel Cc: jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable Hello Maintainers, Please ignore this commit as this duplicate copy of https://lore.kernel.org/linux-usb/20241208152322.1653-1-selvarasu.g@samsung.com/ Thanks, Selva On 12/8/2024 8:43 PM, Faraz Ata wrote: > From: Selvarasu Ganesan <selvarasu.g@samsung.com> > > 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. 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 > > Fixes: 46decc82ffd5 ("usb: gadget: unconditionally allocate hs/ss descriptor in bind operation") > Cc: stable@vger.kernel.org > Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> > --- > 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); > + > /* allocate instance-specific endpoints */ > midi->in_ep = usb_ep_autoconfig(cdev->gadget, &bulk_in_desc); > if (!midi->in_ep) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-08 15:28 ` Selvarasu Ganesan @ 2024-12-08 15:48 ` Greg KH 2024-12-08 16:00 ` Selvarasu Ganesan 0 siblings, 1 reply; 22+ messages in thread From: Greg KH @ 2024-12-08 15:48 UTC (permalink / raw) To: Selvarasu Ganesan Cc: Faraz Ata, quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable On Sun, Dec 08, 2024 at 08:58:32PM +0530, Selvarasu Ganesan wrote: > Hello Maintainers, > > Please ignore this commit as this duplicate copy of > https://lore.kernel.org/linux-usb/20241208152322.1653-1-selvarasu.g@samsung.com/ So which is correct? confused, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries 2024-12-08 15:48 ` Greg KH @ 2024-12-08 16:00 ` Selvarasu Ganesan 0 siblings, 0 replies; 22+ messages in thread From: Selvarasu Ganesan @ 2024-12-08 16:00 UTC (permalink / raw) To: Greg KH Cc: Faraz Ata, quic_jjohnson, kees, abdul.rahim, m.grzeschik, linux-usb, linux-kernel, jh0801.jung, dh10.jung, naushad, akash.m5, rc93.raju, taehyun.cho, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, stable On 12/8/2024 9:18 PM, Greg KH wrote: > On Sun, Dec 08, 2024 at 08:58:32PM +0530, Selvarasu Ganesan wrote: >> Hello Maintainers, >> >> Please ignore this commit as this duplicate copy of >> https://lore.kernel.org/linux-usb/20241208152322.1653-1-selvarasu.g@samsung.com/ > So which is correct? > > confused, Sorry for the confusion. The below is the correct one. https://lore.kernel.org/linux-usb/20241208152322.1653-1-selvarasu.g@samsung.com/ Thanks, Selva > > greg k-h > > ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2025-01-18 6:10 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20241208152338epcas5p4fde427bb4467414417083221067ac7ab@epcas5p4.samsung.com>
2024-12-08 15:23 ` [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries Selvarasu Ganesan
2024-12-10 9:53 ` Selvarasu Ganesan
2024-12-10 10:18 ` Greg KH
2024-12-10 14:11 ` Selvarasu Ganesan
2024-12-10 14:25 ` Greg KH
2024-12-12 12:28 ` Selvarasu Ganesan
2024-12-18 5:31 ` Greg KH
2024-12-18 10:21 ` Selvarasu Ganesan
2024-12-18 15:51 ` Alan Stern
2024-12-19 4:06 ` Selvarasu Ganesan
2024-12-20 12:24 ` Greg KH
2024-12-20 13:32 ` Selvarasu Ganesan
2024-12-20 15:15 ` Greg KH
2024-12-21 18:07 ` Selvarasu Ganesan
2025-01-06 5:17 ` Selvarasu Ganesan
2025-01-16 5:19 ` Selvarasu Ganesan
2025-01-17 11:05 ` Greg KH
2025-01-18 6:09 ` Selvarasu Ganesan
[not found] <CGME20241208151349epcas5p1a94ca45020318f54885072d4987160b3@epcas5p1.samsung.com>
2024-12-08 15:13 ` Faraz Ata
2024-12-08 15:28 ` Selvarasu Ganesan
2024-12-08 15:48 ` Greg KH
2024-12-08 16:00 ` Selvarasu Ganesan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox