Re: [Qemu-devel] [PATCH 10/11] usb-uhci: Add support for being a companion controller

[Hans de Goede]

Hi,

On 06/29/2011 12:57 PM, Gerd Hoffmann wrote:
> Hi,
>
> > + if (s->masterbus) {
> > + USBPort *ports[NB_PORTS];
> > + for(i = 0; i< NB_PORTS; i++) {
> > + s->ports[i].port.ops =&uhci_port_ops;
> > + s->ports[i].port.opaque = s;
> > + s->ports[i].port.index = i;
> > + s->ports[i].port.speedmask =
> > + USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL;
> > + usb_port_location(&s->ports[i].port, NULL, i+1);
> > + ports[i] =&s->ports[i].port;
> > + }
> > + if (usb_bus_register_companion(s->masterbus, ports, NB_PORTS,
> > + s->firstport) != 0) {
> > + return -1;
> > + }
> > + } else {
> > + usb_bus_new(&s->bus,&uhci_bus_ops,&s->dev.qdev);
> > + for(i = 0; i< NB_PORTS; i++) {
> > + usb_register_port(&s->bus,&s->ports[i].port, s, i,&uhci_port_ops,
> > + USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
> > + usb_port_location(&s->ports[i].port, NULL, i+1);
> > + }
>
> This looks like we'll want a usb_register_companion_port() function which looks 
> like usb_register_port() but accepts masterbus & portindex instead of a USBBus 
> pointer.
> Then register the companion ports one by one, so that the code path for the 
companion case looks almost identical to the non-companion case.

I agree, but there is a reason why I went with a usb_bus_register_companion
function instead of with a usb_bus_register_companion_port function, the
uhci controller needs to know how many companion controllers it has
(to report this in one of its registers). When we're registering
ports 1 by 1, it does not know.

We could also pass in a port owner pointer, and make the uhci code keep
a list of known companions and check how many companions there are that
way, but that is quite ugly.

Another problem with registering ports 1 by 1 is that registering a companion
port can fail, and if the 2nd or higher register fails we would need to
undo the previous registers. Granted this is only an issue on hotplug,
and to support hot-unplug we would also need an unregister ..

Thinking more about this I think that the best approach would be to move
the port setup code (setting index, ops, speedmask, etc.) to
usb_bus_register_companion, and keep doing the entire registration
of all the ports in one single call. Would that work for you?

This still leaves the building of the port pointers array, we could
pass in a stride parameter to usb_bus_register_companion and make it
build that list too, I'm not sure if that is a good idea though?

> Otherwise the whole patchset looks very good.

I'm glad you like it :)

Regards,

Hans