Alon Levy <al...@redhat.com> writes:
> On Thu, Nov 11, 2010 at 11:29:36AM +0100, Markus Armbruster wrote:
>> Alon Levy <al...@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 <al...@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 <kra...@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.
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/
