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

[Markus Armbruster]

Alon Levy <address@hidden> writes:

> On Thu, Nov 11, 2010 at 11:29:36AM +0100, Markus Armbruster wrote:
>> Alon Levy <address@hidden> writes:
>> 
>> > On Wed, Nov 10, 2010 at 04:49:38PM +0100, Markus Armbruster wrote:
>> >> Sorry for coming so late to this thread...
>> >> 
>> >> Alon Levy <address@hidden> 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 <address@hidden>
>> >> >> 
>> >> >> 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.

Understand.

>> >> 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).

Agree.

Summary so far:

1. usb_{attach,detach} looks like yet another special-purpose command
   where a general command would make sense, namely
   device_{attach,detach}.  We have a few of those, e.g. usb_add
   vs. device_add.  I'd prefer not to add more, as far as practical.

2. We need chardev_add and chardev_del sooner rather than later.

3. Automatic deletion of host devices (character, block, net) gets in
   the way[*].  We need a way to suppress it.

4. If we have 2. and 3., we don't need 1.

Fair?


[*] Funny coincidence: I just read Neil Brown's essay on the conflated
design anti-pattern, http://lwn.net/Articles/412131/