Re: [RFC/PATCH 1/2] Add usb_endpoint_descriptor to be part of the struct usb_ep

Re: [RFC/PATCH 1/2] Add usb_endpoint_descriptor to be part of the struct usb_ep

[Alan Stern]

On Sun, 3 Oct 2010, tlinder wrote:

> From: Tatyana Linder <tlinder@codeaurora.org>
> 
> Change ep_choose() and usb_ep_enable() prototypes to use endpoint
> descriptor from usb_ep. This optimization prevents the FDs from handling
> the endpoint chosen descriptor.
> This optimization is not full though. To fully exploit this change one
> needs to update all the UDCs as well since in the current implementation
> each of them saves the endpoint descriptor in it's internal (and extended)
> endpoint structure.
> This patch is a preparation for adding SuperSpeed support to the gadget
> framework.
> 
> Signed-off-by: Tatyana Linder <tlinder@codeaurora.org>

...

> diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
> index a857b7a..6264fba 100644
> --- a/drivers/usb/gadget/file_storage.c
> +++ b/drivers/usb/gadget/file_storage.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 2003-2008 Alan Stern
>   * All rights reserved.
> + * Copyright (C) 2010 Code Aurora Forum. All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions

Speaking for myself only, I don't think people should add their own 
copyright notices in files to which they have not made substantial 
changes.  A little code motion and a small API adjustment don't seem 
like large enough changes to justify this.  Some of the other files in 
this patch have even smaller changes!

> diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
> index 484acfb..d5612ed 100644
> --- a/drivers/usb/gadget/storage_common.c
> +++ b/drivers/usb/gadget/storage_common.c
> @@ -2,7 +2,8 @@
>   * storage_common.c -- Common definitions for mass storage functionality
>   *
>   * Copyright (C) 2003-2008 Alan Stern
> - * Copyeight (C) 2009 Samsung Electronics
> + * Copyright (C) 2009 Samsung Electronics
> + * Copyright (C) 2010 Code Aurora Forum. All rights reserved.
>   * Author: Michal Nazarewicz (m.nazarewicz@samsung.com)
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -536,17 +537,6 @@ static struct usb_descriptor_header *fsg_hs_function[] = {
>  	NULL,
>  };
>  
> -/* Maxpacket and other transfer characteristics vary by speed. */
> -static struct usb_endpoint_descriptor *
> -fsg_ep_desc(struct usb_gadget *g, struct usb_endpoint_descriptor *fs,
> -		struct usb_endpoint_descriptor *hs)
> -{
> -	if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
> -		return hs;
> -	return fs;
> -}
> -
> -
>  /* Static strings, in UTF-8 (for simplicity we use only ASCII characters) */
>  static struct usb_string		fsg_strings[] = {
>  #ifndef FSG_NO_DEVICE_STRINGS

This is a good example.  You have contributed _nothing_ to this file --
all you did was _move_ some code to a different file.  This hardly
seems to justify adding a copyright notice.

> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index d3ef42d..bf7dc0b 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -7,6 +7,7 @@
>   *
>   *
>   * (C) Copyright 2002-2004 by David Brownell
> + * (C) Copyright 2010 Code Aurora Forum.
>   * All Rights Reserved.
>   *
>   * This software is licensed under the GNU GPL version 2.
> @@ -133,18 +134,25 @@ struct usb_ep_ops {
>   *      the endpoint descriptor used to configure the endpoint.
>   * @driver_data:for use by the gadget driver.  all other fields are
>   *	read-only to gadget drivers.
> + * @bEndpointAddress: used to identify ep when finding descriptor that matches
> + *	connection speed

This comment isn't very good.  Don't say what bEndpointAddress is 
_used_ for, say what it _is_.

> + * @desc:endpoint descriptor.  this pointer set before endpoint is enabled and
> + *	remains valid until the endpoint is disabled; the data byte order
> + *	is little-endian (usb-standard).
>   *

In addition, you need to point out in the kerneldoc for usb_ep_enable,
that the gadget driver must set these two fields before calling
usb_ep_enable, and you need to change the comment immediately above,
which states that these fields are read-only to gadget drivers!

Alan Stern

Re: [RFC/PATCH 1/2] Add usb_endpoint_descriptor to be part of the struct usb_ep

[David Brownell]

--- On Sun, 10/3/10, Alan Stern <stern@rowland.harvard.edu> wrote:

> 


> Speaking for myself only,

... and me too, for that matter.


 I don't think people should add
> their own 
> copyright notices in files to which they have not made
> substantial 
> changes.  A little code motion and a small API
> adjustment don't seem 
> like large enough changes to justify this.  Some of
> the other files in 
> this patch have even smaller changes!

It's pretty routine that very small patches
get merged without attribution, for that matter,
beyond GIT history.

- Dave

> 

Re: [RFC/PATCH 1/2] Add usb_endpoint_descriptor to be part of the struct usb_ep

[Greg KH]

On Sun, Oct 03, 2010 at 12:35:07PM -0400, Alan Stern wrote:
> On Sun, 3 Oct 2010, tlinder wrote:
> 
> > From: Tatyana Linder <tlinder@codeaurora.org>
> > 
> > Change ep_choose() and usb_ep_enable() prototypes to use endpoint
> > descriptor from usb_ep. This optimization prevents the FDs from handling
> > the endpoint chosen descriptor.
> > This optimization is not full though. To fully exploit this change one
> > needs to update all the UDCs as well since in the current implementation
> > each of them saves the endpoint descriptor in it's internal (and extended)
> > endpoint structure.
> > This patch is a preparation for adding SuperSpeed support to the gadget
> > framework.
> > 
> > Signed-off-by: Tatyana Linder <tlinder@codeaurora.org>
> 
> ...
> 
> > diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
> > index a857b7a..6264fba 100644
> > --- a/drivers/usb/gadget/file_storage.c
> > +++ b/drivers/usb/gadget/file_storage.c
> > @@ -3,6 +3,7 @@
> >   *
> >   * Copyright (C) 2003-2008 Alan Stern
> >   * All rights reserved.
> > + * Copyright (C) 2010 Code Aurora Forum. All rights reserved.
> >   *
> >   * Redistribution and use in source and binary forms, with or without
> >   * modification, are permitted provided that the following conditions
> 
> Speaking for myself only, I don't think people should add their own 
> copyright notices in files to which they have not made substantial 
> changes.  A little code motion and a small API adjustment don't seem 
> like large enough changes to justify this.  Some of the other files in 
> this patch have even smaller changes!

I agree.  Tatyana, please work with your company's lawyers to properly
learn how to add this type of mark to a file in a manner that is
correct.

Hint, this way is not correct :)

thanks,

greg k-h

Re: [RFC/PATCH 1/2] Add usb_endpoint_descriptor to be part of the struct usb_ep

[David Brown]

On Sun, Oct 03, 2010 at 12:35:07PM -0400, Alan Stern wrote:

> > Signed-off-by: Tatyana Linder <tlinder@codeaurora.org>
> ...
> 
> > diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
> > index a857b7a..6264fba 100644
> > --- a/drivers/usb/gadget/file_storage.c
> > +++ b/drivers/usb/gadget/file_storage.c
> > @@ -3,6 +3,7 @@
> >   *
> >   * Copyright (C) 2003-2008 Alan Stern
> >   * All rights reserved.
> > + * Copyright (C) 2010 Code Aurora Forum. All rights reserved.
> >   *
> >   * Redistribution and use in source and binary forms, with or without
> >   * modification, are permitted provided that the following conditions
> 
> Speaking for myself only, I don't think people should add their own 
> copyright notices in files to which they have not made substantial 
> changes.  A little code motion and a small API adjustment don't seem 
> like large enough changes to justify this.  Some of the other files in 
> this patch have even smaller changes!

BTW, we agree on this.  People aren't supposed to be putting these
copyrights in, but they've gotten used to it up until now.

Tatyana, please be careful to not insert a Code Aurora Forum copyright
unless you change a significant portion of the file.

David

RE: [RFC/PATCH 1/2] Add usb_endpoint_descriptor to be part of the struct usb_ep

[Tanya Brokhman]

-----Original Message-----
From: David Brown [mailto:davidb@codeaurora.org] 
Sent: Thursday, November 11, 2010 1:41 AM
To: Alan Stern
Cc: tlinder; USB list; David Brownell; Greg Kroah-Hartman; Michal
Nazarewicz; Andrew Morton; Kernel development list
Subject: Re: [RFC/PATCH 1/2] Add usb_endpoint_descriptor to be part of the
struct usb_ep

On Sun, Oct 03, 2010 at 12:35:07PM -0400, Alan Stern wrote:

> > Signed-off-by: Tatyana Linder <tlinder@codeaurora.org>
> ...
> 
> > diff --git a/drivers/usb/gadget/file_storage.c
b/drivers/usb/gadget/file_storage.c
> > index a857b7a..6264fba 100644
> > --- a/drivers/usb/gadget/file_storage.c
> > +++ b/drivers/usb/gadget/file_storage.c
> > @@ -3,6 +3,7 @@
> >   *
> >   * Copyright (C) 2003-2008 Alan Stern
> >   * All rights reserved.
> > + * Copyright (C) 2010 Code Aurora Forum. All rights reserved.
> >   *
> >   * Redistribution and use in source and binary forms, with or without
> >   * modification, are permitted provided that the following conditions
> 
> Speaking for myself only, I don't think people should add their own 
> copyright notices in files to which they have not made substantial 
> changes.  A little code motion and a small API adjustment don't seem 
> like large enough changes to justify this.  Some of the other files in 
> this patch have even smaller changes!

BTW, we agree on this.  People aren't supposed to be putting these
copyrights in, but they've gotten used to it up until now.

Tatyana, please be careful to not insert a Code Aurora Forum copyright
unless you change a significant portion of the file.

[Brokhman, Tanya] - Yes, this was already removed in the latest version of
the patch. Thanks!

David