Patchwork [5/7] usb: gadget: fsl_udc: postpone freeing current dTD

login
register
mail settings
Submitter Christoph Fritz
Date Oct. 19, 2012, 10:24 a.m.
Message ID <1350642285-8145-5-git-send-email-chf.fritz@googlemail.com>
Download mbox | patch
Permalink /patch/192640/
State Not Applicable
Headers show

Comments

Christoph Fritz - Oct. 19, 2012, 10:24 a.m.
USB controller may access a wrong address for the dTD (endpoint transfer
descriptor) and then hang. This happens a lot when doing tests with
g_ether module and iperf, a tool for measuring maximum TCP and UDP
bandwidth.

This hardware bug is explained in detail by errata number 2858 for i.MX23:
http://cache.freescale.com/files/dsp/doc/errata/IMX23CE.pdf

All (?) SOCs with an IP from chipidea suffer from this problem.
mv_udc_core fixes this bug by commit daec765.  There still may be
unfixed drivers.

Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
Signed-off-by: Christian Hemp <c.hemp@phytec.de>
Reviewed-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/gadget/fsl_udc_core.c |   12 +++++++++++-
 drivers/usb/gadget/fsl_usb2_udc.h |    2 ++
 2 files changed, 13 insertions(+), 1 deletions(-)
Felipe Balbi - Oct. 19, 2012, 10:30 a.m.
On Fri, Oct 19, 2012 at 12:24:43PM +0200, Christoph Fritz wrote:
> USB controller may access a wrong address for the dTD (endpoint transfer
> descriptor) and then hang. This happens a lot when doing tests with
> g_ether module and iperf, a tool for measuring maximum TCP and UDP
> bandwidth.
> 
> This hardware bug is explained in detail by errata number 2858 for i.MX23:
> http://cache.freescale.com/files/dsp/doc/errata/IMX23CE.pdf
> 
> All (?) SOCs with an IP from chipidea suffer from this problem.
> mv_udc_core fixes this bug by commit daec765.  There still may be
> unfixed drivers.

why aren't you using that driver instead ? Is it really necessary to
keep this driver around ? I would really like to see uniformization
towards that, if you use the same IP, then the same driver ought to
suffice.

What's the reason for not using drivers/usb/chipidea ?
Felipe Balbi - Oct. 19, 2012, 10:44 a.m.
Hi,

On Fri, Oct 19, 2012 at 12:46:48PM +0200, Christoph Fritz wrote:
> On Fri, 2012-10-19 at 13:30 +0300, Felipe Balbi wrote:
> > On Fri, Oct 19, 2012 at 12:24:43PM +0200, Christoph Fritz wrote:
> > > USB controller may access a wrong address for the dTD (endpoint transfer
> > > descriptor) and then hang. This happens a lot when doing tests with
> > > g_ether module and iperf, a tool for measuring maximum TCP and UDP
> > > bandwidth.
> > > 
> > > This hardware bug is explained in detail by errata number 2858 for i.MX23:
> > > http://cache.freescale.com/files/dsp/doc/errata/IMX23CE.pdf
> > > 
> > > All (?) SOCs with an IP from chipidea suffer from this problem.
> > > mv_udc_core fixes this bug by commit daec765.  There still may be
> > > unfixed drivers.
> > 
> > why aren't you using that driver instead ? Is it really necessary to
> > keep this driver around ? I would really like to see uniformization
> > towards that, if you use the same IP, then the same driver ought to
> > suffice.
> > 
> > What's the reason for not using drivers/usb/chipidea ?
> > 
> 
> I thought about this too but wasn't able to use chipidea with
> MXC_EHCI_INTERNAL_PHY as it's called in fsl_udc.

that's a matter of writing the PHY driver, right ;-) It has nothing to
do with chipidea, actually :-)
Christoph Fritz - Oct. 19, 2012, 10:46 a.m.
On Fri, 2012-10-19 at 13:30 +0300, Felipe Balbi wrote:
> On Fri, Oct 19, 2012 at 12:24:43PM +0200, Christoph Fritz wrote:
> > USB controller may access a wrong address for the dTD (endpoint transfer
> > descriptor) and then hang. This happens a lot when doing tests with
> > g_ether module and iperf, a tool for measuring maximum TCP and UDP
> > bandwidth.
> > 
> > This hardware bug is explained in detail by errata number 2858 for i.MX23:
> > http://cache.freescale.com/files/dsp/doc/errata/IMX23CE.pdf
> > 
> > All (?) SOCs with an IP from chipidea suffer from this problem.
> > mv_udc_core fixes this bug by commit daec765.  There still may be
> > unfixed drivers.
> 
> why aren't you using that driver instead ? Is it really necessary to
> keep this driver around ? I would really like to see uniformization
> towards that, if you use the same IP, then the same driver ought to
> suffice.
> 
> What's the reason for not using drivers/usb/chipidea ?
> 

I thought about this too but wasn't able to use chipidea with
MXC_EHCI_INTERNAL_PHY as it's called in fsl_udc.

Sascha, do you know if i.mx35 works with usb/chipidea?

Thanks
 -- Christoph
Christoph Fritz - Oct. 20, 2012, 7:12 p.m.
On Fri, 2012-10-19 at 13:44 +0300, Felipe Balbi wrote:
> > I thought about this too but wasn't able to use chipidea with
> > MXC_EHCI_INTERNAL_PHY as it's called in fsl_udc.
> 
> that's a matter of writing the PHY driver, right ;-) It has nothing to
> do with chipidea, actually :-)

Okay, I'll do. But then we should purge the old buggy fsl_udc.

 Thanks
   -- Christoph
Felipe Balbi - Oct. 22, 2012, 7:54 a.m.
Hi,

On Sat, Oct 20, 2012 at 09:12:27PM +0200, Christoph Fritz wrote:
> On Fri, 2012-10-19 at 13:44 +0300, Felipe Balbi wrote:
> > > I thought about this too but wasn't able to use chipidea with
> > > MXC_EHCI_INTERNAL_PHY as it's called in fsl_udc.
> > 
> > that's a matter of writing the PHY driver, right ;-) It has nothing to
> > do with chipidea, actually :-)
> 
> Okay, I'll do. But then we should purge the old buggy fsl_udc.

I'm all for that. We shouldn't have multiple copies of a single driver
hanging around.

cheers

Patch

diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index 53df9c0..deaab62 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -88,8 +88,13 @@  static void done(struct fsl_ep *ep, struct fsl_req *req, int status)
 		curr_td = next_td;
 		if (j != req->dtd_count - 1) {
 			next_td = curr_td->next_td_virt;
+			dma_pool_free(udc->td_pool, curr_td, curr_td->td_dma);
+		} else {
+			if (udc->last_free_td != NULL)
+				dma_pool_free(udc->td_pool, udc->last_free_td,
+						udc->last_free_td->td_dma);
+			udc->last_free_td = curr_td;
 		}
-		dma_pool_free(udc->td_pool, curr_td, curr_td->td_dma);
 	}
 
 	if (req->mapped) {
@@ -2439,6 +2444,7 @@  static int __init fsl_udc_probe(struct platform_device *pdev)
 	udc->gadget.speed = USB_SPEED_UNKNOWN;
 	udc->gadget.name = driver_name;
 	udc->vbus_active = true;
+	udc->last_free_td = NULL;
 
 	/* Setup gadget.dev and register with kernel */
 	dev_set_name(&udc->gadget.dev, "gadget");
@@ -2533,6 +2539,10 @@  static int __exit fsl_udc_remove(struct platform_device *pdev)
 	kfree(udc->status_req);
 	kfree(udc->eps);
 
+	if (udc->last_free_td != NULL)
+		dma_pool_free(udc->td_pool, udc->last_free_td,
+			udc->last_free_td->td_dma);
+
 	dma_pool_destroy(udc->td_pool);
 	free_irq(udc->irq, udc);
 	iounmap(dr_regs);
diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
index f61a967..a0123ae 100644
--- a/drivers/usb/gadget/fsl_usb2_udc.h
+++ b/drivers/usb/gadget/fsl_usb2_udc.h
@@ -497,6 +497,8 @@  struct fsl_udc {
 	size_t ep_qh_size;		/* size after alignment adjustment*/
 	dma_addr_t ep_qh_dma;		/* dma address of QH */
 
+	struct ep_td_struct *last_free_td;
+
 	u32 max_pipes;          /* Device max pipes */
 	u32 bus_reset;		/* Device is bus resetting */
 	u32 resume_state;	/* USB state to resume */