From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:38344) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QKS1B-0006oT-LE for qemu-devel@nongnu.org; Thu, 12 May 2011 05:17:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QKS1A-0003ME-Hs for qemu-devel@nongnu.org; Thu, 12 May 2011 05:17:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38641) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QKS1A-0003MA-9N for qemu-devel@nongnu.org; Thu, 12 May 2011 05:17:12 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p4C9HBMU021536 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 12 May 2011 05:17:11 -0400 Message-ID: <4DCBA594.9050305@redhat.com> Date: Thu, 12 May 2011 11:17:08 +0200 From: Gerd Hoffmann MIME-Version: 1.0 References: <1305023443-8722-1-git-send-email-kraxel@redhat.com> <1305023443-8722-2-git-send-email-kraxel@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] usb-linux: fix device path aka physical port handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On 05/11/11 10:52, Markus Armbruster wrote: > Good stuff, just a few questions. > > Gerd Hoffmann writes: > >> The device path isn't just a number. It specifies the physical port >> the device is connected to and in case the device is connected via >> usb hub you'll have two numbers there, like this: "5.1". The first >> specifies the root port where the hub is plugged into, the second >> specifies the port number of the hub where the device is plugged in. >> With multiple hubs chained the string can become longer. > > "5.1"? Do you mean "5-1"? No. > snprintf(dev->devpath, sizeof dev->devpath, > "%s.%d", parent->devpath, port1); ^^^^^ > $ ls /sys/bus/usb/devices > 1-0:1.0 2-0:1.0 3-0:1.0 4-0:1.0 5-0:1.0 usb1 usb2 usb3 usb4 usb5 Boring, nothing plugged in here ;) Laptop docking stations often have a usb hub built in. If you have one place your laptop there, then connect a usb device to one of the ports of the docking station. You'll see a file named like this: 1-5.3 where "1" is the bus number, "5" is the port number of the root hub and "3" is the port number of the docking station hub. >> @@ -79,6 +79,7 @@ typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath, >> #define USBPROCBUS_PATH "/proc/bus/usb" >> #define PRODUCT_NAME_SZ 32 >> #define MAX_ENDPOINTS 15 >> +#define MAX_PORTLEN 8 > > Where does 8 come from? Pulled out of thin air. Should be enougth, you can build chains with three usb hubs (anyone ever did in practice? did the devices still work?), getting port addresses like "1.2.3.4", and it still fits in. > For what it's worth, kernel's struct usb_device > has char devpath[16]. We can make that 16 too to be on the really safe side. >> @@ -1672,8 +1674,8 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func) >> if (!strncmp(de->d_name, "usb", 3)) { >> tmpstr += 3; >> } > > Sloppy parsing, but that's not your fault. I think this can be zapped now, the new sscanf will fail on them and skip the entries anyway. >> - if (sscanf(tmpstr, "%d-%d",&bus_num,&devpath)< 1) { >> - goto the_end; >> + if (sscanf(tmpstr, "%d-%7[0-9.]",&bus_num, port)< 2) { >> + continue; >> } >> >> if (!usb_host_read_file(line, sizeof(line), "devnum", de->d_name)) { > > The old sscanf() succeeds if at least one item is assigned, i.e. tmpstr > starts with an integer. I suspect this is broken for roots. Consider > d_name "usb1": tmpstr is "1", sscan() returns 1, and devpath remains > uninitialized. It's passed to the func() callback. Bug? If yes, the > commit message should mention it. Indeed. > The new sscan() succeeds only if both items are assigned, i.e. tmpstr > starts with an integer, '-', and up to 7 of [0-9.]. Unlike the old one, > it fails for roots. Intentional? Yes, now the roots are skipped. You can't assign them anyway, so it is pretty pointless to loom at them and to list them in "info usbhost". cheers, Gerd