On Wed, Jul 02 2014, Andrzej Pietrasiewicz <[email protected]> wrote:
> Add support for OS descriptors. The new format of descriptors is used,
> because the "flags" field is required for extensions. os_count gives
> the number of OSDesc[] elements.
> The format of descriptors is given in include/uapi/linux/usb/functionfs.h.
>
> For extended properties descriptor the usb_ext_prop_desc structure covers
> only a part of a descriptor, because the wPropertyNameLength is unknown
> up front.
>
> Signed-off-by: Andrzej Pietrasiewicz <[email protected]>
> ---
> Rebased onto Felipe's testing/next with gadget directory cleanup patches
> applied:
>
> http://www.spinics.net/lists/linux-usb/msg109928.html
>
> This is meant for 3.17.
>
> @Michal: I kindly ask for your review. Your comments will be very
> valuable.
For now I've only skimmed over binding code.
>
> drivers/usb/gadget/function/f_fs.c | 345
> +++++++++++++++++++++++++++++++++++-
> drivers/usb/gadget/function/u_fs.h | 7 +
> include/uapi/linux/usb/functionfs.h | 87 ++++++++-
> 3 files changed, 432 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 88d6fa2..55c0db7 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -34,6 +34,7 @@
>
> #include "u_fs.h"
> #include "u_f.h"
> +#include "u_os_desc.h"
> #include "configfs.h"
>
> #define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest dice roll ;) */
> @@ -1644,11 +1645,18 @@ enum ffs_entity_type {
> FFS_DESCRIPTOR, FFS_INTERFACE, FFS_STRING, FFS_ENDPOINT
> };
>
> +enum ffs_os_desc_type {
> + FFS_OS_DESC, FFS_OS_DESC_EXT_COMPAT, FFS_OS_DESC_EXT_PROP
> +};
> +
> typedef int (*ffs_entity_callback)(enum ffs_entity_type entity,
> u8 *valuep,
> struct usb_descriptor_header *desc,
> void *priv);
>
> +typedef int (*ffs_os_desc_callback)(enum ffs_os_desc_type entity,
> + u8 *valuep, void *data, void *priv);
> +
> static int __must_check ffs_do_desc(char *data, unsigned len,
> ffs_entity_callback entity, void *priv)
> {
> @@ -1855,11 +1863,190 @@ static int __ffs_data_do_entity(enum ffs_entity_type
> type,
> return 0;
> }
>
> +/*
> + * Process all extended compatibility/extended property descriptors
> + * of a feature descriptor
> + */
> +static int __must_check ffs_do_os_desc(char *data, unsigned len, u8 *valuep,
> + u16 feature_count,
> + ffs_os_desc_callback entity, void *priv,
> + struct usb_os_desc_header *desc)
The name of this function has to be changed. Perhaps
ffs_do_os_single_desc? It took me embarrassing amount of time to figure
out that ffs_do_os_descs and ffs_do_os_desc are different functions.
> +{
> + int ret;
> + enum ffs_os_desc_type *type = (enum ffs_os_desc_type *)valuep;
Read the value already:
enum ffs_os_desc_type type = *(enum ffs_os_desc_type *)valuep;
> + const unsigned _len = len;
> +
> + ENTER();
> +
> + /* loop over all ext compat/ext prop descriptors */
> + while (feature_count--) {
> + ret = entity(*type, (u8 *)desc, (void *)data, priv);
You don't have to cast to void *. It's implicit.
> + if (unlikely(ret < 0)) {
> + pr_debug("bad OS descriptor, type: %d\n", *valuep);
> + return ret;
> + }
> + data += ret;
> + len -= ret;
> + }
> + return _len - len;
> +}
> +
> +/* Process a number of complete Feature Descriptors (Ext Compat or Ext Prop)
> */
> +static int __must_check ffs_do_os_descs(unsigned count,
> + char *data, unsigned len,
> + ffs_os_desc_callback entity, void *priv)
> +{
> + const unsigned _len = len;
> + unsigned long num = 0;
> +
> + ENTER();
> +
> + for (;;) {
> + int ret;
> + enum ffs_os_desc_type type;
> + u16 feature_count;
> + struct usb_os_desc_header *desc = (void *)data;
if (len < sizeof(*desc))
return -EINVAL;
With the union addition that I've mentioned near the end of the email,
this would also handle checking if bCount/wCount is there.
> +
> + if (num == count)
> + return _len - len;
for (num = 0; num < count; ++num) {
…;
}
return _len - len;
> +
> + /* Record "descriptor" entity */
> + /*
> + * Process dwLength, bcdVersion, wIndex,
> + * get b/wCount.
> + * Move the data pointer to the beginning of
> + * extended compatibilities proper or
> + * extended properties proper portions of the
> + * data
> + */
> + ret = entity(FFS_OS_DESC, (u8 *)&type, desc, priv);
This casting looks ugly. Just change type of the second argument of
entity callback to void*. You are casting it back and forth anyway.
> + if (unlikely(ret < 0)) {
> + pr_debug("entity OS_DESCRIPTOR(%02lx); ret = %d\n",
> + num, ret);
> + return ret;
> + }
> + if (type == FFS_OS_DESC_EXT_COMPAT) {
> + struct usb_ext_compat_desc_header *h =
> + (struct usb_ext_compat_desc_header *)data;
Add checking if h->Reserved is zero, reject if not.
> + feature_count = h->bCount;
> + } else if (type == FFS_OS_DESC_EXT_PROP) {
> + struct usb_ext_prop_desc_header *h = (void *)data;
> +
> + feature_count = le16_to_cpu(h->wCount);
> + } else {
> + return -EINVAL;
> + }
Perhaps you could take advantage of the fact that in little endian
a 16-bit “?? 00” is the same as 8-bit “??”. Something like:
feature_count = le16_to_cpu(d->wCount);
if (type == FFS_OS_DESC_EXT_COMPAT && feature_count > 255)
return -EINVAL;
> + if (type == FFS_OS_DESC_EXT_COMPAT) {
> + struct usb_ext_compat_desc_header *h =
> + (struct usb_ext_compat_desc_header *)data;
Add checking if h->Reserved is zero, reject if not.
> + feature_count = h->bCount;
> + } else if (type == FFS_OS_DESC_EXT_PROP) {
> + struct usb_ext_prop_desc_header *h = (void *)data;
> +
> + feature_count = le16_to_cpu(h->wCount);
> + } else {
> + return -EINVAL;
> + }
> + len -= (ret + 2);
> + data += (ret + 2);
> +
> + /*
> + * Process all function/property descriptors
> + * of this Feature Descriptor
> + */
> + ret = ffs_do_os_desc(data, len, (u8 *)&type, feature_count,
> + entity, priv, desc);
> + if (unlikely(ret < 0)) {
> + pr_debug("%s returns %d\n", __func__, ret);
> + return ret;
> + }
> +
> + len -= ret;
> + data += ret;
> + ++num;
> + }
> +}
> +
> +/**
> + * Validate contents of the buffer from userspace related to OS descriptors.
> + */
> +static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
> + u8 *valuep, void *data, void *priv)
As commented above, make it “void *valuep”.
> +{
> + struct ffs_data *ffs = priv;
> + __u8 length;
> +
> + ENTER();
> +
> + switch (type) {
> + case FFS_OS_DESC: {
> + struct usb_os_desc_header *desc = data;
> + enum ffs_os_desc_type *next_type =
> + (enum ffs_os_desc_type *)valuep;
> + __u16 bcd_version = le16_to_cpu(desc->bcdVersion);
> + __u16 w_index = le16_to_cpu(desc->wIndex);
> +
> + if (bcd_version != 0x1) {
Is it just me, or does 0x1 look kinda weird?
> + pr_vdebug("unsupported os descriptors version: %d",
> + bcd_version);
> + return -EINVAL;
> + }
> + if (w_index != 0x4 && w_index != 0x5) {
> + pr_vdebug("unsupported os descriptor type: %d",
> + w_index);
> + return -EINVAL;
Stash this as a “default” in the switch below.
> + }
> + switch (w_index) {
> + case 0x4:
> + *next_type = FFS_OS_DESC_EXT_COMPAT;
> + break;
> + case 0x5:
> + *next_type = FFS_OS_DESC_EXT_PROP;
> + break;
> + }
> + length = sizeof(*desc);
> + }
> + break;
> + case FFS_OS_DESC_EXT_COMPAT: {
> + struct usb_ext_compat_desc *d = data;
if (len < sizeof *d)
return -EINVAL;
> +
> + if (d->bFirstInterfaceNumber >= ffs->interfaces_count)
> + return -EINVAL;
> +
Also add checking whether d->Reserved1 and d->Reserved2[] is zero,
reject if not.
> + length = sizeof(struct usb_ext_compat_desc);
> + }
> + break;
> + case FFS_OS_DESC_EXT_PROP: {
> + struct usb_os_desc_header *desc =
> + (struct usb_os_desc_header *)valuep;
> + struct usb_ext_prop_desc *d = data;
> + __le32 type, pdl;
> + __le16 pnl;
Why __le32 and __le16? You want u32 and u16 here.
if (len < sizeof *d)
return -EINVAL;
> +
> + if (desc->interface >= ffs->interfaces_count)
> + return -EINVAL;
> + length = le32_to_cpu(d->dwSize);
> + type = le32_to_cpu(d->dwPropertyDataType);
> + if (type < USB_EXT_PROP_UNICODE ||
> + type > USB_EXT_PROP_UNICODE_MULTI) {
> + pr_vdebug("unsupported os descriptor property type: %d",
> + type);
> + return -EINVAL;
> + }
> + pnl = le16_to_cpu(d->wPropertyNameLength);
> + pdl = le32_to_cpu(*(u32 *)(data + 10 + pnl));
> + if (length != 14 + pnl + pdl) {
> +#define FMT "invalid os descriptor length: %d pnl:%d pdl:%d (descriptor
> %d)\n"
> + pr_vdebug(FMT, length, pnl, pdl, type);
> +#undef FMT
Uh? Why the #define?
> + return -EINVAL;
> + }
> + ++ffs->ms_os_descs_ext_prop_count;
> + ffs->ms_os_descs_ext_prop_name_len += (pnl << 1);
Why is pnl multiplied by two?
> + ffs->ms_os_descs_ext_prop_data_len += pdl;
> + }
> + break;
> + default:
> + pr_vdebug("unknown descriptor: %d\n", type);
> + return -EINVAL;
> + }
> + return length;
> +}
> +
> static int __ffs_data_got_descs(struct ffs_data *ffs,
> char *const _data, size_t len)
> {
> char *data = _data, *raw_descs;
> - unsigned counts[3], flags;
> + unsigned os_descs_count = 0, counts[3], flags;
> int ret = -EINVAL, i;
>
> ENTER();
> @@ -1877,7 +2064,8 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
> flags = get_unaligned_le32(data + 8);
> if (flags & ~(FUNCTIONFS_HAS_FS_DESC |
> FUNCTIONFS_HAS_HS_DESC |
> - FUNCTIONFS_HAS_SS_DESC)) {
> + FUNCTIONFS_HAS_SS_DESC |
> + FUNCTIONFS_HAS_MS_OS_DESC)) {
> ret = -ENOSYS;
> goto error;
> }
> @@ -1900,6 +2088,11 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
> len -= 4;
> }
> }
> + if (flags & (1 << i)) {
> + os_descs_count = get_unaligned_le32(data);
> + data += 4;
> + len -= 4;
> + };
>
> /* Read descriptors */
> raw_descs = data;
> @@ -1913,6 +2106,14 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
> data += ret;
> len -= ret;
> }
> + if (os_descs_count) {
> + ret = ffs_do_os_descs(os_descs_count, data, len,
> + __ffs_data_do_os_desc, ffs);
> + if (ret < 0)
> + goto error;
> + data += ret;
> + len -= ret;
> + }
>
> if (raw_descs == data || len) {
> ret = -EINVAL;
> @@ -1925,6 +2126,7 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
> ffs->fs_descs_count = counts[0];
> ffs->hs_descs_count = counts[1];
> ffs->ss_descs_count = counts[2];
> + ffs->ms_os_descs_count = os_descs_count;
>
> return 0;
>
> @@ -2091,6 +2293,7 @@ static void __ffs_event_add(struct ffs_data *ffs,
> rem_type2 = FUNCTIONFS_SUSPEND;
> /* FALL THROUGH */
> case FUNCTIONFS_SUSPEND:
> +
Unrelated and unnecessary.
> case FUNCTIONFS_SETUP:
> rem_type1 = type;
> /* Discard all similar events */
> diff --git a/include/uapi/linux/usb/functionfs.h
> b/include/uapi/linux/usb/functionfs.h
> index 2a4b4a7..4ec3798 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -33,6 +32,42 @@ struct usb_endpoint_descriptor_no_audio {
> __u8 bInterval;
> } __attribute__((packed));
>
> +/* MS OS Descriptor header */
> +struct usb_os_desc_header {
> + __u8 interface;
> + __u32 dwLength;
> + __u16 bcdVersion;
> + __u16 wIndex;
Is that __u32 or __le32? And same with 16? You are accessing the data
with le*_to_cpu so I assume this should read __le32 and __le16.
Instead of having separate structures, how about adding this:
union {
struct {
__u8 bCount;
__u8 Reserved;
} __attribute__((packed));
__le16 wCount;
};
Feel free to name the union and struct if you are so inclined.
> +} __attribute__((packed));
> +
> +/* MS OS Extended Compatibility Descriptor header */
> +struct usb_ext_compat_desc_header {
> + struct usb_os_desc_header header;
> + __u8 bCount;
> + __u8 Reserved;
> +} __attribute__((packed));
> +
> +struct usb_ext_compat_desc {
> + __u8 bFirstInterfaceNumber;
> + __u8 Reserved1;
> + __u8 CompatibleID[8];
> + __u8 SubCompatibleID[8];
> + __u8 Reserved2[6];
> +};
> +
> +/* MS OS Extended Properties Descriptor header */
> +struct usb_ext_prop_desc_header {
> + struct usb_os_desc_header header;
> + __u16 wCount;
> +} __attribute__((packed));
> +
> +struct usb_ext_prop_desc {
> + __u32 dwSize;
> + __u32 dwPropertyDataType;
> + __u16 wPropertyNameLength;
> +} __attribute__((packed));
> +
> +#ifndef __KERNEL__
>
> /*
> * Descriptors format:
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
