Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] usb:hub set hub->change_bits when over-current happens

13 views
Skip to first unread message

沈光

unread,
Jan 6, 2014, 9:40:02 PM1/6/14
to
set hub->change_bits when we plug in a device which causes
over-current condition, so that hub_events() will check it.

Signed-off-by: Shen Guang <sheng...@gmail.com>
---
drivers/usb/core/hub.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index bd9dc35..98b5679 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1154,7 +1154,8 @@ static void hub_activate(struct usb_hub *hub,
enum hub_activation_type type)
/* Tell khubd to disconnect the device or
* check for a new connection
*/
- if (udev || (portstatus & USB_PORT_STAT_CONNECTION))
+ if (udev || (portstatus & USB_PORT_STAT_CONNECTION) ||
+ (portstatus & USB_PORT_STAT_OVERCURRENT))
set_bit(port1, hub->change_bits);

} else if (portstatus & USB_PORT_STAT_ENABLE) {
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Greg KH

unread,
Jan 6, 2014, 9:50:02 PM1/6/14
to
On Tue, Jan 07, 2014 at 10:33:14AM +0800, 沈光 wrote:
> set hub->change_bits when we plug in a device which causes
> over-current condition, so that hub_events() will check it.

Why?

What does this solve? Is this a bug with existing devices that needs to
be backported to older kernels?

thanks,

greg k-h

沈光

unread,
Jan 6, 2014, 10:40:02 PM1/6/14
to
On Tue, Jan 7, 2014 at 10:40 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Tue, Jan 07, 2014 at 10:33:14AM +0800, 沈光 wrote:
>> set hub->change_bits when we plug in a device which causes
>> over-current condition, so that hub_events() will check it.
>
> Why?
>
> What does this solve? Is this a bug with existing devices that needs to
> be backported to older kernels?
>
> thanks,
>
> greg k-h

This is something that we found when we are doing compliance test with xHCI.
If we enable CONFIG_USB_SUSPEND, and plug in a bad device which causes
over-current condition to the root port, the hub->change_bits will not
be set in current code in the function of hub_activate.
Then hub_events() will ignore this change, below is a code fragment of
hub_events()
/* deal with port status changes */
for (i = 1; i <= hub->descriptor->bNbrPorts; i++) {
if (test_bit(i, hub->busy_bits))
continue;
connect_change = test_bit(i, hub->change_bits);
wakeup_change = test_and_clear_bit(i, hub->wakeup_bits);
if (!test_and_clear_bit(i, hub->event_bits) &&
!connect_change && !wakeup_change)
continue;

According to spec, software need to disable the port if there is an
over-current condition, while current code will not, since it will not
detect such condition.

but if we disable CONFIG_USB_SUSPEND, software can detect the
over-current condition. I am still checking the code to get the
reason.

--
Shen Guang

Greg KH

unread,
Jan 6, 2014, 11:00:01 PM1/6/14
to
On Tue, Jan 07, 2014 at 11:35:50AM +0800, 沈光 wrote:
> On Tue, Jan 7, 2014 at 10:40 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> > On Tue, Jan 07, 2014 at 10:33:14AM +0800, 沈光 wrote:
> >> set hub->change_bits when we plug in a device which causes
> >> over-current condition, so that hub_events() will check it.
> >
> > Why?
> >
> > What does this solve? Is this a bug with existing devices that needs to
> > be backported to older kernels?
> >
> > thanks,
> >
> > greg k-h
>
> This is something that we found when we are doing compliance test with xHCI.
> If we enable CONFIG_USB_SUSPEND, and plug in a bad device which causes
> over-current condition to the root port, the hub->change_bits will not
> be set in current code in the function of hub_activate.

Now that's something that should go in the changelog, don't you think?

thanks,

greg k-h

Shen Guang

unread,
Jan 7, 2014, 1:40:02 AM1/7/14
to
On Tue, Jan 7, 2014 at 11:53 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Tue, Jan 07, 2014 at 11:35:50AM +0800, 沈光 wrote:
>> On Tue, Jan 7, 2014 at 10:40 AM, Greg KH <gre...@linuxfoundation.org> wrote:
>> > On Tue, Jan 07, 2014 at 10:33:14AM +0800, 沈光 wrote:
>> >> set hub->change_bits when we plug in a device which causes
>> >> over-current condition, so that hub_events() will check it.
>> >
>> > Why?
>> >
>> > What does this solve? Is this a bug with existing devices that needs to
>> > be backported to older kernels?
>> >
>> > thanks,
>> >
>> > greg k-h
>>
>> This is something that we found when we are doing compliance test with xHCI.
>> If we enable CONFIG_USB_SUSPEND, and plug in a bad device which causes
>> over-current condition to the root port, the hub->change_bits will not
>> be set in current code in the function of hub_activate.
>
> Now that's something that should go in the changelog, don't you think?
>
> thanks,
>
> greg k-h

Sure, I agree.
I am sorry I didn't make it clear enough.
And the reason why over-current can be detected if CONFIG_USB_SUSPEND
is disabled, is that the interrupt pipe of the hub will report the
change and set hub->event_bits, and then hub_events() will check what
events happened.

Thanks,

Shen Guang

Greg KH

unread,
Jan 7, 2014, 10:30:02 AM1/7/14
to
Great, care to resend the patch with all of this information added to
it?

thanks,

greg k-h

Shen Guang

unread,
Jan 8, 2014, 1:50:02 AM1/8/14
to
When we are doing compliance test with xHCI, we found that if we
enable CONFIG_USB_SUSPEND and plug in a bad device which causes
over-current condition to the root port, software will not be noticed.
The reason is that current code don't set hub->change_bits in
hub_activate() when over-current happens, and then hub_events() will
not check the port status because it thinks nothing changed.
If CONFIG_USB_SUSPEND is disabled, the interrupt pipe of the hub will
report the change and set hub->event_bits, and then hub_events() will
check what events happened.In this case over-current can be detected.

Signed-off-by: Shen Guang <sheng...@gmail.com>
---
drivers/usb/core/hub.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index bd9dc35..98b5679 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1154,7 +1154,8 @@ static void hub_activate(struct usb_hub *hub,
enum hub_activation_type type)
/* Tell khubd to disconnect the device or
* check for a new connection
*/
- if (udev || (portstatus & USB_PORT_STAT_CONNECTION))
+ if (udev || (portstatus & USB_PORT_STAT_CONNECTION) ||
+ (portstatus & USB_PORT_STAT_OVERCURRENT))
set_bit(port1, hub->change_bits);

} else if (portstatus & USB_PORT_STAT_ENABLE) {
--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Greg KH

unread,
Jan 8, 2014, 12:20:02 PM1/8/14
to
Alan and Sarah, any objection to this patch?

thanks,

gre k-h

Alan Stern

unread,
Jan 8, 2014, 1:00:02 PM1/8/14
to
It seems okay to me.

Acked-by: Alan Stern <st...@rowland.harvard.edu>

Sarah Sharp

unread,
Jan 8, 2014, 1:20:01 PM1/8/14
to
Looks fine to me as well.

Acked-by: Sarah Sharp <sarah....@linux.intel.com>

Yuvaraj Cd

unread,
Mar 19, 2014, 9:10:02 AM3/19/14
to
With this patch,I can see usb 3.0 device detection is failing on
exynos5250-smdk5250 boards.
CONFIG_USB_SUSPEND=n

/ $ [ 11.486922] hub 2-0:1.0: connect-debounce failed, port 1 disabled
[ 13.891919] hub 2-0:1.0: connect-debounce failed, port 1 disabled
[ 16.296919] hub 2-0:1.0: connect-debounce failed, port 1 disabled
[ 18.701919] hub 2-0:1.0: connect-debounce failed, port 1 disabled
[ 21.106919] hub 2-0:1.0: connect-debounce failed, port 1 disabled
[ 23.511918] hub 2-0:1.0: connect-debounce failed, port 1 disabled
[ 25.916918] hub 2-0:1.0: connect-debounce failed, port 1 disabled

When I revert this patch, device detection works fine.Anyone have come
across the same on other platforms?
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
0 new messages