lkml.org 
[lkml]   [2014]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [V2 PATCH 03/10] added media agnostic (MA) data structures and handling
    On Mon, Nov 10, 2014 at 06:09:34PM -0800, Stephanie Wallick wrote:
    > +/**
    > + * Returns the number of urbs currently in the MA USB HCD. Will return 0 if the
    > + * MA USB HCD is empty or a negative errno if an error occurs.

    How can this function return a negative number? I don't see that
    codepath here, can you show it to me?

    > + */
    > +int mausb_hcd_urb_count(struct mausb_hcd *mhcd)
    > +{
    > + int count = 0;
    > + struct mausb_host_ep *ma_ep;
    > + struct mausb_dev *mausb_dev;
    > + struct mausb_urb *maurb;
    > + unsigned long irq_flags;
    > +
    > + /* for every device */
    > + spin_lock_irqsave(&mhcd->hcd_lock, irq_flags);
    > + list_for_each_entry(mausb_dev, &mhcd->ma_dev.dev_list, dev_list) {
    > + spin_unlock_irqrestore(&mhcd->hcd_lock, irq_flags);
    > +
    > + /* for every endpoint */
    > + spin_lock_irqsave(&mausb_dev->dev_lock, irq_flags);
    > + list_for_each_entry(ma_ep, &mausb_dev->ep_list, ep_list) {
    > + spin_unlock_irqrestore(&mausb_dev->dev_lock, irq_flags);
    > +
    > + /* for every urb */
    > + spin_lock_irqsave(&ma_ep->ep_lock, irq_flags);
    > + list_for_each_entry(maurb, &ma_ep->urb_list, urb_list) {
    > + ++count;
    > + }
    > +
    > + spin_unlock_irqrestore(&ma_ep->ep_lock, irq_flags);
    > + spin_lock_irqsave(&mausb_dev->dev_lock, irq_flags);
    > + }
    > +
    > + spin_unlock_irqrestore(&mausb_dev->dev_lock, irq_flags);
    > + spin_lock_irqsave(&mhcd->hcd_lock, irq_flags);
    > + }
    > +
    > + spin_unlock_irqrestore(&mhcd->hcd_lock, irq_flags);
    > +
    > + return count;
    > +}

    There honestly is too many things wrong with this function to even know
    where to start. So how about I just ask why you would ever want to know
    this number, and what good it would do to even care about it? You do
    realize that this number is almost always guaranteed to be wrong once
    the function returns, so you better not be doing something with it that
    matters.

    Intel has a whole group of very experienced Linux kernel developers who
    will review code before you sent it out publicly. Please take advantage
    of them and run this all through them before resending this out again.

    If you did run this code through that group, please let me know who it
    was specifically that allowed this stuff to get through, and why they
    didn't want their name on this code submission. I need to have a strong
    word with them...

    Yes, I am holding you to a higher standard than staging code normally
    is, and yes, it is purely because of the company you work for. But I
    only do that because your company knows how to do this stuff right, and
    you have access to the resources and talent to help make this code
    right. Other people and companies do not have the kind of advantage
    that you do.

    Wasting community member's time (i.e. mine) by forcing _them_ to review
    stuff like this, is something that your company knows better than to do,
    as should you as well.

    I want to see some more senior Intel kernel developer's signed-off-by
    lines on this code before I will ever consider accepting it for the
    kernel. Please do not resend this code until that happens.

    greg k-h


    \
     
     \ /
      Last update: 2014-11-11 06:21    [W:4.097 / U:0.368 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site