* [PATCH] usb: hub: fix state change check for not powercycled ports @ 2021-01-18 19:09 Michael Grzeschik 2021-01-25 9:53 ` Sascha Hauer 2021-02-11 12:03 ` Ahmad Fatoum 0 siblings, 2 replies; 5+ messages in thread From: Michael Grzeschik @ 2021-01-18 19:09 UTC (permalink / raw) To: barebox Since we don't power cycle the ports on start since patch "19bb0b2a usb: hub: Do not power-cycle usb devices on init" it is possible that the device on this port is already active from a previous enumeration. This way barebox will never get any change in USB_PORT_STAT_C_CONNECTION bit change. Although the device will probably still work fine after the following port reset, the current code will always miss reenumerating these still plugged devices. This patch fixes this by ignoring the check for STAT_C_CONNECTION bit and only go for USB_PORT_STAT_CONNECTION which should be enough. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/core/hub.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 01653d8c20..d1112248ee 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -336,8 +336,7 @@ static void usb_scan_port(struct usb_device_scan *usb_scan) dev_dbg(&dev->dev, "port%d: Status 0x%04x Change 0x%04x\n", port + 1, portstatus, portchange); - if (!(portchange & USB_PORT_STAT_C_CONNECTION) || - !(portstatus & USB_PORT_STAT_CONNECTION)) { + if (!(portstatus & USB_PORT_STAT_CONNECTION)) { if (get_time_ns() >= hub->connect_timeout) { dev_dbg(&dev->dev, "port%d: timeout\n", port + 1); /* Remove this device from scanning list */ -- 2.30.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: hub: fix state change check for not powercycled ports 2021-01-18 19:09 [PATCH] usb: hub: fix state change check for not powercycled ports Michael Grzeschik @ 2021-01-25 9:53 ` Sascha Hauer 2021-02-11 12:03 ` Ahmad Fatoum 1 sibling, 0 replies; 5+ messages in thread From: Sascha Hauer @ 2021-01-25 9:53 UTC (permalink / raw) To: Michael Grzeschik; +Cc: barebox On Mon, Jan 18, 2021 at 08:09:23PM +0100, Michael Grzeschik wrote: > Since we don't power cycle the ports on start since patch "19bb0b2a usb: > hub: Do not power-cycle usb devices on init" it is possible that the > device on this port is already active from a previous enumeration. This > way barebox will never get any change in USB_PORT_STAT_C_CONNECTION bit > change. > > Although the device will probably still work fine after the following > port reset, the current code will always miss reenumerating these still > plugged devices. This patch fixes this by ignoring the check for > STAT_C_CONNECTION bit and only go for USB_PORT_STAT_CONNECTION which > should be enough. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/usb/core/hub.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Applied, thanks Sascha > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 01653d8c20..d1112248ee 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -336,8 +336,7 @@ static void usb_scan_port(struct usb_device_scan *usb_scan) > dev_dbg(&dev->dev, "port%d: Status 0x%04x Change 0x%04x\n", > port + 1, portstatus, portchange); > > - if (!(portchange & USB_PORT_STAT_C_CONNECTION) || > - !(portstatus & USB_PORT_STAT_CONNECTION)) { > + if (!(portstatus & USB_PORT_STAT_CONNECTION)) { > if (get_time_ns() >= hub->connect_timeout) { > dev_dbg(&dev->dev, "port%d: timeout\n", port + 1); > /* Remove this device from scanning list */ > -- > 2.30.0 > > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: hub: fix state change check for not powercycled ports 2021-01-18 19:09 [PATCH] usb: hub: fix state change check for not powercycled ports Michael Grzeschik 2021-01-25 9:53 ` Sascha Hauer @ 2021-02-11 12:03 ` Ahmad Fatoum 2021-02-16 8:20 ` Michael Grzeschik 1 sibling, 1 reply; 5+ messages in thread From: Ahmad Fatoum @ 2021-02-11 12:03 UTC (permalink / raw) To: Michael Grzeschik, barebox Hello, On 18.01.21 20:09, Michael Grzeschik wrote: > Since we don't power cycle the ports on start since patch "19bb0b2a usb: > hub: Do not power-cycle usb devices on init" it is possible that the > device on this port is already active from a previous enumeration. This > way barebox will never get any change in USB_PORT_STAT_C_CONNECTION bit > change. > > Although the device will probably still work fine after the following > port reset, the current code will always miss reenumerating these still > plugged devices. This patch fixes this by ignoring the check for > STAT_C_CONNECTION bit and only go for USB_PORT_STAT_CONNECTION which > should be enough. This breaks USB enumeration for me on an i.MX6Q. The USB flash driver behind the hub is detected twice, which barebox doesn't like at all: barebox@Embest MarS Board i.MX6Dual:/ usb -s usb: USB: scanning bus for devices... usb1: Bus 001 Device 001: ID 0000:0000 EHCI Host Controller usb1-0: Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] usb1-0-1: Bus 001 Device 003: ID 058f:6387 Mass Storage Using index 0 for the new disk usb1-0-1: Bus 001 Device 004: ID 058f:6387 Mass Storage ERROR: register_device: already registered usb1-0-1 ERROR: usb1-0-1: Failed to register device: Invalid argument usb: 3 USB Device(s) found Bus 001 Device 001: ID 0000:0000 EHCI Host Controller Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] Bus 001 Device 003: ID 058f:6387 Mass Storage This seems to corrupt some internal barebox state as reading /dev/disk0.0 will hang. Reading /dev/disk0 is fine however. With this patch reverted it however works: usb: USB: scanning bus for devices... usb1: Bus 001 Device 001: ID 0000:0000 EHCI Host Controller usb1-0: Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] usb1-0-1: Bus 001 Device 003: ID 058f:6387 Mass Storage Using index 0 for the new disk usb: 3 USB Device(s) found Bus 001 Device 001: ID 0000:0000 EHCI Host Controller Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] Bus 001 Device 003: ID 058f:6387 Mass Storage The USB drive is not self-powered. It doesn't make a difference whether it's done directly after cold reset or: usb -s boot bnet usb -s Cheers, Ahmad > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/usb/core/hub.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 01653d8c20..d1112248ee 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -336,8 +336,7 @@ static void usb_scan_port(struct usb_device_scan *usb_scan) > dev_dbg(&dev->dev, "port%d: Status 0x%04x Change 0x%04x\n", > port + 1, portstatus, portchange); > > - if (!(portchange & USB_PORT_STAT_C_CONNECTION) || > - !(portstatus & USB_PORT_STAT_CONNECTION)) { > + if (!(portstatus & USB_PORT_STAT_CONNECTION)) { > if (get_time_ns() >= hub->connect_timeout) { > dev_dbg(&dev->dev, "port%d: timeout\n", port + 1); > /* Remove this device from scanning list */ > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: hub: fix state change check for not powercycled ports 2021-02-11 12:03 ` Ahmad Fatoum @ 2021-02-16 8:20 ` Michael Grzeschik 2021-02-16 9:33 ` Sascha Hauer 0 siblings, 1 reply; 5+ messages in thread From: Michael Grzeschik @ 2021-02-16 8:20 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox [-- Attachment #1.1: Type: text/plain, Size: 4160 bytes --] Hi! On Thu, Feb 11, 2021 at 01:03:54PM +0100, Ahmad Fatoum wrote: >Hello, > >On 18.01.21 20:09, Michael Grzeschik wrote: >> Since we don't power cycle the ports on start since patch "19bb0b2a usb: >> hub: Do not power-cycle usb devices on init" it is possible that the >> device on this port is already active from a previous enumeration. This >> way barebox will never get any change in USB_PORT_STAT_C_CONNECTION bit >> change. >> >> Although the device will probably still work fine after the following >> port reset, the current code will always miss reenumerating these still >> plugged devices. This patch fixes this by ignoring the check for >> STAT_C_CONNECTION bit and only go for USB_PORT_STAT_CONNECTION which >> should be enough. > >This breaks USB enumeration for me on an i.MX6Q. The USB flash driver behind >the hub is detected twice, which barebox doesn't like at all: > >barebox@Embest MarS Board i.MX6Dual:/ usb -s >usb: USB: scanning bus for devices... >usb1: Bus 001 Device 001: ID 0000:0000 EHCI Host Controller >usb1-0: Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] >usb1-0-1: Bus 001 Device 003: ID 058f:6387 Mass Storage >Using index 0 for the new disk >usb1-0-1: Bus 001 Device 004: ID 058f:6387 Mass Storage >ERROR: register_device: already registered usb1-0-1 >ERROR: usb1-0-1: Failed to register device: Invalid argument >usb: 3 USB Device(s) found >Bus 001 Device 001: ID 0000:0000 EHCI Host Controller >Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] >Bus 001 Device 003: ID 058f:6387 Mass Storage > >This seems to corrupt some internal barebox state as reading >/dev/disk0.0 will hang. Reading /dev/disk0 is fine however. > >With this patch reverted it however works: > >usb: USB: scanning bus for devices... >usb1: Bus 001 Device 001: ID 0000:0000 EHCI Host Controller >usb1-0: Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] >usb1-0-1: Bus 001 Device 003: ID 058f:6387 Mass Storage >Using index 0 for the new disk >usb: 3 USB Device(s) found >Bus 001 Device 001: ID 0000:0000 EHCI Host Controller >Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] >Bus 001 Device 003: ID 058f:6387 Mass Storage > >The USB drive is not self-powered. It doesn't make a difference >whether it's done directly after cold reset or: > usb -s > boot bnet > usb -s > >Cheers, >Ahmad I have reproduced that. My latest findings are that, when I debugged this, usb core on my board I was debugging was behaving uncommon. It was not setting the USB_PORT_STAT_C_CONNECTION in the first place. But the further patches I send did solve this. So this whole issue was due to an uncorrect configured over-current polarity setup. With my other patches applied I see this bit USB_PORT_STAT_C_CONNECTION set even without the power-cycle. My suggestion is to _revert_ this patch, as it is solving a bogus problem not related to the whole usb-core state machine. Thanks for testing! Michael >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> --- >> drivers/usb/core/hub.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >> index 01653d8c20..d1112248ee 100644 >> --- a/drivers/usb/core/hub.c >> +++ b/drivers/usb/core/hub.c >> @@ -336,8 +336,7 @@ static void usb_scan_port(struct usb_device_scan *usb_scan) >> dev_dbg(&dev->dev, "port%d: Status 0x%04x Change 0x%04x\n", >> port + 1, portstatus, portchange); >> >> - if (!(portchange & USB_PORT_STAT_C_CONNECTION) || >> - !(portstatus & USB_PORT_STAT_CONNECTION)) { >> + if (!(portstatus & USB_PORT_STAT_CONNECTION)) { >> if (get_time_ns() >= hub->connect_timeout) { >> dev_dbg(&dev->dev, "port%d: timeout\n", port + 1); >> /* Remove this device from scanning list */ >> -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 149 bytes --] _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: hub: fix state change check for not powercycled ports 2021-02-16 8:20 ` Michael Grzeschik @ 2021-02-16 9:33 ` Sascha Hauer 0 siblings, 0 replies; 5+ messages in thread From: Sascha Hauer @ 2021-02-16 9:33 UTC (permalink / raw) To: Michael Grzeschik; +Cc: barebox, Ahmad Fatoum On Tue, Feb 16, 2021 at 09:20:01AM +0100, Michael Grzeschik wrote: > Hi! > > On Thu, Feb 11, 2021 at 01:03:54PM +0100, Ahmad Fatoum wrote: > > Hello, > > > > On 18.01.21 20:09, Michael Grzeschik wrote: > > > Since we don't power cycle the ports on start since patch "19bb0b2a usb: > > > hub: Do not power-cycle usb devices on init" it is possible that the > > > device on this port is already active from a previous enumeration. This > > > way barebox will never get any change in USB_PORT_STAT_C_CONNECTION bit > > > change. > > > > > > Although the device will probably still work fine after the following > > > port reset, the current code will always miss reenumerating these still > > > plugged devices. This patch fixes this by ignoring the check for > > > STAT_C_CONNECTION bit and only go for USB_PORT_STAT_CONNECTION which > > > should be enough. > > > > This breaks USB enumeration for me on an i.MX6Q. The USB flash driver behind > > the hub is detected twice, which barebox doesn't like at all: > > > > barebox@Embest MarS Board i.MX6Dual:/ usb -s > > usb: USB: scanning bus for devices... > > usb1: Bus 001 Device 001: ID 0000:0000 EHCI Host Controller > > usb1-0: Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] > > usb1-0-1: Bus 001 Device 003: ID 058f:6387 Mass Storage > > Using index 0 for the new disk > > usb1-0-1: Bus 001 Device 004: ID 058f:6387 Mass Storage > > ERROR: register_device: already registered usb1-0-1 > > ERROR: usb1-0-1: Failed to register device: Invalid argument > > usb: 3 USB Device(s) found > > Bus 001 Device 001: ID 0000:0000 EHCI Host Controller > > Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] > > Bus 001 Device 003: ID 058f:6387 Mass Storage > > > > This seems to corrupt some internal barebox state as reading > > /dev/disk0.0 will hang. Reading /dev/disk0 is fine however. > > > > With this patch reverted it however works: > > > > usb: USB: scanning bus for devices... > > usb1: Bus 001 Device 001: ID 0000:0000 EHCI Host Controller > > usb1-0: Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] > > usb1-0-1: Bus 001 Device 003: ID 058f:6387 Mass Storage > > Using index 0 for the new disk > > usb: 3 USB Device(s) found > > Bus 001 Device 001: ID 0000:0000 EHCI Host Controller > > Bus 001 Device 002: ID 1a40:0101 USB 2.0 Hub [MTT] > > Bus 001 Device 003: ID 058f:6387 Mass Storage > > > > The USB drive is not self-powered. It doesn't make a difference > > whether it's done directly after cold reset or: > > usb -s > > boot bnet > > usb -s > > > > Cheers, > > Ahmad > > I have reproduced that. My latest findings are that, when I debugged > this, usb core on my board I was debugging was behaving uncommon. It was > not setting the USB_PORT_STAT_C_CONNECTION in the first place. But the > further patches I send did solve this. > > So this whole issue was due to an uncorrect configured over-current > polarity setup. With my other patches applied I see this bit > USB_PORT_STAT_C_CONNECTION set even without the power-cycle. > > My suggestion is to _revert_ this patch, as it is solving a > bogus problem not related to the whole usb-core state machine. Ok, dropped this patch. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-16 9:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-18 19:09 [PATCH] usb: hub: fix state change check for not powercycled ports Michael Grzeschik 2021-01-25 9:53 ` Sascha Hauer 2021-02-11 12:03 ` Ahmad Fatoum 2021-02-16 8:20 ` Michael Grzeschik 2021-02-16 9:33 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox