usb: gadget: epautoconf: claim smallest endpoints first

PATCH In review Ruslan Bilovol 2 participants 5 messages Jun 29, 2020 – Feb 10, 2023 USB

Share & tools

Ruslan Bilovol

author

2020-06-29 20:18 UTC #0 lore raw

PATCH v2 1/1 usb: gadget: epautoconf: claim smallest endpoints first

UDC hardware may have endpoints with different maxpacket size. Current endpoint matching code takes first matching endpoint from the list.

It's always possible that gadget allocates endpoints for small transfers (maxpacket size) first, then larger ones. That works fine if all matching UDC endpoints have same maxpacket size or are big enough to serve that allocation.

However, some UDCs have first endpoints in the list with bigger maxpacket size, whereas last endpoints are much smaller. In this case endpoint allocation will fail for the gadget (which allocates smaller endpoints first) on final endpoint allocations.

To make endpoint allocation fair, pick up smallest matching endpoints first, leaving bigger ones for heavier applications.

Signed-off-by Ruslan Bilovol <[email protected]>

---

v2: rebased onto latest balbi/next branch

 drivers/usb/gadget/epautoconf.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

drivers/usb/gadget/epautoconf.c +18 -5

index 1eb4fa2e623f..6c453b5d87bb 100644
@@ -66,7 +66,7 @@ struct usb_ep *usb_ep_autoconfig_ss(
 	struct usb_ss_ep_comp_descriptor *ep_comp
 )
 {
-	struct usb_ep	*ep;
+	struct usb_ep	*ep, *ep_min = NULL;
 
 	if (gadget->ops->match_ep) {
 		ep = gadget->ops->match_ep(gadget, desc, ep_comp);

@@ -74,14 +74,27 @@ struct usb_ep *usb_ep_autoconfig_ss(
 			goto found_ep;
 	}
 
-	/* Second, look at endpoints until an unclaimed one looks usable */
+	/*
+	 * Second, look at endpoints until an unclaimed one looks usable.
+	 * Try to find one with smallest maxpacket limit, leaving larger
+	 * endpoints for heavier applications
+	 */
 	list_for_each_entry (ep, &gadget->ep_list, ep_list) {
-		if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
-			goto found_ep;
+		if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp)) {
+			if (desc->wMaxPacketSize == 0)
+				goto found_ep;
+			else if (!ep_min)
+				ep_min = ep;
+			else if (ep->maxpacket_limit < ep_min->maxpacket_limit)
+				ep_min = ep;
+		}
 	}
 
 	/* Fail */
-	return NULL;
+	if (!ep_min)
+		return NULL;
+
+	ep = ep_min;
 found_ep:
 
 	/*

Signature · 1 line

2.17.1

Peter Chen

2020-06-30 01:58 UTC #1 lore raw

Quoted 46 lines · Ruslan Bilovol

UDC hardware may have endpoints with different maxpacket size. Current endpoint matching code takes first matching endpoint from the list.
It's always possible that gadget allocates endpoints for small transfers (maxpacket size) first, then larger ones. That works fine if all matching UDC endpoints have same maxpacket size or are big enough to serve that allocation.
However, some UDCs have first endpoints in the list with bigger maxpacket size, whereas last endpoints are much smaller. In this case endpoint allocation will fail for the gadget (which allocates smaller endpoints first) on final endpoint allocations.
To make endpoint allocation fair, pick up smallest matching endpoints first, leaving bigger ones for heavier applications.
Signed-off-by Ruslan Bilovol <[email protected]>
---
v2: rebased onto latest balbi/next branch
 drivers/usb/gadget/epautoconf.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)
drivers/usb/gadget/epautoconf.c +2 -2
index 1eb4fa2e623f..6c453b5d87bb 100644
@@ -66,7 +66,7 @@ struct usb_ep *usb_ep_autoconfig_ss(
 	struct usb_ss_ep_comp_descriptor *ep_comp
 )
 {
-	struct usb_ep	*ep;
+	struct usb_ep	*ep, *ep_min = NULL;
 
 	if (gadget->ops->match_ep) {
 		ep = gadget->ops->match_ep(gadget, desc, ep_comp);
@@ -74,14 +74,27 @@ struct usb_ep *usb_ep_autoconfig_ss(
 			goto found_ep;
 	}
 
-	/* Second, look at endpoints until an unclaimed one looks usable */
+	/*

drivers/usb/gadget/epautoconf.c struct usb_ep *usb_ep_autoconfig_ss( · line 85

Quoted 10 lines

drivers/usb/gadget/epautoconf.c +7 -2

+	 * Second, look at endpoints until an unclaimed one looks usable.
+	 * Try to find one with smallest maxpacket limit, leaving larger
+	 * endpoints for heavier applications
+	 */
 	list_for_each_entry (ep, &gadget->ep_list, ep_list) {
-		if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
-			goto found_ep;
+		if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp)) {
+			if (desc->wMaxPacketSize == 0)
+				goto found_ep;

Why you do special handling for this? You still could give the smallest maxpacket_limit EP for it, right?

Peter

Quoted 19 lines

drivers/usb/gadget/epautoconf.c +9 -1

+			else if (!ep_min)
+				ep_min = ep;
+			else if (ep->maxpacket_limit < ep_min->maxpacket_limit)
+				ep_min = ep;
+		}
 	}
 
 	/* Fail */
-	return NULL;
+	if (!ep_min)
+		return NULL;
+
+	ep = ep_min;
 found_ep:
 
 	/*

Signature · 1 line

2.17.1

Signature · 2 lines

Thanks,
Peter Chen

Ruslan Bilovol

author

2020-07-03 10:46 UTC #2 lore raw

Quoted 46 lines · Peter Chen

Quoted 44 lines · Ruslan Bilovol
UDC hardware may have endpoints with different maxpacket size. Current endpoint matching code takes first matching endpoint from the list.
It's always possible that gadget allocates endpoints for small transfers (maxpacket size) first, then larger ones. That works fine if all matching UDC endpoints have same maxpacket size or are big enough to serve that allocation.
However, some UDCs have first endpoints in the list with bigger maxpacket size, whereas last endpoints are much smaller. In this case endpoint allocation will fail for the gadget (which allocates smaller endpoints first) on final endpoint allocations.
To make endpoint allocation fair, pick up smallest matching endpoints first, leaving bigger ones for heavier applications.
Signed-off-by Ruslan Bilovol <[email protected]>
---
v2: rebased onto latest balbi/next branch
 drivers/usb/gadget/epautoconf.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)
drivers/usb/gadget/epautoconf.c +1 -1
index 1eb4fa2e623f..6c453b5d87bb 100644
@@ -66,7 +66,7 @@ struct usb_ep *usb_ep_autoconfig_ss(
      struct usb_ss_ep_comp_descriptor *ep_comp
 )
 {
-     struct usb_ep   *ep;
+     struct usb_ep   *ep, *ep_min = NULL;
 
      if (gadget->ops->match_ep) {
              ep = gadget->ops->match_ep(gadget, desc, ep_comp);
@@ -74,14 +74,27 @@ struct usb_ep *usb_ep_autoconfig_ss(
                      goto found_ep;
      }
 

Quoted 15 lines

drivers/usb/gadget/epautoconf.c struct usb_ep *usb_ep_autoconfig_ss( · line 85

Quoted 12 lines

drivers/usb/gadget/epautoconf.c +8 -3

-     /* Second, look at endpoints until an unclaimed one looks usable */
+     /*
+      * Second, look at endpoints until an unclaimed one looks usable.
+      * Try to find one with smallest maxpacket limit, leaving larger
+      * endpoints for heavier applications
+      */
      list_for_each_entry (ep, &gadget->ep_list, ep_list) {
-             if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
-                     goto found_ep;
+             if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp)) {
+                     if (desc->wMaxPacketSize == 0)
+                             goto found_ep;

Why you do special handling for this? You still could give the smallest maxpacket_limit EP for it, right?

Of course it's technically possible. However in case "wMaxPacketSize == 0" gadget driver wants to get maximum possible wMaxPacketSize from endpoint configuration and I was thinking about avoiding regressions if we always provide smaller endpoints.

As I can see, providing smallest endpoint that matches requested wMaxPacketSize is OK, but if gadget driver just wants autoconf core to use it with maximum possible
value, I'm thinking now if we can even change this part and if wMaxPacketSize is zero, find endpoint with maximum possible wMaxPacketSize

Does it make sense?

Thanks
Ruslan

Quoted 27 lines

Peter

Quoted 17 lines

drivers/usb/gadget/epautoconf.c +9 -2

+                     else if (!ep_min)
+                             ep_min = ep;
+                     else if (ep->maxpacket_limit < ep_min->maxpacket_limit)
+                             ep_min = ep;
+             }
      }
 
      /* Fail */
-     return NULL;
+     if (!ep_min)
+             return NULL;
+
+     ep = ep_min;
 found_ep:
 
      /*
--

Quoted 2 lines

2.17.1

Thanks,
Peter Chen

Peter Chen

2020-07-04 10:42 UTC #3 lore raw

Quoted 63 lines · Ruslan Bilovol

Quoted 46 lines · Peter Chen
Quoted 44 lines · Ruslan Bilovol
UDC hardware may have endpoints with different maxpacket size. Current endpoint matching code takes first matching endpoint from the list.
It's always possible that gadget allocates endpoints for small transfers (maxpacket size) first, then larger ones. That works fine if all matching UDC endpoints have same maxpacket size or are big enough to serve that allocation.
However, some UDCs have first endpoints in the list with bigger maxpacket size, whereas last endpoints are much smaller. In this case endpoint allocation will fail for the gadget (which allocates smaller endpoints first) on final endpoint allocations.
To make endpoint allocation fair, pick up smallest matching endpoints first, leaving bigger ones for heavier applications.
Signed-off-by Ruslan Bilovol <[email protected]>
---
v2: rebased onto latest balbi/next branch
 drivers/usb/gadget/epautoconf.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)
drivers/usb/gadget/epautoconf.c +1 -1
index 1eb4fa2e623f..6c453b5d87bb 100644
@@ -66,7 +66,7 @@ struct usb_ep *usb_ep_autoconfig_ss(
      struct usb_ss_ep_comp_descriptor *ep_comp
 )
 {
-     struct usb_ep   *ep;
+     struct usb_ep   *ep, *ep_min = NULL;
 
      if (gadget->ops->match_ep) {
              ep = gadget->ops->match_ep(gadget, desc, ep_comp);
@@ -74,14 +74,27 @@ struct usb_ep *usb_ep_autoconfig_ss(
                      goto found_ep;
      }
 
Quoted 15 lines
drivers/usb/gadget/epautoconf.c struct usb_ep *usb_ep_autoconfig_ss( · line 85
Quoted 12 lines
drivers/usb/gadget/epautoconf.c +8 -3
-     /* Second, look at endpoints until an unclaimed one looks usable */
+     /*
+      * Second, look at endpoints until an unclaimed one looks usable.
+      * Try to find one with smallest maxpacket limit, leaving larger
+      * endpoints for heavier applications
+      */
      list_for_each_entry (ep, &gadget->ep_list, ep_list) {
-             if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
-                     goto found_ep;
+             if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp)) {
+                     if (desc->wMaxPacketSize == 0)
+                             goto found_ep;
Why you do special handling for this? You still could give the smallest maxpacket_limit EP for it, right?

Quoted 4 lines

Of course it's technically possible. However in case "wMaxPacketSize == 0" gadget driver wants to get maximum possible wMaxPacketSize from endpoint configuration and I was thinking about avoiding regressions if we always provide smaller endpoints.

You may only want to change the match logic, not but the special case.

Currently, it returns the first matched endpoint no matter "wMaxPacketSize == 0" or not. And you changed the match logic as returning the smallest maxPacketsize endpoint, you also don't need to consider whether "wMaxPacketSize == 0" or not, otherwise, it may introduce the complexity.

Peter

Quoted 39 lines

As I can see, providing smallest endpoint that matches requested wMaxPacketSize is OK, but if gadget driver just wants autoconf core to use it with maximum possible
value, I'm thinking now if we can even change this part and if wMaxPacketSize is zero, find endpoint with maximum possible wMaxPacketSize
Does it make sense?
Thanks
Ruslan
Quoted 27 lines
Peter
Quoted 17 lines
drivers/usb/gadget/epautoconf.c +9 -2
+                     else if (!ep_min)
+                             ep_min = ep;
+                     else if (ep->maxpacket_limit < ep_min->maxpacket_limit)
+                             ep_min = ep;
+             }
      }
 
      /* Fail */
-     return NULL;
+     if (!ep_min)
+             return NULL;
+
+     ep = ep_min;
 found_ep:
 
      /*
--
Quoted 2 lines
2.17.1
--
Thanks,
Peter Chen

Signature · 2 lines

Thanks,
Peter Chen

Ruslan Bilovol

author

2023-02-10 20:07 UTC #4 lore raw

Quoted 70 lines · Peter Chen

Quoted 63 lines · Ruslan Bilovol
Quoted 46 lines · Peter Chen
Quoted 44 lines · Ruslan Bilovol
UDC hardware may have endpoints with different maxpacket size. Current endpoint matching code takes first matching endpoint from the list.
It's always possible that gadget allocates endpoints for small transfers (maxpacket size) first, then larger ones. That works fine if all matching UDC endpoints have same maxpacket size or are big enough to serve that allocation.
However, some UDCs have first endpoints in the list with bigger maxpacket size, whereas last endpoints are much smaller. In this case endpoint allocation will fail for the gadget (which allocates smaller endpoints first) on final endpoint allocations.
To make endpoint allocation fair, pick up smallest matching endpoints first, leaving bigger ones for heavier applications.
Signed-off-by Ruslan Bilovol <[email protected]>
---
v2: rebased onto latest balbi/next branch
 drivers/usb/gadget/epautoconf.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)
drivers/usb/gadget/epautoconf.c +1 -1
index 1eb4fa2e623f..6c453b5d87bb 100644
@@ -66,7 +66,7 @@ struct usb_ep *usb_ep_autoconfig_ss(
      struct usb_ss_ep_comp_descriptor *ep_comp
 )
 {
-     struct usb_ep   *ep;
+     struct usb_ep   *ep, *ep_min = NULL;
 
      if (gadget->ops->match_ep) {
              ep = gadget->ops->match_ep(gadget, desc, ep_comp);
@@ -74,14 +74,27 @@ struct usb_ep *usb_ep_autoconfig_ss(
                      goto found_ep;
      }
 
Quoted 15 lines
drivers/usb/gadget/epautoconf.c struct usb_ep *usb_ep_autoconfig_ss( · line 85
Quoted 12 lines
drivers/usb/gadget/epautoconf.c +8 -3
-     /* Second, look at endpoints until an unclaimed one looks usable */
+     /*
+      * Second, look at endpoints until an unclaimed one looks usable.
+      * Try to find one with smallest maxpacket limit, leaving larger
+      * endpoints for heavier applications
+      */
      list_for_each_entry (ep, &gadget->ep_list, ep_list) {
-             if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
-                     goto found_ep;
+             if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp)) {
+                     if (desc->wMaxPacketSize == 0)
+                             goto found_ep;
Why you do special handling for this? You still could give the smallest maxpacket_limit EP for it, right?
Quoted 4 lines
Of course it's technically possible. However in case "wMaxPacketSize == 0" gadget driver wants to get maximum possible wMaxPacketSize from endpoint configuration and I was thinking about avoiding regressions if we always provide smaller endpoints.

Quoted 8 lines

You may only want to change the match logic, not but the special case.
Currently, it returns the first matched endpoint no matter "wMaxPacketSize == 0" or not. And you changed the match logic as returning the smallest maxPacketsize endpoint, you also don't need to consider whether "wMaxPacketSize == 0" or not, otherwise, it may introduce the complexity.

The reason I kept the old behavior when "wMaxPacketSize == 0" is because it's a special case. In this case a gadget driver wants to use a whole available MaxPacketSize of claimed endpoint. Since it doesn't know what MaxPacketSize may be in a particular UDC endpoint, it just relies on epautoconf core and gets what's available.
So the behavior looks like the opposite of claiming a smallest endpoint, but rather using as big as possible MaxPacketSize provided by the endpoint.

We even may want to implement a 'claim biggest endpoint' policy for the "wMaxPacketSize == 0" case.

Looking at gadget drivers sources, it seems there is no such usecase in any driver (at least with simple 'grep' over sources), so I'm not sure if we need to worry about it at all, that's why I decided to keep an old behavior here.

Thanks
Ruslan

Quoted 44 lines

Quoted 7 lines
As I can see, providing smallest endpoint that matches requested wMaxPacketSize is OK, but if gadget driver just wants autoconf core to use it with maximum possible
value, I'm thinking now if we can even change this part and if wMaxPacketSize is zero, find endpoint with maximum possible wMaxPacketSize
Quoted 32 lines
Does it make sense?
Thanks
Ruslan
Quoted 27 lines
Peter
Quoted 17 lines
drivers/usb/gadget/epautoconf.c +9 -2
+                     else if (!ep_min)
+                             ep_min = ep;
+                     else if (ep->maxpacket_limit < ep_min->maxpacket_limit)
+                             ep_min = ep;
+             }
      }
 
      /* Fail */
-     return NULL;
+     if (!ep_min)
+             return NULL;
+
+     ep = ep_min;
 found_ep:
 
      /*
--
Quoted 2 lines
2.17.1
--
Thanks,
Peter Chen
--
Thanks,
Peter Chen