Patchwork [U-Boot] eth: remove usb-ethernet devices before re-enumerating them

login
register
mail settings
Submitter Vincent Palatin
Date Jan. 9, 2012, 6:32 p.m.
Message ID <1326133956-25941-1-git-send-email-vpalatin@chromium.org>
Download mbox | patch
Permalink /patch/135089/
State Accepted
Commit e7e982d69c43b89f7e19b63479c8b2e880fbd75c
Delegated to: Marek Vasut
Headers show

Comments

Vincent Palatin - Jan. 9, 2012, 6:32 p.m.
Fix the crash when running several times usb_init() with a USB ethernet
device plugged.

Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
Tested-by: Wolfgang Grandegger <wg@denx.de>
---
 drivers/usb/eth/usb_ether.c |    7 +++++--
 include/net.h               |    1 +
 net/eth.c                   |   29 +++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)
Marek Vasut - Jan. 9, 2012, 8:57 p.m.
> Fix the crash when running several times usb_init() with a USB ethernet
> device plugged.
> 
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Tested-by: Wolfgang Grandegger <wg@denx.de>
> ---
>  drivers/usb/eth/usb_ether.c |    7 +++++--
>  include/net.h               |    1 +
>  net/eth.c                   |   29 +++++++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
> index 6565ea5..73a0790 100644
> --- a/drivers/usb/eth/usb_ether.c
> +++ b/drivers/usb/eth/usb_ether.c
> @@ -127,8 +127,11 @@ int usb_host_eth_scan(int mode)
> 
>  	old_async = usb_disable_asynch(1); /* asynch transfer not allowed */
> 
> -	for (i = 0; i < USB_MAX_ETH_DEV; i++)
> -		memset(&usb_eth[i], 0, sizeof(usb_eth[i]));
> +	/* unregister a previously detected device */
> +	for (i = 0; i < usb_max_eth_dev; i++)
> +		eth_unregister(&usb_eth[i].eth_dev);
> +
> +	memset(usb_eth, 0, sizeof(usb_eth));
> 
>  	for (i = 0; prob_dev[i].probe; i++) {
>  		if (prob_dev[i].before_probe)
> diff --git a/include/net.h b/include/net.h
> index e4d42c2..1707a7f 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -96,6 +96,7 @@ struct eth_device {
> 
>  extern int eth_initialize(bd_t *bis);	/* Initialize network subsystem 
*/
>  extern int eth_register(struct eth_device* dev);/* Register network device
> */ +extern int eth_unregister(struct eth_device *dev);/* Remove network
> device */ extern void eth_try_another(int first_restart);	/* Change the
> device */ extern void eth_set_current(void);		/* set nterface to 
ethcur
> var */ extern struct eth_device *eth_get_dev(void);	/* get the current
> device MAC */ diff --git a/net/eth.c b/net/eth.c
> index b4b9b43..3fb5fb6 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -224,6 +224,35 @@ int eth_register(struct eth_device *dev)
>  	return 0;
>  }
> 
> +int eth_unregister(struct eth_device *dev)
> +{
> +	struct eth_device *cur;
> +
> +	/* No device */
> +	if (!eth_devices)
> +		return -1;
> +
> +	for (cur = eth_devices; cur->next != eth_devices && cur->next != dev;
> +	     cur = cur->next)
> +		;
> +
> +	/* Device not found */
> +	if (cur->next != dev)
> +		return -1;
> +
> +	cur->next = dev->next;
> +
> +	if (eth_devices == dev)
> +		eth_devices = dev->next == eth_devices ? NULL : dev->next;
> +
> +	if (eth_current == dev) {
> +		eth_current = eth_devices;
> +		eth_current_changed();
> +	}
> +
> +	return 0;
> +}
> +
>  int eth_initialize(bd_t *bis)
>  {
>  	int num_devices = 0;

Looks sane. On what hardware did this happen (just for the record)?

M
Vincent Palatin - Jan. 9, 2012, 9:08 p.m.
On Mon, Jan 9, 2012 at 12:57, Marek Vasut <marek.vasut@gmail.com> wrote:
>> Fix the crash when running several times usb_init() with a USB ethernet
>> device plugged.
>>
>> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
>> Tested-by: Wolfgang Grandegger <wg@denx.de>
>> ---
>>  drivers/usb/eth/usb_ether.c |    7 +++++--
>>  include/net.h               |    1 +
>>  net/eth.c                   |   29 +++++++++++++++++++++++++++++
>>  3 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
>> index 6565ea5..73a0790 100644
>> --- a/drivers/usb/eth/usb_ether.c
>> +++ b/drivers/usb/eth/usb_ether.c
>> @@ -127,8 +127,11 @@ int usb_host_eth_scan(int mode)
>>
>>       old_async = usb_disable_asynch(1); /* asynch transfer not allowed */
>>
>> -     for (i = 0; i < USB_MAX_ETH_DEV; i++)
>> -             memset(&usb_eth[i], 0, sizeof(usb_eth[i]));
>> +     /* unregister a previously detected device */
>> +     for (i = 0; i < usb_max_eth_dev; i++)
>> +             eth_unregister(&usb_eth[i].eth_dev);
>> +
>> +     memset(usb_eth, 0, sizeof(usb_eth));
>>
>>       for (i = 0; prob_dev[i].probe; i++) {
>>               if (prob_dev[i].before_probe)
>> diff --git a/include/net.h b/include/net.h
>> index e4d42c2..1707a7f 100644
>> --- a/include/net.h
>> +++ b/include/net.h
>> @@ -96,6 +96,7 @@ struct eth_device {
>>
>>  extern int eth_initialize(bd_t *bis);        /* Initialize network subsystem
> */
>>  extern int eth_register(struct eth_device* dev);/* Register network device
>> */ +extern int eth_unregister(struct eth_device *dev);/* Remove network
>> device */ extern void eth_try_another(int first_restart);     /* Change the
>> device */ extern void eth_set_current(void);          /* set nterface to
> ethcur
>> var */ extern struct eth_device *eth_get_dev(void);   /* get the current
>> device MAC */ diff --git a/net/eth.c b/net/eth.c
>> index b4b9b43..3fb5fb6 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -224,6 +224,35 @@ int eth_register(struct eth_device *dev)
>>       return 0;
>>  }
>>
>> +int eth_unregister(struct eth_device *dev)
>> +{
>> +     struct eth_device *cur;
>> +
>> +     /* No device */
>> +     if (!eth_devices)
>> +             return -1;
>> +
>> +     for (cur = eth_devices; cur->next != eth_devices && cur->next != dev;
>> +          cur = cur->next)
>> +             ;
>> +
>> +     /* Device not found */
>> +     if (cur->next != dev)
>> +             return -1;
>> +
>> +     cur->next = dev->next;
>> +
>> +     if (eth_devices == dev)
>> +             eth_devices = dev->next == eth_devices ? NULL : dev->next;
>> +
>> +     if (eth_current == dev) {
>> +             eth_current = eth_devices;
>> +             eth_current_changed();
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  int eth_initialize(bd_t *bis)
>>  {
>>       int num_devices = 0;
>
> Looks sane. On what hardware did this happen (just for the record)?

I got it on x86 and ARM (tegra2) platforms (both with an EHCI controller).
I did most of my testing with ASIX-based USB-ethernet dongle, but the
issue seems pretty generic.
Wolfgang Grandegger - Jan. 10, 2012, 6:49 a.m.
On 01/09/2012 10:08 PM, Vincent Palatin wrote:
> On Mon, Jan 9, 2012 at 12:57, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> Fix the crash when running several times usb_init() with a USB ethernet
>>> device plugged.
>>>
>>> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
>>> Tested-by: Wolfgang Grandegger <wg@denx.de>
>>> ---
>>>  drivers/usb/eth/usb_ether.c |    7 +++++--
>>>  include/net.h               |    1 +
>>>  net/eth.c                   |   29 +++++++++++++++++++++++++++++
>>>  3 files changed, 35 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
>>> index 6565ea5..73a0790 100644
>>> --- a/drivers/usb/eth/usb_ether.c
>>> +++ b/drivers/usb/eth/usb_ether.c
>>> @@ -127,8 +127,11 @@ int usb_host_eth_scan(int mode)
>>>
>>>       old_async = usb_disable_asynch(1); /* asynch transfer not allowed */
>>>
>>> -     for (i = 0; i < USB_MAX_ETH_DEV; i++)
>>> -             memset(&usb_eth[i], 0, sizeof(usb_eth[i]));
>>> +     /* unregister a previously detected device */
>>> +     for (i = 0; i < usb_max_eth_dev; i++)
>>> +             eth_unregister(&usb_eth[i].eth_dev);
>>> +
>>> +     memset(usb_eth, 0, sizeof(usb_eth));
>>>
>>>       for (i = 0; prob_dev[i].probe; i++) {
>>>               if (prob_dev[i].before_probe)
>>> diff --git a/include/net.h b/include/net.h
>>> index e4d42c2..1707a7f 100644
>>> --- a/include/net.h
>>> +++ b/include/net.h
>>> @@ -96,6 +96,7 @@ struct eth_device {
>>>
>>>  extern int eth_initialize(bd_t *bis);        /* Initialize network subsystem
>> */
>>>  extern int eth_register(struct eth_device* dev);/* Register network device
>>> */ +extern int eth_unregister(struct eth_device *dev);/* Remove network
>>> device */ extern void eth_try_another(int first_restart);     /* Change the
>>> device */ extern void eth_set_current(void);          /* set nterface to
>> ethcur
>>> var */ extern struct eth_device *eth_get_dev(void);   /* get the current
>>> device MAC */ diff --git a/net/eth.c b/net/eth.c
>>> index b4b9b43..3fb5fb6 100644
>>> --- a/net/eth.c
>>> +++ b/net/eth.c
>>> @@ -224,6 +224,35 @@ int eth_register(struct eth_device *dev)
>>>       return 0;
>>>  }
>>>
>>> +int eth_unregister(struct eth_device *dev)
>>> +{
>>> +     struct eth_device *cur;
>>> +
>>> +     /* No device */
>>> +     if (!eth_devices)
>>> +             return -1;
>>> +
>>> +     for (cur = eth_devices; cur->next != eth_devices && cur->next != dev;
>>> +          cur = cur->next)
>>> +             ;
>>> +
>>> +     /* Device not found */
>>> +     if (cur->next != dev)
>>> +             return -1;
>>> +
>>> +     cur->next = dev->next;
>>> +
>>> +     if (eth_devices == dev)
>>> +             eth_devices = dev->next == eth_devices ? NULL : dev->next;
>>> +
>>> +     if (eth_current == dev) {
>>> +             eth_current = eth_devices;
>>> +             eth_current_changed();
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>  int eth_initialize(bd_t *bis)
>>>  {
>>>       int num_devices = 0;
>>
>> Looks sane. On what hardware did this happen (just for the record)?
> 
> I got it on x86 and ARM (tegra2) platforms (both with an EHCI controller).
> I did most of my testing with ASIX-based USB-ethernet dongle, but the
> issue seems pretty generic.

It happens with *any* hardware calling eth_register() more than once
because "struct eth_device *dev*" is memset 0 before calling
eth_register() resulting in an endless loop when scanning the devices.

Viele Grüße,

Wolfgang
Marek Vasut - Jan. 10, 2012, 10:54 a.m.
> On 01/09/2012 10:08 PM, Vincent Palatin wrote:
> > On Mon, Jan 9, 2012 at 12:57, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>> Fix the crash when running several times usb_init() with a USB ethernet
> >>> device plugged.
> >>> 
> >>> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> >>> Tested-by: Wolfgang Grandegger <wg@denx.de>
> >>> ---
> >>> 
> >>>  drivers/usb/eth/usb_ether.c |    7 +++++--
> >>>  include/net.h               |    1 +
> >>>  net/eth.c                   |   29 +++++++++++++++++++++++++++++
> >>>  3 files changed, 35 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
> >>> index 6565ea5..73a0790 100644
> >>> --- a/drivers/usb/eth/usb_ether.c
> >>> +++ b/drivers/usb/eth/usb_ether.c
> >>> @@ -127,8 +127,11 @@ int usb_host_eth_scan(int mode)
> >>> 
> >>>       old_async = usb_disable_asynch(1); /* asynch transfer not allowed
> >>>       */
> >>> 
> >>> -     for (i = 0; i < USB_MAX_ETH_DEV; i++)
> >>> -             memset(&usb_eth[i], 0, sizeof(usb_eth[i]));
> >>> +     /* unregister a previously detected device */
> >>> +     for (i = 0; i < usb_max_eth_dev; i++)
> >>> +             eth_unregister(&usb_eth[i].eth_dev);
> >>> +
> >>> +     memset(usb_eth, 0, sizeof(usb_eth));
> >>> 
> >>>       for (i = 0; prob_dev[i].probe; i++) {
> >>>       
> >>>               if (prob_dev[i].before_probe)
> >>> 
> >>> diff --git a/include/net.h b/include/net.h
> >>> index e4d42c2..1707a7f 100644
> >>> --- a/include/net.h
> >>> +++ b/include/net.h
> >>> @@ -96,6 +96,7 @@ struct eth_device {
> >>> 
> >>>  extern int eth_initialize(bd_t *bis);        /* Initialize network
> >>>  subsystem
> >> 
> >> */
> >> 
> >>>  extern int eth_register(struct eth_device* dev);/* Register network
> >>>  device
> >>> 
> >>> */ +extern int eth_unregister(struct eth_device *dev);/* Remove network
> >>> device */ extern void eth_try_another(int first_restart);     /* Change
> >>> the device */ extern void eth_set_current(void);          /* set
> >>> nterface to
> >> 
> >> ethcur
> >> 
> >>> var */ extern struct eth_device *eth_get_dev(void);   /* get the
> >>> current device MAC */ diff --git a/net/eth.c b/net/eth.c
> >>> index b4b9b43..3fb5fb6 100644
> >>> --- a/net/eth.c
> >>> +++ b/net/eth.c
> >>> @@ -224,6 +224,35 @@ int eth_register(struct eth_device *dev)
> >>> 
> >>>       return 0;
> >>>  
> >>>  }
> >>> 
> >>> +int eth_unregister(struct eth_device *dev)
> >>> +{
> >>> +     struct eth_device *cur;
> >>> +
> >>> +     /* No device */
> >>> +     if (!eth_devices)
> >>> +             return -1;
> >>> +
> >>> +     for (cur = eth_devices; cur->next != eth_devices && cur->next !=
> >>> dev; +          cur = cur->next)
> >>> +             ;
> >>> +
> >>> +     /* Device not found */
> >>> +     if (cur->next != dev)
> >>> +             return -1;
> >>> +
> >>> +     cur->next = dev->next;
> >>> +
> >>> +     if (eth_devices == dev)
> >>> +             eth_devices = dev->next == eth_devices ? NULL :
> >>> dev->next; +
> >>> +     if (eth_current == dev) {
> >>> +             eth_current = eth_devices;
> >>> +             eth_current_changed();
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> 
> >>>  int eth_initialize(bd_t *bis)
> >>>  {
> >>>  
> >>>       int num_devices = 0;
> >> 
> >> Looks sane. On what hardware did this happen (just for the record)?
> > 
> > I got it on x86 and ARM (tegra2) platforms (both with an EHCI
> > controller). I did most of my testing with ASIX-based USB-ethernet
> > dongle, but the issue seems pretty generic.
> 
> It happens with *any* hardware calling eth_register() more than once
> because "struct eth_device *dev*" is memset 0 before calling
> eth_register() resulting in an endless loop when scanning the devices.

Yes, I'll try to remember to test this in the evening.

M

> 
> Viele Grüße,
> 
> Wolfgang
Remy Bohmer - Jan. 15, 2012, 7:41 p.m.
Hi,

2012/1/9 Vincent Palatin <vpalatin@chromium.org>:
> Fix the crash when running several times usb_init() with a USB ethernet
> device plugged.
>
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Tested-by: Wolfgang Grandegger <wg@denx.de>
> ---
>  drivers/usb/eth/usb_ether.c |    7 +++++--
>  include/net.h               |    1 +
>  net/eth.c                   |   29 +++++++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 2 deletions(-)
>
Applied to u-boot-usb. Thanks.

Kind regards,

Remy
Marek Vasut - Feb. 26, 2012, 11:10 p.m.
> Fix the crash when running several times usb_init() with a USB ethernet
> device plugged.
> 
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Tested-by: Wolfgang Grandegger <wg@denx.de>
> ---

Hi,

what's the status of this patch/patchset?

Thanks
M
Simon Glass - March 8, 2012, 7:03 a.m.
Hi Marek,

On Sun, Feb 26, 2012 at 3:10 PM, Marek Vasut <marex@denx.de> wrote:
>> Fix the crash when running several times usb_init() with a USB ethernet
>> device plugged.
>>
>> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
>> Tested-by: Wolfgang Grandegger <wg@denx.de>
>> ---
>
> Hi,
>
> what's the status of this patch/patchset?

I believe it should be applied.

Regards,
Simon

>
> Thanks
> M
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Marek Vasut - March 8, 2012, 2:14 p.m.
Dear Simon Glass,

> Hi Marek,
> 
> On Sun, Feb 26, 2012 at 3:10 PM, Marek Vasut <marex@denx.de> wrote:
> >> Fix the crash when running several times usb_init() with a USB ethernet
> >> device plugged.
> >> 
> >> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> >> Tested-by: Wolfgang Grandegger <wg@denx.de>
> >> ---
> > 
> > Hi,
> > 
> > what's the status of this patch/patchset?
> 
> I believe it should be applied.
> 
> Regards,
> Simon
> 

Should be applied to linux-usb now, thanks!

Best regards,
Marek Vasut

Patch

diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
index 6565ea5..73a0790 100644
--- a/drivers/usb/eth/usb_ether.c
+++ b/drivers/usb/eth/usb_ether.c
@@ -127,8 +127,11 @@  int usb_host_eth_scan(int mode)
 
 	old_async = usb_disable_asynch(1); /* asynch transfer not allowed */
 
-	for (i = 0; i < USB_MAX_ETH_DEV; i++)
-		memset(&usb_eth[i], 0, sizeof(usb_eth[i]));
+	/* unregister a previously detected device */
+	for (i = 0; i < usb_max_eth_dev; i++)
+		eth_unregister(&usb_eth[i].eth_dev);
+
+	memset(usb_eth, 0, sizeof(usb_eth));
 
 	for (i = 0; prob_dev[i].probe; i++) {
 		if (prob_dev[i].before_probe)
diff --git a/include/net.h b/include/net.h
index e4d42c2..1707a7f 100644
--- a/include/net.h
+++ b/include/net.h
@@ -96,6 +96,7 @@  struct eth_device {
 
 extern int eth_initialize(bd_t *bis);	/* Initialize network subsystem */
 extern int eth_register(struct eth_device* dev);/* Register network device */
+extern int eth_unregister(struct eth_device *dev);/* Remove network device */
 extern void eth_try_another(int first_restart);	/* Change the device */
 extern void eth_set_current(void);		/* set nterface to ethcur var */
 extern struct eth_device *eth_get_dev(void);	/* get the current device MAC */
diff --git a/net/eth.c b/net/eth.c
index b4b9b43..3fb5fb6 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -224,6 +224,35 @@  int eth_register(struct eth_device *dev)
 	return 0;
 }
 
+int eth_unregister(struct eth_device *dev)
+{
+	struct eth_device *cur;
+
+	/* No device */
+	if (!eth_devices)
+		return -1;
+
+	for (cur = eth_devices; cur->next != eth_devices && cur->next != dev;
+	     cur = cur->next)
+		;
+
+	/* Device not found */
+	if (cur->next != dev)
+		return -1;
+
+	cur->next = dev->next;
+
+	if (eth_devices == dev)
+		eth_devices = dev->next == eth_devices ? NULL : dev->next;
+
+	if (eth_current == dev) {
+		eth_current = eth_devices;
+		eth_current_changed();
+	}
+
+	return 0;
+}
+
 int eth_initialize(bd_t *bis)
 {
 	int num_devices = 0;