On Thu, 14 Feb 2002, Greg KH wrote:
> > +static int usb_hub_port_status(struct usb_device *hub, int port,
> > + unsigned short *status, unsigned short *change)
>
> status and change should both be u16 types, right?
Well, I've used the same type which was used by the callers. They all have
unsigned short x = le16_to_cpu(y);
So I stayed with this. Note, the portstatus we read from the hub, i.e. y
in this example, is u16. Right, le16_to_cpu() returns u16, but are there
any platforms where assigning an u16 to short unsigned would be a problem?
If so, there is a lot more to fix...
> > + int ret = -1;
>
> Should be:
> int ret = -ENOMEM;
Agreed, would be more specific. OTOH I tried to let things look similar to
other helper routines there - which use -1/0/1 return values to indicate
error/success/disconnect.
> > +/* return: -1 on error, 0 on success, 1 on disconnect. */
>
> As no one is checking for the return value of 1, and a disconnect is
> still an error, shouldn't the code be the following:
>
> static int usb_hub_port_debounce(struct usb_device *hub, int port)
> {
> int delay_time;
> int ret;
> u16 portchange;
> u16 portstatus;
>
> for (delay_time = 0; delay_time < HUB_DEBOUNCE_TIMEOUT; /* empty */ ) {
> /* wait for debounce and for device getting stable buspower */
> wait_ms(HUB_DEBOUNCE_STEP);
>
> ret = usb_hub_port_status(hub, port, &portstatus, &portchange);
> if (ret)
> return ret;
>
> if ((portchange & USB_PORT_STAT_C_CONNECTION)) {
> usb_clear_port_feature(hub, port+1,
>USB_PORT_FEAT_C_CONNECTION);
> delay_time = 0;
> }
> else
> delay_time += HUB_DEBOUNCE_STEP;
> }
> return ((portstatus&USB_PORT_STAT_CONNECTION)) ? 0 : -ENODEV;
> }
Same point: yes, but other helpers use -1/0/1 pattern. And in some cases
the return value is checked for disconnect. Sure, one could change this to
check for -ENODEV instead of 1 - but simple <0 -> error wouldn't work
anymore if disconnect need special care. And I wouldn't say disconnect is
an error (like ENOMEM f.e.) here: basically the hub should work a like
state machine where different events (connect, disconnect, timers ...)
trigger state transition. Hence the disconnects needs to be treated as a
normal event there, IMHO.
> And what's the odds that we can get stuck in this loop for a very noisy
> connection?
Good point. Since this is executed by khubd we would end up looping there
around without making any progress and khubd becomes unuseable. However, I
don't see a good way to avoid this: Sure, we could add some unconditional
timeout (say 1 sec) and bail out with error if it ever happens. But what
happens is the host would immediately detect a new connect and we would
just re-enter the loop :-(
Ok, we could try to switch off the power for the port - but not all types
of hubs are required to support power switching for individual ports, If
so the port would stay powered unless all other ports of the gang are/get
unpowered as well.
And finally, looping there without making progress is IMHO only a minor
issue. If your USB is that noisy (or the device completely broken) things
are unuseable anyway - so the livelocked khubd isn't a big loss. And all
the user has to do to solve the situation is to disconnect the bad guy!
As we are at the delays: The specs say the debounce delay should be 100ms,
restarted on disconnect. For my USB_IrDA dongle I needed 150-200ms to make
it really happy. I was just informed by one of the Brainboxes Bluetooth
users, his dongle started working once he tried with 400ms. And there is
already this RH-bug workaraound with a 400ms delay for low-speed devices.
Shouldn't we just drop this low-speed delay and always use 400 or 500 ms
debounce delay? The delay does only matter during connect - hence 500ms
instead of 100 doesn't really hurt and I'd bet a lot of non-conforming but
existing devices would become useable this way.
Martin
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
