Re: [PATCH] USB: Driver for Freescale QUICC Engine USB Host Controller
On Tue, Dec 23, 2008 at 01:28:40PM -0800, Andrew Morton wrote:
On Wed, 24 Dec 2008 00:03:22 +0300
Anton Vorontsov avoront...@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!
Thanks for the review, Andrew.
Name-space pollution fixed.
[..]
+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?
Apparently. Fixed.
[...]
+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.
No, these are called with a spinlock held. But we don't normally
allocate things via this function, instead we pre-allocate the eds
and tds in the fhci_mem_init, so the kmalloc above is the last resort
when no empty tds/eds left in the appropriate lists (normally should
not happen).
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] USB: Driver for Freescale QUICC Engine USB Host Controller
On Tue, Dec 23, 2008 at 09:45:09PM -0500, Alan Stern wrote:
On Wed, 24 Dec 2008, Anton Vorontsov 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.
It looks like the kernel this was written for is several versions out
of date.
Yeah, originally it was written for 2.6.10. Though the driver I
posted is for the linux-next, and it works quite well w/o the
{un,}link_urb calls. ;-)
The driver is missing some critical calls to functions like
usb_hcd_link_urb_to_ep().
Fixed.
Thanks for review!
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] USB: Driver for Freescale QUICC Engine USB Host Controller
On Wed, 24 Dec 2008 00:03:22 +0300
Anton Vorontsov avoront...@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 =
Re: [PATCH] USB: Driver for Freescale QUICC Engine USB Host Controller
On Wed, 24 Dec 2008, Anton Vorontsov 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. It looks like the kernel this was written for is several versions out of date. The driver is missing some critical calls to functions like usb_hcd_link_urb_to_ep(). Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
