Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Michal Nazarewicz]

On Tue, Jan 05 2016, Peter Chen wrote:
> Why -EINTR, the kernel-doc said it should return -ECONNRESET for
> active request, see include/linux/usb/gadget.h.

Because EINTR is what read returns to the user if the operation has been
interrupted by a signal, see ‘man 2 read’:

   EINTR The call was interrupted by a signal before any data was
   read; see signal(7).


-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Michal Nazarewicz]

On Tue, Jan 05 2016, Peter Chen wrote:
> Why -EINTR, the kernel-doc said it should return -ECONNRESET for
> active request, see include/linux/usb/gadget.h.

Because EINTR is what read returns to the user if the operation has been
interrupted by a signal, see ‘man 2 read’:

   EINTR The call was interrupted by a signal before any data was
   read; see signal(7).


-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +------ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Du, Changbin]

> >
> > You are right, but the problem is the request->status is wrong. If the data
> > send out but report caller as -EINTR, it will introduce duplicate-send
> > issue.
> >
> 
> Why -EINTR, the kernel-doc said it should return -ECONNRESET for active
> request, see include/linux/usb/gadget.h.
> 
> --
> 
> Best Regards,
> Peter Chen
F_fs return -EINTER in its dequeuer case, not udc driver. What I want
to say is driver should return the right status for each usb request.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Peter Chen]

On Tue, Jan 05, 2016 at 04:09:47AM +, Du, Changbin wrote:
> > > To avoid this, just dequeue the request first. After usb_ep_dequeue, the
> > > request must be done or canceled.
> > >
> > > With this change, we can ensure no race condition in f_fs driver. But
> > > actually I found some of the udc driver has analogical issue in its
> > > dequeue implementation. For example,
> > > 1) the dequeue function hold the controller's lock.
> > > 2) before driver request controller  to stop transfer, a request
> > >completed.
> > > 3) the controller trigger a interrupt, but its irq handler need wait
> > >dequeue function to release the lock.
> > > 4) dequeue function give back the request with negative status, and
> > >release lock.
> > > 5) irq handler get lock but the request has already been given back.
> > >
> > 
> > get unlock?
> > 
> > During the interrupt handler, it should only handle the "data complete"
> > interrupt on queued request; if the "data complete" interrupt occurs, but
> > it belongs to nobody, it will handle noop.
> > 
> > 
> > Best Regards,
> > Peter Chen
> 
> You are right, but the problem is the request->status is wrong. If the data
> send out but report caller as -EINTR, it will introduce duplicate-send
> issue.
> 

Why -EINTR, the kernel-doc said it should return -ECONNRESET for active
request, see include/linux/usb/gadget.h.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Du, Changbin]

> > To avoid this, just dequeue the request first. After usb_ep_dequeue, the
> > request must be done or canceled.
> >
> > With this change, we can ensure no race condition in f_fs driver. But
> > actually I found some of the udc driver has analogical issue in its
> > dequeue implementation. For example,
> > 1) the dequeue function hold the controller's lock.
> > 2) before driver request controller  to stop transfer, a request
> >completed.
> > 3) the controller trigger a interrupt, but its irq handler need wait
> >dequeue function to release the lock.
> > 4) dequeue function give back the request with negative status, and
> >release lock.
> > 5) irq handler get lock but the request has already been given back.
> >
> 
> get unlock?
> 
> During the interrupt handler, it should only handle the "data complete"
> interrupt on queued request; if the "data complete" interrupt occurs, but
> it belongs to nobody, it will handle noop.
> 
> 
> Best Regards,
> Peter Chen

You are right, but the problem is the request->status is wrong. If the data
send out but report caller as -EINTR, it will introduce duplicate-send
issue.

Regards,
Du, Changbin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Peter Chen]

On Tue, Dec 29, 2015 at 02:36:58PM +0800, changbin...@intel.com wrote:
> From: "Du, Changbin" 
> 
> ffs_epfile_io and ffs_epfile_io_complete runs in different context, but
> there is no synchronization between them.
> 
> consider the following scenario:
> 1) ffs_epfile_io interrupted by sigal while
> wait_for_completion_interruptible
> 2) then ffs_epfile_io set ret to -EINTR
> 3) just before or during usb_ep_dequeue, the request completed
> 4) ffs_epfile_io return with -EINTR
> 
> In this case, ffs_epfile_io tell caller no transfer success but actually
> it may has been done. This break the caller's pipe.
> 
> Below script can help test it (adbd is the process which lies on f_fs).
> while true
> do
>pkill -19 adbd #SIGSTOP
>pkill -18 adbd #SIGCONT
>sleep 0.1
> done
> 
> To avoid this, just dequeue the request first. After usb_ep_dequeue, the
> request must be done or canceled.
> 
> With this change, we can ensure no race condition in f_fs driver. But
> actually I found some of the udc driver has analogical issue in its
> dequeue implementation. For example,
> 1) the dequeue function hold the controller's lock.
> 2) before driver request controller  to stop transfer, a request
>completed.
> 3) the controller trigger a interrupt, but its irq handler need wait
>dequeue function to release the lock.
> 4) dequeue function give back the request with negative status, and
>release lock.
> 5) irq handler get lock but the request has already been given back.
> 

get unlock?

During the interrupt handler, it should only handle the "data complete"
interrupt on queued request; if the "data complete" interrupt occurs, but
it belongs to nobody, it will handle noop.

> So, the dequeue implementation should take care of this case. IMO, it
> can be done as below steps to dequeue a already started request,
> 1) request controller to stop transfer on the given ep. HW know the
>actual transfer status.
> 2) after hw stop transfer, driver scan if there are any completed one.
> 3) if found, process it with real status. if no, the request can
>canceled.
> 
> Signed-off-by: Du, Changbin 
> ---
>  drivers/usb/gadget/function/f_fs.c | 45 
> --
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index cf43e9e..8050939 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -687,6 +687,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>   struct ffs_ep *ep;
>   char *data = NULL;
>   ssize_t ret, data_len = -EINVAL;
> + bool interrupted = false;
>   int halt;
>  
>   /* Are we still active? */
> @@ -829,26 +830,35 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>  
>   spin_unlock_irq(>ffs->eps_lock);
>  
> - if (unlikely(ret < 0)) {
> - /* nop */
> - } else if (unlikely(
> + if (unlikely(ret < 0))
> + goto error_mutex;
> +
> + if (unlikely(
>  wait_for_completion_interruptible())) {
> - ret = -EINTR;
> - usb_ep_dequeue(ep->ep, req);
> - } else {
>   /*
> -  * XXX We may end up silently droping data
> -  * here.  Since data_len (i.e. req->length) may
> -  * be bigger than len (after being rounded up
> -  * to maxpacketsize), we may end up with more
> -  * data then user space has space for.
> +  * To avoid race condition with
> +  * ffs_epfile_io_complete, dequeue the request
> +  * first then check status. usb_ep_dequeue API
> +  * should guarantee no race condition with
> +  * req->complete callback.
>*/
> - ret = ep->status;
> - if (io_data->read && ret > 0) {
> - ret = copy_to_iter(data, ret, 
> _data->data);
> - if (!ret)
> - ret = -EFAULT;
> - }
> + usb_ep_dequeue(ep->ep, req);
> + interrupted = true;
> + }
> +
> + /*
> +  * XXX We may end up silently droping data
> +  * here.  Since data_len (i.e. req->length) may
> +  * be bigger than len (after being rounded up
> +  * to 

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Michal Nazarewicz]

On Tue, Dec 29 2015, changbin...@intel.com wrote:
> From: "Du, Changbin" 
>
> ffs_epfile_io and ffs_epfile_io_complete runs in different context, but
> there is no synchronization between them.
>
> consider the following scenario:
> 1) ffs_epfile_io interrupted by sigal while
> wait_for_completion_interruptible
> 2) then ffs_epfile_io set ret to -EINTR
> 3) just before or during usb_ep_dequeue, the request completed
> 4) ffs_epfile_io return with -EINTR
>
> In this case, ffs_epfile_io tell caller no transfer success but actually
> it may has been done. This break the caller's pipe.
>
> Below script can help test it (adbd is the process which lies on f_fs).
> while true
> do
>pkill -19 adbd #SIGSTOP
>pkill -18 adbd #SIGCONT
>sleep 0.1
> done
>
> To avoid this, just dequeue the request first. After usb_ep_dequeue, the
> request must be done or canceled.
>
> With this change, we can ensure no race condition in f_fs driver. But
> actually I found some of the udc driver has analogical issue in its
> dequeue implementation. For example,
> 1) the dequeue function hold the controller's lock.
> 2) before driver request controller  to stop transfer, a request
>completed.
> 3) the controller trigger a interrupt, but its irq handler need wait
>dequeue function to release the lock.
> 4) dequeue function give back the request with negative status, and
>release lock.
> 5) irq handler get lock but the request has already been given back.
>
> So, the dequeue implementation should take care of this case. IMO, it
> can be done as below steps to dequeue a already started request,
> 1) request controller to stop transfer on the given ep. HW know the
>actual transfer status.
> 2) after hw stop transfer, driver scan if there are any completed one.
> 3) if found, process it with real status. if no, the request can
>canceled.
>
> Signed-off-by: Du, Changbin 

Acked-by: Michal Nazarewicz 

While reviewing this patch I found two other bugs in ffs_epfile_io and
some more opportunities to refactor the code.

As soon as the kernel compiles I’ll send a patch set which will include
a modified version of this patch which has a smaller difference.

> ---
>  drivers/usb/gadget/function/f_fs.c | 45 
> --
>  1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index cf43e9e..8050939 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -687,6 +687,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>   struct ffs_ep *ep;
>   char *data = NULL;
>   ssize_t ret, data_len = -EINVAL;
> + bool interrupted = false;
>   int halt;
>  
>   /* Are we still active? */
> @@ -829,26 +830,35 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>  
>   spin_unlock_irq(>ffs->eps_lock);
>  
> - if (unlikely(ret < 0)) {
> - /* nop */
> - } else if (unlikely(
> + if (unlikely(ret < 0))
> + goto error_mutex;
> +
> + if (unlikely(
>  wait_for_completion_interruptible())) {
> - ret = -EINTR;
> - usb_ep_dequeue(ep->ep, req);
> - } else {
>   /*
> -  * XXX We may end up silently droping data
> -  * here.  Since data_len (i.e. req->length) may
> -  * be bigger than len (after being rounded up
> -  * to maxpacketsize), we may end up with more
> -  * data then user space has space for.
> +  * To avoid race condition with
> +  * ffs_epfile_io_complete, dequeue the request
> +  * first then check status. usb_ep_dequeue API
> +  * should guarantee no race condition with
> +  * req->complete callback.
>*/
> - ret = ep->status;
> - if (io_data->read && ret > 0) {
> - ret = copy_to_iter(data, ret, 
> _data->data);
> - if (!ret)
> - ret = -EFAULT;
> - }
> + usb_ep_dequeue(ep->ep, req);
> + interrupted = true;
> + }
> +
> + /*
> +  * XXX We may end up silently droping data
> +  * here.  Since data_len (i.e. req->length) may
> +  * be bigger than len 

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Michal Nazarewicz]

On Tue, Dec 29 2015, changbin...@intel.com wrote:
> From: "Du, Changbin" 
>
> ffs_epfile_io and ffs_epfile_io_complete runs in different context, but
> there is no synchronization between them.
>
> consider the following scenario:
> 1) ffs_epfile_io interrupted by sigal while
> wait_for_completion_interruptible
> 2) then ffs_epfile_io set ret to -EINTR
> 3) just before or during usb_ep_dequeue, the request completed
> 4) ffs_epfile_io return with -EINTR
>
> In this case, ffs_epfile_io tell caller no transfer success but actually
> it may has been done. This break the caller's pipe.
>
> Below script can help test it (adbd is the process which lies on f_fs).
> while true
> do
>pkill -19 adbd #SIGSTOP
>pkill -18 adbd #SIGCONT
>sleep 0.1
> done
>
> To avoid this, just dequeue the request first. After usb_ep_dequeue, the
> request must be done or canceled.
>
> With this change, we can ensure no race condition in f_fs driver. But
> actually I found some of the udc driver has analogical issue in its
> dequeue implementation. For example,
> 1) the dequeue function hold the controller's lock.
> 2) before driver request controller  to stop transfer, a request
>completed.
> 3) the controller trigger a interrupt, but its irq handler need wait
>dequeue function to release the lock.
> 4) dequeue function give back the request with negative status, and
>release lock.
> 5) irq handler get lock but the request has already been given back.
>
> So, the dequeue implementation should take care of this case. IMO, it
> can be done as below steps to dequeue a already started request,
> 1) request controller to stop transfer on the given ep. HW know the
>actual transfer status.
> 2) after hw stop transfer, driver scan if there are any completed one.
> 3) if found, process it with real status. if no, the request can
>canceled.
>
> Signed-off-by: Du, Changbin 

Acked-by: Michal Nazarewicz 

While reviewing this patch I found two other bugs in ffs_epfile_io and
some more opportunities to refactor the code.

As soon as the kernel compiles I’ll send a patch set which will include
a modified version of this patch which has a smaller difference.

> ---
>  drivers/usb/gadget/function/f_fs.c | 45 
> --
>  1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index cf43e9e..8050939 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -687,6 +687,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>   struct ffs_ep *ep;
>   char *data = NULL;
>   ssize_t ret, data_len = -EINVAL;
> + bool interrupted = false;
>   int halt;
>  
>   /* Are we still active? */
> @@ -829,26 +830,35 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>  
>   spin_unlock_irq(>ffs->eps_lock);
>  
> - if (unlikely(ret < 0)) {
> - /* nop */
> - } else if (unlikely(
> + if (unlikely(ret < 0))
> + goto error_mutex;
> +
> + if (unlikely(
>  wait_for_completion_interruptible())) {
> - ret = -EINTR;
> - usb_ep_dequeue(ep->ep, req);
> - } else {
>   /*
> -  * XXX We may end up silently droping data
> -  * here.  Since data_len (i.e. req->length) may
> -  * be bigger than len (after being rounded up
> -  * to maxpacketsize), we may end up with more
> -  * data then user space has space for.
> +  * To avoid race condition with
> +  * ffs_epfile_io_complete, dequeue the request
> +  * first then check status. usb_ep_dequeue API
> +  * should guarantee no race condition with
> +  * req->complete callback.
>*/
> - ret = ep->status;
> - if (io_data->read && ret > 0) {
> - ret = copy_to_iter(data, ret, 
> _data->data);
> - if (!ret)
> - ret = -EFAULT;
> - }
> + usb_ep_dequeue(ep->ep, req);
> + interrupted = true;
> + }
> +
> + /*
> +  * XXX We may end up silently droping data
> +  * here.  Since data_len 

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Peter Chen]

On Tue, Dec 29, 2015 at 02:36:58PM +0800, changbin...@intel.com wrote:
> From: "Du, Changbin" 
> 
> ffs_epfile_io and ffs_epfile_io_complete runs in different context, but
> there is no synchronization between them.
> 
> consider the following scenario:
> 1) ffs_epfile_io interrupted by sigal while
> wait_for_completion_interruptible
> 2) then ffs_epfile_io set ret to -EINTR
> 3) just before or during usb_ep_dequeue, the request completed
> 4) ffs_epfile_io return with -EINTR
> 
> In this case, ffs_epfile_io tell caller no transfer success but actually
> it may has been done. This break the caller's pipe.
> 
> Below script can help test it (adbd is the process which lies on f_fs).
> while true
> do
>pkill -19 adbd #SIGSTOP
>pkill -18 adbd #SIGCONT
>sleep 0.1
> done
> 
> To avoid this, just dequeue the request first. After usb_ep_dequeue, the
> request must be done or canceled.
> 
> With this change, we can ensure no race condition in f_fs driver. But
> actually I found some of the udc driver has analogical issue in its
> dequeue implementation. For example,
> 1) the dequeue function hold the controller's lock.
> 2) before driver request controller  to stop transfer, a request
>completed.
> 3) the controller trigger a interrupt, but its irq handler need wait
>dequeue function to release the lock.
> 4) dequeue function give back the request with negative status, and
>release lock.
> 5) irq handler get lock but the request has already been given back.
> 

get unlock?

During the interrupt handler, it should only handle the "data complete"
interrupt on queued request; if the "data complete" interrupt occurs, but
it belongs to nobody, it will handle noop.

> So, the dequeue implementation should take care of this case. IMO, it
> can be done as below steps to dequeue a already started request,
> 1) request controller to stop transfer on the given ep. HW know the
>actual transfer status.
> 2) after hw stop transfer, driver scan if there are any completed one.
> 3) if found, process it with real status. if no, the request can
>canceled.
> 
> Signed-off-by: Du, Changbin 
> ---
>  drivers/usb/gadget/function/f_fs.c | 45 
> --
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index cf43e9e..8050939 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -687,6 +687,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>   struct ffs_ep *ep;
>   char *data = NULL;
>   ssize_t ret, data_len = -EINVAL;
> + bool interrupted = false;
>   int halt;
>  
>   /* Are we still active? */
> @@ -829,26 +830,35 @@ static ssize_t ffs_epfile_io(struct file *file, struct 
> ffs_io_data *io_data)
>  
>   spin_unlock_irq(>ffs->eps_lock);
>  
> - if (unlikely(ret < 0)) {
> - /* nop */
> - } else if (unlikely(
> + if (unlikely(ret < 0))
> + goto error_mutex;
> +
> + if (unlikely(
>  wait_for_completion_interruptible())) {
> - ret = -EINTR;
> - usb_ep_dequeue(ep->ep, req);
> - } else {
>   /*
> -  * XXX We may end up silently droping data
> -  * here.  Since data_len (i.e. req->length) may
> -  * be bigger than len (after being rounded up
> -  * to maxpacketsize), we may end up with more
> -  * data then user space has space for.
> +  * To avoid race condition with
> +  * ffs_epfile_io_complete, dequeue the request
> +  * first then check status. usb_ep_dequeue API
> +  * should guarantee no race condition with
> +  * req->complete callback.
>*/
> - ret = ep->status;
> - if (io_data->read && ret > 0) {
> - ret = copy_to_iter(data, ret, 
> _data->data);
> - if (!ret)
> - ret = -EFAULT;
> - }
> + usb_ep_dequeue(ep->ep, req);
> + interrupted = true;
> + }
> +
> + /*
> +  * XXX We may end up silently droping data
> +  * here.  Since data_len (i.e. req->length) may
> +  * be bigger than len (after 

RE: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Du, Changbin]

> > To avoid this, just dequeue the request first. After usb_ep_dequeue, the
> > request must be done or canceled.
> >
> > With this change, we can ensure no race condition in f_fs driver. But
> > actually I found some of the udc driver has analogical issue in its
> > dequeue implementation. For example,
> > 1) the dequeue function hold the controller's lock.
> > 2) before driver request controller  to stop transfer, a request
> >completed.
> > 3) the controller trigger a interrupt, but its irq handler need wait
> >dequeue function to release the lock.
> > 4) dequeue function give back the request with negative status, and
> >release lock.
> > 5) irq handler get lock but the request has already been given back.
> >
> 
> get unlock?
> 
> During the interrupt handler, it should only handle the "data complete"
> interrupt on queued request; if the "data complete" interrupt occurs, but
> it belongs to nobody, it will handle noop.
> 
> 
> Best Regards,
> Peter Chen

You are right, but the problem is the request->status is wrong. If the data
send out but report caller as -EINTR, it will introduce duplicate-send
issue.

Regards,
Du, Changbin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Peter Chen]

On Tue, Jan 05, 2016 at 04:09:47AM +, Du, Changbin wrote:
> > > To avoid this, just dequeue the request first. After usb_ep_dequeue, the
> > > request must be done or canceled.
> > >
> > > With this change, we can ensure no race condition in f_fs driver. But
> > > actually I found some of the udc driver has analogical issue in its
> > > dequeue implementation. For example,
> > > 1) the dequeue function hold the controller's lock.
> > > 2) before driver request controller  to stop transfer, a request
> > >completed.
> > > 3) the controller trigger a interrupt, but its irq handler need wait
> > >dequeue function to release the lock.
> > > 4) dequeue function give back the request with negative status, and
> > >release lock.
> > > 5) irq handler get lock but the request has already been given back.
> > >
> > 
> > get unlock?
> > 
> > During the interrupt handler, it should only handle the "data complete"
> > interrupt on queued request; if the "data complete" interrupt occurs, but
> > it belongs to nobody, it will handle noop.
> > 
> > 
> > Best Regards,
> > Peter Chen
> 
> You are right, but the problem is the request->status is wrong. If the data
> send out but report caller as -EINTR, it will introduce duplicate-send
> issue.
> 

Why -EINTR, the kernel-doc said it should return -ECONNRESET for active
request, see include/linux/usb/gadget.h.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Du, Changbin]

> >
> > You are right, but the problem is the request->status is wrong. If the data
> > send out but report caller as -EINTR, it will introduce duplicate-send
> > issue.
> >
> 
> Why -EINTR, the kernel-doc said it should return -ECONNRESET for active
> request, see include/linux/usb/gadget.h.
> 
> --
> 
> Best Regards,
> Peter Chen
F_fs return -EINTER in its dequeuer case, not udc driver. What I want
to say is driver should return the right status for each usb request.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/