From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:40235) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qbsmj-0002n2-Kt for qemu-devel@nongnu.org; Wed, 29 Jun 2011 07:18:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qbsmf-0007e1-JT for qemu-devel@nongnu.org; Wed, 29 Jun 2011 07:18:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62342) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qbsmf-0007dF-3B for qemu-devel@nongnu.org; Wed, 29 Jun 2011 07:18:17 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p5TBIFw2000430 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 29 Jun 2011 07:18:15 -0400 Message-ID: <4E0B0A32.7070907@redhat.com> Date: Wed, 29 Jun 2011 13:19:14 +0200 From: Hans de Goede MIME-Version: 1.0 References: <1308943978-6152-1-git-send-email-hdegoede@redhat.com> <1308943978-6152-11-git-send-email-hdegoede@redhat.com> <4E0B0520.7040303@redhat.com> In-Reply-To: <4E0B0520.7040303@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 10/11] usb-uhci: Add support for being a companion controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org 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