On Wed, 24 Dec 2008 00:03:22 +0300
Anton Vorontsov [off-list ref] wrote:

This patch adds support for the FHCI USB controller, as found
in the Freescale MPC836x and MPC832x processors. It can support
Full or Low speed modes.

Quite a lot the hardware is doing by itself (SOF generation, CRC
generation and checking), though scheduling and retransmission is on
software's shoulders.

This controller does not integrate the root hub, so this driver also
fakes one-port hub. External hub is required to support more than
one device.

<quick scan>

Nice-looking driver.  But the namespace pollution is tremendous!

...

+static int fhci_mem_init(struct fhci_hcd *fhci)
+{
+	int i;
+	int error = 0;
+
+	fhci->hc_list = kzalloc(sizeof(*fhci->hc_list), GFP_KERNEL);
+	if (!fhci->hc_list)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&fhci->hc_list->ctrl_list);
+	INIT_LIST_HEAD(&fhci->hc_list->bulk_list);
+	INIT_LIST_HEAD(&fhci->hc_list->iso_list);
+	INIT_LIST_HEAD(&fhci->hc_list->intr_list);
+	INIT_LIST_HEAD(&fhci->hc_list->done_list);
+
+	fhci->vroot_hub = kzalloc(sizeof(*fhci->vroot_hub), GFP_KERNEL);
+	if (!fhci->vroot_hub)
+		return -ENOMEM;

Did we leak fhci->hc_list?

+	INIT_LIST_HEAD(&fhci->empty_eds);
+	INIT_LIST_HEAD(&fhci->empty_tds);
+
+	/* initialize work queue to handle done list */
+	fhci_tasklet.data = (unsigned long)fhci;
+	fhci->process_done_task = &fhci_tasklet;
+
+	for (i = 0; i < MAX_TDS; i++) {
+		struct td *td = kmalloc(sizeof(*td), GFP_KERNEL);
+
+		if (!td) {
+			error = 1;
+			break;
+		}
+		recycle_empty_td(fhci, td);
+	}
+	for (i = 0; i < MAX_EDS; i++) {
+		struct ed *ed = kmalloc(sizeof(*ed), GFP_KERNEL);
+
+		if (!ed) {
+			error = 1;
+			break;
+		}
+		recycle_empty_ed(fhci, ed);
+	}
+
+	if (error) {
+		fhci_mem_free(fhci);
+		return -ENOMEM;
+	}
+
+	fhci->active_urbs = 0;
+
+	return error;
+}
+

...

+void config_transceiver(struct fhci_hcd *fhci, enum fhci_port_status status)

poorly-chosen global identifier.

+{
+	fhci_dbg(fhci, "-> %s: %d\n", __func__, status);
+
+	switch (status) {
+	case FHCI_PORT_POWER_OFF:
+		fhci_gpio_set_value(fhci, GPIO_POWER, false);
+		break;
+	case FHCI_PORT_DISABLED:
+	case FHCI_PORT_WAITING:
+		fhci_gpio_set_value(fhci, GPIO_POWER, true);
+		break;
+	case FHCI_PORT_LOW:
+		fhci_gpio_set_value(fhci, GPIO_SPEED, false);
+		break;
+	case FHCI_PORT_FULL:
+		fhci_gpio_set_value(fhci, GPIO_SPEED, true);
+		break;
+	default:
+		WARN_ON(1);
+		break;
+	}
+
+	fhci_dbg(fhci, "<- %s: %d\n", __func__, status);
+}
+
+/* disable the USB port by clearing the EN bit in the USBMOD register */
+void usb_port_disable(struct fhci_hcd *fhci)

another 

+{
+	struct fhci_usb *usb = (struct fhci_usb *)fhci->usb_lld;
+	enum fhci_port_status port_status;
+
+	fhci_dbg(fhci, "-> %s\n", __func__);
+
+	fhci_stop_sof_timer(fhci);
+
+	flush_all_transmissions(usb);
+
+	fhci_usb_disable_interrupt((struct fhci_usb *)fhci->usb_lld);
+	port_status = usb->port_status;
+	usb->port_status = FHCI_PORT_DISABLED;
+
+	/* Enable IDLE since we want to know if something comes along */
+	usb->saved_msk |= USB_E_IDLE_MASK;
+	out_be16(&usb->fhci->regs->usb_mask, usb->saved_msk);
+
+	/* check if during the disconnection process attached new device */
+	if (port_status == FHCI_PORT_WAITING)
+		device_connected_interrupt(fhci);
+	usb->vroot_hub->port.wPortStatus &= ~USB_PORT_STAT_ENABLE;
+	usb->vroot_hub->port.wPortChange |= USB_PORT_STAT_C_ENABLE;
+	fhci_usb_enable_interrupt((struct fhci_usb *)fhci->usb_lld);
+
+	fhci_dbg(fhci, "<- %s\n", __func__);
+}
+
+/* enable the USB port by setting the EN bit in the USBMOD register */
+void usb_port_enable(void *lld)

another.  Please review all non-static symbols which this driver adds.

+{
+	struct fhci_usb *usb = (struct fhci_usb *)lld;
+	struct fhci_hcd *fhci = usb->fhci;
+
+	fhci_dbg(fhci, "-> %s\n", __func__);
+
+	config_transceiver(fhci, usb->port_status);
+
+	if ((usb->port_status != FHCI_PORT_FULL) &&
+			(usb->port_status != FHCI_PORT_LOW))
+		fhci_start_sof_timer(fhci);
+
+	usb->vroot_hub->port.wPortStatus |= USB_PORT_STAT_ENABLE;
+	usb->vroot_hub->port.wPortChange |= USB_PORT_STAT_C_ENABLE;
+
+	fhci_dbg(fhci, "<- %s\n", __func__);
+}
+

...

+static struct td *get_empty_td(struct fhci_hcd *fhci)
+{
+	struct td *td;
+
+	if (!list_empty(&fhci->empty_tds)) {
+		td = list_entry(fhci->empty_tds.next, struct td, node);
+		list_del(fhci->empty_tds.next);
+	} else {
+		td = kmalloc(sizeof(*td), GFP_ATOMIC);
+		if (!td)
+			fhci_err(fhci, "No memory to allocate to TD\n");
+		else
+			init_td(td);
+	}
+
+	return td;
+}
+
+void recycle_empty_td(struct fhci_hcd *fhci, struct td *td)
+{
+	init_td(td);
+	list_add(&td->node, &fhci->empty_tds);
+}
+
+struct ed *get_empty_ed(struct fhci_hcd *fhci)
+{
+	struct ed *ed;
+
+	if (!list_empty(&fhci->empty_eds)) {
+		ed = list_entry(fhci->empty_eds.next, struct ed, node);
+		list_del(fhci->empty_eds.next);
+	} else {
+		ed = kmalloc(sizeof(*ed), GFP_ATOMIC);
+		if (!ed)
+			fhci_err(fhci, "No memory to allocate to ED\n");
+		else
+			init_ed(ed);
+	}
+
+	return ed;
+}

The GFP_ATOMICs here are regrettable.  Are these functions ever called
from a context in which a more reliable allocation mode can be used? 
If so, the caller should pass in the gfp_t.

...

+u32 create_endpoint(struct fhci_usb *usb, enum fhci_mem_alloc data_mem,
+			   u32 ring_len)

This is a poorly chosen global identifier.  fhci_create_endpoint()
would be better.