From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Mon, 14 Mar 2016 20:47:34 +0100 Subject: [U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling In-Reply-To: <56E6F555.5020807@wwwdotorg.org> References: <1457950693-564-1-git-send-email-sr@denx.de> <1457950693-564-5-git-send-email-sr@denx.de> <56E6F555.5020807@wwwdotorg.org> Message-ID: <56E71556.7010003@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 14-03-16 18:31, Stephen Warren wrote: > On 03/14/2016 04:18 AM, Stefan Roese wrote: >> @@ -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