Re: [PATCH V3 2/3] usb: gadget: function: f_fs: Let ffs_epfile_ioctl wait for enable.

2017-04-19 Thread Michal Nazarewicz 
On Wed, Apr 19 2017, Jerry Zhang wrote:
> This allows users to make an ioctl call as the first action on a
> connection. Ex, some functions might want to get endpoint size
> before making any i/os.
>
> Previously, calling ioctls before read/write would depending on the
> timing of endpoints being enabled.
>
> ESHUTDOWN is now a possible return value and ENODEV is not, so change
> docs accordingly.
>
> Signed-off-by: Jerry Zhang 

Acked-by: Michal Nazarewicz 

> ---
> Changelog since v2:
> - Updated doc to account for -ESHUTDOWN
>
> Changelog since v1:
> - Guarded against epfile->ep changing after taking lock
>
>  drivers/usb/gadget/function/f_fs.c  | 93 
> +
>  include/uapi/linux/usb/functionfs.h |  7 +--
>  2 files changed, 58 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index a0085571824d..04425e6b457a 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1190,6 +1190,7 @@ static long ffs_epfile_ioctl(struct file *file, 
> unsigned code,
>unsigned long value)
>  {
>   struct ffs_epfile *epfile = file->private_data;
> + struct ffs_ep *ep;
>   int ret;
>  
>   ENTER();
> @@ -1197,50 +1198,64 @@ static long ffs_epfile_ioctl(struct file *file, 
> unsigned code,
>   if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
>   return -ENODEV;
>  
> + /* Wait for endpoint to be enabled */
> + ep = epfile->ep;
> + if (!ep) {
> + if (file->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + ret = wait_event_interruptible(epfile->wait, (ep = epfile->ep));
> + if (ret)
> + return -EINTR;
> + }
> +
>   spin_lock_irq(>ffs->eps_lock);
> - if (likely(epfile->ep)) {
> - switch (code) {
> - case FUNCTIONFS_FIFO_STATUS:
> - ret = usb_ep_fifo_status(epfile->ep->ep);
> - break;
> - case FUNCTIONFS_FIFO_FLUSH:
> - usb_ep_fifo_flush(epfile->ep->ep);
> - ret = 0;
> - break;
> - case FUNCTIONFS_CLEAR_HALT:
> - ret = usb_ep_clear_halt(epfile->ep->ep);
> - break;
> - 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];
> + /* In the meantime, endpoint got disabled or changed. */
> + if (epfile->ep != ep) {
> + spin_unlock_irq(>ffs->eps_lock);
> + return -ESHUTDOWN;
> + }
>  
> - spin_unlock_irq(>ffs->eps_lock);
> - ret = copy_to_user((void *)value, desc, desc->bLength);
> - if (ret)
> - ret = -EFAULT;
> - return ret;
> - }
> + switch (code) {
> + case FUNCTIONFS_FIFO_STATUS:
> + ret = usb_ep_fifo_status(epfile->ep->ep);
> + break;
> + case FUNCTIONFS_FIFO_FLUSH:
> + usb_ep_fifo_flush(epfile->ep->ep);
> + ret = 0;
> + break;
> + case FUNCTIONFS_CLEAR_HALT:
> + ret = usb_ep_clear_halt(epfile->ep->ep);
> + break;
> + 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:
> - ret = -ENOTTY;
> + desc_idx = 0;
>   }
> - } else {
> - ret = -ENODEV;
> + desc = epfile->ep->descs[desc_idx];
> +
> + spin_unlock_irq(>ffs->eps_lock);
> + ret = copy_to_user((void *)value, desc, desc->bLength);
> + if (ret)
> + ret = -EFAULT;
> + return ret;

[PATCH V3 2/3] usb: gadget: function: f_fs: Let ffs_epfile_ioctl wait for enable.

2017-04-19 Thread Jerry Zhang 
This allows users to make an ioctl call as the first action on a
connection. Ex, some functions might want to get endpoint size
before making any i/os.

Previously, calling ioctls before read/write would depending on the
timing of endpoints being enabled.

ESHUTDOWN is now a possible return value and ENODEV is not, so change
docs accordingly.

Signed-off-by: Jerry Zhang 
---
Changelog since v2:
- Updated doc to account for -ESHUTDOWN

Changelog since v1:
- Guarded against epfile->ep changing after taking lock

 drivers/usb/gadget/function/f_fs.c  | 93 +
 include/uapi/linux/usb/functionfs.h |  7 +--
 2 files changed, 58 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index a0085571824d..04425e6b457a 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1190,6 +1190,7 @@ static long ffs_epfile_ioctl(struct file *file, unsigned 
code,
 unsigned long value)
 {
struct ffs_epfile *epfile = file->private_data;
+   struct ffs_ep *ep;
int ret;
 
ENTER();
@@ -1197,50 +1198,64 @@ static long ffs_epfile_ioctl(struct file *file, 
unsigned code,
if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
return -ENODEV;
 
+   /* Wait for endpoint to be enabled */
+   ep = epfile->ep;
+   if (!ep) {
+   if (file->f_flags & O_NONBLOCK)
+   return -EAGAIN;
+
+   ret = wait_event_interruptible(epfile->wait, (ep = epfile->ep));
+   if (ret)
+   return -EINTR;
+   }
+
spin_lock_irq(>ffs->eps_lock);
-   if (likely(epfile->ep)) {
-   switch (code) {
-   case FUNCTIONFS_FIFO_STATUS:
-   ret = usb_ep_fifo_status(epfile->ep->ep);
-   break;
-   case FUNCTIONFS_FIFO_FLUSH:
-   usb_ep_fifo_flush(epfile->ep->ep);
-   ret = 0;
-   break;
-   case FUNCTIONFS_CLEAR_HALT:
-   ret = usb_ep_clear_halt(epfile->ep->ep);
-   break;
-   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];
+   /* In the meantime, endpoint got disabled or changed. */
+   if (epfile->ep != ep) {
+   spin_unlock_irq(>ffs->eps_lock);
+   return -ESHUTDOWN;
+   }
 
-   spin_unlock_irq(>ffs->eps_lock);
-   ret = copy_to_user((void *)value, desc, desc->bLength);
-   if (ret)
-   ret = -EFAULT;
-   return ret;
-   }
+   switch (code) {
+   case FUNCTIONFS_FIFO_STATUS:
+   ret = usb_ep_fifo_status(epfile->ep->ep);
+   break;
+   case FUNCTIONFS_FIFO_FLUSH:
+   usb_ep_fifo_flush(epfile->ep->ep);
+   ret = 0;
+   break;
+   case FUNCTIONFS_CLEAR_HALT:
+   ret = usb_ep_clear_halt(epfile->ep->ep);
+   break;
+   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:
-   ret = -ENOTTY;
+   desc_idx = 0;
}
-   } else {
-   ret = -ENODEV;
+   desc = epfile->ep->descs[desc_idx];
+
+   spin_unlock_irq(>ffs->eps_lock);
+   ret = copy_to_user((void *)value, desc, desc->bLength);
+   if (ret)
+   ret = -EFAULT;
+   return ret;
+   }
+   default:
+   ret = -ENOTTY;
}
spin_unlock_irq(>ffs->eps_lock);
 
diff --git a/include/uapi/linux/usb/functionfs.h