From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chun Yan Liu Subject: Re: [PATCH V7 3/7] libxl: add pvusb API Date: Wed, 14 Oct 2015 10:29:45 +0800 Message-ID: <561DBE19.7090909@novell.com> References: <1443147102-6471-1-git-send-email-cyliu@suse.com> <1443147102-6471-4-git-send-email-cyliu@suse.com> <560C2204.9030707@citrix.com> <561BCF8B0200006600072938@relay2.provo.novell.com> <561BB9A6.10100@citrix.com> <561C6264.6090507@novell.com> <561D03F4.5020901@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <561D03F4.5020901@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap , Chun Yan Liu , xen-devel@lists.xen.org Cc: Juergen Gross , wei.liu2@citrix.com, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org On 10/13/2015 09:15 PM, George Dunlap wrote: > On 13/10/15 02:46, Chun Yan Liu wrote: >> >> On 10/12/2015 09:46 PM, George Dunlap wrote: >>> On 12/10/15 08:19, Chun Yan Liu wrote: >>>>>> + >>>>>> + usbinfo->devnum = usb->u.hostdev.hostaddr; >>>>>> + usbinfo->busnum = usb->u.hostdev.hostbus; >>>>>> + >>>>>> + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, >>>>>> + usb->u.hostdev.hostaddr); >>>>>> + if (!busid) { >>>>>> + rc = ERROR_FAIL; >>>>>> + goto out; >>>>>> + } >>>>>> + >>>>>> + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid); >>>>>> + if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL)) >>>>>> + sscanf(buf, "%" SCNx16, &usbinfo->idVendor); >>>>>> + >>>>>> + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid); >>>>>> + if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL)) >>>>>> + sscanf(buf, "%" SCNx16, &usbinfo->idProduct); >>>>>> + >>>>>> + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid); >>>>>> + if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, >>>>>> &buflen) && >>>>>> + buflen > 0) { >>>>>> + /* replace \n to \0 */ >>>>>> + if (((char *)buf)[buflen - 1] == '\n') >>>>>> + ((char *)buf)[buflen - 1] = '\0'; >>>>>> + usbinfo->manuf = libxl__strdup(NOGC, buf); >>>>>> + } >>>>>> + >>>>>> + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid); >>>>>> + if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, >>>>>> &buflen) && >>>>>> + buflen > 0) { >>>>>> + /* replace \n to \0 */ >>>>>> + if (((char *)buf)[buflen - 1] == '\n') >>>>>> + ((char *)buf)[buflen - 1] = '\0'; >>>>>> + usbinfo->prod = libxl__strdup(NOGC, buf); >>>>>> + } >>>>> Basically, starting here, we have information which won't be >>>>> available >>>>> if we're using a pvusb driver domain. >>>>> This information is nice-to-have, but I don't think this >>>>> information is >>>>> directly relevant to libxl or xl; the funcitonality to get this >>>>> information is available from other libraries like libusb. I'm >>>>> inclined >>>>> to say that if we want to have pvusb driver domains (and I think we >>>>> do), >>>>> we should just get rid of this information. >>>> For command 'xl usb-list', those information should be reported to user. >>>> Do you mean we could call libusb to get thoes information directly in >>>> xl toolstack and get rid of this function? >>>> >>>> I think we can keep the function, since every other device type has the >>>> function XXX_getinfo. But here we could check backend_domid, for >>>> backend=dom0, doing above work; for backend!=dom0 (e.g. pvusb >>>> driver domain, no matter how, it should also be able to let users >>>> getting >>>> those information. Can add code in future.) >>> Does xl need that information? Can't the user get that information from >>> lsusb? >>> >>> In any case, I can see why it would be *useful* to have in xl. But >>> about having it in libxl, I think this question sort of goes along with >>> the question about the next patch -- whether libxl should be in the >>> business of providing general information about the USB devices it's >>> handling, or whether it should limit itself to doing what is absolutely >>> necessary to talk to usbback. >>> >>> There's a part of me that sees the point of it: it's not actually that >>> much extra code (at least for Linux), and it makes it easy to add some >>> very useful features to xl. >>> >>> On the other hand, it's not portable to other OSes. At the moment of >>> course pvusb isn't portable either, but once we have qemu USB (providing >>> either emulated or PV usb) then I *think* most of the other >>> functionality will Just Work on any platform that can compile qemu >>> (incl. FreeBSD, NetBSD, &c), won't it? The code you're introducing here >>> would have to be re-implented for those platforms, and for every new >>> platform that wanted to include this functionality, wouldn't it? >> So, about the portability problem, I think it's back to: do need to >> update code to call libusb instead of reading sysfs now? Except >> for this function, still have places reading sysfs to get hostbus, >> hostaddr, vendorId, deviceId. Those are not portable for other >> platform. > I realize I didn't give you very clear guidance; I guess I was hoping to > get an opinion from the tools maintainers. Or perhaps, I was hoping to > let them be the "bad guys" and say, "You can't have this feature in > libxl", so I wouldn't have to. :-) > > In the absence of guidance to the contrary, I suggest that patch series > should focus on getting the core pvusb functionality in, without the > extra usb-querying bits. Then we can discuss a further series which > either adds the usb querying functionality to libxl, or implement it in > xl using libusb. OK. Got it. Thanks. -Chunyan > > -George >