Bug 7482

Summary: usb ehci driver crash on system with more tahn 256MByte memory
Product: Drivers Reporter: Stefan Meyer (reyems)
Component: USBAssignee: Greg Kroah-Hartman (greg)
Status: RESOLVED CODE_FIX    
Severity: high    
Priority: P2    
Hardware: i386   
OS: Linux   
Kernel Version: 2.6.18.2 Subsystem:
Regression: --- Bisected commit-id:

Description Stefan Meyer 2006-11-09 22:00:36 UTC
Most recent kernel where this bug did not occur:2.6.18.2
Hardware Environment: Embedded MPC8347E-PowerPC with 512MByte DDRAM
Software Environment: Linux
Problem Description: USB EHCI-FSL-HOST driver would give inconsistent errors 
when system has more tahn 256MByte RAM installed 

Steps to reproduce: Install more than 256MByte memory
Applying the patch below fix the problem:
--- linux-2.6.18.2/drivers/usb/host/ehci-fsl.c	2006-11-04 03:33:58.000000000 
+0200
+++ linux-2.6.18.2-dash/drivers/usb/host/ehci-fsl.c	2006-11-07 
13:53:55.000000000 +0200
@@ -191,7 +191,7 @@
 	    platform_data;
 	/* Enable PHY interface in the control reg. */
 	out_be32(non_ehci + FSL_SOC_USB_CTRL, 0x00000004);
-	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001b);
+	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001C);
 
 	if (pdata->operating_mode == FSL_USB2_DR_HOST)
 		mpc83xx_setup_phy(ehci, pdata->phy_mode, 0);
Comment 1 Andrew Morton 2006-11-09 22:07:04 UTC
On Thu, 9 Nov 2006 22:01:21 -0800
bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=7482
> 
>            Summary: usb ehci driver crash on system with more tahn 256MByte
>                     memory
>     Kernel Version: 2.6.18.2
>             Status: NEW
>           Severity: high
>              Owner: greg@kroah.com
>          Submitter: reyems@telkomsa.net
> 
> 
> Most recent kernel where this bug did not occur:2.6.18.2

No, we're asking in which kernel did this *not* occur.

> Hardware Environment: Embedded MPC8347E-PowerPC with 512MByte DDRAM
> Software Environment: Linux
> Problem Description: USB EHCI-FSL-HOST driver would give inconsistent errors 
> when system has more tahn 256MByte RAM installed 
> 
> Steps to reproduce: Install more than 256MByte memory
> Applying the patch below fix the problem:
> --- linux-2.6.18.2/drivers/usb/host/ehci-fsl.c	2006-11-04 03:33:58.000000000 
> +0200
> +++ linux-2.6.18.2-dash/drivers/usb/host/ehci-fsl.c	2006-11-07 
> 13:53:55.000000000 +0200
> @@ -191,7 +191,7 @@
>  	    platform_data;
>  	/* Enable PHY interface in the control reg. */
>  	out_be32(non_ehci + FSL_SOC_USB_CTRL, 0x00000004);
> -	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001b);
> +	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001C);
>  
>  	if (pdata->operating_mode == FSL_USB2_DR_HOST)
>  		mpc83xx_setup_phy(ehci, pdata->phy_mode, 0);
> 

Well it looks simple. What does that do?

Comment 2 Anonymous Emailer 2006-11-09 22:36:43 UTC
Reply-To: david-b@pacbell.net

On Thursday 09 November 2006 10:07 pm, Andrew Morton wrote:

> > @@ -191,7 +191,7 @@
> >  	    platform_data;
> >  	/* Enable PHY interface in the control reg. */
> >  	out_be32(non_ehci + FSL_SOC_USB_CTRL, 0x00000004);
> > -	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001b);
> > +	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001C);
> >  
> >  	if (pdata->operating_mode == FSL_USB2_DR_HOST)
> >  		mpc83xx_setup_phy(ehci, pdata->phy_mode, 0);
> > 
> 
> Well it looks simple. What does that do?

According to the comment it affects the PHY interface.

Now, how a PHY could care about DMA ...

If that 0x1c is related to physical memory size, it
probably shouldn't be hard-wired like that.

Comment 3 Andrew Morton 2006-11-09 23:34:45 UTC
On Thu, 9 Nov 2006 22:37:17 -0800
David Brownell <david-b@pacbell.net> wrote:

> On Thursday 09 November 2006 10:07 pm, Andrew Morton wrote:
> 
> > > @@ -191,7 +191,7 @@
> > >  	    platform_data;
> > >  	/* Enable PHY interface in the control reg. */
> > >  	out_be32(non_ehci + FSL_SOC_USB_CTRL, 0x00000004);
> > > -	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001b);
> > > +	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001C);
> > >  
> > >  	if (pdata->operating_mode == FSL_USB2_DR_HOST)
> > >  		mpc83xx_setup_phy(ehci, pdata->phy_mode, 0);
> > > 
> > 
> > Well it looks simple. What does that do?
> 
> According to the comment it affects the PHY interface.
> 
> Now, how a PHY could care about DMA ...
> 
> If that 0x1c is related to physical memory size, it
> probably shouldn't be hard-wired like that.

Stefan replied but he chopped off all the Cc's :(

On Fri, 10 Nov 2006 08:39:54 +0200
"Stefan Meyer" <reyems@telkomsa.net> wrote:

> Hi Andrew
> I am not much of an expert, but according to the datasheets the Snoop1 & 
> Snoop2 registers create a coherent DMA window (I presume for the use of USB 
> related DMA).
> The current setting of  0x0000001b creates a window starting at 0x00000000 
> with a length of 256MByte.
> Changing it to 0x0000001C creates a 512MByte windows (this was of course a 
> non-generic quick fix).
> Running with anything up to 256MByte is fine.
> My system has 512MByte and it took quite a bit of chasing to find this 
> issue.

Comment 4 Stefan Meyer 2006-11-09 23:53:53 UTC
Hi Andrew
I am not much of an expert, but according to the datasheets the Snoop1 &
Snoop2 registers create a coherent DMA window (I presume for the use of USB
related DMA).
The current setting of  0x0000001b creates a window starting at 0x00000000
with a length of 256MByte.
Changing it to 0x0000001C creates a 512MByte windows (this was of course a
non-generic quick fix).
Running with anything up to 256MByte is fine.
My system has 512MByte and it took quite a bit of chasing to find this
issue.

Hope that helps

Regards
Stefan Meyer

----- Original Message ----- 
From: "Andrew Morton" <akpm@osdl.org>
To: <linux-usb-devel@lists.sourceforge.net>; "Alan Stern" 
<stern@rowland.harvard.edu>; "David Brownell" <david-b@pacbell.net>; "Greg 
KH" <greg@kroah.com>
Cc: "bugme-daemon@kernel-bugs.osdl.org" <bugme-daemon@bugzilla.kernel.org>; 
<reyems@telkomsa.net>
Sent: Friday, November 10, 2006 8:07 AM
Subject: Re: [Bugme-new] [Bug 7482] New: usb ehci driver crash on system 
with more tahn 256MByte memory


> On Thu, 9 Nov 2006 22:01:21 -0800
> bugme-daemon@bugzilla.kernel.org wrote:
>
>> http://bugzilla.kernel.org/show_bug.cgi?id=7482
>>
>>            Summary: usb ehci driver crash on system with more tahn 
>> 256MByte
>>                     memory
>>     Kernel Version: 2.6.18.2
>>             Status: NEW
>>           Severity: high
>>              Owner: greg@kroah.com
>>          Submitter: reyems@telkomsa.net
>>
>>
>> Most recent kernel where this bug did not occur:2.6.18.2
>
> No, we're asking in which kernel did this *not* occur.
>
>> Hardware Environment: Embedded MPC8347E-PowerPC with 512MByte DDRAM
>> Software Environment: Linux
>> Problem Description: USB EHCI-FSL-HOST driver would give inconsistent 
>> errors
>> when system has more tahn 256MByte RAM installed
>>
>> Steps to reproduce: Install more than 256MByte memory
>> Applying the patch below fix the problem:
>> --- linux-2.6.18.2/drivers/usb/host/ehci-fsl.c 2006-11-04 
>> 03:33:58.000000000
>> +0200
>> +++ linux-2.6.18.2-dash/drivers/usb/host/ehci-fsl.c 2006-11-07
>> 13:53:55.000000000 +0200
>> @@ -191,7 +191,7 @@
>>      platform_data;
>>  /* Enable PHY interface in the control reg. */
>>  out_be32(non_ehci + FSL_SOC_USB_CTRL, 0x00000004);
>> - out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001b);
>> + out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001C);
>>
>>  if (pdata->operating_mode == FSL_USB2_DR_HOST)
>>  mpc83xx_setup_phy(ehci, pdata->phy_mode, 0);
>>
>
> Well it looks simple. What does that do?
>
>
> 


Comment 5 Anonymous Emailer 2006-11-10 00:47:31 UTC
Reply-To: david-b@pacbell.net

On Thursday 09 November 2006 11:56 pm, Stefan Meyer wrote:

> Changing it to 0x0000001C creates a 512MByte windows (this was of course a
> non-generic quick fix).
> Running with anything up to 256MByte is fine.
> My system has 512MByte and it took quite a bit of chasing to find this
> issue.

A better patch would (a) fix the comment which says both lines of
code affects the PHY, (b) not hard-wire the amount of memory on
the system.

Assuming it's possible to put non-cacheable memory into that chunk
of address space (memory mapped peripherals etc) then it's dangerous
to hard-wire _any_ value.

- Dave

Comment 6 Anonymous Emailer 2007-05-18 03:52:40 UTC
Reply-To: david-b@pacbell.net

On Friday 18 May 2007, Andrew Morton wrote:
> On Fri, 10 Nov 2006 00:47:59 -0800 David Brownell <david-b@pacbell.net> wrote:
> 
> > On Thursday 09 November 2006 11:56 pm, Stefan Meyer wrote:
> > 
> > > Changing it to 0x0000001C creates a 512MByte windows (this was of course a
> > > non-generic quick fix).
> > > Running with anything up to 256MByte is fine.
> > > My system has 512MByte and it took quite a bit of chasing to find this
> > > issue.
> > 
> > A better patch would (a) fix the comment which says both lines of
> > code affects the PHY, (b) not hard-wire the amount of memory on
> > the system.
> > 
> > Assuming it's possible to put non-cacheable memory into that chunk
> > of address space (memory mapped peripherals etc) then it's dangerous
> > to hard-wire _any_ value.
> > 
> 
> So, did this ever get fixed? (Full history at
> http://bugzilla.kernel.org/show_bug.cgi?id=7482)

Not from what I can tell.  I cc'd the Freescale person who seems
to be maintaining EHCI on that platform ...


> 
> Thanks.
> 


Comment 7 Anonymous Emailer 2007-05-18 04:09:53 UTC
Reply-To: LeoLi@freescale.com

> -----Original Message-----
> From: David Brownell [mailto:david-b@pacbell.net]
> Sent: Friday, May 18, 2007 6:49 PM
> To: Andrew Morton
> Cc: Stefan Meyer; linux-usb-devel@lists.sourceforge.net; Alan Stern;
Greg KH;
> bugme-daemon@kernel-bugs.osdl.org; Natalie Protasevich; Li Yang-r58472
> Subject: Re: [Bulk] Re: [Bugme-new] [Bug 7482] New: usb ehci driver
crash on system
> with more tahn 256MByte memory
> 
> On Friday 18 May 2007, Andrew Morton wrote:
> > On Fri, 10 Nov 2006 00:47:59 -0800 David Brownell
<david-b@pacbell.net> wrote:
> >
> > > On Thursday 09 November 2006 11:56 pm, Stefan Meyer wrote:
> > >
> > > > Changing it to 0x0000001C creates a 512MByte windows (this was
of course a
> > > > non-generic quick fix).
> > > > Running with anything up to 256MByte is fine.
> > > > My system has 512MByte and it took quite a bit of chasing to
find this
> > > > issue.
> > >
> > > A better patch would (a) fix the comment which says both lines of
> > > code affects the PHY, (b) not hard-wire the amount of memory on
> > > the system.
> > >
> > > Assuming it's possible to put non-cacheable memory into that chunk
> > > of address space (memory mapped peripherals etc) then it's
dangerous
> > > to hard-wire _any_ value.
> > >
> >
> > So, did this ever get fixed? (Full history at
> > http://bugzilla.kernel.org/show_bug.cgi?id=7482)
> 
> Not from what I can tell.  I cc'd the Freescale person who seems
> to be maintaining EHCI on that platform ...

I see.  The complete fix that I expect is to set snooping on the whole
4G memory space as fsl_usb2_udc did.  Also, we need to take care of the
case that the Soc used on architectures other than powerpc which don't
have snooping mechanism.

Shall I provide a patch to fix this?

- Leo

Comment 8 Anonymous Emailer 2007-05-18 04:36:40 UTC
Reply-To: david-b@pacbell.net

On Friday 18 May 2007, Li Yang-r58472 wrote:

> > > So, did this ever get fixed? (Full history at
> > > http://bugzilla.kernel.org/show_bug.cgi?id=7482)
> > 
> > Not from what I can tell.  I cc'd the Freescale person who seems
> > to be maintaining EHCI on that platform ...
> 
> I see.  The complete fix that I expect is to set snooping on the whole
> 4G memory space as fsl_usb2_udc did.  Also, we need to take care of the
> case that the Soc

Well, that "IP core"...

> used on architectures other than powerpc which don't 
> have snooping mechanism.
> 
> Shall I provide a patch to fix this?

Yes, please.

- Dave

Comment 9 Anonymous Emailer 2007-05-21 20:27:11 UTC
Reply-To: leoli@freescale.com

Here is the patch to fix it.


From: Li Yang <LeoLi@freescale.com>
Date: Mon, 21 May 2007 13:08:57 +0800
Subject: [PATCH] ehci-fsl: fix cache coherency problem on system with large memory

The patch fix bug http://bugzilla.kernel.org/show_bug.cgi?id=7482.
It sets USB snooping on 4G space for PowerPC platforms without
CONFIG_NOT_COHERENT_CACHE defined.

Reported-by: Stefan Meyer <reyems@telkomsa.net>
Signed-off-by: Li Yang <leoli@freescale.com>
---
 drivers/usb/host/ehci-fsl.c |   11 ++++++++++-
 drivers/usb/host/ehci-fsl.h |    1 +
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index e802a13..369ea94 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -194,7 +194,16 @@ static void mpc83xx_usb_setup(struct usb_hcd *hcd)
 	/* Enable PHY interface in the control reg. */
 	temp = in_be32(non_ehci + FSL_SOC_USB_CTRL);
 	out_be32(non_ehci + FSL_SOC_USB_CTRL, temp | 0x00000004);
-	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0000001b);
+#if defined(CONFIG_PPC32) && !defined(CONFIG_NOT_COHERENT_CACHE)
+	/* Turn on cache snooping hardware, since some PowerPC platforms
+	 * wholly rely on hardware to deal with cache coherent. */
+
+	/* Setup Snooping for all the 4GB space */
+	/* SNOOP1 starts from 0x0, size 2G */
+	out_be32(non_ehci + FSL_SOC_USB_SNOOP1, 0x0 | SNOOP_SIZE_2GB);
+	/* SNOOP2 starts from 0x80000000, size 2G */
+	out_be32(non_ehci + FSL_SOC_USB_SNOOP2, 0x80000000 | SNOOP_SIZE_2GB);
+#endif
 
 	if ((pdata->operating_mode == FSL_USB2_DR_HOST) ||
 			(pdata->operating_mode == FSL_USB2_DR_OTG))
diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
index f28736a..b5e59db 100644
--- a/drivers/usb/host/ehci-fsl.h
+++ b/drivers/usb/host/ehci-fsl.h
@@ -34,4 +34,5 @@
 #define FSL_SOC_USB_PRICTRL	0x40c	/* NOTE: big-endian */
 #define FSL_SOC_USB_SICTRL	0x410	/* NOTE: big-endian */
 #define FSL_SOC_USB_CTRL	0x500	/* NOTE: big-endian */
+#define SNOOP_SIZE_2GB		0x1e
 #endif				/* _EHCI_FSL_H */


Comment 10 Andrew Morton 2007-05-22 09:52:51 UTC
Li Yang <leoli@freescale.com> prepared a patch for this, which is in -mm.