| Message ID | 87y4dxltzi.fsf@saruman.tx.rr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Felipe, On Mon, Nov 16, 2015 at 9:44 AM, Felipe Balbi <balbi@ti.com> wrote: >> extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg); >> #else >> static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg) >> { return 0; } >> -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {} >> +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {} >> +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {} >> static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {} >> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {} >> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq) >> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c >> index 27daa42788b1..61601d16e233 100644 >> --- a/drivers/usb/dwc2/core_intr.c >> +++ b/drivers/usb/dwc2/core_intr.c >> @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg) >> dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n", >> hsotg->op_state); >> spin_unlock(&hsotg->lock); >> - dwc2_hcd_disconnect(hsotg); >> + dwc2_hcd_disconnect(hsotg, false); > > because force is unnecessary (it seems to be just an optimization to > me), this change is unnecessary. I added "force" in v2 of the patch in response to John's feedback to v1. He pointed out that when you unload the module when you have a device connected that my v1 patch would not properly disconnect the device (or, rather, it would disconnect it and then reconnect it). That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force of "true". >> /** >> + * dwc2_hcd_connect() - Handles connect of the HCD >> + * >> + * @hsotg: Pointer to struct dwc2_hsotg >> + * >> + * Must be called with interrupt disabled and spinlock held >> + */ >> +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) >> +{ >> + if (hsotg->lx_state != DWC2_L0) >> + usb_hcd_resume_root_hub(hsotg->priv); >> + >> + hsotg->flags.b.port_connect_status_change = 1; >> + hsotg->flags.b.port_connect_status = 1; >> +} > > you make no mention of why this is needed. This is basically a refactor, > not a fix. This new function is called from two places now, isn't it? Basically this is a little bit of code that used to be directly in dwc2_port_intr(). I still want it there, but I also want to call the same bit of code after a disconnect if I detect that the device has been inserted again. >> @@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) >> dwc2_hcd_cleanup_channels(hsotg); >> >> dwc2_host_disconnect(hsotg); >> + >> + /* >> + * Add an extra check here to see if we're actually connected but >> + * we don't have a detection interrupt pending. This can happen if: >> + * 1. hardware sees connect >> + * 2. hardware sees disconnect >> + * 3. hardware sees connect >> + * 4. dwc2_port_intr() - clears connect interrupt >> + * 5. dwc2_handle_common_intr() - calls here >> + * >> + * Without the extra check here we will end calling disconnect >> + * and won't get any future interrupts to handle the connect. >> + */ >> + if (!force) { >> + hprt0 = dwc2_readl(hsotg->regs + HPRT0); >> + if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS)) >> + dwc2_hcd_connect(hsotg); > > what's inside this 'force' branch is what you really need to fix the > problem, right ? It seems like for the -rc cycle, all you need is: > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index 571c21727ff9..967d1e686efe 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) > */ > void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) > { > + u32 hprt0; > u32 intr; > > /* Set status flags for the hub driver */ > @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) > dwc2_hcd_cleanup_channels(hsotg); > > dwc2_host_disconnect(hsotg); > + > + hprt0 = dwc2_readl(hsotg->regs + HPRT0); > + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) { > + if (hsotg->lx_state != DWC2_L0) > + usb_hcd_resume_roothub(hsotg->priv); > + > + hsotg->flags.b.port_connect_status_change = 1; > + hsotg->flags.b.port_connect_status = 1; > + } I'd really rather not add the duplication unless you insist. To me it makes it clearer to include the (small) refactor in the same patch. If the refactor makes this change too big for an RC, then it's OK with me to just skip this for the RC. It's not fixing a regression or anything. I have no requirements to have this land in 4.4. It fixes a bug and I thought that the fix was pretty small and safe (despite having a diffstat that's bigger than the bare minimum). I will leave it to your judgement. >> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c >> index bda0b21b850f..03504ac2fecc 100644 >> --- a/drivers/usb/dwc2/hcd_intr.c >> +++ b/drivers/usb/dwc2/hcd_intr.c >> @@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) >> dev_vdbg(hsotg->dev, >> "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n", >> hprt0); >> - if (hsotg->lx_state != DWC2_L0) >> - usb_hcd_resume_root_hub(hsotg->priv); >> - >> - hsotg->flags.b.port_connect_status_change = 1; >> - hsotg->flags.b.port_connect_status = 1; >> + dwc2_hcd_connect(hsotg); > > unnecessary change. > > Do you agree or do you think 'force' is really necessary for this fix ? As per above, John thought it was a good idea to really make the disconnect happen upon module unload. If you disagree then we could probably just totally remove the "dwc2_hcd_disconnect(hsotg);" from the _dwc2_hcd_stop() function. -Doug
Hi, Doug Anderson <dianders@chromium.org> writes: > Felipe, > > On Mon, Nov 16, 2015 at 9:44 AM, Felipe Balbi <balbi@ti.com> wrote: >>> extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg); >>> #else >>> static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg) >>> { return 0; } >>> -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {} >>> +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {} >>> +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {} >>> static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {} >>> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {} >>> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq) >>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c >>> index 27daa42788b1..61601d16e233 100644 >>> --- a/drivers/usb/dwc2/core_intr.c >>> +++ b/drivers/usb/dwc2/core_intr.c >>> @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg) >>> dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n", >>> hsotg->op_state); >>> spin_unlock(&hsotg->lock); >>> - dwc2_hcd_disconnect(hsotg); >>> + dwc2_hcd_disconnect(hsotg, false); >> >> because force is unnecessary (it seems to be just an optimization to >> me), this change is unnecessary. > > I added "force" in v2 of the patch in response to John's feedback to > v1. He pointed out that when you unload the module when you have a > device connected that my v1 patch would not properly disconnect the > device (or, rather, it would disconnect it and then reconnect it). > > That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force > of "true". There's no mention of this in commit log. It would be great to add a: "while at that, also make sure that we don't try to detect a device on module unload because of foo bar baz as pointed out by John Youn". Or something along these lines. >>> /** >>> + * dwc2_hcd_connect() - Handles connect of the HCD >>> + * >>> + * @hsotg: Pointer to struct dwc2_hsotg >>> + * >>> + * Must be called with interrupt disabled and spinlock held >>> + */ >>> +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) >>> +{ >>> + if (hsotg->lx_state != DWC2_L0) >>> + usb_hcd_resume_root_hub(hsotg->priv); >>> + >>> + hsotg->flags.b.port_connect_status_change = 1; >>> + hsotg->flags.b.port_connect_status = 1; >>> +} >> >> you make no mention of why this is needed. This is basically a refactor, >> not a fix. > > This new function is called from two places now, isn't it? > > Basically this is a little bit of code that used to be directly in > dwc2_port_intr(). I still want it there, but I also want to call the > same bit of code after a disconnect if I detect that the device has > been inserted again. I got that :-) But it's not mentioned in commit and it's apparently unnecessary for fixing the bug :-) Another "we're also adding a new hsotg_disconnect() function by means of refactoring to avoid code duplication" would've been enough. >>> @@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) >>> dwc2_hcd_cleanup_channels(hsotg); >>> >>> dwc2_host_disconnect(hsotg); >>> + >>> + /* >>> + * Add an extra check here to see if we're actually connected but >>> + * we don't have a detection interrupt pending. This can happen if: >>> + * 1. hardware sees connect >>> + * 2. hardware sees disconnect >>> + * 3. hardware sees connect >>> + * 4. dwc2_port_intr() - clears connect interrupt >>> + * 5. dwc2_handle_common_intr() - calls here >>> + * >>> + * Without the extra check here we will end calling disconnect >>> + * and won't get any future interrupts to handle the connect. >>> + */ >>> + if (!force) { >>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0); >>> + if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS)) >>> + dwc2_hcd_connect(hsotg); >> >> what's inside this 'force' branch is what you really need to fix the >> problem, right ? It seems like for the -rc cycle, all you need is: >> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >> index 571c21727ff9..967d1e686efe 100644 >> --- a/drivers/usb/dwc2/hcd.c >> +++ b/drivers/usb/dwc2/hcd.c >> @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) >> */ >> void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) >> { >> + u32 hprt0; >> u32 intr; >> >> /* Set status flags for the hub driver */ >> @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) >> dwc2_hcd_cleanup_channels(hsotg); >> >> dwc2_host_disconnect(hsotg); >> + >> + hprt0 = dwc2_readl(hsotg->regs + HPRT0); >> + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) { >> + if (hsotg->lx_state != DWC2_L0) >> + usb_hcd_resume_roothub(hsotg->priv); >> + >> + hsotg->flags.b.port_connect_status_change = 1; >> + hsotg->flags.b.port_connect_status = 1; >> + } > > I'd really rather not add the duplication unless you insist. To me it > makes it clearer to include the (small) refactor in the same patch. > > If the refactor makes this change too big for an RC, then it's OK with > me to just skip this for the RC. It's not fixing a regression or > anything. I have no requirements to have this land in 4.4. It fixes > a bug and I thought that the fix was pretty small and safe (despite > having a diffstat that's bigger than the bare minimum). I will leave > it to your judgement. let's at least modify commit log to make all these extra changes clear that they are needed because of reason (a) or (b) or whatever. If you just send a patch doing much more than it apparently should without no mention as to why it was done this way, I can't know for sure those changes are needed; next thing you know, Greg refuses to apply my pull request because the change is too large :-) We don't want that to happen :-) >>> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c >>> index bda0b21b850f..03504ac2fecc 100644 >>> --- a/drivers/usb/dwc2/hcd_intr.c >>> +++ b/drivers/usb/dwc2/hcd_intr.c >>> @@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) >>> dev_vdbg(hsotg->dev, >>> "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n", >>> hprt0); >>> - if (hsotg->lx_state != DWC2_L0) >>> - usb_hcd_resume_root_hub(hsotg->priv); >>> - >>> - hsotg->flags.b.port_connect_status_change = 1; >>> - hsotg->flags.b.port_connect_status = 1; >>> + dwc2_hcd_connect(hsotg); >> >> unnecessary change. >> >> Do you agree or do you think 'force' is really necessary for this fix ? > > As per above, John thought it was a good idea to really make the > disconnect happen upon module unload. If you disagree then we could > probably just totally remove the "dwc2_hcd_disconnect(hsotg);" from > the _dwc2_hcd_stop() function. see above. thanks
Felipe, On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi@ti.com> wrote: >> I added "force" in v2 of the patch in response to John's feedback to >> v1. He pointed out that when you unload the module when you have a >> device connected that my v1 patch would not properly disconnect the >> device (or, rather, it would disconnect it and then reconnect it). >> >> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force >> of "true". > > There's no mention of this in commit log. It would be great to add a: > > "while at that, also make sure that we don't try to detect a device on > module unload because of foo bar baz as pointed out by John Youn". > > Or something along these lines. ...well, except that it's not new behavior. In other words: * Without my patch at all: we don't try to detect a device on module unload. * With my v1 patch: we (incorrectly) did try to detect a device on module unload. * With my v2 patch: we're back to not trying to detect a device on module unload. In other words: my v2 patch (correctly) doesn't change the behavior on module unload. That's why I didn't mention it in the commit message. It's in the "v2" changelog though. I'll try to come up with something for the commit message though. See below for new proposed commit message. >>> you make no mention of why this is needed. This is basically a refactor, >>> not a fix. >> >> This new function is called from two places now, isn't it? >> >> Basically this is a little bit of code that used to be directly in >> dwc2_port_intr(). I still want it there, but I also want to call the >> same bit of code after a disconnect if I detect that the device has >> been inserted again. > > I got that :-) But it's not mentioned in commit and it's apparently > unnecessary for fixing the bug :-) Another "we're also adding a new > hsotg_disconnect() function by means of refactoring to avoid code > duplication" would've been enough. OK, sure. >> I'd really rather not add the duplication unless you insist. To me it >> makes it clearer to include the (small) refactor in the same patch. >> >> If the refactor makes this change too big for an RC, then it's OK with >> me to just skip this for the RC. It's not fixing a regression or >> anything. I have no requirements to have this land in 4.4. It fixes >> a bug and I thought that the fix was pretty small and safe (despite >> having a diffstat that's bigger than the bare minimum). I will leave >> it to your judgement. > > let's at least modify commit log to make all these extra changes clear > that they are needed because of reason (a) or (b) or whatever. If you > just send a patch doing much more than it apparently should without no > mention as to why it was done this way, I can't know for sure those > changes are needed; next thing you know, Greg refuses to apply my pull > request because the change is too large :-) > > We don't want that to happen :-) Totally understand. It's your butt on the line for the pull request to Greg, so definitely want to make sure you're comfortable with anything you pass on. As always I definitely appreciate your reviews and your time. How about if we just add a "Notes" to the end of the patch description. I can repost a patch or you can feel free to change the description as per below (just let me know). ...so in total: --- usb: dwc2: host: Fix missing device insertions If you've got your interrupt signals bouncing a bit as you insert your USB device, you might end up in a state when the device is connected but the driver doesn't know it. Specifically, the observed order is: 1. hardware sees connect 2. hardware sees disconnect 3. hardware sees connect 4. dwc2_port_intr() - clears connect interrupt 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() Now you'll be stuck with the cable plugged in and no further interrupts coming in but the driver will think we're disconnected. We'll fix this by checking for the missing connect interrupt and re-connecting after the disconnect is posted. We don't skip the disconnect because if there is a transitory disconnect we really want to de-enumerate and re-enumerate. Notes: 1. As part of this change we add a "force" parameter to dwc2_hcd_disconnect() so that when we're unloading the module we avoid the new behavior. The need for this was pointed out by John Youn. 2. The bit of code needed at the end of dwc2_hcd_disconnect() is exactly the same bit of code from dwc2_port_intr(). To avoid duplication, we refactor that code out into a new function dwc2_hcd_connect(). -Doug
hi, Doug Anderson <dianders@chromium.org> writes: > Felipe, > > On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi@ti.com> wrote: >>> I added "force" in v2 of the patch in response to John's feedback to >>> v1. He pointed out that when you unload the module when you have a >>> device connected that my v1 patch would not properly disconnect the >>> device (or, rather, it would disconnect it and then reconnect it). >>> >>> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force >>> of "true". >> >> There's no mention of this in commit log. It would be great to add a: >> >> "while at that, also make sure that we don't try to detect a device on >> module unload because of foo bar baz as pointed out by John Youn". >> >> Or something along these lines. > > ...well, except that it's not new behavior. In other words: > > * Without my patch at all: we don't try to detect a device on module unload. > > * With my v1 patch: we (incorrectly) did try to detect a device on > module unload. > > * With my v2 patch: we're back to not trying to detect a device on > module unload. > > In other words: my v2 patch (correctly) doesn't change the behavior on > module unload. That's why I didn't mention it in the commit message. > It's in the "v2" changelog though. > > > I'll try to come up with something for the commit message though. See > below for new proposed commit message. > > >>>> you make no mention of why this is needed. This is basically a refactor, >>>> not a fix. >>> >>> This new function is called from two places now, isn't it? >>> >>> Basically this is a little bit of code that used to be directly in >>> dwc2_port_intr(). I still want it there, but I also want to call the >>> same bit of code after a disconnect if I detect that the device has >>> been inserted again. >> >> I got that :-) But it's not mentioned in commit and it's apparently >> unnecessary for fixing the bug :-) Another "we're also adding a new >> hsotg_disconnect() function by means of refactoring to avoid code >> duplication" would've been enough. > > OK, sure. > > >>> I'd really rather not add the duplication unless you insist. To me it >>> makes it clearer to include the (small) refactor in the same patch. >>> >>> If the refactor makes this change too big for an RC, then it's OK with >>> me to just skip this for the RC. It's not fixing a regression or >>> anything. I have no requirements to have this land in 4.4. It fixes >>> a bug and I thought that the fix was pretty small and safe (despite >>> having a diffstat that's bigger than the bare minimum). I will leave >>> it to your judgement. >> >> let's at least modify commit log to make all these extra changes clear >> that they are needed because of reason (a) or (b) or whatever. If you >> just send a patch doing much more than it apparently should without no >> mention as to why it was done this way, I can't know for sure those >> changes are needed; next thing you know, Greg refuses to apply my pull >> request because the change is too large :-) >> >> We don't want that to happen :-) > > Totally understand. It's your butt on the line for the pull request > to Greg, so definitely want to make sure you're comfortable with > anything you pass on. As always I definitely appreciate your reviews > and your time. > > > How about if we just add a "Notes" to the end of the patch > description. I can repost a patch or you can feel free to change the > description as per below (just let me know). ...so in total: > > --- > > usb: dwc2: host: Fix missing device insertions > > If you've got your interrupt signals bouncing a bit as you insert your > USB device, you might end up in a state when the device is connected but > the driver doesn't know it. > > Specifically, the observed order is: > 1. hardware sees connect > 2. hardware sees disconnect > 3. hardware sees connect > 4. dwc2_port_intr() - clears connect interrupt > 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() > > Now you'll be stuck with the cable plugged in and no further interrupts > coming in but the driver will think we're disconnected. > > We'll fix this by checking for the missing connect interrupt and > re-connecting after the disconnect is posted. We don't skip the > disconnect because if there is a transitory disconnect we really want to > de-enumerate and re-enumerate. > > Notes: > > 1. As part of this change we add a "force" parameter to > dwc2_hcd_disconnect() so that when we're unloading the module we > avoid the new behavior. The need for this was pointed out by John > Youn. > 2. The bit of code needed at the end of dwc2_hcd_disconnect() is > exactly the same bit of code from dwc2_port_intr(). To avoid > duplication, we refactor that code out into a new function > dwc2_hcd_connect(). this should be enough, thanks for being so responsive
On Mon, 16 Nov 2015, Doug Anderson wrote: > --- > > usb: dwc2: host: Fix missing device insertions > > If you've got your interrupt signals bouncing a bit as you insert your > USB device, you might end up in a state when the device is connected but > the driver doesn't know it. > > Specifically, the observed order is: > 1. hardware sees connect > 2. hardware sees disconnect > 3. hardware sees connect > 4. dwc2_port_intr() - clears connect interrupt > 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() > > Now you'll be stuck with the cable plugged in and no further interrupts > coming in but the driver will think we're disconnected. > > We'll fix this by checking for the missing connect interrupt and > re-connecting after the disconnect is posted. We don't skip the > disconnect because if there is a transitory disconnect we really want to > de-enumerate and re-enumerate. Why do you need to do anything special here? Normally a driver's interrupt handler should query the hardware status after clearing the interrupt source. That way no transitions ever get missed. In your example, at step 5 the dwc2 driver would check the port status and see that it currently is connected. Therefore the driver would pass a "connect status changed" event to the USB core and set the port status to "connected". No extra checking is needed, and transitory connects or disconnects get handled correctly. Alan Stern
Alan, On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 16 Nov 2015, Doug Anderson wrote: > >> --- >> >> usb: dwc2: host: Fix missing device insertions >> >> If you've got your interrupt signals bouncing a bit as you insert your >> USB device, you might end up in a state when the device is connected but >> the driver doesn't know it. >> >> Specifically, the observed order is: >> 1. hardware sees connect >> 2. hardware sees disconnect >> 3. hardware sees connect >> 4. dwc2_port_intr() - clears connect interrupt >> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() >> >> Now you'll be stuck with the cable plugged in and no further interrupts >> coming in but the driver will think we're disconnected. >> >> We'll fix this by checking for the missing connect interrupt and >> re-connecting after the disconnect is posted. We don't skip the >> disconnect because if there is a transitory disconnect we really want to >> de-enumerate and re-enumerate. > > Why do you need to do anything special here? Normally a driver's > interrupt handler should query the hardware status after clearing the > interrupt source. That way no transitions ever get missed. > > In your example, at step 5 the dwc2 driver would check the port status > and see that it currently is connected. Therefore the driver would > pass a "connect status changed" event to the USB core and set the port > status to "connected". No extra checking is needed, and transitory > connects or disconnects get handled correctly. Things are pretty ugly at the moment because the dwc2 interrupt handler is split in two. There's dwc2_handle_hcd_intr() and dwc2_handle_common_intr(). Both are registered for the same "shared" IRQ. If I had to guess, I'd say that this is probably because someone wanted to assign the ".irq" field in the "struct hc_driver" for the host controller but that they also needed the other interrupt handler to handle things shared between host and gadget mode. In any case, one of these two interrupt routines handles connect and the other disconnect. That's pretty ugly but means that you can't just rely on standard techniques for keeping things in sync. It would probably be a pretty reasonable idea to try to clean that up more, but that would be a very major and intrusive change. -Doug
Another point to note, which I think is what prevents the flow Alan
suggested from working, is this little snippet in DWC2's hub_control
GetPortStatus callback:
if (!hsotg->flags.b.port_connect_status) {
/*
* The port is disconnected, which means the core is
* either in device mode or it soon will be.
Just
* return 0's for the remainder of the port status
* since the port register can't be read if the core
* is in device mode.
*/
*(__le32 *)buf = cpu_to_le32(port_status);
break;
}
This is before it actually checks the register state of the port. So
it relies on dwc2_hcd_connect() and dwc2_hcd_disconnect() to be called
in the right order to give the correct answer for
USB_PORT_STAT_CONNECTION. This could maybe be improved but I don't
know what kind of weird interactions with device mode operation made
that snippet necessary in the first place.
On Mon, 16 Nov 2015, Doug Anderson wrote: > Alan, > > On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, 16 Nov 2015, Doug Anderson wrote: > > > >> --- > >> > >> usb: dwc2: host: Fix missing device insertions > >> > >> If you've got your interrupt signals bouncing a bit as you insert your > >> USB device, you might end up in a state when the device is connected but > >> the driver doesn't know it. > >> > >> Specifically, the observed order is: > >> 1. hardware sees connect > >> 2. hardware sees disconnect > >> 3. hardware sees connect > >> 4. dwc2_port_intr() - clears connect interrupt > >> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() > >> > >> Now you'll be stuck with the cable plugged in and no further interrupts > >> coming in but the driver will think we're disconnected. > >> > >> We'll fix this by checking for the missing connect interrupt and > >> re-connecting after the disconnect is posted. We don't skip the > >> disconnect because if there is a transitory disconnect we really want to > >> de-enumerate and re-enumerate. > > > > Why do you need to do anything special here? Normally a driver's > > interrupt handler should query the hardware status after clearing the > > interrupt source. That way no transitions ever get missed. > > > > In your example, at step 5 the dwc2 driver would check the port status > > and see that it currently is connected. Therefore the driver would > > pass a "connect status changed" event to the USB core and set the port > > status to "connected". No extra checking is needed, and transitory > > connects or disconnects get handled correctly. > > Things are pretty ugly at the moment because the dwc2 interrupt > handler is split in two. There's dwc2_handle_hcd_intr() and > dwc2_handle_common_intr(). Both are registered for the same "shared" > IRQ. If I had to guess, I'd say that this is probably because someone > wanted to assign the ".irq" field in the "struct hc_driver" for the > host controller but that they also needed the other interrupt handler > to handle things shared between host and gadget mode. > > In any case, one of these two interrupt routines handles connect and > the other disconnect. That's pretty ugly but means that you can't > just rely on standard techniques for keeping things in sync. Okay, that is rather weird. Still, I don't see why it should matter. Fundamentally there's no difference between a "connect" interrupt and a "disconnect" interrupt. They should both do exactly the same things: clear the interrupt source, post a "connection change" event, and set the driver's connect status based on the hardware's current state. The second and third parts can be handled by a shared subroutine. If you think of these things as "connect changed" interrupts rather than as "connect" and "disconnect" interrupts, it makes a lot of sense. > It would probably be a pretty reasonable idea to try to clean that up > more, but that would be a very major and intrusive change. Maybe so -- I'll take your word for it since I'm not at all familiar with the dwc2 code. Alan Stern
On Mon, 16 Nov 2015, Julius Werner wrote: > Another point to note, which I think is what prevents the flow Alan > suggested from working, is this little snippet in DWC2's hub_control > GetPortStatus callback: > > if (!hsotg->flags.b.port_connect_status) { > /* > * The port is disconnected, which means the core is > * either in device mode or it soon will be. > Just > * return 0's for the remainder of the port status > * since the port register can't be read if the core > * is in device mode. > */ > *(__le32 *)buf = cpu_to_le32(port_status); > break; > } > > This is before it actually checks the register state of the port. So > it relies on dwc2_hcd_connect() and dwc2_hcd_disconnect() to be called > in the right order to give the correct answer for > USB_PORT_STAT_CONNECTION. This could maybe be improved but I don't > know what kind of weird interactions with device mode operation made > that snippet necessary in the first place. Why does this test hsotg->flags.b.port_connect_status? What it really needs to know is whether the core is in device mode. It should test for that instead. Then it could accurately report the true connection state whenever the core is in host mode, regardless of the order in which dwc2_hcd_connect() and dwc2_hcd_disconnect() get called. No? My guess would be that using the wrong test was simply a misguided attempt at optimization. Alan Stern
Alan, On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 16 Nov 2015, Doug Anderson wrote: > >> Alan, >> >> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Mon, 16 Nov 2015, Doug Anderson wrote: >> > >> >> --- >> >> >> >> usb: dwc2: host: Fix missing device insertions >> >> >> >> If you've got your interrupt signals bouncing a bit as you insert your >> >> USB device, you might end up in a state when the device is connected but >> >> the driver doesn't know it. >> >> >> >> Specifically, the observed order is: >> >> 1. hardware sees connect >> >> 2. hardware sees disconnect >> >> 3. hardware sees connect >> >> 4. dwc2_port_intr() - clears connect interrupt >> >> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() >> >> >> >> Now you'll be stuck with the cable plugged in and no further interrupts >> >> coming in but the driver will think we're disconnected. >> >> >> >> We'll fix this by checking for the missing connect interrupt and >> >> re-connecting after the disconnect is posted. We don't skip the >> >> disconnect because if there is a transitory disconnect we really want to >> >> de-enumerate and re-enumerate. >> > >> > Why do you need to do anything special here? Normally a driver's >> > interrupt handler should query the hardware status after clearing the >> > interrupt source. That way no transitions ever get missed. >> > >> > In your example, at step 5 the dwc2 driver would check the port status >> > and see that it currently is connected. Therefore the driver would >> > pass a "connect status changed" event to the USB core and set the port >> > status to "connected". No extra checking is needed, and transitory >> > connects or disconnects get handled correctly. >> >> Things are pretty ugly at the moment because the dwc2 interrupt >> handler is split in two. There's dwc2_handle_hcd_intr() and >> dwc2_handle_common_intr(). Both are registered for the same "shared" >> IRQ. If I had to guess, I'd say that this is probably because someone >> wanted to assign the ".irq" field in the "struct hc_driver" for the >> host controller but that they also needed the other interrupt handler >> to handle things shared between host and gadget mode. >> >> In any case, one of these two interrupt routines handles connect and >> the other disconnect. That's pretty ugly but means that you can't >> just rely on standard techniques for keeping things in sync. > > Okay, that is rather weird. Still, I don't see why it should matter. > > Fundamentally there's no difference between a "connect" interrupt and a > "disconnect" interrupt. They should both do exactly the same things: > clear the interrupt source, post a "connection change" event, and set > the driver's connect status based on the hardware's current state. > The second and third parts can be handled by a shared subroutine. Ah, sorry I misunderstood. OK, fair enough. So you're saying that the problem is that dwc2_hcd_disconnect() has a line that looks like this: hsotg->flags.b.port_connect_status = 0; ...and the dwc2_port_intr() has a line that looks like this: hsotg->flags.b.port_connect_status = 1; ...and both should just query the status. Do you think we should to block landing this patch on cleaning up how dwc2 handles port_connect_status? I'm not sure what side effects changing port_connect_status will have, so I'll need to test and dig a bit... I'm currently working on trying to fix the microframe scheduler and was planning to post the next series of patches there pretty soon. I'm also planning to keep digging to figure out how to overall increase compatibility with devices (and compatibility with many devices plugged in). If it were up to me, I'd prefer to land this patch in either 4.4 or 4.5 since it does seem to work. ...then put seeing what we can do to cleanup all of the port_connect_status on the todo list. Julius points out in his response that there are comments saying that HPRT0 can't be read in device mode. John: since you're setup to test device mode, can you check if my patch (which now adds a read of HPRT0) will cause problems? Maybe holding this off and keeping it out of the RC is a good idea after all... -Doug
On 11/16/2015 3:14 PM, Doug Anderson wrote: > Alan, > > On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <stern@rowland.harvard.edu> wrote: >> On Mon, 16 Nov 2015, Doug Anderson wrote: >> >>> Alan, >>> >>> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote: >>>> On Mon, 16 Nov 2015, Doug Anderson wrote: >>>> >>>>> --- >>>>> >>>>> usb: dwc2: host: Fix missing device insertions >>>>> >>>>> If you've got your interrupt signals bouncing a bit as you insert your >>>>> USB device, you might end up in a state when the device is connected but >>>>> the driver doesn't know it. >>>>> >>>>> Specifically, the observed order is: >>>>> 1. hardware sees connect >>>>> 2. hardware sees disconnect >>>>> 3. hardware sees connect >>>>> 4. dwc2_port_intr() - clears connect interrupt >>>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() >>>>> >>>>> Now you'll be stuck with the cable plugged in and no further interrupts >>>>> coming in but the driver will think we're disconnected. >>>>> >>>>> We'll fix this by checking for the missing connect interrupt and >>>>> re-connecting after the disconnect is posted. We don't skip the >>>>> disconnect because if there is a transitory disconnect we really want to >>>>> de-enumerate and re-enumerate. >>>> >>>> Why do you need to do anything special here? Normally a driver's >>>> interrupt handler should query the hardware status after clearing the >>>> interrupt source. That way no transitions ever get missed. >>>> >>>> In your example, at step 5 the dwc2 driver would check the port status >>>> and see that it currently is connected. Therefore the driver would >>>> pass a "connect status changed" event to the USB core and set the port >>>> status to "connected". No extra checking is needed, and transitory >>>> connects or disconnects get handled correctly. >>> >>> Things are pretty ugly at the moment because the dwc2 interrupt >>> handler is split in two. There's dwc2_handle_hcd_intr() and >>> dwc2_handle_common_intr(). Both are registered for the same "shared" >>> IRQ. If I had to guess, I'd say that this is probably because someone >>> wanted to assign the ".irq" field in the "struct hc_driver" for the >>> host controller but that they also needed the other interrupt handler >>> to handle things shared between host and gadget mode. >>> >>> In any case, one of these two interrupt routines handles connect and >>> the other disconnect. That's pretty ugly but means that you can't >>> just rely on standard techniques for keeping things in sync. >> >> Okay, that is rather weird. Still, I don't see why it should matter. >> >> Fundamentally there's no difference between a "connect" interrupt and a >> "disconnect" interrupt. They should both do exactly the same things: >> clear the interrupt source, post a "connection change" event, and set >> the driver's connect status based on the hardware's current state. >> The second and third parts can be handled by a shared subroutine. > > Ah, sorry I misunderstood. OK, fair enough. So you're saying that > the problem is that dwc2_hcd_disconnect() has a line that looks like > this: > > hsotg->flags.b.port_connect_status = 0; > > ...and the dwc2_port_intr() has a line that looks like this: > > hsotg->flags.b.port_connect_status = 1; > > ...and both should just query the status. > > > Do you think we should to block landing this patch on cleaning up how > dwc2 handles port_connect_status? I'm not sure what side effects > changing port_connect_status will have, so I'll need to test and dig a > bit... > > I'm currently working on trying to fix the microframe scheduler and > was planning to post the next series of patches there pretty soon. > I'm also planning to keep digging to figure out how to overall > increase compatibility with devices (and compatibility with many > devices plugged in). > > If it were up to me, I'd prefer to land this patch in either 4.4 or > 4.5 since it does seem to work. ...then put seeing what we can do to > cleanup all of the port_connect_status on the todo list. That sound good to me. Though I'd prefer to see this series queued for 4.5 for more testing. > > Julius points out in his response that there are comments saying that > HPRT0 can't be read in device mode. John: since you're setup to test > device mode, can you check if my patch (which now adds a read of > HPRT0) will cause problems? Maybe holding this off and keeping it out > of the RC is a good idea after all... > Yup it's only available in host mode. The same with all the host-mode registers. You will get a ModeMis interrupt if you try to access them in device mode. I gave my test-by on the v2 patches earlier, no problems here. John
John, On Mon, Nov 16, 2015 at 5:53 PM, John Youn <John.Youn@synopsys.com> wrote: > Yup it's only available in host mode. The same with all the > host-mode registers. You will get a ModeMis interrupt if you > try to access them in device mode. > > I gave my test-by on the v2 patches earlier, no problems here. Yup, I appreciate it! I just wanted to confirm that you tested this particular case in gadget mode. ;)
On Mon, 16 Nov 2015, Doug Anderson wrote: > > Fundamentally there's no difference between a "connect" interrupt and a > > "disconnect" interrupt. They should both do exactly the same things: > > clear the interrupt source, post a "connection change" event, and set > > the driver's connect status based on the hardware's current state. > > The second and third parts can be handled by a shared subroutine. > > Ah, sorry I misunderstood. OK, fair enough. So you're saying that > the problem is that dwc2_hcd_disconnect() has a line that looks like > this: > > hsotg->flags.b.port_connect_status = 0; > > ...and the dwc2_port_intr() has a line that looks like this: > > hsotg->flags.b.port_connect_status = 1; > > ...and both should just query the status. Well, I don't know how the driver uses flags.b.port_connect_status. In principle it could do away with that flag completely and always query the hardware status. > Do you think we should to block landing this patch on cleaning up how > dwc2 handles port_connect_status? I'm not sure what side effects > changing port_connect_status will have, so I'll need to test and dig a > bit... > > I'm currently working on trying to fix the microframe scheduler and > was planning to post the next series of patches there pretty soon. > I'm also planning to keep digging to figure out how to overall > increase compatibility with devices (and compatibility with many > devices plugged in). > > If it were up to me, I'd prefer to land this patch in either 4.4 or > 4.5 since it does seem to work. ...then put seeing what we can do to > cleanup all of the port_connect_status on the todo list. It's up to you guys. All I've been doing here is pointing out that your proposed approach didn't seem like the best. Alan Stern
Alan, On Tue, Nov 17, 2015 at 7:40 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 16 Nov 2015, Doug Anderson wrote: > >> > Fundamentally there's no difference between a "connect" interrupt and a >> > "disconnect" interrupt. They should both do exactly the same things: >> > clear the interrupt source, post a "connection change" event, and set >> > the driver's connect status based on the hardware's current state. >> > The second and third parts can be handled by a shared subroutine. >> >> Ah, sorry I misunderstood. OK, fair enough. So you're saying that >> the problem is that dwc2_hcd_disconnect() has a line that looks like >> this: >> >> hsotg->flags.b.port_connect_status = 0; >> >> ...and the dwc2_port_intr() has a line that looks like this: >> >> hsotg->flags.b.port_connect_status = 1; >> >> ...and both should just query the status. > > Well, I don't know how the driver uses flags.b.port_connect_status. In > principle it could do away with that flag completely and always query > the hardware status. > >> Do you think we should to block landing this patch on cleaning up how >> dwc2 handles port_connect_status? I'm not sure what side effects >> changing port_connect_status will have, so I'll need to test and dig a >> bit... >> >> I'm currently working on trying to fix the microframe scheduler and >> was planning to post the next series of patches there pretty soon. >> I'm also planning to keep digging to figure out how to overall >> increase compatibility with devices (and compatibility with many >> devices plugged in). >> >> If it were up to me, I'd prefer to land this patch in either 4.4 or >> 4.5 since it does seem to work. ...then put seeing what we can do to >> cleanup all of the port_connect_status on the todo list. > > It's up to you guys. All I've been doing here is pointing out that > your proposed approach didn't seem like the best. Thanks! Just wanted to make sure you know that I'm very very appreciative of your reviews and suggestions here. Having someone intimately familiar with how other USB host drivers work that's willing to point out how dwc2 can do things better will be very helpful in helping dwc2 grow. -Doug
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 571c21727ff9..967d1e686efe 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) */ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) { + u32 hprt0; u32 intr; /* Set status flags for the hub driver */ @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) dwc2_hcd_cleanup_channels(hsotg); dwc2_host_disconnect(hsotg); + + hprt0 = dwc2_readl(hsotg->regs + HPRT0); + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) { + if (hsotg->lx_state != DWC2_L0) + usb_hcd_resume_roothub(hsotg->priv); + + hsotg->flags.b.port_connect_status_change = 1; + hsotg->flags.b.port_connect_status = 1; + } } /**