[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Hans de Goede]

Hi,

On 14-03-16 18:31, Stephen Warren wrote:
> On 03/14/2016 04:18 AM, Stefan Roese wrote:

<snip>

>> @@ -120,7 +121,21 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>>           pgood_delay = max(pgood_delay,
>>                         (unsigned)simple_strtol(env, NULL, 0));
>>       debug("pgood_delay=%dms\n", pgood_delay);
>> -    mdelay(pgood_delay + 1000);
>> +
>> +    /*
>> +     * Do a minimum delay of the larger value of 100ms or pgood_delay
>> +     * so that the power can stablize before the devices are queried
>> +     */
>> +    dev->query_delay = get_timer(0) + max(100, (int)pgood_delay);
>> +
>> +    /*
>> +     * Record the power-on timeout here. The max. delay (timeout)
>> +     * will be done based on this value in the USB port loop in
>> +     * usb_hub_configure() later.
>> +     */
>> +    dev->connect_timeout = get_timer(0) + pgood_delay + 1000;
>
> I'd be tempted to make that:
>
> dev->connect_timeout = dev->query_delay + 1000;
>
> That way, if the max() used in the calculation of dev->query_delay was dominated by the 100 not the pgood_delay, then we still get a 1000ms time for the device to connect after the power is stable.

Ack, good catch.

> Currently, if pgood_delay<=100 (is that legal?) then the delta might be as little as 900ms (for pgood_delay==0).

AFAIK it is not legal, but guess what the firmware authors of cheap hubs put in the
descriptor when 0 seems to work just fine ?

Rule of thumb: Never trust usb descriptors.

Regards,

Hans



>
>> static LIST_HEAD(usb_scan_list);
>
> You could put that onto the stack in usb_hub_configure() and pass it as a parameter to usb_device_list_scan(). That would avoid some globals, which might make it easier to apply this technique across multiple controllers/hubs/... in the future?
>
>> +static int usb_scan_port(struct usb_device_scan *usb_scan)
>
>> +    ret = usb_get_port_status(dev, i + 1, portsts);
>> +    if (ret < 0) {
>> +        debug("get_port_status failed\n");
>> +        return 0;
>
> Shouldn't this cause a timeout eventually if it repeats forever?
>
>> +    /* Test if the connection came up, and if so, exit. */
>> +    if (portstatus & USB_PORT_STAT_CONNECTION) {
>
> If you invert that test, you can return early and remove and indentation level from the rest of the function.
>
>> +        debug("devnum=%d port=%d: USB dev found\n", dev->devnum, i + 1);
>> +        /* Remove this device from scanning list */
>> +        list_del(&usb_scan->list);
>> +
>> +        /* A new USB device is ready at this point */
>> +
>> +        debug("Port %d Status %X Change %X\n",
>> +              i + 1, portstatus, portchange);
>
> It might be nice to print this a little earlier (outside the USB_PORT_STAT_CONNECTION if block) so that any USB_PORT_STAT_C_CONNECTION triggers the print, not just if C_CONNECTION && CONNECTION. That might help debug, and match the existing code.
>
>> +        if (portchange & USB_PORT_STAT_C_CONNECTION) {
>  > +            debug("port %d connection change\n", i + 1);
>  > +            usb_hub_port_connect_change(dev, i);
>  > +        }
>
> That test is always true, or the function would have returned earlier.
>
>> +static int usb_device_list_scan(void)
> ...
>> +    static int running;
>> +    int ret = 0;
>> +
>> +    /* Only run this loop once for each controller */
>> +    if (running)
>> +        return 0;
>> +
>> +    running = 1;
> ...
>> +out:
>> +    /*
>> +     * This USB controller has has finished scanning all its connected
>> +     * USB devices. Set "running" back to 0, so that other USB controllers
>> +     * will scan their devices too.
>> +     */
>> +    running = 0;
>
> Doesn't this function only get called a single time from usb_hub_configure()? If so, I don't think the "running" variable is required.
>
>>   static int usb_hub_configure(struct usb_device *dev)
>
>> +    for (i = 0; i < dev->maxchild; i++) {
>> +        struct usb_device_scan *usb_scan;
>>
>> +        usb_scan = calloc(1, sizeof(*usb_scan));
>> +        if (!usb_scan) {
>> +            printf("Can't allocate memory for USB device!\n");
>> +            return -ENOMEM;
>
> I think there should be some resource cleanup for the previously allocated list entries here. Perhaps that's what you meant in your other email where you talked about some missing free()s?
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot