Re: [Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v3)

[Alon Levy <alevy@redhat.com>]

On Thu, Nov 11, 2010 at 11:29:36AM +0100, Markus Armbruster wrote:
> Alon Levy <alevy@redhat.com> writes:
> 
> > On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
> >> Sorry for coming so late to this thread...
> >> 
> >> Alon Levy <alevy@redhat.com> writes:
> >> 
> >> > On Thu, Oct 21, 2010 at 08:13:19AM -0500, Anthony Liguori wrote:
> >> >> On 10/21/2010 08:03 AM, Gerd Hoffmann wrote:
> >> >> >On 10/21/10 08:36, Alon Levy wrote:
> >> >> >>v2->v3 changes:
> >> >> >>  * add configure parameter
> >> >> >>  * fix docs
> >> >> >>
> >> >> >>v2 message:
> >> >> >>This patchset uses id like device_del for attaching/detaching usb
> >> >> >>devices. The first two patches ready the way:
> >> >> >>  1. makes qdev_find_recursive non static and in qdev.h
> >> >> >>  2. adds a usb_device_by_id which goes over the usb buses calling
> >> >> >>   qdev_find_recursive
> >> >> >>  3. adds the commands that use usb_device_by_id
> >> >> >>
> >> >> >>Alon Levy (3):
> >> >> >>   qdev: make qdev_find_recursive public
> >> >> >>   usb: add public usb_device_by_id
> >> >> >>   monitor: add usb_attach and usb_detach (v2)
> >> >> >>
> >> >> >
> >> >> >Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> >> >> 
> >> >> Okay, I am still confused about the use-case for this and I don't
> >> >> see any further explanation in the commit messages.  I've seen
> >> >> "debugging" but can you be a bit more specific about which cases
> >> >> it's needed for?
> >> >> 
> >> >
> >> > I use it for debugging the usb-ccid device. I think it's useful for
> >> > any other usb device tests as well. The existing commands are not
> >> > good enough to do a remove/insert of a usb device, since deleting
> >> > a device also deletes any chardev associated with it, and there is
> >> > no monitor command to add a chardev. Also sometimes you don't want
> >> > to close the chardev, just have the guest see a removal/reinsert of
> >> > the device.
> >> [...]
> >> 
> >> Let's see whether I get you: detach removes the device, but doesn't
> >> destroy it.  The only thing you can do with a detached device is attach
> >> it.  Detach+attach is basically the same as del+add with the same
> >> configuration.  Except shortcomings in our command set make it
> >> impossible to recreate the configuration sometimes.  Correct?
> > So the problems with the current commands from my pov:
> >  - device deletion removes associated chardev
> >   - no way to do it without removing chardev
> >   - no way to add chardev later and use it for device add
> > The outcome of which is that you can't do a guest wise attach/detach
> > from monitor if your device relies on a chardev association. This
> > happens with my passthrough ccid device.
> 
> Commands chardev_add, chardev_del look feasible to me.
> 
> I hate device_del destroying chardevs automatically.  If it was created
> separately, it should be destroyed separately.  But any fix needs to be
> backwards compatible somehow.  How to do that without embarrassingly
> ugly warts isn't obvious to me.
> 
> >> Questions:
> >> 
> >> 1. If we add commands so that you can always recreate the configuration,
> >>    is detach+attach still useful?  Why?
> > If you make it so you can do a device_del and not remove the chardev, and
> > later device_add using the already existing chardev, then that will be
> > equivalent for me.
> 
> Would chardev_add suffice, or do you need a way to reuse the existing
> chardev?
> 
I'd love chardev_add / chardev_del for testing in general, but they don't
work for my use case, because chardev_del closes the socket (in my case).
I could of course fix my client to work with reconnect, but it doesn't make
this pretty.

> >> 2. Why is this a USB problem, and not a general problem?  In other
> >>    words, why usb_{detach,attach}, and not device_{detach,attach}?
> > I guess attach/detach is a don't-free-some-resources del/add. If you
> > think there are users for a device_attach/detach and it makes sense
> > conceptually (what's a detach/attach for an ide bus? for a pci it's
> > pretty clear, for sata, etc.) then you could blow this up to a device
> > specific callback or something like that (assuming that's how you
> > would implement this).
> 
> For buses that don't support hot plug, such as IDE, detach makes as much
> sense as delete: none.
> 
> For buses that do (USB, PCI, SCSI, virtio-serial-bus), detach looks like
> the first half of delete to me: shut down, remove from device tree
> (second half is destroying the device object).
> 
> Likewise, attach looks like the second have of add: insert into device
> tree, start up (first half is creating the device object).
> 
> Pitfall: to make re-attach work, qdev method init() needs to work not
> just for newly created objects, but after a qdev exit() as well.  This
> is a change of contract for these two methods.  I wouldn't be surprised
> if not all of our device were happy with that.
> 

We could flag which devices can do re-attach. Or you go across the board
and add a info->detach, info->attach, split from info->exit, info->init.
Not a small amount of work :/ Actually, I think you'd need to do that anyway
to get any benefit from the detach/attach commands (apart from not deleting
associated chardevs).