r8a66597-hcd panics when a USB device is removed from an attached

r8a66597-hcd panics when a USB device is removed from an attached

[Pietrek, Markus]

Hi folks,

when a USB hub is attached to the r8a66597-hcd on the SH7723 and a device (e.g. mass-storage) is removed from that hub, it's likely that a kernel panic follows.

The reason is that the r8a66597-hcd is being polled in r8a66597_hub_status_data() by core/hcd.c:usb_hcd_poll_rh_status() and will eventually call free_usb_address() when a device change has occurred. Yet the r8a66597 is unaware that the hub as already disconnected the device with core/hub.c:usb_disconnect() and freed udev, so when accessing udev it crashes.

If the mass-storage device is attached directly to the r8a66597-hcd, the driver will receive a device detached interrupt and call free_usb_address() itself and so remove it from the mapping.

The question now is, how can we do it for devices connected by a hub?

Unable to handle kernel paging request at virtual address 01000040
pc = 881cc056
*pde = 00000000
Oops: 0001 [#1]
last sysfs file: /sys/devices/platform/r8a66597_hcd.0/usb1/1-1/1-1.3/usb_device/usbdev1.3/dev
Modules linked in:

Pid : 0, Comm:          swapper
CPU : 0                 Not tainted  (2.6.33em0-06350-g3a5ea80-svn1025-dirty #629)

PC is at dev_set_drvdata+0x16/0x40
PR is at free_usb_address+0xae/0xe0
PC  : 881cc056 SP  : 8849fe4c SR  : 400081f0 TEA : 01000040
R0  : 0000000a R1  : 01000040 R2  : 8849e000 R3  : 88003e00
R4  : 8f5cf464 R5  : 00000000 R6  : ffffffff R7  : a4e30014
R8  : 8f5cf464 R9  : 00000000 R10 : 00000003 R11 : 8f5ba384
R12 : 00000008 R13 : 8f5ba0cc R14 : 00000001
MACH: 000081c9 MACL: 21642dbc GBR : 2975f450 PR  : 8822de4e

Call trace:
 [<8822de4e>] free_usb_address+0xae/0xe0
 [<8822e6b8>] r8a66597_hub_status_data+0x178/0x320
 [<88003e40>] __raw_local_save_flags+0x0/0x20
 [<88003e00>] raw_local_irq_restore+0x0/0x40
 [<8821d67c>] usb_hcd_poll_rh_status+0x3c/0x1c0
 [<8821dde0>] rh_timer_func+0x0/0x20
 [<88028dee>] run_timer_softirq+0xce/0x220
 [<8821dde0>] rh_timer_func+0x0/0x20
 [<88023d32>] __do_softirq+0x72/0x120
 [<88023e46>] do_softirq+0x66/0xa0
 [<883c41c0>] schedule+0x0/0x4c0

Bye,
Markus


_____________________________________

Amtsgericht Mannheim
HRB 110 300
Gesch?ftsf?hrer: Dieter Baur, Ramona Maurer
_____________________________________

Important Note:
- This e-mail may contain trade secrets or privileged, undisclosed or otherwise confidential information.
- If you have received this e-mail in error, you are hereby notified that any review, copying or distribution of it is strictly prohibited.
- Please inform us immediately and destroy the original transmittal.

Thank you for your cooperation.

Re: r8a66597-hcd panics when a USB device is removed from an attached hub

[Paul Mundt]

(Adding linux-usb to the Cc..)

On Fri, Mar 12, 2010 at 02:30:01PM +0100, Pietrek, Markus wrote:
> when a USB hub is attached to the r8a66597-hcd on the SH7723 and a
> device (e.g. mass-storage) is removed from that hub, it's likely that a
> kernel panic follows.
> 
> The reason is that the r8a66597-hcd is being polled in
> r8a66597_hub_status_data() by core/hcd.c:usb_hcd_poll_rh_status() and
> will eventually call free_usb_address() when a device change has
> occurred. Yet the r8a66597 is unaware that the hub as already
> disconnected the device with core/hub.c:usb_disconnect() and freed
> udev, so when accessing udev it crashes.
> 
> If the mass-storage device is attached directly to the r8a66597-hcd,
> the driver will receive a device detached interrupt and call
> free_usb_address() itself and so remove it from the mapping.
> 
> The question now is, how can we do it for devices connected by a hub?

Perhaps checking if dev->state = USB_STATE_NOTATTACHED and
short-circuiting the cleanup is simplest? We could obviously test
dev->udev before dev_set_drvdata(), but that's a bit ugly.

I'm not familiar enough with the USB subsystem to have any specific
suggestions with regards to what is and isn't acceptable though, or if
we're only in this mess because of some pre-existing layering violation.

> Unable to handle kernel paging request at virtual address 01000040
> pc = 881cc056
> *pde = 00000000
> Oops: 0001 [#1]
> last sysfs file: /sys/devices/platform/r8a66597_hcd.0/usb1/1-1/1-1.3/usb_device/usbdev1.3/dev
> Modules linked in:
> 
> Pid : 0, Comm:          swapper
> CPU : 0                 Not tainted  (2.6.33em0-06350-g3a5ea80-svn1025-dirty #629)
> 
> PC is at dev_set_drvdata+0x16/0x40
> PR is at free_usb_address+0xae/0xe0
> PC  : 881cc056 SP  : 8849fe4c SR  : 400081f0 TEA : 01000040
> R0  : 0000000a R1  : 01000040 R2  : 8849e000 R3  : 88003e00
> R4  : 8f5cf464 R5  : 00000000 R6  : ffffffff R7  : a4e30014
> R8  : 8f5cf464 R9  : 00000000 R10 : 00000003 R11 : 8f5ba384
> R12 : 00000008 R13 : 8f5ba0cc R14 : 00000001
> MACH: 000081c9 MACL: 21642dbc GBR : 2975f450 PR  : 8822de4e
> 
> Call trace:
>  [<8822de4e>] free_usb_address+0xae/0xe0
>  [<8822e6b8>] r8a66597_hub_status_data+0x178/0x320
>  [<88003e40>] __raw_local_save_flags+0x0/0x20
>  [<88003e00>] raw_local_irq_restore+0x0/0x40
>  [<8821d67c>] usb_hcd_poll_rh_status+0x3c/0x1c0
>  [<8821dde0>] rh_timer_func+0x0/0x20
>  [<88028dee>] run_timer_softirq+0xce/0x220
>  [<8821dde0>] rh_timer_func+0x0/0x20
>  [<88023d32>] __do_softirq+0x72/0x120
>  [<88023e46>] do_softirq+0x66/0xa0
>  [<883c41c0>] schedule+0x0/0x4c0

Re: r8a66597-hcd panics when a USB device is removed from an attached

[Alan Stern]

On Mon, 15 Mar 2010, Paul Mundt wrote:

> (Adding linux-usb to the Cc..)
> 
> On Fri, Mar 12, 2010 at 02:30:01PM +0100, Pietrek, Markus wrote:
> > when a USB hub is attached to the r8a66597-hcd on the SH7723 and a
> > device (e.g. mass-storage) is removed from that hub, it's likely that a
> > kernel panic follows.
> > 
> > The reason is that the r8a66597-hcd is being polled in
> > r8a66597_hub_status_data() by core/hcd.c:usb_hcd_poll_rh_status() and
> > will eventually call free_usb_address() when a device change has
> > occurred. Yet the r8a66597 is unaware that the hub as already
> > disconnected the device with core/hub.c:usb_disconnect() and freed
> > udev, so when accessing udev it crashes.
> > 
> > If the mass-storage device is attached directly to the r8a66597-hcd,
> > the driver will receive a device detached interrupt and call
> > free_usb_address() itself and so remove it from the mapping.
> > 
> > The question now is, how can we do it for devices connected by a hub?
> 
> Perhaps checking if dev->state = USB_STATE_NOTATTACHED and
> short-circuiting the cleanup is simplest? We could obviously test
> dev->udev before dev_set_drvdata(), but that's a bit ugly.
> 
> I'm not familiar enough with the USB subsystem to have any specific
> suggestions with regards to what is and isn't acceptable though, or if
> we're only in this mess because of some pre-existing layering violation.
> 
> > Unable to handle kernel paging request at virtual address 01000040
> > pc = 881cc056
> > *pde = 00000000
> > Oops: 0001 [#1]
> > last sysfs file: /sys/devices/platform/r8a66597_hcd.0/usb1/1-1/1-1.3/usb_device/usbdev1.3/dev
> > Modules linked in:
> > 
> > Pid : 0, Comm:          swapper
> > CPU : 0                 Not tainted  (2.6.33em0-06350-g3a5ea80-svn1025-dirty #629)
> > 
> > PC is at dev_set_drvdata+0x16/0x40
> > PR is at free_usb_address+0xae/0xe0
> > PC  : 881cc056 SP  : 8849fe4c SR  : 400081f0 TEA : 01000040
> > R0  : 0000000a R1  : 01000040 R2  : 8849e000 R3  : 88003e00
> > R4  : 8f5cf464 R5  : 00000000 R6  : ffffffff R7  : a4e30014
> > R8  : 8f5cf464 R9  : 00000000 R10 : 00000003 R11 : 8f5ba384
> > R12 : 00000008 R13 : 8f5ba0cc R14 : 00000001
> > MACH: 000081c9 MACL: 21642dbc GBR : 2975f450 PR  : 8822de4e
> > 
> > Call trace:
> >  [<8822de4e>] free_usb_address+0xae/0xe0
> >  [<8822e6b8>] r8a66597_hub_status_data+0x178/0x320
> >  [<88003e40>] __raw_local_save_flags+0x0/0x20
> >  [<88003e00>] raw_local_irq_restore+0x0/0x40
> >  [<8821d67c>] usb_hcd_poll_rh_status+0x3c/0x1c0
> >  [<8821dde0>] rh_timer_func+0x0/0x20
> >  [<88028dee>] run_timer_softirq+0xce/0x220
> >  [<8821dde0>] rh_timer_func+0x0/0x20
> >  [<88023d32>] __do_softirq+0x72/0x120
> >  [<88023e46>] do_softirq+0x66/0xa0
> >  [<883c41c0>] schedule+0x0/0x4c0

Is there some reason why you guys haven't tried asking the author of 
r8a66597-hcd (CC'ed)?  He's in a better position than anyone else to 
fix the problem.

Alan Stern

Re: r8a66597-hcd panics when a USB device is removed from an attached hub

[Paul Mundt]

On Mon, Mar 15, 2010 at 10:00:07AM -0400, Alan Stern wrote:
> Is there some reason why you guys haven't tried asking the author of 
> r8a66597-hcd (CC'ed)?  He's in a better position than anyone else to 
> fix the problem.
> 
He's on the list as well, just unresponsive as of late, which is why I
waited some time before replying and adding linux-usb to the Cc. 

Re: r8a66597-hcd panics when a USB device is removed from an attached

[Yoshihiro Shimoda]

Paul Mundt wrote:
> On Mon, Mar 15, 2010 at 10:00:07AM -0400, Alan Stern wrote:
>> Is there some reason why you guys haven't tried asking the author of 
>> r8a66597-hcd (CC'ed)?  He's in a better position than anyone else to 
>> fix the problem.
>>
> He's on the list as well, just unresponsive as of late, which is why I
> waited some time before replying and adding linux-usb to the Cc. 
> 
I'm sorry for my lazy response.
I check this issue, and I fixed it.


Hello Mr. Markus,

Thank you for your detail report!
I made a patch. If possible, would you test it on your environment?

Thanks,
Yoshihiro Shimoda
---

Subject: [PATCH] usb: r8a66597-hcd: fix removed from an attached hub

fix the problem that when a USB hub is attached to the r8a66597-hcd and
a device is removed from that hub, it's likely that a kernel panic follows.

Reported-by: Markus Pietrek <Markus.Pietrek@emtrion.de>
Signed-off-by: Yoshihiro Shimoda <shimoda.yoshihiro@renesas.com>
---
 drivers/usb/host/r8a66597-hcd.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index bee558a..f71a73a 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -418,7 +418,7 @@ static u8 alloc_usb_address(struct r8a66597 *r8a66597, struct urb *urb)

 /* this function must be called with interrupt disabled */
 static void free_usb_address(struct r8a66597 *r8a66597,
-			     struct r8a66597_device *dev)
+			     struct r8a66597_device *dev, int reset)
 {
 	int port;

@@ -430,7 +430,13 @@ static void free_usb_address(struct r8a66597 *r8a66597,
 	dev->state = USB_STATE_DEFAULT;
 	r8a66597->address_map &= ~(1 << dev->address);
 	dev->address = 0;
-	dev_set_drvdata(&dev->udev->dev, NULL);
+	/*
+	 * Only when resetting USB, it is necessary to erase drvdata. When
+	 * a usb device with usb hub is disconnect, "dev->udev" is already
+	 * freed on usb_desconnect(). So we cannot access the data.
+	 */
+	if (reset)
+		dev_set_drvdata(&dev->udev->dev, NULL);
 	list_del(&dev->device_list);
 	kfree(dev);

@@ -1069,7 +1075,7 @@ static void r8a66597_usb_disconnect(struct r8a66597 *r8a66597, int port)
 	struct r8a66597_device *dev = r8a66597->root_hub[port].dev;

 	disable_r8a66597_pipe_all(r8a66597, dev);
-	free_usb_address(r8a66597, dev);
+	free_usb_address(r8a66597, dev, 0);

 	start_root_hub_sampling(r8a66597, port, 0);
 }
@@ -2085,7 +2091,7 @@ static void update_usb_address_map(struct r8a66597 *r8a66597,
 				spin_lock_irqsave(&r8a66597->lock, flags);
 				dev = get_r8a66597_device(r8a66597, addr);
 				disable_r8a66597_pipe_all(r8a66597, dev);
-				free_usb_address(r8a66597, dev);
+				free_usb_address(r8a66597, dev, 0);
 				put_child_connect_map(r8a66597, addr);
 				spin_unlock_irqrestore(&r8a66597->lock, flags);
 			}
@@ -2228,7 +2234,7 @@ static int r8a66597_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			rh->port |= (1 << USB_PORT_FEAT_RESET);

 			disable_r8a66597_pipe_all(r8a66597, dev);
-			free_usb_address(r8a66597, dev);
+			free_usb_address(r8a66597, dev, 1);

 			r8a66597_mdfy(r8a66597, USBRST, USBRST | UACT,
 				      get_dvstctr_reg(port));
-- 
1.5.5

AW: r8a66597-hcd panics when a USB device is removed from an

[Pietrek, Markus]

Hello Mr. Yoshihiro

patch works, many thanks.

Mit freundlichen Grüßen / Best Regards

Markus Pietrek
Software engineer

--
emtrion GmbH
Greschbachstr. 12
76229 Karlsruhe
GERMANY

Phone  +49 721 62725-45
Fax    +49 721 62725-19
E-mail Markus.Pietrek@emtrion.de
http://www.emtrion.de



> -----Ursprüngliche Nachricht-----
> Von: shimoda.yoshihiro@renesas.com
> [mailto:shimoda.yoshihiro@renesas.com]
> Gesendet: Dienstag, 16. März 2010 04:30
> An: Paul Mundt; Pietrek, Markus
> Cc: Alan Stern; linux-sh@vger.kernel.org; USB list
> Betreff: Re: r8a66597-hcd panics when a USB device is removed from an
> attached hub
>
> Paul Mundt wrote:
> > On Mon, Mar 15, 2010 at 10:00:07AM -0400, Alan Stern wrote:
> >> Is there some reason why you guys haven't tried asking the author of
> >> r8a66597-hcd (CC'ed)?  He's in a better position than anyone else to
> >> fix the problem.
> >>
> > He's on the list as well, just unresponsive as of late, which is why
> I
> > waited some time before replying and adding linux-usb to the Cc.
> >
> I'm sorry for my lazy response.
> I check this issue, and I fixed it.
>
>
> Hello Mr. Markus,
>
> Thank you for your detail report!
> I made a patch. If possible, would you test it on your environment?
>
> Thanks,
> Yoshihiro Shimoda
> ---
>
> Subject: [PATCH] usb: r8a66597-hcd: fix removed from an attached hub
>
> fix the problem that when a USB hub is attached to the r8a66597-hcd and
> a device is removed from that hub, it's likely that a kernel panic
> follows.
>
> Reported-by: Markus Pietrek <Markus.Pietrek@emtrion.de>
> Signed-off-by: Yoshihiro Shimoda <shimoda.yoshihiro@renesas.com>
> ---
>  drivers/usb/host/r8a66597-hcd.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/r8a66597-hcd.c
> b/drivers/usb/host/r8a66597-hcd.c
> index bee558a..f71a73a 100644
> --- a/drivers/usb/host/r8a66597-hcd.c
> +++ b/drivers/usb/host/r8a66597-hcd.c
> @@ -418,7 +418,7 @@ static u8 alloc_usb_address(struct r8a66597
> *r8a66597, struct urb *urb)
>
>  /* this function must be called with interrupt disabled */
>  static void free_usb_address(struct r8a66597 *r8a66597,
> -                          struct r8a66597_device *dev)
> +                          struct r8a66597_device *dev, int reset)
>  {
>       int port;
>
> @@ -430,7 +430,13 @@ static void free_usb_address(struct r8a66597
> *r8a66597,
>       dev->state = USB_STATE_DEFAULT;
>       r8a66597->address_map &= ~(1 << dev->address);
>       dev->address = 0;
> -     dev_set_drvdata(&dev->udev->dev, NULL);
> +     /*
> +      * Only when resetting USB, it is necessary to erase drvdata.
> When
> +      * a usb device with usb hub is disconnect, "dev->udev" is
> already
> +      * freed on usb_desconnect(). So we cannot access the data.
> +      */
> +     if (reset)
> +             dev_set_drvdata(&dev->udev->dev, NULL);
>       list_del(&dev->device_list);
>       kfree(dev);
>
> @@ -1069,7 +1075,7 @@ static void r8a66597_usb_disconnect(struct
> r8a66597 *r8a66597, int port)
>       struct r8a66597_device *dev = r8a66597->root_hub[port].dev;
>
>       disable_r8a66597_pipe_all(r8a66597, dev);
> -     free_usb_address(r8a66597, dev);
> +     free_usb_address(r8a66597, dev, 0);
>
>       start_root_hub_sampling(r8a66597, port, 0);
>  }
> @@ -2085,7 +2091,7 @@ static void update_usb_address_map(struct
> r8a66597 *r8a66597,
>                               spin_lock_irqsave(&r8a66597->lock, flags);
>                               dev = get_r8a66597_device(r8a66597, addr);
>                               disable_r8a66597_pipe_all(r8a66597, dev);
> -                             free_usb_address(r8a66597, dev);
> +                             free_usb_address(r8a66597, dev, 0);
>                               put_child_connect_map(r8a66597, addr);
>                               spin_unlock_irqrestore(&r8a66597->lock, flags);
>                       }
> @@ -2228,7 +2234,7 @@ static int r8a66597_hub_control(struct usb_hcd
> *hcd, u16 typeReq, u16 wValue,
>                       rh->port |= (1 << USB_PORT_FEAT_RESET);
>
>                       disable_r8a66597_pipe_all(r8a66597, dev);
> -                     free_usb_address(r8a66597, dev);
> +                     free_usb_address(r8a66597, dev, 1);
>
>                       r8a66597_mdfy(r8a66597, USBRST, USBRST | UACT,
>                                     get_dvstctr_reg(port));
> --
> 1.5.5


_____________________________________

Amtsgericht Mannheim
HRB 110 300
Geschäftsführer: Dieter Baur, Ramona Maurer
_____________________________________

Important Note:
- This e-mail may contain trade secrets or privileged, undisclosed or otherwise confidential information.
- If you have received this e-mail in error, you are hereby notified that any review, copying or distribution of it is strictly prohibited.
- Please inform us immediately and destroy the original transmittal.

Thank you for your cooperation.

Re: AW: r8a66597-hcd panics when a USB device is removed from an

[Yoshihiro Shimoda]

Hello Mr. Markus,

Pietrek, Markus wrote:
> Hello Mr. Yoshihiro
> 
> patch works, many thanks.

Thank you very much for your testing!

Thanks,
Yoshihiro Shimoda