[Xen-devel] Re: [linux-usb-devel] Error recovery in Xen's paravirtualizing USB driver for Linux

[Harry Butterworth <harry@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, 2005-12-08 at 13:53 -0500, Alan Stern wrote:
> On Thu, 8 Dec 2005, Harry Butterworth wrote:
> 
> > > I'm not sure what you mean by that.  Do you _want_ to virtualize power 
> > > management operations?
> > 
> > I want to do the minimum amount of work to get the system working
> > reliably.  Virtualization of power management isn't a high priority.
> 
> Then for the moment you can keep your current strategy.  Eventually it 
> will make a difference, though.  The front-end will want to power-manage 
> its devices and you'll have to cope with it.

OK.

> 
> > > A complication is that suspended USB devices may send remote wakeup 
> > > requests.  If you only pretend to suspend the device then those wakeup 
> > > requests will never get sent.  Maybe you don't care about such esoteric 
> > > details...
> > 
> > Are the remote wakeup requests actually needed for anything if the
> > device isn't really suspended?
> 
> Well yes -- they're needed for telling the front-end that the virtual
> device wants to be resumed.  (Actually, according to the USB spec, remote
> wakeup messages mean that the device already _has_ been resumed.  It's a
> little peculiar...)
> 
> For example, suppose the front-end is so tremendously power-conscious that 
> it suspends USB keyboards between keystrokes.  The remote wakeup mechanism 
> then will be critical for letting the front-end know that a new keystroke 
> has arrived and the host needs to process it.

OK.  I think I understand this point now.

> 
> 
> > > You have to do this sort of thing anyway.  How else can you handle port
> > > resets?  They occur as a normal part of the device initialization 
> > > sequence 
> > > and as part of error recovery in usb-storage, among other things.
> > 
> > Yes, port resets are currently handled like this in the front-end and
> > forwarded to the back-end.  They aren't actually actioned in the
> > back-end at the moment (except to reset the simulated device address)
> > but I can put in a call to reset the real device if it is necessary.
> 
> You will find that it _is_ necessary for error recovery in usb-storage.  
> You'll have to call usb_reset_device.

OK.

> 
> > > > I'd prefer to let the back-end do it if at all possible.
> > > 
> > > Prefer to let the back-end do what?
> > 
> > Any power management.
> 
> But that means preventing the front-end from trying to do power 
> management.  How do you do that?  Only allow front-end kernels that have 
> been built with CONFIG_PM not set?

I meant the real power management.  I was assuming the front-end would
fake-up power management.  But you have changed my mind.  Now I think
the right thing to do is let the front-end do the power management of
the devices exported to it subject to the isolation requirements of
sharing the hub with other front-ends.  Implementing this can wait
though.  For the time being I'll keep the current strategy.

> 
> > Any idea what I should do for set-configuration? Currently I have the
> > following code: /* FIXME: what to do for set configuration? */ :-)
> 
> That's a bit of a problem when the front-end is trying to install config
> 0.  Actually doing it in the back-end would unbind your stub driver,
> making the front-end think the device had been unplugged.  The same would
> be true whenever the new config was different from the old one.  This is 
> one of those places where binding your stub driver to the device instead 
> of to the interfaces would help.

Yes.  At the moment my code does nothing which I think means it's only
going to work for devices which don't have to change the configuration.

> 
> 
> > > Part of the problem is that the stub drivers on the back-end are forced to
> > > bind to USB interfaces instead of USB devices.  It would make life simpler
> > > for you guys if the stub driver could bind to the entire device (replacing
> > > the usb_generic driver).  Do you think that's worth pursuing?
> > 
> > I'll look into it.  If it means that there is a cleaner split in
> > responsibility between the front-end and the back-end then it might
> > help.  One issue is the problem of letting the front-end have any
> > control over device configurations because it won't know about the other
> > devices connected to the back-end hub and so can't choose configurations
> > or manage the hub power accordingly.
> 
> I've got a patch-in-progress that will help the hub-power issue.
> 
> Letting a driver bind to devices would make things somewhat cleaner.  It's 
> not implemented currently in the kernel; it's one of the things I've been 
> considering adding.

Any idea how easy this would be?  Any chance of me being able to
implement a patch?

> 
> 
> > I'm still a bit confused by power management.  You say that the hub does
> > device power management and the drivers do interface power management...
> > 
> > Currently my back-end code is just a client of the normal linux USB
> > stack so, presumably the linux hub driver in the back-end will keep
> > doing whatever device power management it was doing before for the
> > devices that I'm exporting.  I'm not making any explicit power
> > management calls for the devices in the back-end.
> > 
> > Any device power management in the front-end is also presumably done by
> > the linux hub driver in the front-end and my front-end virtual hub just
> > fakes it out or ignores it.
> > 
> > So, is this an OK solution for device power management?
> 
> I think it's okay, except for that sticky point concerning remote wakeup.  
> As long as the front-end doesn't try to do aggressive power management 
> (which Linux has not yet implemented) you should be fine.
> 
> > As far as interface power management goes, all I can find in the spec is
> > a reference to another document describing the INTERFACE_POWER
> > descriptor which a bit of googling seems to indicate was withdrawn from
> > the standard shortly after being released.
> > 
> > What exactly do you mean by interface power management?
> 
> It's not really a formal USB term.  I mean notifying drivers (which manage 
> interfaces) about power management events.  Basically, a driver needs to 
> cancel all its URBs when the device is about to be suspended and resubmit 
> them when the device resumes.  In between, of course, it should not try to 
> submit any URBs.
> 
> > I'm assuming that anything the driver is doing to its interface is done
> > by submitting an URB in the front-end which will be forwarded to the
> > back-end and submitted to the back-end device interface.  Since the
> > back-end driver is not submitting any of its own URBs there should be no
> > interference between back and front-end drivers for any interface power
> > management.
> > 
> > Is this correct?
> 
> There should be no interference provided the back-end doesn't try to
> suspend any devices all by itself.  To protect against that possibility,
> your stub driver should be written so that its suspend method sends the
> front-end a virtual disconnect event and its resume method sends a virtual
> connect event.  That's probably about the same as what the probe and
> disconnect methods do now.

OK.

> 
> (I expect you're careful about sending these connect and disconnect 
> events for devices with multiple interfaces.  Like, send them only the 
> _first_ time an interface is probed/resumed and the _last_ time an 
> interface is disconnected/suspended.)

The code currently claims all the interfaces of a device when the first
interface of the device is probed and releases them all when the first
interface of the device is released:

+int usbback_driver_port_probe_usb(struct usb_interface *intf)
+{
+       int return_value = 0, i;
+       struct usb_device *usbdev = interface_to_usbdev(intf);
+       struct usbback_driver_port *port = NULL, *cur = NULL;
+       trace();
+       printk(KERN_INFO "usbback: Probe for usb device %s\n",
+                                                       usbdev->devpath);
+       down_read(&usbback_driver_port_list_sem);
+       list_for_each_entry(cur, &usbback_driver_port_list, link) {
+               printk(KERN_INFO "usbback: Testing configured path %s:",
+                                                               cur->path);
+               if (strncmp(cur->path, usbdev->devpath, sizeof(cur->path))
+                                                                       == 0) {
+                       printk(" matches.\n");
+                       port = cur;
+                       break;
+               } else {
+                       printk(" doesn't match.\n");
+               }
+       }
+       if (port == NULL) {
+               printk(KERN_INFO "usbback: No match found.\n");
+               return_value = -ENODEV;
+               goto exit_no_port;
+       }
+       for (i = 0; i < usbdev->actconfig->desc.bNumInterfaces; i++) {
+               struct usb_interface *other_intf = usb_ifnum_to_if(usbdev, i);
+               if (other_intf != intf) {
+                       if (usb_interface_claimed(other_intf)) {
+                               printk(KERN_INFO "usbback: An interface of the"
+                                       " matching device is already in use by"
+                                       " another driver.\n");
+                               return_value = -EBUSY;
+                               goto exit_interface_already_claimed;
+                       }
+               }
+       }
+       for (i = 0; i < usbdev->actconfig->desc.bNumInterfaces; i++) {
+               struct usb_interface *other_intf = usb_ifnum_to_if(usbdev, i);
+               if (other_intf != intf) {
+                       int error = usb_driver_claim_interface(
+                               &usbback_driver_usb_driver, other_intf, port);
+                       /* We already checked the interfaces were available. */
+                       ASSERT(error == 0);
+               }
+       }
+       usbdev = usb_get_dev(usbdev);
+       usb_set_intfdata(intf, port);
+       spin_lock_irq(&port->lock);
+       port->usbdev_is_connected = 1;
+       port->usbdev = usbdev;
+       usbback_driver_port_handle_stimulus(port,
+                                       usbback_driver_port_stimulus_up);
+       spin_unlock_irq(&port->lock);
+ exit_interface_already_claimed:
+ exit_no_port:
+       up_read(&usbback_driver_port_list_sem);
+       return return_value;
+}
+
+void usbback_driver_port_disconnect_usb(struct usb_interface *intf)
+{
+       struct usbback_driver_port *port = usb_get_intfdata(intf);
+       trace();
+       /* Protect against reentrant call when we release other interfaces. */
+       if (port != NULL) {
+               struct usb_device *usbdev;
+               int i;
+               ASSERT(port->usbdev_is_connected);
+               spin_lock_irq(&port->lock);
+               port->usbdev_is_connected = 0;
+               port->enabled = 0;
+               port->usb_disconnect = 0;
+               usbback_driver_port_handle_stimulus(port,
+                                       usbback_driver_port_stimulus_ud);
+               spin_unlock_irq(&port->lock);
+               xenidc_work_until(port->usb_disconnect);
+               usbdev = port->usbdev;
+               for (i = 0; i < usbdev->actconfig->desc.bNumInterfaces; i++) {
+                       struct usb_interface *other_intf = usb_ifnum_to_if(
+                                                               usbdev, i);
+                       if (other_intf != intf) {
+                               /* Protect against reentrant call when we    */
+                               /* release other interfaces.                 */
+                               usb_set_intfdata(other_intf, NULL);
+                               usb_driver_release_interface(
+                                               &usbback_driver_usb_driver,
+                                                               other_intf);
+                       }
+               }
+               usb_set_intfdata(intf, NULL);
+               usb_put_dev(usbdev);
+       }
+}
> 
> > So, I think the power management strategy for my current code might be
> > reasonable.
> > 
> > Can you still see anything wrong with it?
> 
> So long as you can ignore the remote wakeup problem, you should be good.

Good enough for the time being.  Eventually the power management can be
virtualized.

Many thanks for all your help.  I have a fair amount of work to do to
fix the protocol issues for unlinking and resubmitting URBs.  I'll
probably resurface in Jan.

Harry.

-- 
Harry Butterworth <harry@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel