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.
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).
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. 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.
Thanks,
Elina Pasheva,
Rory Filer
--
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
