On Sun, 27 Feb 2011, Paul Bolle wrote:
> On a laptop I see these errors on (most) resumes:
> hub 3-0:1.0: over-current change on port 1
> hub 3-0:1.0: over-current change on port 2
>
> Since over-current conditions can disappear quite quickly it's better to
> downgrade that message to debug level, recheck for an over-current
> condition a little later and only treat an over-current condition as an
> error if it still exists when it's rechecked.
>
> Signed-off-by: Paul Bolle <pebolle@xxxxxxxxxx>
> ---
> This seems to do the trick. Note that this patch took some (educated)
> guesswork about the kernel's USB API, so a review would appreciated.
This is a good start, but not quite right.
> Also note that hub over-current changes are currently handled rather
> differently. Is there a reason to treat these events differently?
They probably should be treated the same. Perhaps a single subroutine
could handle both, or perhaps it would be too awkward -- I'm not sure.
> drivers/usb/core/hub.c | 19 +++++++++++++++++--
> 1 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index d041c68..2309600 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3412,12 +3412,27 @@ static void hub_events(void)
> }
>
> if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
> - dev_err (hub_dev,
> + u16 status;
> + u16 unused;
> +
> + dev_dbg (hub_dev,
Don't copy the formatting errors of existing code. Get rid of the
space before the '('.
> "over-current change on port %d\n",
> i);
> clear_port_feature(hdev, i,
> USB_PORT_FEAT_C_OVER_CURRENT);
> - hub_power_on(hub, true);
According to the USB-2.0 spec, an over-current condition is supposed to
cause the hub to turn off power to the affected port. Hence the
hub_power_on() call is always necessary; it can't be skipped. But
maybe it should go after the msleep(), giving the electronics a little
more time to recover from transient problems.
Also, the clear_port_feature() call should go at the end. It may not
matter -- but the spec says that the C_PORT_OVER_CURRENT status bit
gets set whenever PORT_OVER_CURRENT changes from 0 to 1 or from 1 to 0.
In addition, I'm not at all sure that the port status will be
meaningful while the port power is turned off. This suggests that the
hub_power_on() call belongs between the msleep() and the
hub_port_status().
> + msleep(100); /* Cool down */
> +
> + ret = hub_port_status(hub, i, &status,
> + &unused);
> + if (ret < 0)
> + continue;
This seems like a bad idea. I'd forget about ret; just set status to 0
before calling hub_port_status().
> + if (status & USB_PORT_STAT_OVERCURRENT) {
> + dev_err (hub_dev,
> + "over-current condition on port "
> + "%d\n", i);
> + hub_power_on(hub, true);
At this point we're stuck. We don't want to turn the port power back
on until the over-current condition is fixed, but we don't know when
that will occur. It's probably safest to do nothing here.
> + }
> }
>
> if (portchange & USB_PORT_STAT_C_RESET) {
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
