Patchwork [v2] usb: fsl_udc: errata - postpone freeing current dTD

login
register
mail settings
Submitter Christoph Fritz
Date June 4, 2012, 11:30 a.m.
Message ID <1338809424.3371.20.camel@mars>
Download mbox | patch
Permalink /patch/162747/
State New
Headers show

Comments

Christoph Fritz - June 4, 2012, 11:30 a.m.
Hi,

On Mon, 2012-05-21 at 22:04 +0300, Felipe Balbi wrote:
> On Mon, May 21, 2012 at 08:57:22AM +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.
> > 
> > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
> > Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> > ---
> >  drivers/usb/gadget/fsl_udc_core.c |   15 ++++++++++++++-
> >  1 files changed, 14 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> > index 55abfb6..72f2139 100644
> > --- a/drivers/usb/gadget/fsl_udc_core.c
> > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > @@ -65,6 +65,8 @@ static struct usb_sys_interface *usb_sys_regs;
> >  /* it is initialized in probe()  */
> >  static struct fsl_udc *udc_controller = NULL;
> >  
> > +static struct ep_td_struct *last_free_td;
> 
> I don't want to see global variables anymore. In fact, please convert
> this to the new udc_start()/udc_stop() calls and use the generic
> map/unmap routines.
> 
> That'll help you get rid of a bunch of useless code on the driver. After
> that you should remove all <asm/*> header includes and drop the ARCH
> dependency.
> 
> You can also drop the big-/little-endian helpers as you can make use of
> generic writel()/readl() routines.
> 
> Please make sure these series comes in with enough time to reach v3.6
> merge window in about 3 months.
> 
> You can put this fix together on that series after you drop the global.

Before I came to do the proposed changes, I stumbled upon this:

In file included from drivers/usb/gadget/fsl_udc_core.c:49:
drivers/usb/gadget/fsl_usb2_udc.h: In function ‘get_qh_by_ep’:
drivers/usb/gadget/fsl_usb2_udc.h:585: error: ‘struct fsl_ep’ has no member named ‘desc’
drivers/usb/gadget/fsl_udc_core.c: In function ‘done’:
drivers/usb/gadget/fsl_udc_core.c:187: error: ‘struct fsl_ep’ has no member named ‘desc’
drivers/usb/gadget/fsl_udc_core.c:187: error: ‘struct fsl_ep’ has no member named ‘desc’
<snip>

my proposed regression patch:
---
From: Christoph Fritz <chf.fritz@googlemail.com>
Date: Mon, 4 Jun 2012 12:58:21 +0200
Subject: [PATCH] usb: gadget: regression fix - useage of usb_ep

This patch removes redundant pointer to struct usb_endpoint_descriptor which
were missed in commit 79149b8:

 usb: gadget: Update fsl_udc_core to use usb_endpoint_descriptor inside the
 struct usb_ep

Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
---
 drivers/usb/gadget/fsl_udc_core.c |    2 +-
 drivers/usb/gadget/fsl_usb2_udc.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
Christoph Fritz - June 4, 2012, 11:37 a.m.
On Mon, 2012-06-04 at 13:30 +0200, Christoph Fritz wrote:
> Hi,
> 
> On Mon, 2012-05-21 at 22:04 +0300, Felipe Balbi wrote:
> > On Mon, May 21, 2012 at 08:57:22AM +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.
> > > 
> > > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
> > > Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> > > ---
> > >  drivers/usb/gadget/fsl_udc_core.c |   15 ++++++++++++++-
> > >  1 files changed, 14 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> > > index 55abfb6..72f2139 100644
> > > --- a/drivers/usb/gadget/fsl_udc_core.c
> > > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > > @@ -65,6 +65,8 @@ static struct usb_sys_interface *usb_sys_regs;
> > >  /* it is initialized in probe()  */
> > >  static struct fsl_udc *udc_controller = NULL;
> > >  
> > > +static struct ep_td_struct *last_free_td;
> > 
> > I don't want to see global variables anymore. In fact, please convert
> > this to the new udc_start()/udc_stop() calls and use the generic
> > map/unmap routines.
> > 
> > That'll help you get rid of a bunch of useless code on the driver. After
> > that you should remove all <asm/*> header includes and drop the ARCH
> > dependency.
> > 
> > You can also drop the big-/little-endian helpers as you can make use of
> > generic writel()/readl() routines.
> > 
> > Please make sure these series comes in with enough time to reach v3.6
> > merge window in about 3 months.
> > 
> > You can put this fix together on that series after you drop the global.
> 
> Before I came to do the proposed changes, I stumbled upon this:
> 
> In file included from drivers/usb/gadget/fsl_udc_core.c:49:
> drivers/usb/gadget/fsl_usb2_udc.h: In function ‘get_qh_by_ep’:
> drivers/usb/gadget/fsl_usb2_udc.h:585: error: ‘struct fsl_ep’ has no member named ‘desc’
> drivers/usb/gadget/fsl_udc_core.c: In function ‘done’:
> drivers/usb/gadget/fsl_udc_core.c:187: error: ‘struct fsl_ep’ has no member named ‘desc’
> drivers/usb/gadget/fsl_udc_core.c:187: error: ‘struct fsl_ep’ has no member named ‘desc’
> <snip>
> 
> my proposed regression patch:
> ---
> From: Christoph Fritz <chf.fritz@googlemail.com>
> Date: Mon, 4 Jun 2012 12:58:21 +0200
> Subject: [PATCH] usb: gadget: regression fix - useage of usb_ep
<snip>

After that, I stumbled upon this dmesg:

Freescale High-Speed USB SOC Device Controller driver (Apr 20, 2007)
fsl-usb2-udc fsl-usb2-udc: clk_get("usb") failed
fsl-usb2-udc: probe of fsl-usb2-udc failed with error -2

Sascha, could you give me a hint?

 Thanks,
  -- Christoph
Felipe Balbi - June 4, 2012, 2:59 p.m.
On Mon, Jun 04, 2012 at 01:30:24PM +0200, Christoph Fritz wrote:
> Hi,
> 
> On Mon, 2012-05-21 at 22:04 +0300, Felipe Balbi wrote:
> > On Mon, May 21, 2012 at 08:57:22AM +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.
> > > 
> > > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
> > > Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> > > ---
> > >  drivers/usb/gadget/fsl_udc_core.c |   15 ++++++++++++++-
> > >  1 files changed, 14 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> > > index 55abfb6..72f2139 100644
> > > --- a/drivers/usb/gadget/fsl_udc_core.c
> > > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > > @@ -65,6 +65,8 @@ static struct usb_sys_interface *usb_sys_regs;
> > >  /* it is initialized in probe()  */
> > >  static struct fsl_udc *udc_controller = NULL;
> > >  
> > > +static struct ep_td_struct *last_free_td;
> > 
> > I don't want to see global variables anymore. In fact, please convert
> > this to the new udc_start()/udc_stop() calls and use the generic
> > map/unmap routines.
> > 
> > That'll help you get rid of a bunch of useless code on the driver. After
> > that you should remove all <asm/*> header includes and drop the ARCH
> > dependency.
> > 
> > You can also drop the big-/little-endian helpers as you can make use of
> > generic writel()/readl() routines.
> > 
> > Please make sure these series comes in with enough time to reach v3.6
> > merge window in about 3 months.
> > 
> > You can put this fix together on that series after you drop the global.
> 
> Before I came to do the proposed changes, I stumbled upon this:
> 
> In file included from drivers/usb/gadget/fsl_udc_core.c:49:
> drivers/usb/gadget/fsl_usb2_udc.h: In function ‘get_qh_by_ep’:
> drivers/usb/gadget/fsl_usb2_udc.h:585: error: ‘struct fsl_ep’ has no member named ‘desc’
> drivers/usb/gadget/fsl_udc_core.c: In function ‘done’:
> drivers/usb/gadget/fsl_udc_core.c:187: error: ‘struct fsl_ep’ has no member named ‘desc’
> drivers/usb/gadget/fsl_udc_core.c:187: error: ‘struct fsl_ep’ has no member named ‘desc’
> <snip>
> 
> my proposed regression patch:
> ---
> From: Christoph Fritz <chf.fritz@googlemail.com>
> Date: Mon, 4 Jun 2012 12:58:21 +0200
> Subject: [PATCH] usb: gadget: regression fix - useage of usb_ep
> 
> This patch removes redundant pointer to struct usb_endpoint_descriptor which
> were missed in commit 79149b8:
> 
>  usb: gadget: Update fsl_udc_core to use usb_endpoint_descriptor inside the
>  struct usb_ep
> 
> Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
> ---
>  drivers/usb/gadget/fsl_udc_core.c |    2 +-
>  drivers/usb/gadget/fsl_usb2_udc.h |    4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> index 2831685..678ec4d 100644
> --- a/drivers/usb/gadget/fsl_udc_core.c
> +++ b/drivers/usb/gadget/fsl_udc_core.c
> @@ -2575,7 +2575,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
>  	/* for ep0: the desc defined here;
>  	 * for other eps, gadget layer called ep_enable with defined desc
>  	 */
> -	udc_controller->eps[0].desc = &fsl_ep0_desc;
> +	udc_controller->eps[0].ep.desc = &fsl_ep0_desc;
>  	udc_controller->eps[0].ep.maxpacket = USB_MAX_CTRL_PAYLOAD;
>  
>  	/* setup the udc->eps[] for non-control endpoints and link
> diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
> index 5cd7b7e..f61a967 100644
> --- a/drivers/usb/gadget/fsl_usb2_udc.h
> +++ b/drivers/usb/gadget/fsl_usb2_udc.h
> @@ -568,10 +568,10 @@ static void dump_msg(const char *label, const u8 * buf, unsigned int length)
>  /*
>   * ### internal used help routines.
>   */
> -#define ep_index(EP)		((EP)->desc->bEndpointAddress&0xF)
> +#define ep_index(EP)		((EP)->ep.desc->bEndpointAddress&0xF)
>  #define ep_maxpacket(EP)	((EP)->ep.maxpacket)
>  #define ep_is_in(EP)	( (ep_index(EP) == 0) ? (EP->udc->ep0_dir == \
> -			USB_DIR_IN ):((EP)->desc->bEndpointAddress \
> +			USB_DIR_IN) : ((EP)->ep.desc->bEndpointAddress \
>  			& USB_DIR_IN)==USB_DIR_IN)
>  #define get_ep_by_pipe(udc, pipe)	((pipe == 1)? &udc->eps[0]: \
>  					&udc->eps[pipe])
> -- 
> 1.7.2.5

Please send as a proper patch so I can apply.
Christoph Fritz - June 12, 2012, 7:40 p.m.
Hi Fabio

On Sun, 2012-06-10 at 15:41 -0300, Fabio Estevam wrote:
> Hi Christoph,
> 
> On Mon, Jun 4, 2012 at 8:37 AM, Christoph Fritz
> <chf.fritz@googlemail.com> wrote:
> 
> > After that, I stumbled upon this dmesg:
> >
> > Freescale High-Speed USB SOC Device Controller driver (Apr 20, 2007)
> > fsl-usb2-udc fsl-usb2-udc: clk_get("usb") failed
> > fsl-usb2-udc: probe of fsl-usb2-udc failed with error -2
> >
> > Sascha, could you give me a hint?
> 
> Does the patch below fix your problem?

Thanks for your patch. It does indeed load
"Freescale High-Speed USB SOC Device Controller driver (Apr 20, 2007)"
fine - but now when I want to use it:

modprobe g_ether
[   17.099363] g_ether gadget: using random self ethernet address
[   17.105316] g_ether gadget: using random host ethernet address
[   17.111974] usb0: MAC 1a:c7:9e:76:cc:45
[   17.115866] usb0: HOST MAC 66:a2:4a:0a:46:17
[   17.120199] g_ether gadget: Ethernet Gadget, version: Memorial Day 2008
[   17.126861] g_ether gadget: g_ether ready

these are "the last words": System hangs completely. Same behavior with
g_serial (these two I've tested).

Currently I can't investigate this further, not because of a bug - but a
flu.


Thanks,
 -- Christoph
Fabio Estevam - June 13, 2012, 1:17 a.m.
On Tue, Jun 12, 2012 at 4:40 PM, Christoph Fritz
<chf.fritz@googlemail.com> wrote:

> Thanks for your patch. It does indeed load
> "Freescale High-Speed USB SOC Device Controller driver (Apr 20, 2007)"
> fine - but now when I want to use it:
>
> modprobe g_ether
> [   17.099363] g_ether gadget: using random self ethernet address
> [   17.105316] g_ether gadget: using random host ethernet address
> [   17.111974] usb0: MAC 1a:c7:9e:76:cc:45
> [   17.115866] usb0: HOST MAC 66:a2:4a:0a:46:17
> [   17.120199] g_ether gadget: Ethernet Gadget, version: Memorial Day 2008
> [   17.126861] g_ether gadget: g_ether ready
>
> these are "the last words": System hangs completely. Same behavior with
> g_serial (these two I've tested).

Ok, I just sent a new patch to the linux-usb mailing list.

Tested it on a mx31 and mx51 and it probed g_ether fine. I don't have
a mx35 handy.

>
> Currently I can't investigate this further, not because of a bug - but a
> flu.

Hope you get better soon.

Regards,

Fabio Estevam

Patch

diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index 2831685..678ec4d 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -2575,7 +2575,7 @@  static int __init fsl_udc_probe(struct platform_device *pdev)
 	/* for ep0: the desc defined here;
 	 * for other eps, gadget layer called ep_enable with defined desc
 	 */
-	udc_controller->eps[0].desc = &fsl_ep0_desc;
+	udc_controller->eps[0].ep.desc = &fsl_ep0_desc;
 	udc_controller->eps[0].ep.maxpacket = USB_MAX_CTRL_PAYLOAD;
 
 	/* setup the udc->eps[] for non-control endpoints and link
diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
index 5cd7b7e..f61a967 100644
--- a/drivers/usb/gadget/fsl_usb2_udc.h
+++ b/drivers/usb/gadget/fsl_usb2_udc.h
@@ -568,10 +568,10 @@  static void dump_msg(const char *label, const u8 * buf, unsigned int length)
 /*
  * ### internal used help routines.
  */
-#define ep_index(EP)		((EP)->desc->bEndpointAddress&0xF)
+#define ep_index(EP)		((EP)->ep.desc->bEndpointAddress&0xF)
 #define ep_maxpacket(EP)	((EP)->ep.maxpacket)
 #define ep_is_in(EP)	( (ep_index(EP) == 0) ? (EP->udc->ep0_dir == \
-			USB_DIR_IN ):((EP)->desc->bEndpointAddress \
+			USB_DIR_IN) : ((EP)->ep.desc->bEndpointAddress \
 			& USB_DIR_IN)==USB_DIR_IN)
 #define get_ep_by_pipe(udc, pipe)	((pipe == 1)? &udc->eps[0]: \
 					&udc->eps[pipe])