More QE UDC fixes

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 <[email protected]>

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

drivers/usb/gadget/fsl_qe_udc.c +8 -1

index 94c38e4..60b9279 100644
@@ -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);
 

Signature · 6 lines

1.5.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-       spin_lock_irqsave(&udc->lock, flags);
+       if (lock)
+               spin_lock_irqsave(lock, flags);

Quoted 3 lines · Anton Vorontsov

-       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.)

+       spin_lock_irqsave(&udc->lock, flags); +       ret = __qe_ep_queue(_ep, _req, gfp_flags); +       spin_unlock_irqrestore(&udc->lock, flags);

Quoted 3 lines · Anton Vorontsov

+       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.

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 <[email protected]>

---

Quoted 7 lines · David Brownell

Quoted 3 lines · Anton Vorontsov

+       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(-)

drivers/usb/gadget/fsl_qe_udc.c +18 -8

index 60b9279..3587970 100644
@@ -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;

Signature · 6 lines

1.5.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

---
 drivers/usb/gadget/fsl_qe_udc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

drivers/usb/gadget/fsl_qe_udc.c +3 -0

index 1789f65..7bb5f36 100644
@@ -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);
 

Signature · 6 lines

1.5.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

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

drivers/usb/gadget/fsl_qe_udc.c +1 -0

index 7bb5f36..cb47337 100644
@@ -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));

Signature · 6 lines

1.5.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

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

drivers/usb/gadget/fsl_qe_udc.c +1 -0

index cb47337..37c8575 100644
@@ -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);
 

Signature · 5 lines

1.5.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html