| Message ID | 1326133956-25941-1-git-send-email-vpalatin@chromium.org |
|---|---|
| State | Accepted |
| Commit | e7e982d69c43b89f7e19b63479c8b2e880fbd75c |
| Delegated to: | Marek Vasut |
| Headers | show |
> 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
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.
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
> 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
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
> 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
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
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
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;