Messages in this thread | | | Date | Tue, 23 Dec 2008 13:28:40 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH] USB: Driver for Freescale QUICC Engine USB Host Controller |
| |
On Wed, 24 Dec 2008 00:03:22 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> 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.
| |