lkml.org 
[lkml]   [2008]   [Dec]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 <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!

    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: cbouatmailru@gmail.com
    irc://irc.freenode.net/bd2


    \
     
     \ /
      Last update: 2008-12-24 20:13    [W:0.024 / U:0.560 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site