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

[Stefan Roese <sr@denx.de>]

Hi Stephan,

On 14.03.2016 18:31, Stephen Warren wrote:
> On 03/14/2016 04:18 AM, Stefan Roese wrote:
>> This patch changes the USB port scanning procedure and timeout
>> handling in the following ways:
> 
> A few nits/typos in the description, and some review comments below.

Thanks, will update the commit text in v4.

<snip>
 
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> 
>> @@ -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. Currently, if 
> pgood_delay<=100 (is that legal?) then the delta might be as little as 
> 900ms (for pgood_delay==0).

Fixed.
 
>> 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?

Multiple controllers/hubs should be supported currently (at least
in my tests). Its the parallel scanning of those controllers which
might be problematic. usb_hub.c needs some general rework, as it
has some more globals:

/* TODO(sjg at chromium.org): Remove this when CONFIG_DM_USB is defined */
static struct usb_hub_device hub_dev[USB_MAX_HUB];
static int usb_hub_index;

I've moved "usb_scan_list" to these declarations for now, so that
all of them can be cleaned up once somebody works on task.
 
>> +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?

Okay. I've added a timeout check here to remove such a non-functional
device.
 
>> +    /* 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.

Good catch. Changed in v4.
 
>> +        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.

Changed in v4.
 
>> +        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.

Changed in v4.
 
>> +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.

Its called recursively, if hubs are stacked. So its called e.g.
5 times in this setup:

=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
  |  u-boot EHCI Host Controller 
  |
  +-2  Hub (480 Mb/s, 0mA)
    |
    +-3  Hub (480 Mb/s, 100mA)
    | |
    | +-7  Hub (12 Mb/s, 100mA)
    |    
    +-4  Hub (480 Mb/s, 0mA)
    | |
    | +-8  Mass Storage (480 Mb/s, 200mA)
    | |    Kingston DataTraveler 2.0 50E549C688C4BE7189942766
    | |  
    | +-9  Mass Storage (480 Mb/s, 98mA)
    |      USBest Technology USB Mass Storage Device 09092207fbf0c4
    |    
    +-5  Mass Storage (480 Mb/s, 200mA)
    |    6989 Intenso Alu Line EE706F5E
    |  
    +-6  Mass Storage (480 Mb/s, 200mA)
         JetFlash Mass Storage Device 3281440601
 
>>   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?

Correct. I already noticed that the malloc is missing the free part.
Its added in v4.

Thanks for the review,
Stefan