Skip to contentSkip to conversation

USB: Fixes for fsl_qe_udc driver

PATCH In review Anton Vorontsov 3 participants 14 messages Dec 25, 2008 – Feb 4, 2009 USB

lore
Share & tools
lore
#0 lore raw

cover USB: Fixes for fsl_qe_udc driver

Hi all,

Just resending some fixes that seem to be lost...

Signature · 3 lines
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
#1 lore raw

PATCH 1/6 USB: fsl_qe_udc: Fix oops on QE UDC probe failure

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.

---
 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 d15be3d..db6fe04 100644
@@ -2604,6 +2604,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);
@@ -2625,6 +2629,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,
@@ -2648,7 +2654,7 @@ err2:
 	iounmap(udc_controller->usb_regs);
 err1:
 	kfree(udc_controller);
-
+	udc_controller = NULL;
 	return ret;
 }
 
@@ -2710,6 +2716,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 · 1 line
1.5.6.5
#2 lore raw

PATCH 2/6 USB: fsl_qe_udc: Fix recursive locking bug in ch9getstatus()

In reply to 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().

---
 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 db6fe04..2677c9e 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 · 1 line
1.5.6.5
#3 lore raw

PATCH 3/6 USB: fsl_qe_udc: Fix QE USB controller initialization

In reply to 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.

---
 drivers/usb/gadget/fsl_qe_udc.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
drivers/usb/gadget/fsl_qe_udc.c +5 -1
index 2677c9e..b460c6d 100644
@@ -2452,8 +2452,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);
Signature · 1 line
1.5.6.5
#4 lore raw

PATCH 4/6 USB: fsl_qe_udc: Fix disconnects reporting during bus reset

In reply to 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.

---
 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 b460c6d..4726582 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 · 1 line
1.5.6.5
#5 lore raw

PATCH 5/6 USB: fsl_qe_udc: Fix muram corruption by disabled endpoints

In reply to 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.

---
 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 4726582..ea42088 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 · 1 line
1.5.6.5
#6 lore raw

PATCH 6/6 USB: fsl_qe_udc: Fix stalled TX requests bug

In reply to 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 patch 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).

---
 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 ea42088..69ccbeb 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 · 1 line
1.5.6.5
#8 lore raw
Quoted 10 lines · Kumar Gala
Quoted 3 lines · Anton Vorontsov

Hi all,

Just resending some fixes that seem to be lost...

Greg,

What happened w/this patch set?

As it was sent during the hollidays (on Christmas day at that), it's still in my "to-review" queue. I'll get to it in a few days after I resync the USB tree with Linus.

thanks,

greg k-h

#9 lore raw
Quoted 8 lines · Greg KH
Quoted 7 lines · Kumar Gala
Quoted 3 lines · Anton Vorontsov

Hi all,

Just resending some fixes that seem to be lost...

Quoted 7 lines
Quoted 3 lines

Greg,

What happened w/this patch set?

As it was sent during the hollidays (on Christmas day at that), it's still in my "to-review" queue. I'll get to it in a few days after I resync the USB tree with Linus.

Any update on the review of these 6 patches?

- k

#10 lore raw
Quoted 13 lines · Kumar Gala
Quoted 10 lines · Greg KH
Quoted 9 lines · Kumar Gala
Quoted 3 lines · Anton Vorontsov

Hi all,

Just resending some fixes that seem to be lost...

Greg,

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

What happened w/this patch set?

As it was sent during the hollidays (on Christmas day at that), it's still in my "to-review" queue. I'll get to it in a few days after I resync the USB tree with Linus.

Any update on the review of these 6 patches?

It's in my queue, been busy with "real work" for a bit right now, sorry...

thanks,

greg k-h

#11 lore raw
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Quoted 1 line

Just resending some fixes that seem to be lost...

Greg,

What happened w/this patch set?

As it was sent during the hollidays (on Christmas day at that), it's still in my "to-review" queue. I'll get to it in a few days after I resync the USB tree with Linus.

Quoted 8 lines
Quoted 1 line

Any update on the review of these 6 patches?

It's in my queue, been busy with "real work" for a bit right now, sorry...

thanks,

greg k-h

np. Just going through my patchworks queue and hadn't seen these show up in any tree.

- k

#12 lore raw
In reply to Greg KH
Quoted 12 lines · Greg KH
Quoted 11 lines · Kumar Gala
Quoted 10 lines · Greg KH
Quoted 8 lines · Kumar Gala
Quoted 3 lines · Anton Vorontsov

Hi all,

Just resending some fixes that seem to be lost...

Greg,

What happened w/this patch set?

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

As it was sent during the hollidays (on Christmas day at that), it's still in my "to-review" queue. I'll get to it in a few days after I resync the USB tree with Linus.

Any update on the review of these 6 patches?

It's in my queue, been busy with "real work" for a bit right now, sorry...

Hello Greg,

Can you please look into these patches? I think they all are 2.6.29 material, since the QE UDC driver is barely usable w/o these fixes.

Thanks,

p.s. And thank you for merging the FHCI driver!

Signature · 3 lines
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
#13 lore raw
Quoted 19 lines · Anton Vorontsov
Quoted 18 lines · Greg KH
Quoted 16 lines · Kumar Gala
Quoted 6 lines · Greg KH
Quoted 5 lines · Kumar Gala
Quoted 3 lines · Anton Vorontsov

Hi all,

Just resending some fixes that seem to be lost...

Quoted 7 lines
Quoted 3 lines

Greg,

What happened w/this patch set?

As it was sent during the hollidays (on Christmas day at that), it's still in my "to-review" queue. I'll get to it in a few days after I resync the USB tree with Linus.

Any update on the review of these 6 patches?

Quoted 7 lines
Quoted 2 lines

It's in my queue, been busy with "real work" for a bit right now, sorry...

Hello Greg,

Can you please look into these patches? I think they all are 2.6.29 material, since the QE UDC driver is barely usable w/o these fixes.

My appologies, I've now applied them all and will queue them up to Linus before .29 is out.

thanks,

greg k-h