[PATCH 0/6] More QE UDC fixes

[PATCH 0/6] More QE UDC fixes

[Anton Vorontsov]

Hi all,

A few more qe_udc fixes...

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/6] usb/fsl_qe_udc: Fix oops on QE UDC probe failure

[Anton Vorontsov]

In case of probing errors the driver kfrees the udc_controller, but it
doesn't set the pointer to NULL.

When usb_gadget_register_driver is called, it checks for udc_controller
!= NULL, the check passes and the driver accesses nonexistent memory.
Fix this by setting udc_controller to NULL in case of errors.

While at it, also implement irq_of_parse_and_map()'s failure and cleanup
cases.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/usb/gadget/fsl_qe_udc.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index 94c38e4..60b9279 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -2601,6 +2601,10 @@ static int __devinit qe_udc_probe(struct of_device *ofdev,
 			(unsigned long)udc_controller);
 	/* request irq and disable DR  */
 	udc_controller->usb_irq = irq_of_parse_and_map(np, 0);
+	if (!udc_controller->usb_irq) {
+		ret = -EINVAL;
+		goto err_noirq;
+	}
 
 	ret = request_irq(udc_controller->usb_irq, qe_udc_irq, 0,
 				driver_name, udc_controller);
@@ -2622,6 +2626,8 @@ static int __devinit qe_udc_probe(struct of_device *ofdev,
 err6:
 	free_irq(udc_controller->usb_irq, udc_controller);
 err5:
+	irq_dispose_mapping(udc_controller->usb_irq);
+err_noirq:
 	if (udc_controller->nullmap) {
 		dma_unmap_single(udc_controller->gadget.dev.parent,
 			udc_controller->nullp, 256,
@@ -2645,7 +2651,7 @@ err2:
 	iounmap(udc_controller->usb_regs);
 err1:
 	kfree(udc_controller);
-
+	udc_controller = NULL;
 	return ret;
 }
 
@@ -2707,6 +2713,7 @@ static int __devexit qe_udc_remove(struct of_device *ofdev)
 	kfree(ep->txframe);
 
 	free_irq(udc_controller->usb_irq, udc_controller);
+	irq_dispose_mapping(udc_controller->usb_irq);
 
 	tasklet_kill(&udc_controller->rx_tasklet);
 
-- 
1.5.6.3

[PATCH 2/6] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()

[Anton Vorontsov]

The call chain is this:

qe_udc_irq() <- grabs the udc->lock spinlock
rx_irq()
qe_ep0_rx()
ep0_setup_handle()
setup_received_handle()
ch9getstatus()
qe_ep_queue() <- tries to grab the udc->lock again

It seems unsafe to temporarily drop the lock in the ch9getstatus(),
so to fix that bug the __qe_ep_queue() function implemented and used
by the ch9getstatus().

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/usb/gadget/fsl_qe_udc.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index 60b9279..abcb35d 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -1681,14 +1681,13 @@ static void qe_free_request(struct usb_ep *_ep, struct usb_request *_req)
 		kfree(req);
 }
 
-/* queues (submits) an I/O request to an endpoint */
-static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
-				gfp_t gfp_flags)
+static int __qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
+			 gfp_t gfp_flags, spinlock_t *lock)
 {
 	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
 	struct qe_req *req = container_of(_req, struct qe_req, req);
 	struct qe_udc *udc;
-	unsigned long flags;
+	unsigned long flags = 0; /* shut up gcc */
 	int reval;
 
 	udc = ep->udc;
@@ -1732,7 +1731,8 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
 	list_add_tail(&req->queue, &ep->queue);
 	dev_vdbg(udc->dev, "gadget have request in %s! %d\n",
 			ep->name, req->req.length);
-	spin_lock_irqsave(&udc->lock, flags);
+	if (lock)
+		spin_lock_irqsave(lock, flags);
 	/* push the request to device */
 	if (ep_is_in(ep))
 		reval = ep_req_send(ep, req);
@@ -1748,11 +1748,21 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
 	if (ep->dir == USB_DIR_OUT)
 		reval = ep_req_receive(ep, req);
 
-	spin_unlock_irqrestore(&udc->lock, flags);
+	if (lock)
+		spin_unlock_irqrestore(lock, flags);
 
 	return 0;
 }
 
+/* queues (submits) an I/O request to an endpoint */
+static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
+		       gfp_t gfp_flags)
+{
+	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
+
+	return __qe_ep_queue(_ep, _req, gfp_flags, &ep->udc->lock);
+}
+
 /* dequeues (cancels, unlinks) an I/O request from an endpoint */
 static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
@@ -2008,7 +2018,7 @@ static void ch9getstatus(struct qe_udc *udc, u8 request_type, u16 value,
 	udc->ep0_dir = USB_DIR_IN;
 
 	/* data phase */
-	status = qe_ep_queue(&ep->ep, &req->req, GFP_ATOMIC);
+	status = __qe_ep_queue(&ep->ep, &req->req, GFP_ATOMIC, NULL);
 
 	if (status == 0)
 		return;
-- 
1.5.6.3

[PATCH 3/6] usb/fsl_qe_udc: Fix QE USB controller initialization

[Anton Vorontsov]

qe_udc_reg_init() leaves the USB controller enabled before muram memory
initialized. Sometimes the uninitialized muram memory confuses the
controller, and it start sending the busy interrupts.

Fix this by disabling the controller, it will be enabled later by
the gadget driver, at bind time.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/usb/gadget/fsl_qe_udc.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index abcb35d..1789f65 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -2449,8 +2449,12 @@ static int __devinit qe_udc_reg_init(struct qe_udc *udc)
 	struct usb_ctlr __iomem *qe_usbregs;
 	qe_usbregs = udc->usb_regs;
 
-	/* Init the usb register */
+	/* Spec says that we must enable the USB controller to change mode. */
 	out_8(&qe_usbregs->usb_usmod, 0x01);
+	/* Mode changed, now disable it, since muram isn't initialized yet. */
+	out_8(&qe_usbregs->usb_usmod, 0x00);
+
+	/* Initialize the rest. */
 	out_be16(&qe_usbregs->usb_usbmr, 0);
 	out_8(&qe_usbregs->usb_uscom, 0);
 	out_be16(&qe_usbregs->usb_usber, USBER_ALL_CLEAR);
-- 
1.5.6.3

[PATCH 4/6] usb/fsl_qe_udc: Fix disconnects reporting during bus reset

[Anton Vorontsov]

Freescale QE UDC controllers can't report the "port change" states,
so the only way to handle disconnects is to process bus reset
interrupts. The bus reset can take some time, that is, few irqs.
Gadgets may print the disconnection events, and this causes few
repetitive messages in the kernel log.

This patch fixes the issue by using the usb_state machine, if the
usb controller has been already reset, just quit the reset irq
early.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/usb/gadget/fsl_qe_udc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index 1789f65..7bb5f36 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -2161,6 +2161,9 @@ static int reset_irq(struct qe_udc *udc)
 {
 	unsigned char i;
 
+	if (udc->usb_state == USB_STATE_DEFAULT)
+		return 0;
+
 	qe_usb_disable();
 	out_8(&udc->usb_regs->usb_usadr, 0);
 
-- 
1.5.6.3

[PATCH 5/6] usb/fsl_qe_udc: Fix muram corruption by disabled endpoints

[Anton Vorontsov]

Before freeing an endpoint's muram memory, we should stop all activity
of the endpoint, otherwise the QE UDC controller might do nasty things
with the muram memory that isn't belong to that endpoint anymore.

The qe_ep_reset() effectively flushes the hardware fifos, finishes all
late transaction and thus prevents the corruption.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/usb/gadget/fsl_qe_udc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index 7bb5f36..cb47337 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -1622,6 +1622,7 @@ static int qe_ep_disable(struct usb_ep *_ep)
 	nuke(ep, -ESHUTDOWN);
 	ep->desc = NULL;
 	ep->stopped = 1;
+	qe_ep_reset(udc, ep->epnum);
 	spin_unlock_irqrestore(&udc->lock, flags);
 
 	cpm_muram_free(cpm_muram_offset(ep->rxbase));
-- 
1.5.6.3

[PATCH 6/6] usb/fsl_qe_udc: Fix stalled TX requests bug

[Anton Vorontsov]

While disabling an endpoint the driver nuking any pending requests,
thus completing them with -ESHUTDOWN status. But the driver doesn't
clear the tx_req, which means that a next TX request (after
ep_enable), might get stalled, since the driver won't queue the new
reqests.

This fixes a bug I'm observing with ethernet gadget while playing
with ifconfig usb0 up/down (the up/down sequence disables and
enables `in' and `out' endpoints).

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/usb/gadget/fsl_qe_udc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index cb47337..37c8575 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -1622,6 +1622,7 @@ static int qe_ep_disable(struct usb_ep *_ep)
 	nuke(ep, -ESHUTDOWN);
 	ep->desc = NULL;
 	ep->stopped = 1;
+	ep->tx_req = NULL;
 	qe_ep_reset(udc, ep->epnum);
 	spin_unlock_irqrestore(&udc->lock, flags);
 
-- 
1.5.6.3

Re: [PATCH 4/6] usb/fsl_qe_udc: Fix disconnects reporting during bus reset

[David Brownell]

On Tuesday 11 November 2008, Anton Vorontsov wrote:
> Freescale QE UDC controllers can't report the "port change" states,
> so the only way to handle disconnects is to process bus reset
> interrupts. The bus reset can take some time, that is, few irqs.
> Gadgets may print the disconnection events, and this causes few
> repetitive messages in the kernel log.
> 
> This patch fixes the issue by using the usb_state machine, if the
> usb controller has been already reset, just quit the reset irq
> early.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

Note that the "port change" you reference is more typically
packaged as "VBUS detect" ... often handled with a GPIO.

> ---
>  drivers/usb/gadget/fsl_qe_udc.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index 1789f65..7bb5f36 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -2161,6 +2161,9 @@ static int reset_irq(struct qe_udc *udc)
>  {
>  	unsigned char i;
>  
> +	if (udc->usb_state == USB_STATE_DEFAULT)
> +		return 0;
> +
>  	qe_usb_disable();
>  	out_8(&udc->usb_regs->usb_usadr, 0);
>  
> -- 
> 1.5.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 5/6] usb/fsl_qe_udc: Fix muram corruption by disabled endpoints

[David Brownell]

On Tuesday 11 November 2008, Anton Vorontsov wrote:
> Before freeing an endpoint's muram memory, we should stop all activity
> of the endpoint, otherwise the QE UDC controller might do nasty things
> with the muram memory that isn't belong to that endpoint anymore.
> 
> The qe_ep_reset() effectively flushes the hardware fifos, finishes all
> late transaction and thus prevents the corruption.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>


> ---
>  drivers/usb/gadget/fsl_qe_udc.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index 7bb5f36..cb47337 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -1622,6 +1622,7 @@ static int qe_ep_disable(struct usb_ep *_ep)
>  	nuke(ep, -ESHUTDOWN);
>  	ep->desc = NULL;
>  	ep->stopped = 1;
> +	qe_ep_reset(udc, ep->epnum);
>  	spin_unlock_irqrestore(&udc->lock, flags);
>  
>  	cpm_muram_free(cpm_muram_offset(ep->rxbase));
> -- 
> 1.5.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 6/6] usb/fsl_qe_udc: Fix stalled TX requests bug

[David Brownell]

On Tuesday 11 November 2008, Anton Vorontsov wrote:
> While disabling an endpoint the driver nuking any pending requests,
> thus completing them with -ESHUTDOWN status. But the driver doesn't
> clear the tx_req, which means that a next TX request (after
> ep_enable), might get stalled, since the driver won't queue the new
> reqests.
> 
> This fixes a bug I'm observing with ethernet gadget while playing
> with ifconfig usb0 up/down (the up/down sequence disables and
> enables `in' and `out' endpoints).
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

> ---
>  drivers/usb/gadget/fsl_qe_udc.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index cb47337..37c8575 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -1622,6 +1622,7 @@ static int qe_ep_disable(struct usb_ep *_ep)
>  	nuke(ep, -ESHUTDOWN);
>  	ep->desc = NULL;
>  	ep->stopped = 1;
> +	ep->tx_req = NULL;
>  	qe_ep_reset(udc, ep->epnum);
>  	spin_unlock_irqrestore(&udc->lock, flags);
>  
> -- 
> 1.5.6.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 1/6] usb/fsl_qe_udc: Fix oops on QE UDC probe failure

[David Brownell]

On Tuesday 11 November 2008, Anton Vorontsov wrote:
> In case of probing errors the driver kfrees the udc_controller, but it
> doesn't set the pointer to NULL.
> 
> When usb_gadget_register_driver is called, it checks for udc_controller
> != NULL, the check passes and the driver accesses nonexistent memory.
> Fix this by setting udc_controller to NULL in case of errors.
> 
> While at it, also implement irq_of_parse_and_map()'s failure and cleanup
> cases.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

I seem to detect a lot of bugfix activity here, which
tends to reflect usage ... good!  :)


> ---
>  drivers/usb/gadget/fsl_qe_udc.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index 94c38e4..60b9279 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -2601,6 +2601,10 @@ static int __devinit qe_udc_probe(struct of_device *ofdev,
>  			(unsigned long)udc_controller);
>  	/* request irq and disable DR  */
>  	udc_controller->usb_irq = irq_of_parse_and_map(np, 0);
> +	if (!udc_controller->usb_irq) {
> +		ret = -EINVAL;
> +		goto err_noirq;
> +	}
>  
>  	ret = request_irq(udc_controller->usb_irq, qe_udc_irq, 0,
>  				driver_name, udc_controller);
> @@ -2622,6 +2626,8 @@ static int __devinit qe_udc_probe(struct of_device *ofdev,
>  err6:
>  	free_irq(udc_controller->usb_irq, udc_controller);
>  err5:
> +	irq_dispose_mapping(udc_controller->usb_irq);
> +err_noirq:
>  	if (udc_controller->nullmap) {
>  		dma_unmap_single(udc_controller->gadget.dev.parent,
>  			udc_controller->nullp, 256,
> @@ -2645,7 +2651,7 @@ err2:
>  	iounmap(udc_controller->usb_regs);
>  err1:
>  	kfree(udc_controller);
> -
> +	udc_controller = NULL;
>  	return ret;
>  }
>  
> @@ -2707,6 +2713,7 @@ static int __devexit qe_udc_remove(struct of_device *ofdev)
>  	kfree(ep->txframe);
>  
>  	free_irq(udc_controller->usb_irq, udc_controller);
> +	irq_dispose_mapping(udc_controller->usb_irq);
>  
>  	tasklet_kill(&udc_controller->rx_tasklet);
>  
> -- 
> 1.5.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 2/6] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()

[David Brownell]

On Tuesday 11 November 2008, Anton Vorontsov wrote:
> -       spin_lock_irqsave(&udc->lock, flags);
> +       if (lock)
> +               spin_lock_irqsave(lock, flags);

Ugly ugly ugly.  Conditional locking is error prone ... don't.

Couldn't you just have the usb_ep_queue() method wrap lock calls
around a common routine called by that and the status reporting code?

Or just have the status reporting code stuff the two bytes directly
into the FIFO?  (Which is what a lot of other drivers do.)

- Dave

[PATCH 2/6 v2] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()

[Anton Vorontsov]

The call chain is this:

qe_udc_irq() <- grabs the udc->lock spinlock
rx_irq()
qe_ep0_rx()
ep0_setup_handle()
setup_received_handle()
ch9getstatus()
qe_ep_queue() <- tries to grab the udc->lock again

It seems unsafe to temporarily drop the lock in the ch9getstatus(),
so to fix that bug the lock-less __qe_ep_queue() function
implemented and used by the ch9getstatus().

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Mon, Nov 17, 2008 at 05:59:42PM -0800, David Brownell wrote:
> On Tuesday 11 November 2008, Anton Vorontsov wrote:
> > -       spin_lock_irqsave(&udc->lock, flags);
> > +       if (lock)
> > +               spin_lock_irqsave(lock, flags);
> 
> Ugly ugly ugly.  Conditional locking is error prone ... don't.
> 
> Couldn't you just have the usb_ep_queue() method wrap lock calls
> around a common routine called by that and the status reporting code?

Yeah, that's much better approach.

> Or just have the status reporting code stuff the two bytes directly
> into the FIFO?  (Which is what a lot of other drivers do.)

Well, I think that using common code path for data transmission is
better, and simpler.

Thanks for the review.

 drivers/usb/gadget/fsl_qe_udc.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index 60b9279..165ff76 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -1681,14 +1681,12 @@ static void qe_free_request(struct usb_ep *_ep, struct usb_request *_req)
 		kfree(req);
 }
 
-/* queues (submits) an I/O request to an endpoint */
-static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
-				gfp_t gfp_flags)
+static int __qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
+			 gfp_t gfp_flags)
 {
 	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
 	struct qe_req *req = container_of(_req, struct qe_req, req);
 	struct qe_udc *udc;
-	unsigned long flags;
 	int reval;
 
 	udc = ep->udc;
@@ -1732,7 +1730,7 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
 	list_add_tail(&req->queue, &ep->queue);
 	dev_vdbg(udc->dev, "gadget have request in %s! %d\n",
 			ep->name, req->req.length);
-	spin_lock_irqsave(&udc->lock, flags);
+
 	/* push the request to device */
 	if (ep_is_in(ep))
 		reval = ep_req_send(ep, req);
@@ -1748,11 +1746,24 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
 	if (ep->dir == USB_DIR_OUT)
 		reval = ep_req_receive(ep, req);
 
-	spin_unlock_irqrestore(&udc->lock, flags);
-
 	return 0;
 }
 
+/* queues (submits) an I/O request to an endpoint */
+static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
+		       gfp_t gfp_flags)
+{
+	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
+	struct qe_udc *udc = ep->udc;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&udc->lock, flags);
+	ret = __qe_ep_queue(_ep, _req, gfp_flags);
+	spin_unlock_irqrestore(&udc->lock, flags);
+	return ret;
+}
+
 /* dequeues (cancels, unlinks) an I/O request from an endpoint */
 static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
@@ -2008,7 +2019,7 @@ static void ch9getstatus(struct qe_udc *udc, u8 request_type, u16 value,
 	udc->ep0_dir = USB_DIR_IN;
 
 	/* data phase */
-	status = qe_ep_queue(&ep->ep, &req->req, GFP_ATOMIC);
+	status = __qe_ep_queue(&ep->ep, &req->req, GFP_ATOMIC);
 
 	if (status == 0)
 		return;
-- 
1.5.6.3

Re: [PATCH 2/6 v2] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()

[David Brownell]

On Tuesday 18 November 2008, Anton Vorontsov wrote:
> +       spin_lock_irqsave(&udc->lock, flags);
> +       ret = __qe_ep_queue(_ep, _req, gfp_flags);
> +       spin_unlock_irqrestore(&udc->lock, flags);

Why are you passing "gfp_flags"?  Especially without
checking ... GFP_KERNEL will be illegal, for example.

[PATCH 2/6 v3] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()

[Anton Vorontsov]

The call chain is this:

qe_udc_irq() <- grabs the udc->lock spinlock
rx_irq()
qe_ep0_rx()
ep0_setup_handle()
setup_received_handle()
ch9getstatus()
qe_ep_queue() <- tries to grab the udc->lock again

It seems unsafe to temporarily drop the lock in the ch9getstatus(),
so to fix that bug the lock-less __qe_ep_queue() function
implemented and used by the ch9getstatus().

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Tue, Nov 18, 2008 at 02:13:30PM -0800, David Brownell wrote:
> On Tuesday 18 November 2008, Anton Vorontsov wrote:
> > +       spin_lock_irqsave(&udc->lock, flags);
> > +       ret = __qe_ep_queue(_ep, _req, gfp_flags);
> > +       spin_unlock_irqrestore(&udc->lock, flags);
> 
> Why are you passing "gfp_flags"?  Especially without
> checking ... GFP_KERNEL will be illegal, for example.

Ugh, the gfp_flags aren't used at all. How about that
patch?

 drivers/usb/gadget/fsl_qe_udc.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index 60b9279..3587970 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -1681,14 +1681,11 @@ static void qe_free_request(struct usb_ep *_ep, struct usb_request *_req)
 		kfree(req);
 }
 
-/* queues (submits) an I/O request to an endpoint */
-static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
-				gfp_t gfp_flags)
+static int __qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
 	struct qe_req *req = container_of(_req, struct qe_req, req);
 	struct qe_udc *udc;
-	unsigned long flags;
 	int reval;
 
 	udc = ep->udc;
@@ -1732,7 +1729,7 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
 	list_add_tail(&req->queue, &ep->queue);
 	dev_vdbg(udc->dev, "gadget have request in %s! %d\n",
 			ep->name, req->req.length);
-	spin_lock_irqsave(&udc->lock, flags);
+
 	/* push the request to device */
 	if (ep_is_in(ep))
 		reval = ep_req_send(ep, req);
@@ -1748,11 +1745,24 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
 	if (ep->dir == USB_DIR_OUT)
 		reval = ep_req_receive(ep, req);
 
-	spin_unlock_irqrestore(&udc->lock, flags);
-
 	return 0;
 }
 
+/* queues (submits) an I/O request to an endpoint */
+static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
+		       gfp_t gfp_flags)
+{
+	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
+	struct qe_udc *udc = ep->udc;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&udc->lock, flags);
+	ret = __qe_ep_queue(_ep, _req);
+	spin_unlock_irqrestore(&udc->lock, flags);
+	return ret;
+}
+
 /* dequeues (cancels, unlinks) an I/O request from an endpoint */
 static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
@@ -2008,7 +2018,7 @@ static void ch9getstatus(struct qe_udc *udc, u8 request_type, u16 value,
 	udc->ep0_dir = USB_DIR_IN;
 
 	/* data phase */
-	status = qe_ep_queue(&ep->ep, &req->req, GFP_ATOMIC);
+	status = __qe_ep_queue(&ep->ep, &req->req);
 
 	if (status == 0)
 		return;
-- 
1.5.6.3

Re: [PATCH 2/6 v3] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()

[David Brownell]

On Tuesday 18 November 2008, Anton Vorontsov wrote:
> The call chain is this:
> 
> qe_udc_irq() <- grabs the udc->lock spinlock
> rx_irq()
> qe_ep0_rx()
> ep0_setup_handle()
> setup_received_handle()
> ch9getstatus()
> qe_ep_queue() <- tries to grab the udc->lock again
> 
> It seems unsafe to temporarily drop the lock in the ch9getstatus(),
> so to fix that bug the lock-less __qe_ep_queue() function
> implemented and used by the ch9getstatus().
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>


... noting that this *also* fixes a locking bug in qe_ep_queue(),
where it modified the queue head without holding the spinlock.

(So for example an IRQ in the middle of that update could end
up updating the incompletely updated list head ... oopsie!)

And I'd have made the __qe_ep_queue() routine take a qe_ep
and qe_req, but that's just a minor issue.


> ---
> 
> On Tue, Nov 18, 2008 at 02:13:30PM -0800, David Brownell wrote:
> > On Tuesday 18 November 2008, Anton Vorontsov wrote:
> > > +       spin_lock_irqsave(&udc->lock, flags);
> > > +       ret = __qe_ep_queue(_ep, _req, gfp_flags);
> > > +       spin_unlock_irqrestore(&udc->lock, flags);
> > 
> > Why are you passing "gfp_flags"?  Especially without
> > checking ... GFP_KERNEL will be illegal, for example.
> 
> Ugh, the gfp_flags aren't used at all. How about that
> patch?
> 
>  drivers/usb/gadget/fsl_qe_udc.c |   26 ++++++++++++++++++--------
>  1 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index 60b9279..3587970 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -1681,14 +1681,11 @@ static void qe_free_request(struct usb_ep *_ep, struct usb_request *_req)
>  		kfree(req);
>  }
>  
> -/* queues (submits) an I/O request to an endpoint */
> -static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
> -				gfp_t gfp_flags)
> +static int __qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req)
>  {
>  	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
>  	struct qe_req *req = container_of(_req, struct qe_req, req);
>  	struct qe_udc *udc;
> -	unsigned long flags;
>  	int reval;
>  
>  	udc = ep->udc;
> @@ -1732,7 +1729,7 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
>  	list_add_tail(&req->queue, &ep->queue);
>  	dev_vdbg(udc->dev, "gadget have request in %s! %d\n",
>  			ep->name, req->req.length);
> -	spin_lock_irqsave(&udc->lock, flags);
> +
>  	/* push the request to device */
>  	if (ep_is_in(ep))
>  		reval = ep_req_send(ep, req);
> @@ -1748,11 +1745,24 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
>  	if (ep->dir == USB_DIR_OUT)
>  		reval = ep_req_receive(ep, req);
>  
> -	spin_unlock_irqrestore(&udc->lock, flags);
> -
>  	return 0;
>  }
>  
> +/* queues (submits) an I/O request to an endpoint */
> +static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
> +		       gfp_t gfp_flags)
> +{
> +	struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
> +	struct qe_udc *udc = ep->udc;
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&udc->lock, flags);
> +	ret = __qe_ep_queue(_ep, _req);
> +	spin_unlock_irqrestore(&udc->lock, flags);
> +	return ret;
> +}
> +
>  /* dequeues (cancels, unlinks) an I/O request from an endpoint */
>  static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>  {
> @@ -2008,7 +2018,7 @@ static void ch9getstatus(struct qe_udc *udc, u8 request_type, u16 value,
>  	udc->ep0_dir = USB_DIR_IN;
>  
>  	/* data phase */
> -	status = qe_ep_queue(&ep->ep, &req->req, GFP_ATOMIC);
> +	status = __qe_ep_queue(&ep->ep, &req->req);
>  
>  	if (status == 0)
>  		return;
> -- 
> 1.5.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>