From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chun Yan Liu" Subject: Re: [PATCH V8 3/7] libxl: add pvusb API Date: Tue, 10 Nov 2015 19:37:18 -0700 Message-ID: <56431A5E0200006600080D99@relay2.provo.novell.com> References: <1445418510-19614-1-git-send-email-cyliu@suse.com> <1445418510-19614-4-git-send-email-cyliu@suse.com> <22080.57829.461049.37192@mariner.uk.xensource.com> <56421E2B020000660008019B@relay2.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline 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 Cc: Juergen Gross , Wei Liu , Ian Campbell , Ian Jackson , "xen-devel@lists.xen.org" , Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org >>> On 11/11/2015 at 01:57 AM, in message , George Dunlap wrote: > On Tue, Nov 10, 2015 at 8:41 AM, Chun Yan Liu wrote: > >> > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, > >> > + libxl_device_usbctrl *usbctrl, > >> > + libxl__ao_device *aodev) > >> > +{ > >> > >> Thanks for adjusting the error-handling patterns in these functions. > >> The new way is good, except that: > >> > >> > +out: > >> > + aodev->rc = rc; > >> > + if (rc) aodev->callback(egc, aodev); > >> > >> Here, rc is always set, and indeed the code would be wrong if it were > >> not. So can you remove the conditional please ? Ie: > > > > Reading the codes, libxl__wait_device_connection will call aodev->callback > > properly. So here, only if (rc != 0), that means error happens, then we > need to > > call aodev->callback to end the process. (Refer to current > libxl__device_disk_add, > > all current code does similar work.) So I think the code is not wrong (?) > > >> > +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid, > >> > + uint8_t *bus, uint8_t *addr) > >> > +{ > >> > + char *filename; > >> > + void *buf; > >> > + > >> > + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid); > >> > + if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL)) > >> > + *bus = atoi((char *)buf); > >> > >> I don't think this cast (and the one for addr) are necessary ? > > > > Which cast? Here, we want to get a uint_8 value, but buf is a string, > > we need to do atoi. > > atoi() isn't a cast, it's a function call. The cast is the (char *) bit. > > I think probably you could get away with declaring buf as (char *). > &buf should be converted to void** automatically without any warnings, > I think. It will report warning if declaring char *buf. > > >> > +/* Encode usb interface so that it could be written to xenstore as a key. > >> > + * > >> > + * Since xenstore key cannot include '.' or ':', we'll change '.' to '_', > >> > + * change ':' to '-'. For example, 3-1:2.1 will be encoded to 3-1-2_1. > >> > + * This will be used to save original driver of USB device to xenstore. > >> > + */ > >> > >> What is the syntax of the incoming busid ? Could it contain _ or - ? > >> You should perhaps spot them and reject if it does. > > > > The busid is in syntax like 3-1:2.1. It does contain '-'. But since we only > use > > the single direction, never decode back, so it won't harm. > > So the only potential problem is that 3-1:2, 3-1-2, 3:1-2, and 3:1:2 > all collapse down to 3-1-2. Is there any risk of something like that > happening? (Ian: NB these are all read from sysfs.) > > Since this isn't really being read by anyone, maybe it would be > better to replace ':' with another character, just to be safe. It > could even be something like 'c' if no other punctuation is available. OK. Will update. > > >> > +/* Bind USB device to "usbback" driver. > >> > + * > >> > + * If there are many interfaces under USB device, check each interface, > >> > + * unbind from original driver and bind to "usbback" driver. > >> > + */ > >> > +static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb) > >> > +{ > >> ... > >> > + if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0) > { > >> > + LOG(WARN, "Write of %s to node %s failed", drvpath, > path); > >> > + } > >> > >> One of the advantages of libxl__xs_write_checked is that it logs > >> errors. So you can leave that log message out. > >> > >> However, if the xs write fails, you should presumably stop, rather > >> than carrying on. > >> > >> I think you probably do things in the wrong order here. You should > >> write the old driver info to xenstore first, so that if the bind to > >> usbback fails, you have the old driver info. > > Ian, I don't understand what you're saying here. The code I'm looking at > does: > 1. unbind + get old driver path in drvpath > 2. if(drvpath) write to xenstore > 3. bind to usbback > > If the bind fails, then we do have drvpath in xenstore. Or am I > missing something? If bind to usbback fails, it will goto 'out_rebind', that is we'll try to rebind it to its original driver and remove xenstore info. > > Bailing out if the above xenstore write fails seems sensible though. > > >> > +/* Operation to remove usb device. > >> > + * > >> > + * Generally, it does: > >> > + * 1) check if the usb device is assigned to the domain > >> > + * 2) remove the usb device from xenstore controller/port. > >> > + * 3) unbind usb device from usbback and rebind to its original driver. > >> > + * If usb device has many interfaces, do it to each interface. > >> > + */ > >> > +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid, > >> > + libxl_device_usb *usb) > >> > +{ > >> > + int rc; > >> > + > >> > + if (usb->ctrl < 0 || usb->port < 1) { > >> > + LOG(ERROR, "Invalid USB device"); > >> > + rc = ERROR_FAIL; > >> > + goto out; > >> > + } > >> > + > >> > + if (usb->devtype == LIBXL_USBDEV_TYPE_HOSTDEV && > >> > + (usb->u.hostdev.hostbus < 1 || usb->u.hostdev.hostaddr < 1)) { > >> > + LOG(ERROR, "Invalid USB device of hostdev"); > >> > + rc = ERROR_FAIL; > >> > + goto out; > >> > + } > >> > >> Why are these checks here rather than in do_usb_remove ? > >> > >> For that matter, why is this function not the same as do_usb_remove ? > >> > >> I might ask the same question about do_usb_add and > >> libxl__device_usb_add ? > > > > Using libxl__device_usb_add and within it extracting do_usb_add is to: > > * doing some initialization work and necessary check work in > > libxl__device_usb_add (this part are common to PVUSB and QEMU USB) > > * doing actual adding usb operations in do_usb_add (this part will diverge > > for PVUSB and QEMU USB) . > > > > Same to libxl__device_usb_remove and do_usb_remove. > > > > Certainly, we could merge into one function if that is better. George may > > have some opinions on this? > > I think it would be nice to have things broken down that way, but it's > likely I'll have to do some adjustments when I come to add in > devicemodel usb anyway. So making it one big function wouldn't hurt. > (Nor would leaving it the way it is, as far as I'm concerned.) Got it. Will update. > > The big thing though is with the removal interface here, where we > again have somewhat of a confusion between naming things based on the > *host* name (as we do in pci pass-through) vs the *guest* names (as we > do for disks and nics). > > If you have the controller and virtual port, there's no need for the > caller to *also* go and look up the host bus and host address; or vice > versa -- if you have the hostbus and hostaddr, you shouldn't also need > to go look up the usbctrl and virtual port. > > It looks like for libxl_device_{disk,nic,vtpm}_remove(), it gets a > libxl_device struct using libxl__device_from_{disk,nic,vtpm}; and in > all cases, the information used to construct the "device" is the > backend + either a devid (for nic and vtpm) or vdev, for disks. It > looks like for those devices, any conversion from other labels (mac > address for nic, uuid for vtpm) is done in xl_cmdimpli.c. > > I think we want to follow suit here -- libxl_device_usb_remove() > should take a usbctrl and port number only, and should look up > whatever information it needs to do the removal from that. > > All we really need for the re-assignment is the busid, which is > already stored in a xenstore location we can find using domid, ctrl, > and port. Something like the attached patch (compile-tested only). > What do you think? Thanks. Much better. Change interface to use 'busid' instead of 'struct usb' usbback_dev_assign and usbback_dev_unassign makes things much cleaner. > > (NB the patch doesn't fix the gc or aliasing issues in > usb_interface_xenstore_encode() -- those still need to be addressed.) I'll take care of the left. - Chunyan > > -George >