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

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

[沈光]

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 <shenguang10@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

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

[Greg KH]

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

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

[沈光]

On Tue, Jan 7, 2014 at 10:40 AM, Greg KH <gregkh@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

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

[Greg KH]

On Tue, Jan 07, 2014 at 11:35:50AM +0800, 沈光 wrote:
> On Tue, Jan 7, 2014 at 10:40 AM, Greg KH <gregkh@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

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

[Shen Guang]

On Tue, Jan 7, 2014 at 11:53 AM, Greg KH <gregkh@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 <gregkh@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

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

[Greg KH]

On Tue, Jan 07, 2014 at 02:38:36PM +0800, Shen Guang wrote:
> On Tue, Jan 7, 2014 at 11:53 AM, Greg KH <gregkh@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 <gregkh@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.

Great, care to resend the patch with all of this information added to
it?

thanks,

greg k-h

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

[Shen Guang]

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 <shenguang10@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
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg KH]

On Wed, Jan 08, 2014 at 02:45:42PM +0800, Shen Guang wrote:
> 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 <shenguang10@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

Alan and Sarah, any objection to this patch?

thanks,

gre k-h

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

[Alan Stern]

On Wed, 8 Jan 2014, Greg KH wrote:

> On Wed, Jan 08, 2014 at 02:45:42PM +0800, Shen Guang wrote:
> > 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 <shenguang10@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
> 
> Alan and Sarah, any objection to this patch?

It seems okay to me.

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

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

[Sarah Sharp]

On Wed, Jan 08, 2014 at 12:49:57PM -0500, Alan Stern wrote:
> On Wed, 8 Jan 2014, Greg KH wrote:
> 
> > On Wed, Jan 08, 2014 at 02:45:42PM +0800, Shen Guang wrote:
> > > 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 <shenguang10@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
> > 
> > Alan and Sarah, any objection to this patch?
> 
> It seems okay to me.
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Looks fine to me as well.

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

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

[Yuvaraj Cd]

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?

On Wed, Jan 8, 2014 at 11:49 PM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Wed, Jan 08, 2014 at 12:49:57PM -0500, Alan Stern wrote:
>> On Wed, 8 Jan 2014, Greg KH wrote:
>>
>> > On Wed, Jan 08, 2014 at 02:45:42PM +0800, Shen Guang wrote:
>> > > 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 <shenguang10@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
>> >
>> > Alan and Sarah, any objection to this patch?
>>
>> It seems okay to me.
>>
>> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>
> Looks fine to me as well.
>
> Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html