[PATCH v5 0/3] usb: gadget: f_fs: userspace API fixes and improvements

[PATCH v5 0/3] usb: gadget: f_fs: userspace API fixes and improvements

[Robert Baldyga]

This patchset contains changes in FunctionFS making it easier and
safer to use. It fixes bug in endpoint files handling code, adds new
ioctl allowing to obtain endpoint descriptor, and introduces virtual
address mapping which allows to separate endpoint address space in
function from physical endpoint addresses, and introduces new endpoint
files naming convention.

Changelog:

v5:
- fix typo pointed by Sergei Shtylyov

v4: https://lkml.org/lkml/2014/8/20/277
- change if() sequence into switch() statement

v3: https://lkml.org/lkml/2014/7/30/115
- move fix for the redundant ep files problem into sepatare patch
- merge user space API affecting changes into single patch
- add flag switching between old and new style API

v2: https://lkml.org/lkml/2014/7/25/296
- return proper endpont address in setup request handling
- add patch usb: gadget: f_fs: add ioctl returning ep descriptor
- add patch usb: gadget: f_fs: make numbers in ep file names the same
  as ep addresses

v1: https://lkml.org/lkml/2014/7/18/1010

Robert Baldyga (3):
  usb: gadget: f_fs: fix the redundant ep files problem
  usb: gadget: f_fs: add ioctl returning ep descriptor
  usb: gadget: f_fs: virtual endpoint address mapping

 drivers/usb/gadget/function/f_fs.c  | 111 +++++++++++++++++++++++++++++++-----
 drivers/usb/gadget/function/u_fs.h  |   4 ++
 include/uapi/linux/usb/functionfs.h |   7 +++
 3 files changed, 109 insertions(+), 13 deletions(-)

-- 
1.9.1

[PATCH v5 1/3] usb: gadget: f_fs: fix the redundant ep files problem

[Robert Baldyga]

Up to now, when endpoint addresses in descriptors were non-consecutive,
there were created redundant files, which could cause problems in kernel,
when user tryed to read/write to them. It was result of fact that maximum
endpoint address was taken as total number of endpoints in funciton.

This patch adds endpoint descriptors counting and storing their addresses
in eps_addrmap to verify their cohesion in each speed.

Endpoint address map would be also useful for further features, just like
vitual endpoint address mapping.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_fs.c | 67 +++++++++++++++++++++++++++++++-------
 drivers/usb/gadget/function/u_fs.h |  2 ++
 2 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index dc30adf..8096f22 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -155,6 +155,12 @@ struct ffs_io_data {
 	struct usb_request *req;
 };
 
+struct ffs_desc_helper {
+	struct ffs_data *ffs;
+	unsigned interfaces_count;
+	unsigned eps_count;
+};
+
 static int  __must_check ffs_epfiles_create(struct ffs_data *ffs);
 static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count);
 
@@ -1830,7 +1836,8 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 				u8 *valuep, struct usb_descriptor_header *desc,
 				void *priv)
 {
-	struct ffs_data *ffs = priv;
+	struct ffs_desc_helper *helper = priv;
+	struct usb_endpoint_descriptor *d;
 
 	ENTER();
 
@@ -1844,8 +1851,8 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 		 * encountered interface "n" then there are at least
 		 * "n+1" interfaces.
 		 */
-		if (*valuep >= ffs->interfaces_count)
-			ffs->interfaces_count = *valuep + 1;
+		if (*valuep >= helper->interfaces_count)
+			helper->interfaces_count = *valuep + 1;
 		break;
 
 	case FFS_STRING:
@@ -1853,14 +1860,23 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
 		 * Strings are indexed from 1 (0 is magic ;) reserved
 		 * for languages list or some such)
 		 */
-		if (*valuep > ffs->strings_count)
-			ffs->strings_count = *valuep;
+		if (*valuep > helper->ffs->strings_count)
+			helper->ffs->strings_count = *valuep;
 		break;
 
 	case FFS_ENDPOINT:
-		/* Endpoints are indexed from 1 as well. */
-		if ((*valuep & USB_ENDPOINT_NUMBER_MASK) > ffs->eps_count)
-			ffs->eps_count = (*valuep & USB_ENDPOINT_NUMBER_MASK);
+		d = (void *)desc;
+		helper->eps_count++;
+		if (helper->eps_count >= 15)
+			return -EINVAL;
+		if (!helper->ffs->eps_count && !helper->ffs->interfaces_count)
+			helper->ffs->eps_addrmap[helper->eps_count] =
+				d->bEndpointAddress;
+		else if (helper->ffs->eps_count < helper->eps_count)
+			return -EINVAL;
+		else if (helper->ffs->eps_addrmap[helper->eps_count] !=
+				d->bEndpointAddress)
+			return -EINVAL;
 		break;
 	}
 
@@ -2053,6 +2069,7 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 	char *data = _data, *raw_descs;
 	unsigned os_descs_count = 0, counts[3], flags;
 	int ret = -EINVAL, i;
+	struct ffs_desc_helper helper;
 
 	ENTER();
 
@@ -2101,13 +2118,29 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 
 	/* Read descriptors */
 	raw_descs = data;
+	helper.ffs = ffs;
 	for (i = 0; i < 3; ++i) {
 		if (!counts[i])
 			continue;
+		helper.interfaces_count = 0;
+		helper.eps_count = 0;
 		ret = ffs_do_descs(counts[i], data, len,
-				   __ffs_data_do_entity, ffs);
+				   __ffs_data_do_entity, &helper);
 		if (ret < 0)
 			goto error;
+		if (!ffs->eps_count && !ffs->interfaces_count) {
+			ffs->eps_count = helper.eps_count;
+			ffs->interfaces_count = helper.interfaces_count;
+		} else {
+			if (ffs->eps_count != helper.eps_count) {
+				ret = -EINVAL;
+				goto error;
+			}
+			if (ffs->interfaces_count != helper.interfaces_count) {
+				ret = -EINVAL;
+				goto error;
+			}
+		}
 		data += ret;
 		len  -= ret;
 	}
@@ -2342,9 +2375,18 @@ static void ffs_event_add(struct ffs_data *ffs,
 	spin_unlock_irqrestore(&ffs->ev.waitq.lock, flags);
 }
 
-
 /* Bind/unbind USB function hooks *******************************************/
 
+static int ffs_ep_addr2idx(struct ffs_data *ffs, u8 endpoint_address)
+{
+	int i;
+
+	for (i = 1; i < 15; ++i)
+		if (ffs->eps_addrmap[i] == endpoint_address)
+			return i;
+	return -ENOENT;
+}
+
 static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
 				    struct usb_descriptor_header *desc,
 				    void *priv)
@@ -2378,7 +2420,10 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
 	if (!desc || desc->bDescriptorType != USB_DT_ENDPOINT)
 		return 0;
 
-	idx = (ds->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) - 1;
+	idx = ffs_ep_addr2idx(func->ffs, ds->bEndpointAddress) - 1;
+	if (idx < 0)
+		return idx;
+
 	ffs_ep = func->eps + idx;
 
 	if (unlikely(ffs_ep->descs[ep_desc_id])) {
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index 63d6e71..d48897e 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -224,6 +224,8 @@ struct ffs_data {
 	void				*ms_os_descs_ext_prop_name_avail;
 	void				*ms_os_descs_ext_prop_data_avail;
 
+	u8				eps_addrmap[15];
+
 	unsigned short			strings_count;
 	unsigned short			interfaces_count;
 	unsigned short			eps_count;
-- 
1.9.1

[PATCH v5 2/3] usb: gadget: f_fs: add ioctl returning ep descriptor

[Robert Baldyga]

This patch introduces ioctl named FUNCTIONFS_ENDPOINT_DESC, which
returns endpoint descriptor to userspace. It works only if function
is active.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_fs.c  | 21 +++++++++++++++++++++
 include/uapi/linux/usb/functionfs.h |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 8096f22..ac7b16d 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1032,6 +1032,27 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 		case FUNCTIONFS_ENDPOINT_REVMAP:
 			ret = epfile->ep->num;
 			break;
+		case FUNCTIONFS_ENDPOINT_DESC:
+		{
+			int desc_idx;
+			struct usb_endpoint_descriptor *desc;
+
+			switch (epfile->ffs->gadget->speed) {
+			case USB_SPEED_SUPER:
+				desc_idx = 2;
+				break;
+			case USB_SPEED_HIGH:
+				desc_idx = 1;
+				break;
+			default:
+				desc_idx = 0;
+			}
+			desc = epfile->ep->descs[desc_idx];
+			ret = copy_to_user((void *)value, desc, sizeof(*desc));
+			if (ret)
+				ret = -EFAULT;
+			break;
+		}
 		default:
 			ret = -ENOTTY;
 		}
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 0154b28..7677108 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -265,6 +265,12 @@ struct usb_functionfs_event {
  */
 #define	FUNCTIONFS_ENDPOINT_REVMAP	_IO('g', 129)
 
+/*
+ * Returns endpoint descriptor. If function is not active returns -ENODEV.
+ */
+#define	FUNCTIONFS_ENDPOINT_DESC	_IOR('g', 130, \
+					     struct usb_endpoint_descriptor)
+
 
 
 #endif /* _UAPI__LINUX_FUNCTIONFS_H__ */
-- 
1.9.1

[PATCH v5 3/3] usb: gadget: f_fs: virtual endpoint address mapping

[Robert Baldyga]

This patch introduces virtual endpoint address mapping. It separates
function logic form physical endpoint addresses making it more hardware
independent.

Following modifications changes user space API, so to enable them user
have to switch on the FUNCTIONFS_VIRTUAL_ADDR flag in descriptors.

Endpoints are now refered using virtual endpoint addresses chosen by
user in endpoint descpriptors. This applies to each context when endpoint
address can be used:
- when accessing endpoint files in FunctionFS filesystemi (in file name),
- in setup requests directed to specific endpoint (in wIndex field),
- in descriptors returned by FUNCTIONFS_ENDPOINT_DESC ioctl.

In endpoint file names the endpoint address number is formatted as
double-digit hexadecimal value ("ep%02x") which has few advantages -
it is easy to parse, allows to easly recognize endpoint direction basing
on its name (IN endpoint number starts with digit 8, and OUT with 0)
which can be useful for debugging purpose, and it makes easier to introduce
further features allowing to use each endpoint number in both directions
to have more endpoints available for function if hardware supports this
(for example we could have ep01 which is endpoint 1 with OUT direction,
and ep81 which is endpoint 1 with IN direction).

Physical endpoint address can be still obtained using ioctl named
FUNCTIONFS_ENDPOINT_REVMAP, but now it's not neccesary to handle
USB transactions properly.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_fs.c  | 23 +++++++++++++++++++++--
 drivers/usb/gadget/function/u_fs.h  |  2 ++
 include/uapi/linux/usb/functionfs.h |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index ac7b16d..a20ac8d 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1555,7 +1555,10 @@ static int ffs_epfiles_create(struct ffs_data *ffs)
 		epfile->ffs = ffs;
 		mutex_init(&epfile->mutex);
 		init_waitqueue_head(&epfile->wait);
-		sprintf(epfiles->name, "ep%u",  i);
+		if (ffs->user_flags & FUNCTIONFS_VIRTUAL_ADDR)
+			sprintf(epfiles->name, "ep%02x", ffs->eps_addrmap[i]);
+		else
+			sprintf(epfiles->name, "ep%u", i);
 		if (!unlikely(ffs_sb_create_file(ffs->sb, epfiles->name, epfile,
 						 &ffs_epfile_operations,
 						 &epfile->dentry))) {
@@ -2105,10 +2108,12 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 		break;
 	case FUNCTIONFS_DESCRIPTORS_MAGIC_V2:
 		flags = get_unaligned_le32(data + 8);
+		ffs->user_flags = flags;
 		if (flags & ~(FUNCTIONFS_HAS_FS_DESC |
 			      FUNCTIONFS_HAS_HS_DESC |
 			      FUNCTIONFS_HAS_SS_DESC |
-			      FUNCTIONFS_HAS_MS_OS_DESC)) {
+			      FUNCTIONFS_HAS_MS_OS_DESC |
+			      FUNCTIONFS_VIRTUAL_ADDR)) {
 			ret = -ENOSYS;
 			goto error;
 		}
@@ -2463,7 +2468,13 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
 	} else {
 		struct usb_request *req;
 		struct usb_ep *ep;
+		u8 bEndpointAddress;
 
+		/*
+		 * We back up bEndpointAddress because autoconfig overwrites
+		 * it with physical endpoint address.
+		 */
+		bEndpointAddress = ds->bEndpointAddress;
 		pr_vdebug("autoconfig\n");
 		ep = usb_ep_autoconfig(func->gadget, ds);
 		if (unlikely(!ep))
@@ -2478,6 +2489,12 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
 		ffs_ep->req = req;
 		func->eps_revmap[ds->bEndpointAddress &
 				 USB_ENDPOINT_NUMBER_MASK] = idx + 1;
+		/*
+		 * If we use virtual address mapping, we restore
+		 * original bEndpointAddress value.
+		 */
+		if (func->ffs->user_flags & FUNCTIONFS_VIRTUAL_ADDR)
+			ds->bEndpointAddress = bEndpointAddress;
 	}
 	ffs_dump_mem(": Rewritten ep desc", ds, ds->bLength);
 
@@ -2922,6 +2939,8 @@ static int ffs_func_setup(struct usb_function *f,
 		ret = ffs_func_revmap_ep(func, le16_to_cpu(creq->wIndex));
 		if (unlikely(ret < 0))
 			return ret;
+		if (func->ffs->user_flags & FUNCTIONFS_VIRTUAL_ADDR)
+			ret = func->ffs->eps_addrmap[ret];
 		break;
 
 	default:
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index d48897e..cd128e3 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -224,6 +224,8 @@ struct ffs_data {
 	void				*ms_os_descs_ext_prop_name_avail;
 	void				*ms_os_descs_ext_prop_data_avail;
 
+	unsigned			user_flags;
+
 	u8				eps_addrmap[15];
 
 	unsigned short			strings_count;
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 7677108..d2ca92e 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -19,6 +19,7 @@ enum functionfs_flags {
 	FUNCTIONFS_HAS_HS_DESC = 2,
 	FUNCTIONFS_HAS_SS_DESC = 4,
 	FUNCTIONFS_HAS_MS_OS_DESC = 8,
+	FUNCTIONFS_VIRTUAL_ADDR = 16,
 };
 
 /* Descriptor of an non-audio endpoint */
-- 
1.9.1

Re: [PATCH v5 1/3] usb: gadget: f_fs: fix the redundant ep files problem

[Michal Nazarewicz]

On Thu, Aug 21 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
> Up to now, when endpoint addresses in descriptors were non-consecutive,
> there were created redundant files, which could cause problems in kernel,
> when user tryed to read/write to them. It was result of fact that maximum
            ^^^^^  -- tried

> endpoint address was taken as total number of endpoints in funciton.
>
> This patch adds endpoint descriptors counting and storing their addresses
> in eps_addrmap to verify their cohesion in each speed.
>
> Endpoint address map would be also useful for further features, just like
> vitual endpoint address mapping.
>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> @@ -1853,14 +1860,23 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
>  		 * Strings are indexed from 1 (0 is magic ;) reserved
>  		 * for languages list or some such)
>  		 */
> -		if (*valuep > ffs->strings_count)
> -			ffs->strings_count = *valuep;
> +		if (*valuep > helper->ffs->strings_count)
> +			helper->ffs->strings_count = *valuep;
>  		break;
>  
>  	case FFS_ENDPOINT:
> -		/* Endpoints are indexed from 1 as well. */
> -		if ((*valuep & USB_ENDPOINT_NUMBER_MASK) > ffs->eps_count)
> -			ffs->eps_count = (*valuep & USB_ENDPOINT_NUMBER_MASK);
> +		d = (void *)desc;
> +		helper->eps_count++;
> +		if (helper->eps_count >= 15)
> +			return -EINVAL;
> +		if (!helper->ffs->eps_count && !helper->ffs->interfaces_count)

I'd check helper->ffs->eps_count only.  helper->ffs->interfaces_count is
zero only because endpoints and interfaces are interpret at the same
time, but otherwise the interfaces_count is unrelated to what we're
doing here.

Also, perhaps adding a comment describing what !helper->ffs->eps_count
means makes sense here.

> +			helper->ffs->eps_addrmap[helper->eps_count] =
> +				d->bEndpointAddress;
> +		else if (helper->ffs->eps_count < helper->eps_count)
> +			return -EINVAL;

Doesn't this duplicate check in __ffs_data_got_descs?

> +		else if (helper->ffs->eps_addrmap[helper->eps_count] !=
> +				d->bEndpointAddress)
> +			return -EINVAL;
>  		break;
>  	}
>  
> @@ -2101,13 +2118,29 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
>  
>  	/* Read descriptors */
>  	raw_descs = data;
> +	helper.ffs = ffs;
>  	for (i = 0; i < 3; ++i) {
>  		if (!counts[i])
>  			continue;
> +		helper.interfaces_count = 0;
> +		helper.eps_count = 0;
>  		ret = ffs_do_descs(counts[i], data, len,
> -				   __ffs_data_do_entity, ffs);
> +				   __ffs_data_do_entity, &helper);
>  		if (ret < 0)
>  			goto error;
> +		if (!ffs->eps_count && !ffs->interfaces_count) {
> +			ffs->eps_count = helper.eps_count;
> +			ffs->interfaces_count = helper.interfaces_count;
> +		} else {
> +			if (ffs->eps_count != helper.eps_count) {
> +				ret = -EINVAL;
> +				goto error;
> +			}
> +			if (ffs->interfaces_count != helper.interfaces_count) {
> +				ret = -EINVAL;
> +				goto error;
> +			}
> +		}
>  		data += ret;
>  		len  -= ret;
>  	}
> @@ -2342,9 +2375,18 @@ static void ffs_event_add(struct ffs_data *ffs,
>  	spin_unlock_irqrestore(&ffs->ev.waitq.lock, flags);
>  }
>  
> -
>  /* Bind/unbind USB function hooks *******************************************/
>  
> +static int ffs_ep_addr2idx(struct ffs_data *ffs, u8 endpoint_address)
> +{
> +	int i;
> +
> +	for (i = 1; i < 15; ++i)

+	for (i = 1; i < ARRAY_SIZE(ffs->eps_addrmap); ++i)

> +		if (ffs->eps_addrmap[i] == endpoint_address)
> +			return i;
> +	return -ENOENT;
> +}
> +
>  static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
>  				    struct usb_descriptor_header *desc,
>  				    void *priv)

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH v5 2/3] usb: gadget: f_fs: add ioctl returning ep descriptor

[Michal Nazarewicz]

On Thu, Aug 21 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
> This patch introduces ioctl named FUNCTIONFS_ENDPOINT_DESC, which
> returns endpoint descriptor to userspace. It works only if function
> is active.
>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>

With the change described below:

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> ---
>  drivers/usb/gadget/function/f_fs.c  | 21 +++++++++++++++++++++
>  include/uapi/linux/usb/functionfs.h |  6 ++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 8096f22..ac7b16d 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1032,6 +1032,27 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
>  		case FUNCTIONFS_ENDPOINT_REVMAP:
>  			ret = epfile->ep->num;
>  			break;
> +		case FUNCTIONFS_ENDPOINT_DESC:
> +		{
> +			int desc_idx;
> +			struct usb_endpoint_descriptor *desc;
> +
> +			switch (epfile->ffs->gadget->speed) {
> +			case USB_SPEED_SUPER:
> +				desc_idx = 2;
> +				break;
> +			case USB_SPEED_HIGH:
> +				desc_idx = 1;
> +				break;
> +			default:
> +				desc_idx = 0;
> +			}
> +			desc = epfile->ep->descs[desc_idx];
> +			ret = copy_to_user((void *)value, desc, sizeof(*desc));

This is called under a spin lock, so nope…  The simplest will be to just
unlock prior to copying and then return instead of breaking from the switch:

+			struct usb_endpoint_descriptor desc;
…
+			desc = *epfile->ep->descs[desc_idx];
+			spin_unlock_irq(&epfile->ffs->eps_lock);
+			ret = copy_to_user((void *)value, &desc, sizeof(desc));
+			return ret ? -EFAULT : 0;

> +			if (ret)
> +				ret = -EFAULT;
> +			break;
> +		}
>  		default:
>  			ret = -ENOTTY;
>  		}

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH v5 3/3] usb: gadget: f_fs: virtual endpoint address mapping

[Michal Nazarewicz]

On Thu, Aug 21 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
> This patch introduces virtual endpoint address mapping. It separates
> function logic form physical endpoint addresses making it more hardware
> independent.
>
> Following modifications changes user space API, so to enable them user
> have to switch on the FUNCTIONFS_VIRTUAL_ADDR flag in descriptors.
>
> Endpoints are now refered using virtual endpoint addresses chosen by
> user in endpoint descpriptors. This applies to each context when endpoint
> address can be used:
> - when accessing endpoint files in FunctionFS filesystemi (in file name),
> - in setup requests directed to specific endpoint (in wIndex field),
> - in descriptors returned by FUNCTIONFS_ENDPOINT_DESC ioctl.
>
> In endpoint file names the endpoint address number is formatted as
> double-digit hexadecimal value ("ep%02x") which has few advantages -
> it is easy to parse, allows to easly recognize endpoint direction basing
> on its name (IN endpoint number starts with digit 8, and OUT with 0)
> which can be useful for debugging purpose, and it makes easier to introduce
> further features allowing to use each endpoint number in both directions
> to have more endpoints available for function if hardware supports this
> (for example we could have ep01 which is endpoint 1 with OUT direction,
> and ep81 which is endpoint 1 with IN direction).
>
> Physical endpoint address can be still obtained using ioctl named
> FUNCTIONFS_ENDPOINT_REVMAP, but now it's not neccesary to handle
> USB transactions properly.
>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH v5 1/3] usb: gadget: f_fs: fix the redundant ep files problem

[Robert Baldyga]

On 08/24/2014 04:14 PM, Michal Nazarewicz wrote:
> On Thu, Aug 21 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
>> Up to now, when endpoint addresses in descriptors were non-consecutive,
>> there were created redundant files, which could cause problems in kernel,
>> when user tryed to read/write to them. It was result of fact that maximum
>             ^^^^^  -- tried
> 
>> endpoint address was taken as total number of endpoints in funciton.
>>
>> This patch adds endpoint descriptors counting and storing their addresses
>> in eps_addrmap to verify their cohesion in each speed.
>>
>> Endpoint address map would be also useful for further features, just like
>> vitual endpoint address mapping.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> 
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> 
>> @@ -1853,14 +1860,23 @@ static int __ffs_data_do_entity(enum ffs_entity_type type,
>>  		 * Strings are indexed from 1 (0 is magic ;) reserved
>>  		 * for languages list or some such)
>>  		 */
>> -		if (*valuep > ffs->strings_count)
>> -			ffs->strings_count = *valuep;
>> +		if (*valuep > helper->ffs->strings_count)
>> +			helper->ffs->strings_count = *valuep;
>>  		break;
>>  
>>  	case FFS_ENDPOINT:
>> -		/* Endpoints are indexed from 1 as well. */
>> -		if ((*valuep & USB_ENDPOINT_NUMBER_MASK) > ffs->eps_count)
>> -			ffs->eps_count = (*valuep & USB_ENDPOINT_NUMBER_MASK);
>> +		d = (void *)desc;
>> +		helper->eps_count++;
>> +		if (helper->eps_count >= 15)
>> +			return -EINVAL;
>> +		if (!helper->ffs->eps_count && !helper->ffs->interfaces_count)
> 
> I'd check helper->ffs->eps_count only.  helper->ffs->interfaces_count is
> zero only because endpoints and interfaces are interpret at the same
> time, but otherwise the interfaces_count is unrelated to what we're
> doing here.

Besides simple descriptor counting there are done checks if endpoints
number and their addressed are identical for each speed. For this reason
we need to know inside this function if descriptors for any speed was
already done. If it's true, we check if endpoint descriptors for current
speed has proper addresses.

There is possibility that interface has no endpoints (endpoint 0 only),
so we can recognize that descriptors for one speed were already parsed
only basing on interfaces_count. In that case if FFS_ENDPOINT will
appear, eps_count will exceed number of endpoints in previously parsed
descriptors and error will be returned.

> 
> Also, perhaps adding a comment describing what !helper->ffs->eps_count
> means makes sense here.
> 
>> +			helper->ffs->eps_addrmap[helper->eps_count] =
>> +				d->bEndpointAddress;
>> +		else if (helper->ffs->eps_count < helper->eps_count)
>> +			return -EINVAL;
> 
> Doesn't this duplicate check in __ffs_data_got_descs?
> 
>> +		else if (helper->ffs->eps_addrmap[helper->eps_count] !=
>> +				d->bEndpointAddress)
>> +			return -EINVAL;
>>  		break;
>>  	}
>>  
>> @@ -2101,13 +2118,29 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
>>  
>>  	/* Read descriptors */
>>  	raw_descs = data;
>> +	helper.ffs = ffs;
>>  	for (i = 0; i < 3; ++i) {
>>  		if (!counts[i])
>>  			continue;
>> +		helper.interfaces_count = 0;
>> +		helper.eps_count = 0;
>>  		ret = ffs_do_descs(counts[i], data, len,
>> -				   __ffs_data_do_entity, ffs);
>> +				   __ffs_data_do_entity, &helper);
>>  		if (ret < 0)
>>  			goto error;
>> +		if (!ffs->eps_count && !ffs->interfaces_count) {
>> +			ffs->eps_count = helper.eps_count;
>> +			ffs->interfaces_count = helper.interfaces_count;
>> +		} else {
>> +			if (ffs->eps_count != helper.eps_count) {
>> +				ret = -EINVAL;
>> +				goto error;
>> +			}
>> +			if (ffs->interfaces_count != helper.interfaces_count) {
>> +				ret = -EINVAL;
>> +				goto error;
>> +			}
>> +		}
>>  		data += ret;
>>  		len  -= ret;
>>  	}
>> @@ -2342,9 +2375,18 @@ static void ffs_event_add(struct ffs_data *ffs,
>>  	spin_unlock_irqrestore(&ffs->ev.waitq.lock, flags);
>>  }
>>  
>> -
>>  /* Bind/unbind USB function hooks *******************************************/
>>  
>> +static int ffs_ep_addr2idx(struct ffs_data *ffs, u8 endpoint_address)
>> +{
>> +	int i;
>> +
>> +	for (i = 1; i < 15; ++i)
> 
> +	for (i = 1; i < ARRAY_SIZE(ffs->eps_addrmap); ++i)
> 
>> +		if (ffs->eps_addrmap[i] == endpoint_address)
>> +			return i;
>> +	return -ENOENT;
>> +}
>> +
>>  static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
>>  				    struct usb_descriptor_header *desc,
>>  				    void *priv)
> 

Thanks,
Robert Baldyga