Currently automatic gadget endpoint selection based on required features
doesn't work. Raw Gadget tries iterating over the list of available
endpoints and finding one that has the right direction and transfer type.
Unfortunately selecting arbitrary gadget endpoints (even if they satisfy
feature requirements) doesn't work, as (depending on the UDC driver) they
might have fixed addresses, and one also needs to provide matching
endpoint addresses in the descriptors sent to the host.
The composite framework deals with this by assigning endpoint addresses
in usb_ep_autoconfig() before enumeration starts. This approach won't work
with Raw Gadget as the endpoints are supposed to be enabled after a
set_configuration/set_interface request from the host, so it's too late to
patch the endpoint descriptors that had already been sent to the host.
For Raw Gadget we take the another approach. Similarly to GadgetFS, we
allow the user to make the decision as to which gadget endpoints to use.
This patch adds another Raw Gadget ioctl USB_RAW_IOCTL_EPS_INFO that
exposes information about all non-control endpoints that a currently
connected UDC has. This information includes endpoints addresses, as well
as their capabilities and limits to allow the user to chose the most
fitting gadget endpoint.
The USB_RAW_IOCTL_EP_ENABLE ioctl is updated to use the proper endpoint
validation routine usb_gadget_ep_match_desc() and also now accepts an
optional usb_ss_ep_comp_descriptor argument.
These changes affect the portability of the gadgets that use Raw Gadget
when running on different UDCs. Nevertheless, as long as the user relies
on the information provided by USB_RAW_IOCTL_EPS_INFO to dynamically
choose endpoint addresses, UDC-agnostic gadgets can still be written with
Raw Gadget.
Signed-off-by: Andrey Konovalov <[email protected]>
---
Documentation/usb/raw-gadget.rst | 6 +-
drivers/usb/gadget/legacy/raw_gadget.c | 194 ++++++++++++++++---------
include/uapi/linux/usb/raw_gadget.h | 84 ++++++++++-
3 files changed, 207 insertions(+), 77 deletions(-)
diff --git a/Documentation/usb/raw-gadget.rst b/Documentation/usb/raw-gadget.rst
index 9e78cb858f86..42bd446d72b2 100644
--- a/Documentation/usb/raw-gadget.rst
+++ b/Documentation/usb/raw-gadget.rst
@@ -27,11 +27,7 @@ differences are:
3. Raw Gadget provides a way to select a UDC device/driver to bind to,
while GadgetFS currently binds to the first available UDC.
-4. Raw Gadget uses predictable endpoint names (handles) across different
- UDCs (as long as UDCs have enough endpoints of each required transfer
- type).
-
-5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
+4. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
Userspace interface
~~~~~~~~~~~~~~~~~~~
diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
index 7b241992ad5a..cbf4b0572188 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -7,6 +7,7 @@
*/
#include <linux/compiler.h>
+#include <linux/ctype.h>
#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/kref.h>
@@ -123,8 +124,6 @@ static void raw_event_queue_destroy(struct raw_event_queue *queue)
struct raw_dev;
-#define USB_RAW_MAX_ENDPOINTS 32
-
enum ep_state {
STATE_EP_DISABLED,
STATE_EP_ENABLED,
@@ -134,6 +133,7 @@ struct raw_ep {
struct raw_dev *dev;
enum ep_state state;
struct usb_ep *ep;
+ u8 addr;
struct usb_request *req;
bool urb_queued;
bool disabling;
@@ -168,7 +168,8 @@ struct raw_dev {
bool ep0_out_pending;
bool ep0_urb_queued;
ssize_t ep0_status;
- struct raw_ep eps[USB_RAW_MAX_ENDPOINTS];
+ struct raw_ep eps[USB_RAW_EPS_NUM_MAX];
+ int eps_num;
struct completion ep0_done;
struct raw_event_queue queue;
@@ -202,7 +203,7 @@ static void dev_free(struct kref *kref)
usb_ep_free_request(dev->gadget->ep0, dev->req);
}
raw_event_queue_destroy(&dev->queue);
- for (i = 0; i < USB_RAW_MAX_ENDPOINTS; i++) {
+ for (i = 0; i < USB_RAW_EPS_NUM_MAX; i++) {
if (dev->eps[i].state != STATE_EP_ENABLED)
continue;
usb_ep_disable(dev->eps[i].ep);
@@ -249,12 +250,26 @@ static void gadget_ep0_complete(struct usb_ep *ep, struct usb_request *req)
complete(&dev->ep0_done);
}
+static u8 get_ep_addr(const char *name)
+{
+ /* If the endpoint has fixed function (named as e.g. "ep12out-bulk"),
+ * parse the endpoint address from its name. We deliberately use
+ * deprecated simple_strtoul() function here, as the number isn't
+ * followed by '\0' nor '\n'.
+ */
+ if (isdigit(name[2]))
+ return simple_strtoul(&name[2], NULL, 10);
+ /* Otherwise the endpoint is configurable (named as e.g. "ep-a"). */
+ return USB_RAW_EP_ADDR_ANY;
+}
+
static int gadget_bind(struct usb_gadget *gadget,
struct usb_gadget_driver *driver)
{
- int ret = 0;
+ int ret = 0, i = 0;
struct raw_dev *dev = container_of(driver, struct raw_dev, driver);
struct usb_request *req;
+ struct usb_ep *ep;
unsigned long flags;
if (strcmp(gadget->name, dev->udc_name) != 0)
@@ -273,6 +288,13 @@ static int gadget_bind(struct usb_gadget *gadget,
dev->req->context = dev;
dev->req->complete = gadget_ep0_complete;
dev->gadget = gadget;
+ gadget_for_each_ep(ep, dev->gadget) {
+ dev->eps[i].ep = ep;
+ dev->eps[i].addr = get_ep_addr(ep->name);
+ dev->eps[i].state = STATE_EP_DISABLED;
+ i++;
+ }
+ dev->eps_num = i;
spin_unlock_irqrestore(&dev->lock, flags);
/* Matches kref_put() in gadget_unbind(). */
@@ -555,7 +577,7 @@ static void *raw_alloc_io_data(struct usb_raw_ep_io *io, void __user *ptr,
if (copy_from_user(io, ptr, sizeof(*io)))
return ERR_PTR(-EFAULT);
- if (io->ep >= USB_RAW_MAX_ENDPOINTS)
+ if (io->ep >= USB_RAW_EPS_NUM_MAX)
return ERR_PTR(-EINVAL);
if (!usb_raw_io_flags_valid(io->flags))
return ERR_PTR(-EINVAL);
@@ -682,52 +704,30 @@ static int raw_ioctl_ep0_read(struct raw_dev *dev, unsigned long value)
return ret;
}
-static bool check_ep_caps(struct usb_ep *ep,
- struct usb_endpoint_descriptor *desc)
-{
- switch (usb_endpoint_type(desc)) {
- case USB_ENDPOINT_XFER_ISOC:
- if (!ep->caps.type_iso)
- return false;
- break;
- case USB_ENDPOINT_XFER_BULK:
- if (!ep->caps.type_bulk)
- return false;
- break;
- case USB_ENDPOINT_XFER_INT:
- if (!ep->caps.type_int)
- return false;
- break;
- default:
- return false;
- }
-
- if (usb_endpoint_dir_in(desc) && !ep->caps.dir_in)
- return false;
- if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out)
- return false;
-
- return true;
-}
-
static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
{
int ret = 0, i;
unsigned long flags;
- struct usb_endpoint_descriptor *desc;
- struct usb_ep *ep = NULL;
+ struct usb_raw_ep_descs *descs;
+ struct usb_endpoint_descriptor *ep_desc;
+ struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
+ struct raw_ep *ep;
- desc = memdup_user((void __user *)value, sizeof(*desc));
- if (IS_ERR(desc))
- return PTR_ERR(desc);
+ descs = memdup_user((void __user *)value, sizeof(*descs));
+ if (IS_ERR(descs))
+ return PTR_ERR(descs);
+
+ ep_desc = &descs->ep;
+ if (descs->comp.bLength != 0)
+ comp_desc = &descs->comp;
/*
* Endpoints with a maxpacket length of 0 can cause crashes in UDC
* drivers.
*/
- if (usb_endpoint_maxp(desc) == 0) {
+ if (usb_endpoint_maxp(ep_desc) == 0) {
dev_dbg(dev->dev, "fail, bad endpoint maxpacket\n");
- kfree(desc);
+ kfree(descs);
return -EINVAL;
}
@@ -743,41 +743,38 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
goto out_free;
}
- for (i = 0; i < USB_RAW_MAX_ENDPOINTS; i++) {
- if (dev->eps[i].state == STATE_EP_ENABLED)
+ for (i = 0; i < dev->eps_num; i++) {
+ ep = &dev->eps[i];
+ if (ep->state != STATE_EP_DISABLED)
continue;
- break;
- }
- if (i == USB_RAW_MAX_ENDPOINTS) {
- dev_dbg(&dev->gadget->dev,
- "fail, no device endpoints available\n");
- ret = -EBUSY;
- goto out_free;
- }
-
- gadget_for_each_ep(ep, dev->gadget) {
- if (ep->enabled)
+ if (ep->addr != usb_endpoint_num(ep_desc) &&
+ ep->addr != USB_RAW_EP_ADDR_ANY)
continue;
- if (!check_ep_caps(ep, desc))
+ if (!usb_gadget_ep_match_desc(dev->gadget, ep->ep,
+ ep_desc, comp_desc))
continue;
- ep->desc = desc;
- ret = usb_ep_enable(ep);
+ /* Gadget subsystem requires us to assign only a pointer to the
+ * endpoint descriptor here, but technically we store pointer
+ * to a usb_raw_ep_descs struct with both ep and comp
+ * descriptors to be later freed by raw_ioctl_ep_disable().
+ */
+ ep->ep->desc = ep_desc;
+ ret = usb_ep_enable(ep->ep);
if (ret < 0) {
dev_err(&dev->gadget->dev,
"fail, usb_ep_enable returned %d\n", ret);
goto out_free;
}
- dev->eps[i].req = usb_ep_alloc_request(ep, GFP_ATOMIC);
- if (!dev->eps[i].req) {
+ ep->req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC);
+ if (!ep->req) {
dev_err(&dev->gadget->dev,
"fail, usb_ep_alloc_request failed\n");
- usb_ep_disable(ep);
+ usb_ep_disable(ep->ep);
ret = -ENOMEM;
goto out_free;
}
- dev->eps[i].ep = ep;
- dev->eps[i].state = STATE_EP_ENABLED;
- ep->driver_data = &dev->eps[i];
+ ep->state = STATE_EP_ENABLED;
+ ep->ep->driver_data = ep;
ret = i;
goto out_unlock;
}
@@ -786,7 +783,7 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
ret = -EBUSY;
out_free:
- kfree(desc);
+ kfree(descs);
out_unlock:
spin_unlock_irqrestore(&dev->lock, flags);
return ret;
@@ -796,9 +793,9 @@ static int raw_ioctl_ep_disable(struct raw_dev *dev, unsigned long value)
{
int ret = 0, i = value;
unsigned long flags;
- const void *desc;
+ const void *descs;
- if (i < 0 || i >= USB_RAW_MAX_ENDPOINTS)
+ if (i < 0 || i >= USB_RAW_EPS_NUM_MAX)
return -EINVAL;
spin_lock_irqsave(&dev->lock, flags);
@@ -836,10 +833,10 @@ static int raw_ioctl_ep_disable(struct raw_dev *dev, unsigned long value)
spin_lock_irqsave(&dev->lock, flags);
usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
- desc = dev->eps[i].ep->desc;
+ descs = dev->eps[i].ep->desc;
dev->eps[i].ep = NULL;
dev->eps[i].state = STATE_EP_DISABLED;
- kfree(desc);
+ kfree(descs);
dev->eps[i].disabling = false;
out_unlock:
@@ -1027,6 +1024,64 @@ static int raw_ioctl_vbus_draw(struct raw_dev *dev, unsigned long value)
return ret;
}
+static void fill_ep_caps(struct usb_ep_caps *caps,
+ struct usb_raw_ep_caps *raw_caps)
+{
+ raw_caps->type_control = caps->type_control;
+ raw_caps->type_iso = caps->type_iso;
+ raw_caps->type_bulk = caps->type_bulk;
+ raw_caps->type_int = caps->type_int;
+ raw_caps->dir_in = caps->dir_in;
+ raw_caps->dir_out = caps->dir_out;
+}
+
+static void fill_ep_limits(struct usb_ep *ep, struct usb_raw_ep_limits *limits)
+{
+ limits->maxpacket_limit = ep->maxpacket_limit;
+ limits->max_streams = ep->max_streams;
+ limits->maxburst = ep->maxburst;
+}
+
+static int raw_ioctl_eps_info(struct raw_dev *dev, unsigned long value)
+{
+ int ret = 0, i;
+ unsigned long flags;
+ struct usb_raw_eps_info info;
+ struct raw_ep *ep;
+
+ spin_lock_irqsave(&dev->lock, flags);
+ if (dev->state != STATE_DEV_RUNNING) {
+ dev_dbg(dev->dev, "fail, device is not running\n");
+ ret = -EINVAL;
+ spin_unlock_irqrestore(&dev->lock, flags);
+ goto out;
+ }
+ if (!dev->gadget) {
+ dev_dbg(dev->dev, "fail, gadget is not bound\n");
+ ret = -EBUSY;
+ spin_unlock_irqrestore(&dev->lock, flags);
+ goto out;
+ }
+
+ memset(&info, 0, sizeof(info));
+ for (i = 0; i < dev->eps_num; i++) {
+ ep = &dev->eps[i];
+ strscpy(&info.eps[i].name[0], ep->ep->name,
+ USB_RAW_EP_NAME_MAX);
+ info.eps[i].addr = ep->addr;
+ fill_ep_caps(&ep->ep->caps, &info.eps[i].caps);
+ fill_ep_limits(ep->ep, &info.eps[i].limits);
+ }
+ ret = dev->eps_num;
+ spin_unlock_irqrestore(&dev->lock, flags);
+
+ if (copy_to_user((void __user *)value, &info, sizeof(info)))
+ ret = -EFAULT;
+
+out:
+ return ret;
+}
+
static long raw_ioctl(struct file *fd, unsigned int cmd, unsigned long value)
{
struct raw_dev *dev = fd->private_data;
@@ -1069,6 +1124,9 @@ static long raw_ioctl(struct file *fd, unsigned int cmd, unsigned long value)
case USB_RAW_IOCTL_VBUS_DRAW:
ret = raw_ioctl_vbus_draw(dev, value);
break;
+ case USB_RAW_IOCTL_EPS_INFO:
+ ret = raw_ioctl_eps_info(dev, value);
+ break;
default:
ret = -EINVAL;
}
diff --git a/include/uapi/linux/usb/raw_gadget.h b/include/uapi/linux/usb/raw_gadget.h
index 8544802b25bd..6ae626d17f0c 100644
--- a/include/uapi/linux/usb/raw_gadget.h
+++ b/include/uapi/linux/usb/raw_gadget.h
@@ -93,6 +93,75 @@ struct usb_raw_ep_io {
__u8 data[0];
};
+/*
+ * struct usb_raw_ep_descs - argument for USB_RAW_IOCTL_EP_ENABLE ioctl.
+ * @ep: Endpoint descriptor.
+ * @comp: SuperSpeed Endpoint Companion descriptor.
+ *
+ * Both of these descriptors are only used by the gadget subsystem including
+ * the UDC driver and don't affect the descriptors that are sent to the host.
+ * However, the user must make sure that the host and the gadget sides agree
+ * on the used endpoint parameters (such as e.g. endpoint addresses).
+ */
+struct usb_raw_ep_descs {
+ struct usb_endpoint_descriptor ep;
+ struct usb_ss_ep_comp_descriptor comp;
+};
+
+/* Maximum number of non-control endpoints in struct usb_raw_eps_info. */
+#define USB_RAW_EPS_NUM_MAX 30
+
+/* Maximum length of UDC endpoint name in struct usb_raw_ep_info. */
+#define USB_RAW_EP_NAME_MAX 16
+
+/* Used as addr in struct usb_raw_ep_info if endpoint accepts any address. */
+#define USB_RAW_EP_ADDR_ANY 0xff
+
+/*
+ * struct usb_raw_ep_caps - exposes endpoint capabilities from
+ * struct usb_ep_caps.
+ */
+struct usb_raw_ep_caps {
+ __u32 type_control:1;
+ __u32 type_iso:1;
+ __u32 type_bulk:1;
+ __u32 type_int:1;
+ __u32 dir_in:1;
+ __u32 dir_out:1;
+};
+
+/*
+ * struct usb_raw_ep_limits - exposes endpoint limits from struct usb_ep.
+ */
+struct usb_raw_ep_limits {
+ __u32 maxpacket_limit;
+ __u32 max_streams;
+ __u32 maxburst;
+};
+
+/*
+ * struct usb_raw_ep_info - stores information about a gadget endpoint.
+ * @name: Name of the endpoint as it is defined in the UDC driver.
+ * @addr: Address of the endpoint that must be specified in the endpoint
+ * descriptor passed to USB_RAW_IOCTL_EP_ENABLE ioctl.
+ * @caps: Endpoint capabilities.
+ * @limits: Endpoint limits.
+ */
+struct usb_raw_ep_info {
+ __u8 name[USB_RAW_EP_NAME_MAX];
+ __u32 addr;
+ struct usb_raw_ep_caps caps;
+ struct usb_raw_ep_limits limits;
+};
+
+/*
+ * struct usb_raw_eps_info - argument for USB_RAW_IOCTL_EPS_INFO ioctl.
+ * eps: Structures that store information about non-control endpoints.
+ */
+struct usb_raw_eps_info {
+ struct usb_raw_ep_info eps[USB_RAW_EPS_NUM_MAX];
+};
+
/*
* Initializes a Raw Gadget instance.
* Accepts a pointer to the usb_raw_init struct as an argument.
@@ -126,12 +195,12 @@ struct usb_raw_ep_io {
#define USB_RAW_IOCTL_EP0_READ _IOWR('U', 4, struct usb_raw_ep_io)
/*
- * Finds an endpoint that supports the transfer type specified in the
- * descriptor and enables it.
- * Accepts a pointer to the usb_endpoint_descriptor struct as an argument.
+ * Finds an endpoint that satisfies the parameters specified in the provided
+ * descriptors (address, transfer type, etc.) and enables it.
+ * Accepts a pointer to the usb_raw_ep_descs struct as an argument.
* Returns enabled endpoint handle on success or negative error code on failure.
*/
-#define USB_RAW_IOCTL_EP_ENABLE _IOW('U', 5, struct usb_endpoint_descriptor)
+#define USB_RAW_IOCTL_EP_ENABLE _IOW('U', 5, struct usb_raw_ep_descs)
/* Disables specified endpoint.
* Accepts endpoint handle as an argument.
@@ -164,4 +233,11 @@ struct usb_raw_ep_io {
*/
#define USB_RAW_IOCTL_VBUS_DRAW _IOW('U', 10, __u32)
+/* Fills in the usb_raw_eps_info structure with information about non-control
+ * endpoints available for the currently connected UDC.
+ * Returns the number of available endpoints on success or negative error code
+ * on failure.
+ */
+#define USB_RAW_IOCTL_EPS_INFO _IOR('U', 11, struct usb_raw_eps_info)
+
#endif /* _UAPI__LINUX_USB_RAW_GADGET_H */
--
2.26.2.303.gf8c07b1a785-goog
On Tue, Apr 28, 2020 at 9:46 PM Andrey Konovalov <[email protected]> wrote:
>
> Currently automatic gadget endpoint selection based on required features
> doesn't work. Raw Gadget tries iterating over the list of available
> endpoints and finding one that has the right direction and transfer type.
> Unfortunately selecting arbitrary gadget endpoints (even if they satisfy
> feature requirements) doesn't work, as (depending on the UDC driver) they
> might have fixed addresses, and one also needs to provide matching
> endpoint addresses in the descriptors sent to the host.
>
> The composite framework deals with this by assigning endpoint addresses
> in usb_ep_autoconfig() before enumeration starts. This approach won't work
> with Raw Gadget as the endpoints are supposed to be enabled after a
> set_configuration/set_interface request from the host, so it's too late to
> patch the endpoint descriptors that had already been sent to the host.
>
> For Raw Gadget we take the another approach. Similarly to GadgetFS, we
> allow the user to make the decision as to which gadget endpoints to use.
>
> This patch adds another Raw Gadget ioctl USB_RAW_IOCTL_EPS_INFO that
> exposes information about all non-control endpoints that a currently
> connected UDC has. This information includes endpoints addresses, as well
> as their capabilities and limits to allow the user to chose the most
> fitting gadget endpoint.
>
> The USB_RAW_IOCTL_EP_ENABLE ioctl is updated to use the proper endpoint
> validation routine usb_gadget_ep_match_desc() and also now accepts an
> optional usb_ss_ep_comp_descriptor argument.
>
> These changes affect the portability of the gadgets that use Raw Gadget
> when running on different UDCs. Nevertheless, as long as the user relies
> on the information provided by USB_RAW_IOCTL_EPS_INFO to dynamically
> choose endpoint addresses, UDC-agnostic gadgets can still be written with
> Raw Gadget.
>
> Signed-off-by: Andrey Konovalov <[email protected]>
Sorry, forgot:
Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface")
> ---
> Documentation/usb/raw-gadget.rst | 6 +-
> drivers/usb/gadget/legacy/raw_gadget.c | 194 ++++++++++++++++---------
> include/uapi/linux/usb/raw_gadget.h | 84 ++++++++++-
> 3 files changed, 207 insertions(+), 77 deletions(-)
>
> diff --git a/Documentation/usb/raw-gadget.rst b/Documentation/usb/raw-gadget.rst
> index 9e78cb858f86..42bd446d72b2 100644
> --- a/Documentation/usb/raw-gadget.rst
> +++ b/Documentation/usb/raw-gadget.rst
> @@ -27,11 +27,7 @@ differences are:
> 3. Raw Gadget provides a way to select a UDC device/driver to bind to,
> while GadgetFS currently binds to the first available UDC.
>
> -4. Raw Gadget uses predictable endpoint names (handles) across different
> - UDCs (as long as UDCs have enough endpoints of each required transfer
> - type).
> -
> -5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
> +4. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
>
> Userspace interface
> ~~~~~~~~~~~~~~~~~~~
> diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> index 7b241992ad5a..cbf4b0572188 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/compiler.h>
> +#include <linux/ctype.h>
> #include <linux/debugfs.h>
> #include <linux/delay.h>
> #include <linux/kref.h>
> @@ -123,8 +124,6 @@ static void raw_event_queue_destroy(struct raw_event_queue *queue)
>
> struct raw_dev;
>
> -#define USB_RAW_MAX_ENDPOINTS 32
> -
> enum ep_state {
> STATE_EP_DISABLED,
> STATE_EP_ENABLED,
> @@ -134,6 +133,7 @@ struct raw_ep {
> struct raw_dev *dev;
> enum ep_state state;
> struct usb_ep *ep;
> + u8 addr;
> struct usb_request *req;
> bool urb_queued;
> bool disabling;
> @@ -168,7 +168,8 @@ struct raw_dev {
> bool ep0_out_pending;
> bool ep0_urb_queued;
> ssize_t ep0_status;
> - struct raw_ep eps[USB_RAW_MAX_ENDPOINTS];
> + struct raw_ep eps[USB_RAW_EPS_NUM_MAX];
> + int eps_num;
>
> struct completion ep0_done;
> struct raw_event_queue queue;
> @@ -202,7 +203,7 @@ static void dev_free(struct kref *kref)
> usb_ep_free_request(dev->gadget->ep0, dev->req);
> }
> raw_event_queue_destroy(&dev->queue);
> - for (i = 0; i < USB_RAW_MAX_ENDPOINTS; i++) {
> + for (i = 0; i < USB_RAW_EPS_NUM_MAX; i++) {
> if (dev->eps[i].state != STATE_EP_ENABLED)
> continue;
> usb_ep_disable(dev->eps[i].ep);
> @@ -249,12 +250,26 @@ static void gadget_ep0_complete(struct usb_ep *ep, struct usb_request *req)
> complete(&dev->ep0_done);
> }
>
> +static u8 get_ep_addr(const char *name)
> +{
> + /* If the endpoint has fixed function (named as e.g. "ep12out-bulk"),
> + * parse the endpoint address from its name. We deliberately use
> + * deprecated simple_strtoul() function here, as the number isn't
> + * followed by '\0' nor '\n'.
> + */
> + if (isdigit(name[2]))
> + return simple_strtoul(&name[2], NULL, 10);
> + /* Otherwise the endpoint is configurable (named as e.g. "ep-a"). */
> + return USB_RAW_EP_ADDR_ANY;
> +}
> +
> static int gadget_bind(struct usb_gadget *gadget,
> struct usb_gadget_driver *driver)
> {
> - int ret = 0;
> + int ret = 0, i = 0;
> struct raw_dev *dev = container_of(driver, struct raw_dev, driver);
> struct usb_request *req;
> + struct usb_ep *ep;
> unsigned long flags;
>
> if (strcmp(gadget->name, dev->udc_name) != 0)
> @@ -273,6 +288,13 @@ static int gadget_bind(struct usb_gadget *gadget,
> dev->req->context = dev;
> dev->req->complete = gadget_ep0_complete;
> dev->gadget = gadget;
> + gadget_for_each_ep(ep, dev->gadget) {
> + dev->eps[i].ep = ep;
> + dev->eps[i].addr = get_ep_addr(ep->name);
> + dev->eps[i].state = STATE_EP_DISABLED;
> + i++;
> + }
> + dev->eps_num = i;
> spin_unlock_irqrestore(&dev->lock, flags);
>
> /* Matches kref_put() in gadget_unbind(). */
> @@ -555,7 +577,7 @@ static void *raw_alloc_io_data(struct usb_raw_ep_io *io, void __user *ptr,
>
> if (copy_from_user(io, ptr, sizeof(*io)))
> return ERR_PTR(-EFAULT);
> - if (io->ep >= USB_RAW_MAX_ENDPOINTS)
> + if (io->ep >= USB_RAW_EPS_NUM_MAX)
> return ERR_PTR(-EINVAL);
> if (!usb_raw_io_flags_valid(io->flags))
> return ERR_PTR(-EINVAL);
> @@ -682,52 +704,30 @@ static int raw_ioctl_ep0_read(struct raw_dev *dev, unsigned long value)
> return ret;
> }
>
> -static bool check_ep_caps(struct usb_ep *ep,
> - struct usb_endpoint_descriptor *desc)
> -{
> - switch (usb_endpoint_type(desc)) {
> - case USB_ENDPOINT_XFER_ISOC:
> - if (!ep->caps.type_iso)
> - return false;
> - break;
> - case USB_ENDPOINT_XFER_BULK:
> - if (!ep->caps.type_bulk)
> - return false;
> - break;
> - case USB_ENDPOINT_XFER_INT:
> - if (!ep->caps.type_int)
> - return false;
> - break;
> - default:
> - return false;
> - }
> -
> - if (usb_endpoint_dir_in(desc) && !ep->caps.dir_in)
> - return false;
> - if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out)
> - return false;
> -
> - return true;
> -}
> -
> static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
> {
> int ret = 0, i;
> unsigned long flags;
> - struct usb_endpoint_descriptor *desc;
> - struct usb_ep *ep = NULL;
> + struct usb_raw_ep_descs *descs;
> + struct usb_endpoint_descriptor *ep_desc;
> + struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
> + struct raw_ep *ep;
>
> - desc = memdup_user((void __user *)value, sizeof(*desc));
> - if (IS_ERR(desc))
> - return PTR_ERR(desc);
> + descs = memdup_user((void __user *)value, sizeof(*descs));
> + if (IS_ERR(descs))
> + return PTR_ERR(descs);
> +
> + ep_desc = &descs->ep;
> + if (descs->comp.bLength != 0)
> + comp_desc = &descs->comp;
>
> /*
> * Endpoints with a maxpacket length of 0 can cause crashes in UDC
> * drivers.
> */
> - if (usb_endpoint_maxp(desc) == 0) {
> + if (usb_endpoint_maxp(ep_desc) == 0) {
> dev_dbg(dev->dev, "fail, bad endpoint maxpacket\n");
> - kfree(desc);
> + kfree(descs);
> return -EINVAL;
> }
>
> @@ -743,41 +743,38 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
> goto out_free;
> }
>
> - for (i = 0; i < USB_RAW_MAX_ENDPOINTS; i++) {
> - if (dev->eps[i].state == STATE_EP_ENABLED)
> + for (i = 0; i < dev->eps_num; i++) {
> + ep = &dev->eps[i];
> + if (ep->state != STATE_EP_DISABLED)
> continue;
> - break;
> - }
> - if (i == USB_RAW_MAX_ENDPOINTS) {
> - dev_dbg(&dev->gadget->dev,
> - "fail, no device endpoints available\n");
> - ret = -EBUSY;
> - goto out_free;
> - }
> -
> - gadget_for_each_ep(ep, dev->gadget) {
> - if (ep->enabled)
> + if (ep->addr != usb_endpoint_num(ep_desc) &&
> + ep->addr != USB_RAW_EP_ADDR_ANY)
> continue;
> - if (!check_ep_caps(ep, desc))
> + if (!usb_gadget_ep_match_desc(dev->gadget, ep->ep,
> + ep_desc, comp_desc))
> continue;
> - ep->desc = desc;
> - ret = usb_ep_enable(ep);
> + /* Gadget subsystem requires us to assign only a pointer to the
> + * endpoint descriptor here, but technically we store pointer
> + * to a usb_raw_ep_descs struct with both ep and comp
> + * descriptors to be later freed by raw_ioctl_ep_disable().
> + */
> + ep->ep->desc = ep_desc;
> + ret = usb_ep_enable(ep->ep);
> if (ret < 0) {
> dev_err(&dev->gadget->dev,
> "fail, usb_ep_enable returned %d\n", ret);
> goto out_free;
> }
> - dev->eps[i].req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> - if (!dev->eps[i].req) {
> + ep->req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC);
> + if (!ep->req) {
> dev_err(&dev->gadget->dev,
> "fail, usb_ep_alloc_request failed\n");
> - usb_ep_disable(ep);
> + usb_ep_disable(ep->ep);
> ret = -ENOMEM;
> goto out_free;
> }
> - dev->eps[i].ep = ep;
> - dev->eps[i].state = STATE_EP_ENABLED;
> - ep->driver_data = &dev->eps[i];
> + ep->state = STATE_EP_ENABLED;
> + ep->ep->driver_data = ep;
> ret = i;
> goto out_unlock;
> }
> @@ -786,7 +783,7 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
> ret = -EBUSY;
>
> out_free:
> - kfree(desc);
> + kfree(descs);
> out_unlock:
> spin_unlock_irqrestore(&dev->lock, flags);
> return ret;
> @@ -796,9 +793,9 @@ static int raw_ioctl_ep_disable(struct raw_dev *dev, unsigned long value)
> {
> int ret = 0, i = value;
> unsigned long flags;
> - const void *desc;
> + const void *descs;
>
> - if (i < 0 || i >= USB_RAW_MAX_ENDPOINTS)
> + if (i < 0 || i >= USB_RAW_EPS_NUM_MAX)
> return -EINVAL;
>
> spin_lock_irqsave(&dev->lock, flags);
> @@ -836,10 +833,10 @@ static int raw_ioctl_ep_disable(struct raw_dev *dev, unsigned long value)
>
> spin_lock_irqsave(&dev->lock, flags);
> usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
> - desc = dev->eps[i].ep->desc;
> + descs = dev->eps[i].ep->desc;
> dev->eps[i].ep = NULL;
> dev->eps[i].state = STATE_EP_DISABLED;
> - kfree(desc);
> + kfree(descs);
> dev->eps[i].disabling = false;
>
> out_unlock:
> @@ -1027,6 +1024,64 @@ static int raw_ioctl_vbus_draw(struct raw_dev *dev, unsigned long value)
> return ret;
> }
>
> +static void fill_ep_caps(struct usb_ep_caps *caps,
> + struct usb_raw_ep_caps *raw_caps)
> +{
> + raw_caps->type_control = caps->type_control;
> + raw_caps->type_iso = caps->type_iso;
> + raw_caps->type_bulk = caps->type_bulk;
> + raw_caps->type_int = caps->type_int;
> + raw_caps->dir_in = caps->dir_in;
> + raw_caps->dir_out = caps->dir_out;
> +}
> +
> +static void fill_ep_limits(struct usb_ep *ep, struct usb_raw_ep_limits *limits)
> +{
> + limits->maxpacket_limit = ep->maxpacket_limit;
> + limits->max_streams = ep->max_streams;
> + limits->maxburst = ep->maxburst;
> +}
> +
> +static int raw_ioctl_eps_info(struct raw_dev *dev, unsigned long value)
> +{
> + int ret = 0, i;
> + unsigned long flags;
> + struct usb_raw_eps_info info;
> + struct raw_ep *ep;
> +
> + spin_lock_irqsave(&dev->lock, flags);
> + if (dev->state != STATE_DEV_RUNNING) {
> + dev_dbg(dev->dev, "fail, device is not running\n");
> + ret = -EINVAL;
> + spin_unlock_irqrestore(&dev->lock, flags);
> + goto out;
> + }
> + if (!dev->gadget) {
> + dev_dbg(dev->dev, "fail, gadget is not bound\n");
> + ret = -EBUSY;
> + spin_unlock_irqrestore(&dev->lock, flags);
> + goto out;
> + }
> +
> + memset(&info, 0, sizeof(info));
> + for (i = 0; i < dev->eps_num; i++) {
> + ep = &dev->eps[i];
> + strscpy(&info.eps[i].name[0], ep->ep->name,
> + USB_RAW_EP_NAME_MAX);
> + info.eps[i].addr = ep->addr;
> + fill_ep_caps(&ep->ep->caps, &info.eps[i].caps);
> + fill_ep_limits(ep->ep, &info.eps[i].limits);
> + }
> + ret = dev->eps_num;
> + spin_unlock_irqrestore(&dev->lock, flags);
> +
> + if (copy_to_user((void __user *)value, &info, sizeof(info)))
> + ret = -EFAULT;
> +
> +out:
> + return ret;
> +}
> +
> static long raw_ioctl(struct file *fd, unsigned int cmd, unsigned long value)
> {
> struct raw_dev *dev = fd->private_data;
> @@ -1069,6 +1124,9 @@ static long raw_ioctl(struct file *fd, unsigned int cmd, unsigned long value)
> case USB_RAW_IOCTL_VBUS_DRAW:
> ret = raw_ioctl_vbus_draw(dev, value);
> break;
> + case USB_RAW_IOCTL_EPS_INFO:
> + ret = raw_ioctl_eps_info(dev, value);
> + break;
> default:
> ret = -EINVAL;
> }
> diff --git a/include/uapi/linux/usb/raw_gadget.h b/include/uapi/linux/usb/raw_gadget.h
> index 8544802b25bd..6ae626d17f0c 100644
> --- a/include/uapi/linux/usb/raw_gadget.h
> +++ b/include/uapi/linux/usb/raw_gadget.h
> @@ -93,6 +93,75 @@ struct usb_raw_ep_io {
> __u8 data[0];
> };
>
> +/*
> + * struct usb_raw_ep_descs - argument for USB_RAW_IOCTL_EP_ENABLE ioctl.
> + * @ep: Endpoint descriptor.
> + * @comp: SuperSpeed Endpoint Companion descriptor.
> + *
> + * Both of these descriptors are only used by the gadget subsystem including
> + * the UDC driver and don't affect the descriptors that are sent to the host.
> + * However, the user must make sure that the host and the gadget sides agree
> + * on the used endpoint parameters (such as e.g. endpoint addresses).
> + */
> +struct usb_raw_ep_descs {
> + struct usb_endpoint_descriptor ep;
> + struct usb_ss_ep_comp_descriptor comp;
> +};
> +
> +/* Maximum number of non-control endpoints in struct usb_raw_eps_info. */
> +#define USB_RAW_EPS_NUM_MAX 30
> +
> +/* Maximum length of UDC endpoint name in struct usb_raw_ep_info. */
> +#define USB_RAW_EP_NAME_MAX 16
> +
> +/* Used as addr in struct usb_raw_ep_info if endpoint accepts any address. */
> +#define USB_RAW_EP_ADDR_ANY 0xff
> +
> +/*
> + * struct usb_raw_ep_caps - exposes endpoint capabilities from
> + * struct usb_ep_caps.
> + */
> +struct usb_raw_ep_caps {
> + __u32 type_control:1;
> + __u32 type_iso:1;
> + __u32 type_bulk:1;
> + __u32 type_int:1;
> + __u32 dir_in:1;
> + __u32 dir_out:1;
> +};
> +
> +/*
> + * struct usb_raw_ep_limits - exposes endpoint limits from struct usb_ep.
> + */
> +struct usb_raw_ep_limits {
> + __u32 maxpacket_limit;
> + __u32 max_streams;
> + __u32 maxburst;
> +};
> +
> +/*
> + * struct usb_raw_ep_info - stores information about a gadget endpoint.
> + * @name: Name of the endpoint as it is defined in the UDC driver.
> + * @addr: Address of the endpoint that must be specified in the endpoint
> + * descriptor passed to USB_RAW_IOCTL_EP_ENABLE ioctl.
> + * @caps: Endpoint capabilities.
> + * @limits: Endpoint limits.
> + */
> +struct usb_raw_ep_info {
> + __u8 name[USB_RAW_EP_NAME_MAX];
> + __u32 addr;
> + struct usb_raw_ep_caps caps;
> + struct usb_raw_ep_limits limits;
> +};
> +
> +/*
> + * struct usb_raw_eps_info - argument for USB_RAW_IOCTL_EPS_INFO ioctl.
> + * eps: Structures that store information about non-control endpoints.
> + */
> +struct usb_raw_eps_info {
> + struct usb_raw_ep_info eps[USB_RAW_EPS_NUM_MAX];
> +};
> +
> /*
> * Initializes a Raw Gadget instance.
> * Accepts a pointer to the usb_raw_init struct as an argument.
> @@ -126,12 +195,12 @@ struct usb_raw_ep_io {
> #define USB_RAW_IOCTL_EP0_READ _IOWR('U', 4, struct usb_raw_ep_io)
>
> /*
> - * Finds an endpoint that supports the transfer type specified in the
> - * descriptor and enables it.
> - * Accepts a pointer to the usb_endpoint_descriptor struct as an argument.
> + * Finds an endpoint that satisfies the parameters specified in the provided
> + * descriptors (address, transfer type, etc.) and enables it.
> + * Accepts a pointer to the usb_raw_ep_descs struct as an argument.
> * Returns enabled endpoint handle on success or negative error code on failure.
> */
> -#define USB_RAW_IOCTL_EP_ENABLE _IOW('U', 5, struct usb_endpoint_descriptor)
> +#define USB_RAW_IOCTL_EP_ENABLE _IOW('U', 5, struct usb_raw_ep_descs)
>
> /* Disables specified endpoint.
> * Accepts endpoint handle as an argument.
> @@ -164,4 +233,11 @@ struct usb_raw_ep_io {
> */
> #define USB_RAW_IOCTL_VBUS_DRAW _IOW('U', 10, __u32)
>
> +/* Fills in the usb_raw_eps_info structure with information about non-control
> + * endpoints available for the currently connected UDC.
> + * Returns the number of available endpoints on success or negative error code
> + * on failure.
> + */
> +#define USB_RAW_IOCTL_EPS_INFO _IOR('U', 11, struct usb_raw_eps_info)
> +
> #endif /* _UAPI__LINUX_USB_RAW_GADGET_H */
> --
> 2.26.2.303.gf8c07b1a785-goog
>
On Tue, Apr 28, 2020 at 9:46 PM Andrey Konovalov <[email protected]> wrote:
>
> Currently automatic gadget endpoint selection based on required features
> doesn't work. Raw Gadget tries iterating over the list of available
> endpoints and finding one that has the right direction and transfer type.
> Unfortunately selecting arbitrary gadget endpoints (even if they satisfy
> feature requirements) doesn't work, as (depending on the UDC driver) they
> might have fixed addresses, and one also needs to provide matching
> endpoint addresses in the descriptors sent to the host.
>
> The composite framework deals with this by assigning endpoint addresses
> in usb_ep_autoconfig() before enumeration starts. This approach won't work
> with Raw Gadget as the endpoints are supposed to be enabled after a
> set_configuration/set_interface request from the host, so it's too late to
> patch the endpoint descriptors that had already been sent to the host.
>
> For Raw Gadget we take the another approach. Similarly to GadgetFS, we
> allow the user to make the decision as to which gadget endpoints to use.
>
> This patch adds another Raw Gadget ioctl USB_RAW_IOCTL_EPS_INFO that
> exposes information about all non-control endpoints that a currently
> connected UDC has. This information includes endpoints addresses, as well
> as their capabilities and limits to allow the user to chose the most
> fitting gadget endpoint.
>
> The USB_RAW_IOCTL_EP_ENABLE ioctl is updated to use the proper endpoint
> validation routine usb_gadget_ep_match_desc() and also now accepts an
> optional usb_ss_ep_comp_descriptor argument.
>
> These changes affect the portability of the gadgets that use Raw Gadget
> when running on different UDCs. Nevertheless, as long as the user relies
> on the information provided by USB_RAW_IOCTL_EPS_INFO to dynamically
> choose endpoint addresses, UDC-agnostic gadgets can still be written with
> Raw Gadget.
>
> Signed-off-by: Andrey Konovalov <[email protected]>
Hi Alan,
This patch uses the approach that I mentioned in the discussion about
endpoint selection. Does this look acceptable?
I'm not sure which endpoint limits it makes sense to expose via
USB_RAW_IOCTL_EPS_INFO. I'm more or less sure about maxpacket_limit
and max_streams, but I don't exactly know what maxburst is used for.
Maybe there are some others?
I also wonder if we need to expose ep0 limits via USB_RAW_IOCTL_EPS_INFO too.
expose ep0 parameters?
Thanks!
> ---
> Documentation/usb/raw-gadget.rst | 6 +-
> drivers/usb/gadget/legacy/raw_gadget.c | 194 ++++++++++++++++---------
> include/uapi/linux/usb/raw_gadget.h | 84 ++++++++++-
> 3 files changed, 207 insertions(+), 77 deletions(-)
>
> diff --git a/Documentation/usb/raw-gadget.rst b/Documentation/usb/raw-gadget.rst
> index 9e78cb858f86..42bd446d72b2 100644
> --- a/Documentation/usb/raw-gadget.rst
> +++ b/Documentation/usb/raw-gadget.rst
> @@ -27,11 +27,7 @@ differences are:
> 3. Raw Gadget provides a way to select a UDC device/driver to bind to,
> while GadgetFS currently binds to the first available UDC.
>
> -4. Raw Gadget uses predictable endpoint names (handles) across different
> - UDCs (as long as UDCs have enough endpoints of each required transfer
> - type).
> -
> -5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
> +4. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
>
> Userspace interface
> ~~~~~~~~~~~~~~~~~~~
> diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> index 7b241992ad5a..cbf4b0572188 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/compiler.h>
> +#include <linux/ctype.h>
> #include <linux/debugfs.h>
> #include <linux/delay.h>
> #include <linux/kref.h>
> @@ -123,8 +124,6 @@ static void raw_event_queue_destroy(struct raw_event_queue *queue)
>
> struct raw_dev;
>
> -#define USB_RAW_MAX_ENDPOINTS 32
> -
> enum ep_state {
> STATE_EP_DISABLED,
> STATE_EP_ENABLED,
> @@ -134,6 +133,7 @@ struct raw_ep {
> struct raw_dev *dev;
> enum ep_state state;
> struct usb_ep *ep;
> + u8 addr;
> struct usb_request *req;
> bool urb_queued;
> bool disabling;
> @@ -168,7 +168,8 @@ struct raw_dev {
> bool ep0_out_pending;
> bool ep0_urb_queued;
> ssize_t ep0_status;
> - struct raw_ep eps[USB_RAW_MAX_ENDPOINTS];
> + struct raw_ep eps[USB_RAW_EPS_NUM_MAX];
> + int eps_num;
>
> struct completion ep0_done;
> struct raw_event_queue queue;
> @@ -202,7 +203,7 @@ static void dev_free(struct kref *kref)
> usb_ep_free_request(dev->gadget->ep0, dev->req);
> }
> raw_event_queue_destroy(&dev->queue);
> - for (i = 0; i < USB_RAW_MAX_ENDPOINTS; i++) {
> + for (i = 0; i < USB_RAW_EPS_NUM_MAX; i++) {
> if (dev->eps[i].state != STATE_EP_ENABLED)
> continue;
> usb_ep_disable(dev->eps[i].ep);
> @@ -249,12 +250,26 @@ static void gadget_ep0_complete(struct usb_ep *ep, struct usb_request *req)
> complete(&dev->ep0_done);
> }
>
> +static u8 get_ep_addr(const char *name)
> +{
> + /* If the endpoint has fixed function (named as e.g. "ep12out-bulk"),
> + * parse the endpoint address from its name. We deliberately use
> + * deprecated simple_strtoul() function here, as the number isn't
> + * followed by '\0' nor '\n'.
> + */
> + if (isdigit(name[2]))
> + return simple_strtoul(&name[2], NULL, 10);
> + /* Otherwise the endpoint is configurable (named as e.g. "ep-a"). */
> + return USB_RAW_EP_ADDR_ANY;
> +}
> +
> static int gadget_bind(struct usb_gadget *gadget,
> struct usb_gadget_driver *driver)
> {
> - int ret = 0;
> + int ret = 0, i = 0;
> struct raw_dev *dev = container_of(driver, struct raw_dev, driver);
> struct usb_request *req;
> + struct usb_ep *ep;
> unsigned long flags;
>
> if (strcmp(gadget->name, dev->udc_name) != 0)
> @@ -273,6 +288,13 @@ static int gadget_bind(struct usb_gadget *gadget,
> dev->req->context = dev;
> dev->req->complete = gadget_ep0_complete;
> dev->gadget = gadget;
> + gadget_for_each_ep(ep, dev->gadget) {
> + dev->eps[i].ep = ep;
> + dev->eps[i].addr = get_ep_addr(ep->name);
> + dev->eps[i].state = STATE_EP_DISABLED;
> + i++;
> + }
> + dev->eps_num = i;
> spin_unlock_irqrestore(&dev->lock, flags);
>
> /* Matches kref_put() in gadget_unbind(). */
> @@ -555,7 +577,7 @@ static void *raw_alloc_io_data(struct usb_raw_ep_io *io, void __user *ptr,
>
> if (copy_from_user(io, ptr, sizeof(*io)))
> return ERR_PTR(-EFAULT);
> - if (io->ep >= USB_RAW_MAX_ENDPOINTS)
> + if (io->ep >= USB_RAW_EPS_NUM_MAX)
> return ERR_PTR(-EINVAL);
> if (!usb_raw_io_flags_valid(io->flags))
> return ERR_PTR(-EINVAL);
> @@ -682,52 +704,30 @@ static int raw_ioctl_ep0_read(struct raw_dev *dev, unsigned long value)
> return ret;
> }
>
> -static bool check_ep_caps(struct usb_ep *ep,
> - struct usb_endpoint_descriptor *desc)
> -{
> - switch (usb_endpoint_type(desc)) {
> - case USB_ENDPOINT_XFER_ISOC:
> - if (!ep->caps.type_iso)
> - return false;
> - break;
> - case USB_ENDPOINT_XFER_BULK:
> - if (!ep->caps.type_bulk)
> - return false;
> - break;
> - case USB_ENDPOINT_XFER_INT:
> - if (!ep->caps.type_int)
> - return false;
> - break;
> - default:
> - return false;
> - }
> -
> - if (usb_endpoint_dir_in(desc) && !ep->caps.dir_in)
> - return false;
> - if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out)
> - return false;
> -
> - return true;
> -}
> -
> static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
> {
> int ret = 0, i;
> unsigned long flags;
> - struct usb_endpoint_descriptor *desc;
> - struct usb_ep *ep = NULL;
> + struct usb_raw_ep_descs *descs;
> + struct usb_endpoint_descriptor *ep_desc;
> + struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
> + struct raw_ep *ep;
>
> - desc = memdup_user((void __user *)value, sizeof(*desc));
> - if (IS_ERR(desc))
> - return PTR_ERR(desc);
> + descs = memdup_user((void __user *)value, sizeof(*descs));
> + if (IS_ERR(descs))
> + return PTR_ERR(descs);
> +
> + ep_desc = &descs->ep;
> + if (descs->comp.bLength != 0)
> + comp_desc = &descs->comp;
>
> /*
> * Endpoints with a maxpacket length of 0 can cause crashes in UDC
> * drivers.
> */
> - if (usb_endpoint_maxp(desc) == 0) {
> + if (usb_endpoint_maxp(ep_desc) == 0) {
> dev_dbg(dev->dev, "fail, bad endpoint maxpacket\n");
> - kfree(desc);
> + kfree(descs);
> return -EINVAL;
> }
>
> @@ -743,41 +743,38 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
> goto out_free;
> }
>
> - for (i = 0; i < USB_RAW_MAX_ENDPOINTS; i++) {
> - if (dev->eps[i].state == STATE_EP_ENABLED)
> + for (i = 0; i < dev->eps_num; i++) {
> + ep = &dev->eps[i];
> + if (ep->state != STATE_EP_DISABLED)
> continue;
> - break;
> - }
> - if (i == USB_RAW_MAX_ENDPOINTS) {
> - dev_dbg(&dev->gadget->dev,
> - "fail, no device endpoints available\n");
> - ret = -EBUSY;
> - goto out_free;
> - }
> -
> - gadget_for_each_ep(ep, dev->gadget) {
> - if (ep->enabled)
> + if (ep->addr != usb_endpoint_num(ep_desc) &&
> + ep->addr != USB_RAW_EP_ADDR_ANY)
> continue;
> - if (!check_ep_caps(ep, desc))
> + if (!usb_gadget_ep_match_desc(dev->gadget, ep->ep,
> + ep_desc, comp_desc))
> continue;
> - ep->desc = desc;
> - ret = usb_ep_enable(ep);
> + /* Gadget subsystem requires us to assign only a pointer to the
> + * endpoint descriptor here, but technically we store pointer
> + * to a usb_raw_ep_descs struct with both ep and comp
> + * descriptors to be later freed by raw_ioctl_ep_disable().
> + */
> + ep->ep->desc = ep_desc;
> + ret = usb_ep_enable(ep->ep);
> if (ret < 0) {
> dev_err(&dev->gadget->dev,
> "fail, usb_ep_enable returned %d\n", ret);
> goto out_free;
> }
> - dev->eps[i].req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> - if (!dev->eps[i].req) {
> + ep->req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC);
> + if (!ep->req) {
> dev_err(&dev->gadget->dev,
> "fail, usb_ep_alloc_request failed\n");
> - usb_ep_disable(ep);
> + usb_ep_disable(ep->ep);
> ret = -ENOMEM;
> goto out_free;
> }
> - dev->eps[i].ep = ep;
> - dev->eps[i].state = STATE_EP_ENABLED;
> - ep->driver_data = &dev->eps[i];
> + ep->state = STATE_EP_ENABLED;
> + ep->ep->driver_data = ep;
> ret = i;
> goto out_unlock;
> }
> @@ -786,7 +783,7 @@ static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
> ret = -EBUSY;
>
> out_free:
> - kfree(desc);
> + kfree(descs);
> out_unlock:
> spin_unlock_irqrestore(&dev->lock, flags);
> return ret;
> @@ -796,9 +793,9 @@ static int raw_ioctl_ep_disable(struct raw_dev *dev, unsigned long value)
> {
> int ret = 0, i = value;
> unsigned long flags;
> - const void *desc;
> + const void *descs;
>
> - if (i < 0 || i >= USB_RAW_MAX_ENDPOINTS)
> + if (i < 0 || i >= USB_RAW_EPS_NUM_MAX)
> return -EINVAL;
>
> spin_lock_irqsave(&dev->lock, flags);
> @@ -836,10 +833,10 @@ static int raw_ioctl_ep_disable(struct raw_dev *dev, unsigned long value)
>
> spin_lock_irqsave(&dev->lock, flags);
> usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
> - desc = dev->eps[i].ep->desc;
> + descs = dev->eps[i].ep->desc;
> dev->eps[i].ep = NULL;
> dev->eps[i].state = STATE_EP_DISABLED;
> - kfree(desc);
> + kfree(descs);
> dev->eps[i].disabling = false;
>
> out_unlock:
> @@ -1027,6 +1024,64 @@ static int raw_ioctl_vbus_draw(struct raw_dev *dev, unsigned long value)
> return ret;
> }
>
> +static void fill_ep_caps(struct usb_ep_caps *caps,
> + struct usb_raw_ep_caps *raw_caps)
> +{
> + raw_caps->type_control = caps->type_control;
> + raw_caps->type_iso = caps->type_iso;
> + raw_caps->type_bulk = caps->type_bulk;
> + raw_caps->type_int = caps->type_int;
> + raw_caps->dir_in = caps->dir_in;
> + raw_caps->dir_out = caps->dir_out;
> +}
> +
> +static void fill_ep_limits(struct usb_ep *ep, struct usb_raw_ep_limits *limits)
> +{
> + limits->maxpacket_limit = ep->maxpacket_limit;
> + limits->max_streams = ep->max_streams;
> + limits->maxburst = ep->maxburst;
> +}
> +
> +static int raw_ioctl_eps_info(struct raw_dev *dev, unsigned long value)
> +{
> + int ret = 0, i;
> + unsigned long flags;
> + struct usb_raw_eps_info info;
> + struct raw_ep *ep;
> +
> + spin_lock_irqsave(&dev->lock, flags);
> + if (dev->state != STATE_DEV_RUNNING) {
> + dev_dbg(dev->dev, "fail, device is not running\n");
> + ret = -EINVAL;
> + spin_unlock_irqrestore(&dev->lock, flags);
> + goto out;
> + }
> + if (!dev->gadget) {
> + dev_dbg(dev->dev, "fail, gadget is not bound\n");
> + ret = -EBUSY;
> + spin_unlock_irqrestore(&dev->lock, flags);
> + goto out;
> + }
> +
> + memset(&info, 0, sizeof(info));
> + for (i = 0; i < dev->eps_num; i++) {
> + ep = &dev->eps[i];
> + strscpy(&info.eps[i].name[0], ep->ep->name,
> + USB_RAW_EP_NAME_MAX);
> + info.eps[i].addr = ep->addr;
> + fill_ep_caps(&ep->ep->caps, &info.eps[i].caps);
> + fill_ep_limits(ep->ep, &info.eps[i].limits);
> + }
> + ret = dev->eps_num;
> + spin_unlock_irqrestore(&dev->lock, flags);
> +
> + if (copy_to_user((void __user *)value, &info, sizeof(info)))
> + ret = -EFAULT;
> +
> +out:
> + return ret;
> +}
> +
> static long raw_ioctl(struct file *fd, unsigned int cmd, unsigned long value)
> {
> struct raw_dev *dev = fd->private_data;
> @@ -1069,6 +1124,9 @@ static long raw_ioctl(struct file *fd, unsigned int cmd, unsigned long value)
> case USB_RAW_IOCTL_VBUS_DRAW:
> ret = raw_ioctl_vbus_draw(dev, value);
> break;
> + case USB_RAW_IOCTL_EPS_INFO:
> + ret = raw_ioctl_eps_info(dev, value);
> + break;
> default:
> ret = -EINVAL;
> }
> diff --git a/include/uapi/linux/usb/raw_gadget.h b/include/uapi/linux/usb/raw_gadget.h
> index 8544802b25bd..6ae626d17f0c 100644
> --- a/include/uapi/linux/usb/raw_gadget.h
> +++ b/include/uapi/linux/usb/raw_gadget.h
> @@ -93,6 +93,75 @@ struct usb_raw_ep_io {
> __u8 data[0];
> };
>
> +/*
> + * struct usb_raw_ep_descs - argument for USB_RAW_IOCTL_EP_ENABLE ioctl.
> + * @ep: Endpoint descriptor.
> + * @comp: SuperSpeed Endpoint Companion descriptor.
> + *
> + * Both of these descriptors are only used by the gadget subsystem including
> + * the UDC driver and don't affect the descriptors that are sent to the host.
> + * However, the user must make sure that the host and the gadget sides agree
> + * on the used endpoint parameters (such as e.g. endpoint addresses).
> + */
> +struct usb_raw_ep_descs {
> + struct usb_endpoint_descriptor ep;
> + struct usb_ss_ep_comp_descriptor comp;
> +};
> +
> +/* Maximum number of non-control endpoints in struct usb_raw_eps_info. */
> +#define USB_RAW_EPS_NUM_MAX 30
> +
> +/* Maximum length of UDC endpoint name in struct usb_raw_ep_info. */
> +#define USB_RAW_EP_NAME_MAX 16
> +
> +/* Used as addr in struct usb_raw_ep_info if endpoint accepts any address. */
> +#define USB_RAW_EP_ADDR_ANY 0xff
> +
> +/*
> + * struct usb_raw_ep_caps - exposes endpoint capabilities from
> + * struct usb_ep_caps.
> + */
> +struct usb_raw_ep_caps {
> + __u32 type_control:1;
> + __u32 type_iso:1;
> + __u32 type_bulk:1;
> + __u32 type_int:1;
> + __u32 dir_in:1;
> + __u32 dir_out:1;
> +};
> +
> +/*
> + * struct usb_raw_ep_limits - exposes endpoint limits from struct usb_ep.
> + */
> +struct usb_raw_ep_limits {
> + __u32 maxpacket_limit;
> + __u32 max_streams;
> + __u32 maxburst;
> +};
> +
> +/*
> + * struct usb_raw_ep_info - stores information about a gadget endpoint.
> + * @name: Name of the endpoint as it is defined in the UDC driver.
> + * @addr: Address of the endpoint that must be specified in the endpoint
> + * descriptor passed to USB_RAW_IOCTL_EP_ENABLE ioctl.
> + * @caps: Endpoint capabilities.
> + * @limits: Endpoint limits.
> + */
> +struct usb_raw_ep_info {
> + __u8 name[USB_RAW_EP_NAME_MAX];
> + __u32 addr;
> + struct usb_raw_ep_caps caps;
> + struct usb_raw_ep_limits limits;
> +};
> +
> +/*
> + * struct usb_raw_eps_info - argument for USB_RAW_IOCTL_EPS_INFO ioctl.
> + * eps: Structures that store information about non-control endpoints.
> + */
> +struct usb_raw_eps_info {
> + struct usb_raw_ep_info eps[USB_RAW_EPS_NUM_MAX];
> +};
> +
> /*
> * Initializes a Raw Gadget instance.
> * Accepts a pointer to the usb_raw_init struct as an argument.
> @@ -126,12 +195,12 @@ struct usb_raw_ep_io {
> #define USB_RAW_IOCTL_EP0_READ _IOWR('U', 4, struct usb_raw_ep_io)
>
> /*
> - * Finds an endpoint that supports the transfer type specified in the
> - * descriptor and enables it.
> - * Accepts a pointer to the usb_endpoint_descriptor struct as an argument.
> + * Finds an endpoint that satisfies the parameters specified in the provided
> + * descriptors (address, transfer type, etc.) and enables it.
> + * Accepts a pointer to the usb_raw_ep_descs struct as an argument.
> * Returns enabled endpoint handle on success or negative error code on failure.
> */
> -#define USB_RAW_IOCTL_EP_ENABLE _IOW('U', 5, struct usb_endpoint_descriptor)
> +#define USB_RAW_IOCTL_EP_ENABLE _IOW('U', 5, struct usb_raw_ep_descs)
>
> /* Disables specified endpoint.
> * Accepts endpoint handle as an argument.
> @@ -164,4 +233,11 @@ struct usb_raw_ep_io {
> */
> #define USB_RAW_IOCTL_VBUS_DRAW _IOW('U', 10, __u32)
>
> +/* Fills in the usb_raw_eps_info structure with information about non-control
> + * endpoints available for the currently connected UDC.
> + * Returns the number of available endpoints on success or negative error code
> + * on failure.
> + */
> +#define USB_RAW_IOCTL_EPS_INFO _IOR('U', 11, struct usb_raw_eps_info)
> +
> #endif /* _UAPI__LINUX_USB_RAW_GADGET_H */
> --
> 2.26.2.303.gf8c07b1a785-goog
>
Hi Andrey,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v5.7-rc3 next-20200428]
[cannot apply to balbi-usb/next peter.chen-usb/ci-for-usb-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Andrey-Konovalov/usb-raw-gadget-fix-gadget-endpoint-selection/20200429-060106
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=c6x
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/usb/gadget/legacy/raw_gadget.c: In function 'raw_ioctl_eps_info':
>> drivers/usb/gadget/legacy/raw_gadget.c:1079:1: warning: the frame size of 1096 bytes is larger than 1024 bytes [-Wframe-larger-than=]
1079 | }
| ^
vim +1079 drivers/usb/gadget/legacy/raw_gadget.c
1040
1041 static int raw_ioctl_eps_info(struct raw_dev *dev, unsigned long value)
1042 {
1043 int ret = 0, i;
1044 unsigned long flags;
1045 struct usb_raw_eps_info info;
1046 struct raw_ep *ep;
1047
1048 spin_lock_irqsave(&dev->lock, flags);
1049 if (dev->state != STATE_DEV_RUNNING) {
1050 dev_dbg(dev->dev, "fail, device is not running\n");
1051 ret = -EINVAL;
1052 spin_unlock_irqrestore(&dev->lock, flags);
1053 goto out;
1054 }
1055 if (!dev->gadget) {
1056 dev_dbg(dev->dev, "fail, gadget is not bound\n");
1057 ret = -EBUSY;
1058 spin_unlock_irqrestore(&dev->lock, flags);
1059 goto out;
1060 }
1061
1062 memset(&info, 0, sizeof(info));
1063 for (i = 0; i < dev->eps_num; i++) {
1064 ep = &dev->eps[i];
1065 strscpy(&info.eps[i].name[0], ep->ep->name,
1066 USB_RAW_EP_NAME_MAX);
1067 info.eps[i].addr = ep->addr;
1068 fill_ep_caps(&ep->ep->caps, &info.eps[i].caps);
1069 fill_ep_limits(ep->ep, &info.eps[i].limits);
1070 }
1071 ret = dev->eps_num;
1072 spin_unlock_irqrestore(&dev->lock, flags);
1073
1074 if (copy_to_user((void __user *)value, &info, sizeof(info)))
1075 ret = -EFAULT;
1076
1077 out:
1078 return ret;
> 1079 }
1080
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Tue, 28 Apr 2020, Andrey Konovalov wrote:
> Hi Alan,
>
> This patch uses the approach that I mentioned in the discussion about
> endpoint selection. Does this look acceptable?
I haven't had time to look through it yet.
> I'm not sure which endpoint limits it makes sense to expose via
> USB_RAW_IOCTL_EPS_INFO. I'm more or less sure about maxpacket_limit
> and max_streams, but I don't exactly know what maxburst is used for.
> Maybe there are some others?
maxburst is a USB-3 thing. It mainly affects just throughput, not
functionality, and it's handled pretty much entirely by the hardware.
You shouldn't worry about it, at least, not now.
> I also wonder if we need to expose ep0 limits via USB_RAW_IOCTL_EPS_INFO too.
> expose ep0 parameters?
I don't think there are any significant attributes for ep0. In
general, gadget drivers have to live with what the hardware supports --
or else fail to run at all. After all, the driver can't substitute a
different endpoint for ep0.
Alan Stern
On Wed, Apr 29, 2020 at 3:14 AM Alan Stern <[email protected]> wrote:
>
> On Tue, 28 Apr 2020, Andrey Konovalov wrote:
>
> > Hi Alan,
> >
> > This patch uses the approach that I mentioned in the discussion about
> > endpoint selection. Does this look acceptable?
>
> I haven't had time to look through it yet.
>
> > I'm not sure which endpoint limits it makes sense to expose via
> > USB_RAW_IOCTL_EPS_INFO. I'm more or less sure about maxpacket_limit
> > and max_streams, but I don't exactly know what maxburst is used for.
> > Maybe there are some others?
>
> maxburst is a USB-3 thing. It mainly affects just throughput, not
> functionality, and it's handled pretty much entirely by the hardware.
> You shouldn't worry about it, at least, not now.
The question is whether it will be needed when/if I ever add proper
USB3 support. It would be good to figure out which endpoint attributes
we need to expose now, rather than having to add another ioctl later.
> > I also wonder if we need to expose ep0 limits via USB_RAW_IOCTL_EPS_INFO too.
> > expose ep0 parameters?
>
> I don't think there are any significant attributes for ep0. In
> general, gadget drivers have to live with what the hardware supports --
> or else fail to run at all. After all, the driver can't substitute a
> different endpoint for ep0.
This is a good point. No need to export ep0 attributes then I guess.
Thanks!
On Wed, 29 Apr 2020, Andrey Konovalov wrote:
> On Wed, Apr 29, 2020 at 3:14 AM Alan Stern <[email protected]> wrote:
> >
> > On Tue, 28 Apr 2020, Andrey Konovalov wrote:
> >
> > > Hi Alan,
> > >
> > > This patch uses the approach that I mentioned in the discussion about
> > > endpoint selection. Does this look acceptable?
> >
> > I haven't had time to look through it yet.
> >
> > > I'm not sure which endpoint limits it makes sense to expose via
> > > USB_RAW_IOCTL_EPS_INFO. I'm more or less sure about maxpacket_limit
> > > and max_streams, but I don't exactly know what maxburst is used for.
> > > Maybe there are some others?
> >
> > maxburst is a USB-3 thing. It mainly affects just throughput, not
> > functionality, and it's handled pretty much entirely by the hardware.
> > You shouldn't worry about it, at least, not now.
>
> The question is whether it will be needed when/if I ever add proper
> USB3 support. It would be good to figure out which endpoint attributes
> we need to expose now, rather than having to add another ioctl later.
You might as well expose anything that looks like it might be relevant.
Better to include things that will never be needed than to omit things
which may be needed later.
Alan Stern