Re: [V2 PATCH 03/10] added media agnostic (MA) data structures and handling

[Greg KH]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/