All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michal Nazarewicz <mina86@mina86.com>
To: balbi@ti.com, jackp@codeaurora.org, pheatwol@codeaurora.org
Cc: linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	benoit@android.com, andrzej.p@samsung.com,
	gregkh@linuxfoundation.org, Manu Gautam <mgautam@codeaurora.org>
Subject: [RFC] usb: gadget: f_fs: Add flags to descriptors block
Date: Tue, 25 Feb 2014 15:01:33 +0100	[thread overview]
Message-ID: <xa1tsir7b5uz.fsf@mina86.com> (raw)
In-Reply-To: <1387877408-17567-1-git-send-email-mgautam@codeaurora.org>

This reworks the way SuperSpeed descriptors are added and instead of
having a magick after full and high speed descriptors, it reworks the
whole descriptors block to include a flags field which lists which
descriptors are present and makes future extensions possible.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
Just to repeat what I wrote on my phone which is unable to send
text/plain messages:

On Tue, Feb 25 2014, Michał Nazarewicz wrote:
> On Feb 25, 2014 5:47 AM, "Manu Gautam" <mgautam@codeaurora.org> wrote:
>> Is this patch good now to be added to your queue (if not already in)?
>> It applied cleanly on your next branch.
>
> Actually, recent LWN article about flags in syscalls got me thinking and
> maybe it's good this patch is not yet in Felipe's queue.
>
> I admit I screwed up when I designed the headers format, but now that we
> change it to accommodate SuperSpeed, I think we should go ahead and change
> the format by adding flags field.
>
> […]
>
> Sorry to mention this only now, but I believe that's the right thing to do.

This patch implements this new format and in doing so simplifies, in
my opinion, the code a bit.

It needs to be applied on top of Manu's patch. 

It has not been tested at all, not even compile-tested.  This is
because whenever I try to compile the kernel, I get this nice message
in my logs:

    mce: [Hardware Error]: Machine check events logged
    thermal_sys: Critical temperature reached(100 C),shutting down
    thermal_sys: Critical temperature reached(100 C),shutting down

Sorry about that.

 drivers/usb/gadget/f_fs.c           | 136 +++++++++++++++---------------------
 drivers/usb/gadget/u_fs.h           |  12 ++--
 include/uapi/linux/usb/functionfs.h |  49 ++++++++-----
 3 files changed, 94 insertions(+), 103 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 928959d..7d01bd6 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1167,7 +1167,7 @@ static void ffs_data_clear(struct ffs_data *ffs)
 	if (ffs->epfiles)
 		ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
 
-	kfree(ffs->raw_descs);
+	kfree(ffs->raw_descs_data);
 	kfree(ffs->raw_strings);
 	kfree(ffs->stringtabs);
 }
@@ -1179,12 +1179,12 @@ static void ffs_data_reset(struct ffs_data *ffs)
 	ffs_data_clear(ffs);
 
 	ffs->epfiles = NULL;
+	ffs->raw_descs_data = NULL;
 	ffs->raw_descs = NULL;
 	ffs->raw_strings = NULL;
 	ffs->stringtabs = NULL;
 
-	ffs->raw_fs_hs_descs_length = 0;
-	ffs->raw_ss_descs_length = 0;
+	ffs->real_descs_length = 0;
 	ffs->fs_descs_count = 0;
 	ffs->hs_descs_count = 0;
 	ffs->ss_descs_count = 0;
@@ -1598,89 +1598,76 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 static int __ffs_data_got_descs(struct ffs_data *ffs,
 				char *const _data, size_t len)
 {
-	unsigned fs_count, hs_count, ss_count = 0;
-	int fs_len, hs_len, ss_len, ret = -EINVAL;
-	char *data = _data;
+	char *data = _data, *raw_descs;
+	unsigned counts[3], flags;
+	int ret = -EINVAL, i;
 
 	ENTER();
 
-	if (unlikely(get_unaligned_le32(data) != FUNCTIONFS_DESCRIPTORS_MAGIC ||
-		     get_unaligned_le32(data + 4) != len))
+	if (get_unaligned_le32(data + 4) != len)
 		goto error;
-	fs_count = get_unaligned_le32(data +  8);
-	hs_count = get_unaligned_le32(data + 12);
-
-	data += 16;
-	len  -= 16;
 
-	if (likely(fs_count)) {
-		fs_len = ffs_do_descs(fs_count, data, len,
-				      __ffs_data_do_entity, ffs);
-		if (unlikely(fs_len < 0)) {
-			ret = fs_len;
+	switch (get_unaligned_le32(data)) {
+	case FUNCTIONFS_DESCRIPTORS_MAGIC:
+		flags = FUNCTIONFS_HAS_FS_DESC | FUNCTIONFS_HAS_HS_DESC;
+		data += 8;
+		len  -= 8;
+		break;
+	case FUNCTIONFS_DESCRIPTORS_MAGIC_V2:
+		flags = get_unaligned_le32(data + 8);
+		if (flags & ~(FUNCTIONFS_HAS_FS_DESC |
+			      FUNCTIONFS_HAS_HS_DESC |
+			      FUNCTIONFS_HAS_SS_DESC)) {
+			ret = -ENOSYS;
 			goto error;
 		}
-
-		data += fs_len;
-		len  -= fs_len;
-	} else {
-		fs_len = 0;
+		data += 12;
+		len  -= 12;
+		break;
+	default:
+		goto error;
 	}
 
-	if (likely(hs_count)) {
-		hs_len = ffs_do_descs(hs_count, data, len,
-				   __ffs_data_do_entity, ffs);
-		if (unlikely(hs_len < 0)) {
-			ret = hs_len;
+	/* Read fs_count, hs_count and ss_count (if present) */
+	for (i = 0; i < 3; ++i) {
+		if (!(flags & (1 << i))) {
+			counts[i] = 0;
+		} else if (len < 4) {
 			goto error;
+		} else {
+			counts[i] = get_unaligned_le32(data);
+			data += 4;
+			len  -= 4;
 		}
-
-		data += hs_len;
-		len  -= hs_len;
-	} else {
-		hs_len = 0;
 	}
 
-	if (len >= 8) {
-		/* Check SS_MAGIC for presence of ss_descs and get SS_COUNT */
-		if (get_unaligned_le32(data) != FUNCTIONFS_SS_DESC_MAGIC)
-			goto einval;
-
-		ss_count = get_unaligned_le32(data + 4);
-		data += 8;
-		len  -= 8;
-	}
-
-	if (!fs_count && !hs_count && !ss_count)
-		goto einval;
-
-	if (ss_count) {
-		ss_len = ffs_do_descs(ss_count, data, len,
+	/* Read descriptors */
+	raw_descs = data;
+	for (i = 0; i < 3; ++i) {
+		if (!counts[i])
+			continue;
+		ret = ffs_do_descs(descs[i].count, data, len,
 				   __ffs_data_do_entity, ffs);
-		if (unlikely(ss_len < 0)) {
-			ret = ss_len;
+		if (ret < 0)
 			goto error;
-		}
-		ret = ss_len;
-	} else {
-		ss_len = 0;
-		ret = 0;
+		data += ret;
+		len  -= ret;
 	}
 
-	if (unlikely(len != ret))
-		goto einval;
+	if (raw_descs == data || len) {
+		ret = -EINVAL;
+		goto error;
+	}
 
-	ffs->raw_fs_hs_descs_length	 = fs_len + hs_len;
-	ffs->raw_ss_descs_length	 = ss_len;
-	ffs->raw_descs			 = _data;
-	ffs->fs_descs_count		 = fs_count;
-	ffs->hs_descs_count		 = hs_count;
-	ffs->ss_descs_count		 = ss_count;
+	ffs->raw_descs_data	= _data;
+	ffs->raw_descs		= raw_descs;
+	ffs->raw_descs_length	= data - raw_descs;
+	ffs->fs_descs_count	= counts[0];
+	ffs->hs_descs_count	= counts[1];
+	ffs->ss_descs_count	= counts[2];
 
 	return 0;
 
-einval:
-	ret = -EINVAL;
 error:
 	kfree(_data);
 	return ret;
@@ -2092,8 +2079,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	vla_item_with_sz(d, struct usb_descriptor_header *, ss_descs,
 		super ? ffs->ss_descs_count + 1 : 0);
 	vla_item_with_sz(d, short, inums, ffs->interfaces_count);
-	vla_item_with_sz(d, char, raw_descs,
-			ffs->raw_fs_hs_descs_length + ffs->raw_ss_descs_length);
+	vla_item_with_sz(d, char, raw_descs, ffs->raw_raw_descs_length);
 	char *vlabuf;
 
 	ENTER();
@@ -2109,15 +2095,9 @@ static int _ffs_func_bind(struct usb_configuration *c,
 
 	/* Zero */
 	memset(vla_ptr(vlabuf, d, eps), 0, d_eps__sz);
-	/* Copy only raw (hs,fs) descriptors (until ss_magic and ss_count) */
-	memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs + 16,
-		ffs->raw_fs_hs_descs_length);
-	/* Copy SS descs present @ header + hs_fs_descs + ss_magic + ss_count */
-	if (func->ffs->ss_descs_count)
-		memcpy(vla_ptr(vlabuf, d, raw_descs) +
-				ffs->raw_fs_hs_descs_length,
-		       ffs->raw_descs + 16 + ffs->raw_fs_hs_descs_length + 8,
-		       ffs->raw_ss_descs_length);
+	/* Copy descriptors  */
+	memcpy(vla_ptr(vlabuf, d, raw_descs), ffs->raw_descs,
+	       ffs->raw_descs_length);
 
 	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
 	for (ret = ffs->eps_count; ret; --ret) {
@@ -2599,7 +2579,7 @@ static int _ffs_name_dev(struct ffs_dev *dev, const char *name)
 	existing = _ffs_find_dev(name);
 	if (existing)
 		return -EBUSY;
-	
+
 	dev->name = name;
 
 	return 0;
@@ -2682,7 +2662,7 @@ static void ffs_release_dev(struct ffs_data *ffs_data)
 	ffs_dev = ffs_data->private_data;
 	if (ffs_dev)
 		ffs_dev->mounted = false;
-	
+
 	if (ffs_dev->ffs_release_dev_callback)
 		ffs_dev->ffs_release_dev_callback(ffs_dev);
 
diff --git a/drivers/usb/gadget/u_fs.h b/drivers/usb/gadget/u_fs.h
index 08bf26e..866c848 100644
--- a/drivers/usb/gadget/u_fs.h
+++ b/drivers/usb/gadget/u_fs.h
@@ -210,15 +210,13 @@ struct ffs_data {
 
 	/* filled by __ffs_data_got_descs() */
 	/*
-	 * Real descriptors are 16 bytes after raw_descs (so you need
-	 * to skip 16 bytes (ie. ffs->raw_descs + 16) to get to the
-	 * first full speed descriptor).
-	 * raw_fs_hs_descs_length does not have those 16 bytes added.
-	 * ss_descs are 8 bytes (ss_magic + count) pass the hs_descs
+	 * raw_descs is what you kfree, real_descs points inside of raw_descs,
+	 * where full speed, high speed and super speed descriptors start.
+	 * real_descs_length is the length of all those descriptors.
 	 */
 	const void			*raw_descs;
-	unsigned			raw_fs_hs_descs_length;
-	unsigned			raw_ss_descs_length;
+	const void			*real_descs;
+	unsigned			real_descs_length;
 	unsigned			fs_descs_count;
 	unsigned			hs_descs_count;
 	unsigned			ss_descs_count;
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 0f8f7be..ee6fcbc 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -10,7 +10,14 @@
 
 enum {
 	FUNCTIONFS_DESCRIPTORS_MAGIC = 1,
-	FUNCTIONFS_STRINGS_MAGIC     = 2
+	FUNCTIONFS_STRINGS_MAGIC = 2,
+	FUNCTIONFS_DESCRIPTORS_MAGIC_V2 = 3,
+};
+
+enum functionfs_flags {
+	FUNCTIONFS_HAS_FS_DESC = 1,
+	FUNCTIONFS_HAS_HS_DESC = 2,
+	FUNCTIONFS_HAS_SS_DESC = 4,
 };
 
 #define FUNCTIONFS_SS_DESC_MAGIC 0x0055DE5C
@@ -30,33 +37,39 @@ struct usb_endpoint_descriptor_no_audio {
 
 
 /*
- * All numbers must be in little endian order.
- */
-
-struct usb_functionfs_descs_head {
-	__le32 magic;
-	__le32 length;
-	__le32 fs_count;
-	__le32 hs_count;
-} __attribute__((packed));
-
-/*
  * Descriptors format:
  *
  * | off | name      | type         | description                          |
  * |-----+-----------+--------------+--------------------------------------|
- * |   0 | magic     | LE32         | FUNCTIONFS_{FS,HS}_DESCRIPTORS_MAGIC |
+ * |   0 | magic     | LE32         | FUNCTIONFS_DESCRIPTORS_MAGIC_V2      |
+ * |   4 | length    | LE32         | length of the whole data chunk       |
+ * |   8 | flags     | LE32         | combination of functionfs_flags      |
+ * |     | fs_count  | LE32         | number of full-speed descriptors     |
+ * |     | hs_count  | LE32         | number of high-speed descriptors     |
+ * |     | ss_count  | LE32         | number of high-speed descriptors     |
+ * |     | fs_descrs | Descriptor[] | list of full-speed descriptors       |
+ * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
+ * |     | ss_descrs | Descriptor[] | list of high-speed descriptors       |
+ *
+ * Depending on which flags are set, various fields may be missing in the
+ * structure.  Any flags that are not recognised cause the whole block to be
+ * rejected with -ENOSYS.
+ *
+ * Legacy descriptors format:
+ *
+ * | off | name      | type         | description                          |
+ * |-----+-----------+--------------+--------------------------------------|
+ * |   0 | magic     | LE32         | FUNCTIONFS_DESCRIPTORS_MAGIC         |
  * |   4 | length    | LE32         | length of the whole data chunk       |
  * |   8 | fs_count  | LE32         | number of full-speed descriptors     |
  * |  12 | hs_count  | LE32         | number of high-speed descriptors     |
  * |  16 | fs_descrs | Descriptor[] | list of full-speed descriptors       |
  * |     | hs_descrs | Descriptor[] | list of high-speed descriptors       |
- * |     | ss_magic  | LE32         | FUNCTIONFS_SS_DESC_MAGIC             |
- * |     | ss_count  | LE32         | number of super-speed descriptors    |
- * |     | ss_descrs | Descriptor[] | list of super-speed descriptors      |
  *
- * ss_magic: if present then it implies that SS_DESCs are also present
- * descs are just valid USB descriptors and have the following format:
+ * All numbers must be in little endian order.
+ *
+ * Descriptor[] is an array of valid USB descriptors which have the following
+ * format:
  *
  * | off | name            | type | description              |
  * |-----+-----------------+------+--------------------------|
-- 
1.9.0.rc1.175.g0b1dcb5

  parent reply	other threads:[~2014-02-25 14:54 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-24  9:30 [PATCH v5 1/1] usb: gadget: f_fs: Add support for SuperSpeed Mode Manu Gautam
     [not found] ` <1387877408-17567-1-git-send-email-mgautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2014-02-25  4:47   ` Manu Gautam
2014-02-25 14:01 ` Michal Nazarewicz [this message]
     [not found]   ` <xa1tsir7b5uz.fsf-deATy8a+UHjQT0dZR+AlfA@public.gmane.org>
2014-02-25 17:02     ` [PATCHv2] usb: gadget: f_fs: Add flags to descriptors block Michal Nazarewicz
2014-02-25 18:32       ` Felipe Balbi
2014-02-28 10:11       ` Manu Gautam
     [not found]       ` <xa1tob1vazxh.fsf-deATy8a+UHjQT0dZR+AlfA@public.gmane.org>
2014-06-04 14:24         ` Krzysztof Opasiak
     [not found]           ` <005901cf8000$c16ec090$444c41b0$%opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2014-06-04 19:06             ` Michal Nazarewicz
2014-06-05 11:32               ` Krzysztof Opasiak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xa1tsir7b5uz.fsf@mina86.com \
    --to=mina86@mina86.com \
    --cc=andrzej.p@samsung.com \
    --cc=balbi@ti.com \
    --cc=benoit@android.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jackp@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mgautam@codeaurora.org \
    --cc=pheatwol@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.