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

[PATCH v2 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 introduces virtual address mapping which allows
to separate endpoint address space in function from physical endpoint
addresses, and allow to refer to endpoints with address chosen by user.
It also adds new ioctl allowing to obtain endpoint descriptor, and
introduces new endpoint files naming convention to unify number identifying
each single endpoint.

Changelog:

v2:
- 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: virtual address mapping
  usb: gadget: f_fs: add ioctl returning ep descriptor
  usb: gadget: f_fs: make numbers in ep file names the same as ep
    addresses

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

-- 
1.9.1

[PATCH v2 1/3] usb: gadget: f_fs: virtual address mapping

[Robert Baldyga]

This patch adds virtual endpoint address mapping to functionfs.

So far endpoint addresses given by user through endpoint descriptors
were ignored, and replaced by physical endpoint addresses. Endpoint
address in wIndex field of setup requesti, addressed to endpoint, was
the physical endpoint address, and names of files in functionfs
directory was numered in order, and were the same as indexes of
ffs_epfile in epfile array. In result user has no way to indicate
which file in functionfs is associated with which particular
requested endpoint. He also didn't know which endpoint is recipient
of setup request.

There was also one more problem - if 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 solves this problems by introducing virtual endpoint address
mapping. Now each function has separate endpoint address space. Numbers
of endpoint files in functionfs and addresses in setup requests are
mapped to addresses choosen by user in endpoint descriptors.

It also introduces additional testing if desctiptors given by user are
consistent - if number of endpoints and their addresses in each speed
are the same.

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

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 165c2dd..490b30f 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -154,6 +154,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);
 
@@ -1527,7 +1533,8 @@ 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);
+		sprintf(epfiles->name, "ep%u",
+			ffs->eps_addrmap[i] & USB_ENDPOINT_NUMBER_MASK);
 		if (!unlikely(ffs_sb_create_file(ffs->sb, epfiles->name, epfile,
 						 &ffs_epfile_operations,
 						 &epfile->dentry))) {
@@ -1820,7 +1827,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();
 
@@ -1834,8 +1842,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:
@@ -1843,14 +1851,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;
 	}
 
@@ -1863,6 +1880,7 @@ static int __ffs_data_got_descs(struct ffs_data *ffs,
 	char *data = _data, *raw_descs;
 	unsigned counts[3], flags;
 	int ret = -EINVAL, i;
+	struct ffs_desc_helper helper;
 
 	ENTER();
 
@@ -1905,13 +1923,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;
 	}
@@ -2137,9 +2171,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)
@@ -2173,7 +2216,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])) {
@@ -2192,7 +2238,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))
@@ -2207,6 +2259,7 @@ 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;
+		ds->bEndpointAddress = bEndpointAddress;
 	}
 	ffs_dump_mem(": Rewritten ep desc", ds, ds->bLength);
 
@@ -2530,6 +2583,7 @@ 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;
+		ret = func->ffs->eps_addrmap[ret];
 		break;
 
 	default:
diff --git a/drivers/usb/gadget/u_fs.h b/drivers/usb/gadget/u_fs.h
index bf0ba37..fe31eba 100644
--- a/drivers/usb/gadget/u_fs.h
+++ b/drivers/usb/gadget/u_fs.h
@@ -217,6 +217,8 @@ struct ffs_data {
 	unsigned			hs_descs_count;
 	unsigned			ss_descs_count;
 
+	u8				eps_addrmap[15];
+
 	unsigned short			strings_count;
 	unsigned short			interfaces_count;
 	unsigned short			eps_count;
-- 
1.9.1

[PATCH v2 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/f_fs.c           | 17 +++++++++++++++++
 include/uapi/linux/usb/functionfs.h |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 490b30f..a2e18cc 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1031,6 +1031,23 @@ 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;
+
+			if (epfile->ffs->gadget->speed == USB_SPEED_SUPER)
+				desc_idx = 2;
+			else if (epfile->ffs->gadget->speed == USB_SPEED_HIGH)
+				desc_idx = 1;
+			else
+				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 1dc473a..1ab6f06 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -197,6 +197,12 @@ struct usb_functionfs_event {
  */
 #define	FUNCTIONFS_ENDPOINT_REVMAP	_IO('g', 129)
 
+/*
+ * Returns endpoint descriptor. In funciton 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 v2 3/3] usb: gadget: f_fs: make numbers in ep file names the same as ep addresses

[Robert Baldyga]

This patch adds FUNCTIONFS_ADDR_NAMES flag to user flags set in
descriptors, which makes numbers in endpoint file names the same as
value of bEndpointAddress in endpoint descriptor. It simplifies endpoint
handling, because now it can be refered using one unique number.

Numbers are in hexadecimal format to have each name of the same lenght,
and to simplify debugging. The first digit can be 0 or 8 which means
OUT or IN endpoint direction, and the second digit is simply hexadecimal
value of endpoint number (which is between 1 and 15).

It needed to store user flags to the moment of endpoint files creation,
and for this reason there is new field in struct ffs_data named user_flags.

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

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index a2e18cc..0b8040b 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1550,8 +1550,11 @@ 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",
-			ffs->eps_addrmap[i] & USB_ENDPOINT_NUMBER_MASK);
+		if (ffs->user_flags & FUNCTIONFS_ADDR_NAMES)
+			sprintf(epfiles->name, "ep%02x", ffs->eps_addrmap[i]);
+		else
+			sprintf(epfiles->name, "ep%u",
+				ffs->eps_addrmap[i] & USB_ENDPOINT_NUMBER_MASK);
 		if (!unlikely(ffs_sb_create_file(ffs->sb, epfiles->name, epfile,
 						 &ffs_epfile_operations,
 						 &epfile->dentry))) {
@@ -1912,9 +1915,11 @@ 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_SS_DESC |
+			      FUNCTIONFS_ADDR_NAMES)) {
 			ret = -ENOSYS;
 			goto error;
 		}
diff --git a/drivers/usb/gadget/u_fs.h b/drivers/usb/gadget/u_fs.h
index fe31eba..adc6568 100644
--- a/drivers/usb/gadget/u_fs.h
+++ b/drivers/usb/gadget/u_fs.h
@@ -217,6 +217,8 @@ struct ffs_data {
 	unsigned			hs_descs_count;
 	unsigned			ss_descs_count;
 
+	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 1ab6f06..a29d910 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -18,6 +18,7 @@ enum functionfs_flags {
 	FUNCTIONFS_HAS_FS_DESC = 1,
 	FUNCTIONFS_HAS_HS_DESC = 2,
 	FUNCTIONFS_HAS_SS_DESC = 4,
+	FUNCTIONFS_ADDR_NAMES = 8,
 };
 
 #ifndef __KERNEL__
-- 
1.9.1

Re: [PATCH v2 3/3] usb: gadget: f_fs: make numbers in ep file names the same as ep addresses

[Michal Nazarewicz]

On Fri, Jul 25 2014, Robert Baldyga wrote:
> This patch adds FUNCTIONFS_ADDR_NAMES flag to user flags set in
> descriptors, which makes numbers in endpoint file names the same as
> value of bEndpointAddress in endpoint descriptor. It simplifies endpoint
> handling, because now it can be refered using one unique number.
>
> Numbers are in hexadecimal format to have each name of the same lenght,
> and to simplify debugging. The first digit can be 0 or 8 which means
> OUT or IN endpoint direction, and the second digit is simply hexadecimal
> value of endpoint number (which is between 1 and 15).
>
> It needed to store user flags to the moment of endpoint files creation,
> and for this reason there is new field in struct ffs_data named user_flags.
>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>

I don't like this to be honest.  IMO it adds code for little benefit.

> ---
>  drivers/usb/gadget/f_fs.c           | 11 ++++++++---
>  drivers/usb/gadget/u_fs.h           |  2 ++
>  include/uapi/linux/usb/functionfs.h |  1 +
>  3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index a2e18cc..0b8040b 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -1550,8 +1550,11 @@ 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",
> -			ffs->eps_addrmap[i] & USB_ENDPOINT_NUMBER_MASK);
> +		if (ffs->user_flags & FUNCTIONFS_ADDR_NAMES)
> +			sprintf(epfiles->name, "ep%02x", ffs->eps_addrmap[i]);
> +		else
> +			sprintf(epfiles->name, "ep%u",
> +				ffs->eps_addrmap[i] & USB_ENDPOINT_NUMBER_MASK);
>  		if (!unlikely(ffs_sb_create_file(ffs->sb, epfiles->name, epfile,
>  						 &ffs_epfile_operations,
>  						 &epfile->dentry))) {
> @@ -1912,9 +1915,11 @@ 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_SS_DESC |
> +			      FUNCTIONFS_ADDR_NAMES)) {
>  			ret = -ENOSYS;
>  			goto error;
>  		}
> diff --git a/drivers/usb/gadget/u_fs.h b/drivers/usb/gadget/u_fs.h
> index fe31eba..adc6568 100644
> --- a/drivers/usb/gadget/u_fs.h
> +++ b/drivers/usb/gadget/u_fs.h
> @@ -217,6 +217,8 @@ struct ffs_data {
>  	unsigned			hs_descs_count;
>  	unsigned			ss_descs_count;
>  
> +	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 1ab6f06..a29d910 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -18,6 +18,7 @@ enum functionfs_flags {
>  	FUNCTIONFS_HAS_FS_DESC = 1,
>  	FUNCTIONFS_HAS_HS_DESC = 2,
>  	FUNCTIONFS_HAS_SS_DESC = 4,
> +	FUNCTIONFS_ADDR_NAMES = 8,
>  };
>  
>  #ifndef __KERNEL__
> -- 
> 1.9.1
>

-- 
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 v2 2/3] usb: gadget: f_fs: add ioctl returning ep descriptor

[Michal Nazarewicz]

On Fri, Jul 25 2014, Robert Baldyga wrote:
> This patch introduces ioctl named FUNCTIONFS_ENDPOINT_DESC, which
> returns endpoint descriptor to userspace. It works only if function
> is active.

I would argue that user space should never need to know the real
descriptor.  Is this ioctl needed for anything in particular?

>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/gadget/f_fs.c           | 17 +++++++++++++++++
>  include/uapi/linux/usb/functionfs.h |  6 ++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 490b30f..a2e18cc 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -1031,6 +1031,23 @@ 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;
> +
> +			if (epfile->ffs->gadget->speed == USB_SPEED_SUPER)
> +				desc_idx = 2;
> +			else if (epfile->ffs->gadget->speed == USB_SPEED_HIGH)
> +				desc_idx = 1;
> +			else
> +				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 1dc473a..1ab6f06 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -197,6 +197,12 @@ struct usb_functionfs_event {
>   */
>  #define	FUNCTIONFS_ENDPOINT_REVMAP	_IO('g', 129)
>  
> +/*
> + * Returns endpoint descriptor. In funciton is not active returns -ENODEV.
> + */
> +#define	FUNCTIONFS_ENDPOINT_DESC	_IOR('g', 130, \
> +					     struct usb_endpoint_descriptor)
> +
>  
>  
>  #endif /* _UAPI__LINUX_FUNCTIONFS_H__ */
> -- 
> 1.9.1
>

-- 
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 v2 1/3] usb: gadget: f_fs: virtual address mapping

[Michal Nazarewicz]

On Fri, Jul 25 2014, Robert Baldyga wrote:
> This patch adds virtual endpoint address mapping to functionfs.
>
> So far endpoint addresses given by user through endpoint descriptors
> were ignored, and replaced by physical endpoint addresses. Endpoint
> address in wIndex field of setup requesti, addressed to endpoint, was
> the physical endpoint address, and names of files in functionfs
> directory was numered in order, and were the same as indexes of
> ffs_epfile in epfile array. In result user has no way to indicate
> which file in functionfs is associated with which particular
> requested endpoint. He also didn't know which endpoint is recipient
> of setup request.

Couldn't that be solved by simply providing the mapping to user space?

> There was also one more problem - if 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 is kinda unrelated though.  I mean it's a separate bug.

> This patch solves this problems by introducing virtual endpoint address
> mapping. Now each function has separate endpoint address space. Numbers
> of endpoint files in functionfs and addresses in setup requests are
> mapped to addresses choosen by user in endpoint descriptors.
>
> It also introduces additional testing if desctiptors given by user are
> consistent - if number of endpoints and their addresses in each speed
> are the same.
>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/gadget/f_fs.c | 78 +++++++++++++++++++++++++++++++++++++++--------
>  drivers/usb/gadget/u_fs.h |  2 ++
>  2 files changed, 68 insertions(+), 12 deletions(-)

-- 
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 v2 1/3] usb: gadget: f_fs: virtual address mapping

[Robert Baldyga]

On 07/25/2014 04:18 PM, Michal Nazarewicz wrote:
> On Fri, Jul 25 2014, Robert Baldyga wrote:
>> This patch adds virtual endpoint address mapping to functionfs.
>>
>> So far endpoint addresses given by user through endpoint descriptors
>> were ignored, and replaced by physical endpoint addresses. Endpoint
>> address in wIndex field of setup requesti, addressed to endpoint, was
>> the physical endpoint address, and names of files in functionfs
>> directory was numered in order, and were the same as indexes of
>> ffs_epfile in epfile array. In result user has no way to indicate
>> which file in functionfs is associated with which particular
>> requested endpoint. He also didn't know which endpoint is recipient
>> of setup request.
> 
> Couldn't that be solved by simply providing the mapping to user space?

There would be only small differences in code (add mapping instead of
changing file names) so why would we not want do it in more intuitive way?

> 
>> There was also one more problem - if 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 is kinda unrelated though.  I mean it's a separate bug.

Yes, but it can be fixed by the way, as a side effect, so there is no
sense (and probably no simple way) to move it into separate patch.

> 
>> This patch solves this problems by introducing virtual endpoint address
>> mapping. Now each function has separate endpoint address space. Numbers
>> of endpoint files in functionfs and addresses in setup requests are
>> mapped to addresses choosen by user in endpoint descriptors.
>>
>> It also introduces additional testing if desctiptors given by user are
>> consistent - if number of endpoints and their addresses in each speed
>> are the same.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/usb/gadget/f_fs.c | 78 +++++++++++++++++++++++++++++++++++++++--------
>>  drivers/usb/gadget/u_fs.h |  2 ++
>>  2 files changed, 68 insertions(+), 12 deletions(-)
> 

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

[Robert Baldyga]

On 07/25/2014 04:15 PM, Michal Nazarewicz wrote:
> On Fri, Jul 25 2014, Robert Baldyga wrote:
>> This patch introduces ioctl named FUNCTIONFS_ENDPOINT_DESC, which
>> returns endpoint descriptor to userspace. It works only if function
>> is active.
> 
> I would argue that user space should never need to know the real
> descriptor.  Is this ioctl needed for anything in particular?

It's needed, at least, to inform user space about maximum possible
wMaxPacketSize value for the endpoint, which is returned by autoconfig
through the endpoint descriptor when value set by user is zero.

Descriptor returned form this ioctl is mostly the same as descriptor
created by user. I guess you meant that real descriptor is this one,
which contains real endpoint address, because it's probably only datum
in real descriptor which should be never known by user space.

This ioctl would be also useful in case, when FunctionFS daemon obtains
endpoint descriptors from another daemon, and do not have access to
original descriptor structures given do FunctionFS.

> 
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/usb/gadget/f_fs.c           | 17 +++++++++++++++++
>>  include/uapi/linux/usb/functionfs.h |  6 ++++++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
>> index 490b30f..a2e18cc 100644
>> --- a/drivers/usb/gadget/f_fs.c
>> +++ b/drivers/usb/gadget/f_fs.c
>> @@ -1031,6 +1031,23 @@ 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;
>> +
>> +			if (epfile->ffs->gadget->speed == USB_SPEED_SUPER)
>> +				desc_idx = 2;
>> +			else if (epfile->ffs->gadget->speed == USB_SPEED_HIGH)
>> +				desc_idx = 1;
>> +			else
>> +				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 1dc473a..1ab6f06 100644
>> --- a/include/uapi/linux/usb/functionfs.h
>> +++ b/include/uapi/linux/usb/functionfs.h
>> @@ -197,6 +197,12 @@ struct usb_functionfs_event {
>>   */
>>  #define	FUNCTIONFS_ENDPOINT_REVMAP	_IO('g', 129)
>>  
>> +/*
>> + * Returns endpoint descriptor. In funciton is not active returns -ENODEV.
>> + */
>> +#define	FUNCTIONFS_ENDPOINT_DESC	_IOR('g', 130, \
>> +					     struct usb_endpoint_descriptor)
>> +
>>  
>>  
>>  #endif /* _UAPI__LINUX_FUNCTIONFS_H__ */
>> -- 
>> 1.9.1
>>
> 

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

[Marek Szyprowski]

Hello,

On 2014-07-25 16:15, Michal Nazarewicz wrote:
> On Fri, Jul 25 2014, Robert Baldyga wrote:
>> This patch introduces ioctl named FUNCTIONFS_ENDPOINT_DESC, which
>> returns endpoint descriptor to userspace. It works only if function
>> is active.
> I would argue that user space should never need to know the real
> descriptor.  Is this ioctl needed for anything in particular?

Some proprietary broken usb protocols might embed real physical endpoint
(or interface) number into the data stream. In such case functionfs driver
is unable to do proper endpoint/interface number fixup.

>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>   drivers/usb/gadget/f_fs.c           | 17 +++++++++++++++++
>>   include/uapi/linux/usb/functionfs.h |  6 ++++++
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
>> index 490b30f..a2e18cc 100644
>> --- a/drivers/usb/gadget/f_fs.c
>> +++ b/drivers/usb/gadget/f_fs.c
>> @@ -1031,6 +1031,23 @@ 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;
>> +
>> +			if (epfile->ffs->gadget->speed == USB_SPEED_SUPER)
>> +				desc_idx = 2;
>> +			else if (epfile->ffs->gadget->speed == USB_SPEED_HIGH)
>> +				desc_idx = 1;
>> +			else
>> +				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 1dc473a..1ab6f06 100644
>> --- a/include/uapi/linux/usb/functionfs.h
>> +++ b/include/uapi/linux/usb/functionfs.h
>> @@ -197,6 +197,12 @@ struct usb_functionfs_event {
>>    */
>>   #define	FUNCTIONFS_ENDPOINT_REVMAP	_IO('g', 129)
>>   
>> +/*
>> + * Returns endpoint descriptor. In funciton is not active returns -ENODEV.
>> + */
>> +#define	FUNCTIONFS_ENDPOINT_DESC	_IOR('g', 130, \
>> +					     struct usb_endpoint_descriptor)
>> +
>>   
>>   
>>   #endif /* _UAPI__LINUX_FUNCTIONFS_H__ */
>> -- 
>> 1.9.1

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Greg KH]

On Mon, Jul 28, 2014 at 08:42:44AM +0200, Marek Szyprowski wrote:
> Hello,
> 
> On 2014-07-25 16:15, Michal Nazarewicz wrote:
> >On Fri, Jul 25 2014, Robert Baldyga wrote:
> >>This patch introduces ioctl named FUNCTIONFS_ENDPOINT_DESC, which
> >>returns endpoint descriptor to userspace. It works only if function
> >>is active.
> >I would argue that user space should never need to know the real
> >descriptor.  Is this ioctl needed for anything in particular?
> 
> Some proprietary broken usb protocols might embed real physical endpoint
> (or interface) number into the data stream. In such case functionfs driver
> is unable to do proper endpoint/interface number fixup.

Do you have an example of such a messed up thing?

greg k-h

Re: [PATCH v2 1/3] usb: gadget: f_fs: virtual address mapping

[Michal Nazarewicz]

On Mon, Jul 28 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
> On 07/25/2014 04:18 PM, Michal Nazarewicz wrote:
>> On Fri, Jul 25 2014, Robert Baldyga wrote:
>>> This patch adds virtual endpoint address mapping to functionfs.
>>>
>>> So far endpoint addresses given by user through endpoint descriptors
>>> were ignored, and replaced by physical endpoint addresses. Endpoint
>>> address in wIndex field of setup requesti, addressed to endpoint, was
>>> the physical endpoint address, and names of files in functionfs
>>> directory was numered in order, and were the same as indexes of
>>> ffs_epfile in epfile array. In result user has no way to indicate
>>> which file in functionfs is associated with which particular
>>> requested endpoint. He also didn't know which endpoint is recipient
>>> of setup request.
>> 
>> Couldn't that be solved by simply providing the mapping to user space?
>
> There would be only small differences in code (add mapping instead of
> changing file names) so why would we not want do it in more intuitive
> way?

So I'm confused again.  With your patch, the endpoint number read from
user space will have barring *only* on the file name and it *will not*
correspond to the real/physical endpoint number, right?  Why do we want
that?  What's the advantage over having endpoints numbered in the order
they were specified?

To know what physical number endpoint has, user space would have to read
the descriptor via the new proposed ioctl or get a mapping from ep file
names to physical endpoint numbers.

>>> There was also one more problem - if 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 is kinda unrelated though.  I mean it's a separate bug.
>
> Yes, but it can be fixed by the way, as a side effect, so there is no
> sense (and probably no simple way) to move it into separate patch.

Right, but now, we're arguing about the whole patch as opposed to having
part of it already acked. :P

-- 
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 v2 2/3] usb: gadget: f_fs: add ioctl returning ep descriptor

[Michal Nazarewicz]

>> On Fri, Jul 25 2014, Robert Baldyga wrote:
>>> This patch introduces ioctl named FUNCTIONFS_ENDPOINT_DESC, which
>>> returns endpoint descriptor to userspace. It works only if function
>>> is active.

> On 07/25/2014 04:15 PM, Michal Nazarewicz wrote:
>> I would argue that user space should never need to know the real
>> descriptor.  Is this ioctl needed for anything in particular?

On Mon, Jul 28 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
> It's needed, at least, to inform user space about maximum possible
> wMaxPacketSize value for the endpoint, which is returned by autoconfig
> through the endpoint descriptor when value set by user is zero.
>
> Descriptor returned form this ioctl is mostly the same as descriptor
> created by user. I guess you meant that real descriptor is this one,
> which contains real endpoint address, because it's probably only datum
> in real descriptor which should be never known by user space.
>
> This ioctl would be also useful in case, when FunctionFS daemon obtains
> endpoint descriptors from another daemon, and do not have access to
> original descriptor structures given do FunctionFS.

All right, I think I can go with that.

>>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.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 v2 1/3] usb: gadget: f_fs: virtual address mapping

[Robert Baldyga]

On 07/28/2014 12:22 PM, Michal Nazarewicz wrote:
> On Mon, Jul 28 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
>> On 07/25/2014 04:18 PM, Michal Nazarewicz wrote:
>>> On Fri, Jul 25 2014, Robert Baldyga wrote:
>>>> This patch adds virtual endpoint address mapping to functionfs.
>>>>
>>>> So far endpoint addresses given by user through endpoint descriptors
>>>> were ignored, and replaced by physical endpoint addresses. Endpoint
>>>> address in wIndex field of setup requesti, addressed to endpoint, was
>>>> the physical endpoint address, and names of files in functionfs
>>>> directory was numered in order, and were the same as indexes of
>>>> ffs_epfile in epfile array. In result user has no way to indicate
>>>> which file in functionfs is associated with which particular
>>>> requested endpoint. He also didn't know which endpoint is recipient
>>>> of setup request.
>>>
>>> Couldn't that be solved by simply providing the mapping to user space?
>>
>> There would be only small differences in code (add mapping instead of
>> changing file names) so why would we not want do it in more intuitive
>> way?
> 
> So I'm confused again.  With your patch, the endpoint number read from
> user space will have barring *only* on the file name and it *will not*
> correspond to the real/physical endpoint number, right?  Why do we want
> that?  What's the advantage over having endpoints numbered in the order
> they were specified?
> 
> To know what physical number endpoint has, user space would have to read
> the descriptor via the new proposed ioctl or get a mapping from ep file
> names to physical endpoint numbers.

No, endpoint number read from user space will also affects on value in
wIndex field in setup request. The goal is to have only one number
identifying each endpoint. User can choose this number through endpoint
descriptor, and then use it in each context - to open endpoint file, to
find recipient of setup request, to recognize which endpoint is linked
with particular fd using FUNCTIONFS_ENDPOINT_DESC ioctl.

Yes, this ioctl returns bEndpointAddress in function address space. To
obtain physical endpoint address (which shouldn't be needed in functions
compatible with USB specification), there is another ioctl -
FUNCTIONFS_ENDPOINT_REVMAP.

The main advantage is that each function has its own "sandbox" and it
can work as if it was the only function in gadget. Furthermore it's less
confusing for user than ignoring endpoint number chosen in descriptor,
create endpoint files numbered in order they were specified, and refer
to endpoints using physical addresses in setup requests...

> 
>>>> There was also one more problem - if 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 is kinda unrelated though.  I mean it's a separate bug.
>>
>> Yes, but it can be fixed by the way, as a side effect, so there is no
>> sense (and probably no simple way) to move it into separate patch.
> 
> Right, but now, we're arguing about the whole patch as opposed to having
> part of it already acked. :P
> 

Re: [PATCH v2 1/3] usb: gadget: f_fs: virtual address mapping

[Michal Nazarewicz]

OK, I see, I misunderstood your change before.  So what you're saying is
that now we have:
1. numbers user space provides in bEndpoindAddress,
2. physical addresses assigned when endpoint is configured, and
3. numbers for file names which go sequentially;
and what you want is to change the code so that 1 and 3 are the same.

Yes, I agree that would be better, and it was quite an omission on my
part that I did not implement it that way, but at this point, I would
argue that breaking backwards compatibility is not really worth it.

-- 
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 v2 1/3] usb: gadget: f_fs: virtual address mapping

[Robert Baldyga]

On 07/28/2014 05:21 PM, Michal Nazarewicz wrote:
> OK, I see, I misunderstood your change before.  So what you're saying is
> that now we have:
> 1. numbers user space provides in bEndpoindAddress,
> 2. physical addresses assigned when endpoint is configured, and
> 3. numbers for file names which go sequentially;
> and what you want is to change the code so that 1 and 3 are the same.
> 
> Yes, I agree that would be better, and it was quite an omission on my
> part that I did not implement it that way, but at this point, I would
> argue that breaking backwards compatibility is not really worth it.

Code of examples still works after this changes. We can also assume that
the vast majority of users numbered endpoints sequentially. So there is
very little part of cases when API change can break the function daemon.

Eventually we can add new flag to user descriptors which turns on files
naming convention change (maybe it could be merged with my another patch
https://lkml.org/lkml/2014/7/25/297).

And there is another one change - when endpoint is recipient of setup
request, the physical endpoint address is translated into address chosen
by user in ep descriptor. It also affects on user space API, and
probably should be switched with the user flag.

I can try to move fixes which do not affect on user space API into
separate patch, and then prepare another one, which will add new API
features in case of user flag selection.