On Tue, May 12, 2009 at 05:42:06PM -0700, Elina Pasheva wrote:
> On Tue, 2009-05-12 at 17:19 -0700, Greg KH wrote:
> > On Tue, May 12, 2009 at 01:13:10PM -0700, Elina Pasheva wrote:
> > > Subject: [PATCH 004/004] USB: serial: sierra driver interface blacklisting
> > > From: Elina Pasheva <epasheva@xxxxxxxxxxxxxxxxxx>
> > >
> > > The following is summary of changes we have made to sierra.c driver in
> > > [PATCH 004/004] dealing with interface blacklisting support:
> > > - Modified sierra_probe() and sierra_calc_interface() to handle blacklisted
> > > interfaces accordingly
> > > - Added data list structure and function to support blacklisting
> >
> > But you never use this structure. Why add it?
> >
> > > - Improved comments in id_table
> > > Signed-off-by: Elina Pasheva <epasheva@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >
> > > drivers/usb/serial/sierra.c | 45 +++++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 42 insertions(+), 3 deletions(-)
> > >
> > > --- a/drivers/usb/serial/sierra.c 2009-05-01 13:17:14.000000000 -0700
> > > +++ b/drivers/usb/serial/sierra.c 2009-05-01 17:30:05.000000000 -0700
> > > @@ -47,6 +47,12 @@
> > > static int debug;
> > > static int nmea;
> > >
> > > +/* list of interface numbers - used for constructing interface blacklists */
> > > +struct list {
> > > + const u32 listlen; /* number of interface numbers on list */
> > > + const u8 *list; /* pointer to the array holding the numbers */
> > > +};
> >
> > The kernel has built in list functions, I would recommend using them
> > instead of creating something so generic as "struct list" in your
> > driver.
> >
> > And how is this interface list used to create a blacklist? I see you do
> > create one:
> > > +static const u8 direct_ip_non_serial_ifaces[] = { 7, 8, 9 };
> > > +static const struct list direct_ip_interface_blacklist = {
> > > + .listlen = ARRAY_SIZE(direct_ip_non_serial_ifaces),
> > > + .list = direct_ip_non_serial_ifaces,
> > > +};
> >
> > But then you never refer to it in the code.
> >
> > Am I missing something here?
> >
> > confused,
> >
> > greg k-h
> >
>
> Hi Greg,
>
> Sorry for the confusion with this series of 4 patches. The answer to
> your questions about why we added these items without using them is that
> they actually do get used in the upcoming series of 8 patches. If you
> will allow this "strange" patch just this once you will see everything
> come together in the next series of 8. NOTE: The current series of 4
> patches will build and run as-is.
Don't add infrastructure, and tables that you don't use until later
patches, without at least saying so :)
> As for your comment about Linux supporting built-in list functions, we
> will add it to our jobjar to fix this in a future patch (i.e. do it the
> Linux way).
I can't take the patch as-is, sorry. Please fix it up so that it is
mergable.
> By way of justifying the long and arduous series of patches we've been
> sending, perhaps we took the advice to keep our changes finely grained
> too much to heart. As you can see, we made *a lot* of changes to the
> driver and are now paying the price with a barrage of patches.
There's nothing wrong with a lot of little patches like this, you did it
great. But you might want to rethink it a bit:
- don't change the version number a lot, that's pointless.
- do one thing per patch, and explain why you are doing it.
Your blacklist table patch did not do this.
> We have learned our lesson and will be thinking about how best to
> structure any future changes to this driver to minimize the patches
> any changes will generate. We see a few more things that need to be
> addressed, but we're ignoring all that until the kernel is up to date
> with our CVS repository.
Try using quilt, it makes stuff like this much easier, if not almost
trivial.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
