| Message ID | 201009052212.37015.linux@rainbow-software.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, Sep 05, 2010 at 10:12:33PM +0200, Ondrej Zary wrote: > Ignore ADSL routers, which can have the same vendor and product IDs > as ADSL modems but should be handled by the cx82310_eth driver. > > This intentionally ignores device IDs that aren't currently handled > by cx82310_eth. There may be other device IDs that perhaps shouldn't > be claimed by cxacru. > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org> > > --- linux-2.6.36-rc3-orig/drivers/usb/atm/cxacru.c 2010-08-29 17:36:04.000000000 +0200 > +++ linux-2.6.36-rc3/drivers/usb/atm/cxacru.c 2010-09-05 18:54:56.000000000 +0200 > @@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive > .tx_padding = 11, > }; > > -static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) > -{ > +static int cxacru_usb_probe(struct usb_interface *intf, > + const struct usb_device_id *id) { Ick, what? Did you run this patch through scripts/checkpatch.pl? As you didn't change the function arguments, just leave the long line alone, but even if you did change it, the '{' has to be on the next line by itself. See Documentation/CodingStyle for details. > + struct usb_device *usb_dev = interface_to_usbdev(intf); > + char buf[15]; > + > + /* avoid ADSL routers (cx82310_eth) > + * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */ > + if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC > + && usb_string(usb_dev, usb_dev->descriptor.iProduct, > + buf, sizeof(buf)) > 0) { > + if (!strcmp(buf, "USB NET CARD")) { > + dev_info(&intf->dev, "ignoring cx82310_eth device\n"); > + return -ENODEV; > + } > + } In thinking about this a bit more, don't you also want to check the vendor and product id? You can't always be sure about the string of any old device, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, September 5, 2010 22:04, Greg KH wrote: > On Sun, Sep 05, 2010 at 10:12:33PM +0200, Ondrej Zary wrote: >> Ignore ADSL routers, which can have the same vendor and product IDs >> as ADSL modems but should be handled by the cx82310_eth driver. >> >> This intentionally ignores device IDs that aren't currently handled >> by cx82310_eth. There may be other device IDs that perhaps shouldn't >> be claimed by cxacru. >> >> Signed-off-by: Ondrej Zary <linux@rainbow-software.org> Missing Signed-off-by: you're modifying my changes. >> --- linux-2.6.36-rc3-orig/drivers/usb/atm/cxacru.c 2010-08-29 17:36:04.000000000 +0200 >> +++ linux-2.6.36-rc3/drivers/usb/atm/cxacru.c 2010-09-05 18:54:56.000000000 +0200 >> @@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive >> .tx_padding = 11, >> }; >> >> -static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) >> -{ >> +static int cxacru_usb_probe(struct usb_interface *intf, >> + const struct usb_device_id *id) { > > Ick, what? Sorry, this was my fault. I wasn't thinking when I changed it. >> + struct usb_device *usb_dev = interface_to_usbdev(intf); >> + char buf[15]; >> + >> + /* avoid ADSL routers (cx82310_eth) >> + * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */ >> + if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC >> + && usb_string(usb_dev, usb_dev->descriptor.iProduct, >> + buf, sizeof(buf)) > 0) { >> + if (!strcmp(buf, "USB NET CARD")) { >> + dev_info(&intf->dev, "ignoring cx82310_eth device\n"); >> + return -ENODEV; >> + } >> + } > > In thinking about this a bit more, don't you also want to check the > vendor and product id? You can't always be sure about the string of any > old device, right? This is already checked using the driver's id_table. I don't know if the vendor and product IDs for the cx82310_eth device should be claimed by cxacru or not. Either Conexant are sharing IDs between devices, or someone added it without confirming it works. There's no comment in the code from 2003 explaining what device it's supposed to be. For this reason I'd prefer to ignore all cxacru-claimed devices that appear to be an "USB NET CARD", and log that it did so. The 3 variants of the hardware I have all use an iProduct of "ADSL USB MODEM", but I don't want to restrict cxacru to just that in case some devices have different values.
On Monday 06 September 2010, Simon Arlott wrote: > On Sun, September 5, 2010 22:04, Greg KH wrote: > > On Sun, Sep 05, 2010 at 10:12:33PM +0200, Ondrej Zary wrote: > >> Ignore ADSL routers, which can have the same vendor and product IDs > >> as ADSL modems but should be handled by the cx82310_eth driver. > >> > >> This intentionally ignores device IDs that aren't currently handled > >> by cx82310_eth. There may be other device IDs that perhaps shouldn't > >> be claimed by cxacru. > >> > >> Signed-off-by: Ondrej Zary <linux@rainbow-software.org> > > Missing Signed-off-by: you're modifying my changes. Oops, sorry for that. I wanted to remove only the line between the signed-offs... > >> --- linux-2.6.36-rc3-orig/drivers/usb/atm/cxacru.c 2010-08-29 > >> 17:36:04.000000000 +0200 +++ > >> linux-2.6.36-rc3/drivers/usb/atm/cxacru.c 2010-09-05 18:54:56.000000000 > >> +0200 @@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive > >> .tx_padding = 11, > >> }; > >> > >> -static int cxacru_usb_probe(struct usb_interface *intf, const struct > >> usb_device_id *id) -{ > >> +static int cxacru_usb_probe(struct usb_interface *intf, > >> + const struct usb_device_id *id) { > > > > Ick, what? > > Sorry, this was my fault. I wasn't thinking when I changed it. > > >> + struct usb_device *usb_dev = interface_to_usbdev(intf); > >> + char buf[15]; > >> + > >> + /* avoid ADSL routers (cx82310_eth) > >> + * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */ > >> + if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC > >> + && usb_string(usb_dev, usb_dev->descriptor.iProduct, > >> + buf, sizeof(buf)) > 0) { > >> + if (!strcmp(buf, "USB NET CARD")) { > >> + dev_info(&intf->dev, "ignoring cx82310_eth device\n"); > >> + return -ENODEV; > >> + } > >> + } > > > > In thinking about this a bit more, don't you also want to check the > > vendor and product id? You can't always be sure about the string of any > > old device, right? > > This is already checked using the driver's id_table. > > I don't know if the vendor and product IDs for the cx82310_eth device > should be claimed by cxacru or not. Either Conexant are sharing IDs between > devices, or someone added it without confirming it works. There's no > comment in the code from 2003 explaining what device it's supposed to be. > For this reason I'd prefer to ignore all cxacru-claimed devices that appear > to be an "USB NET CARD", and log that it did so. Yes, these "USB NET CARD" devices will not work with cxacru either. We don't lose anything even if they are ignored by cxacru but not claimed by cx82310_eth. If someone has such device, (s)he can report the IDs to be added to cx82310_eth. > The 3 variants of the hardware I have all use an iProduct of "ADSL USB > MODEM", but I don't want to restrict cxacru to just that in case some > devices have different values.
Please submit a fixed up version of this patch based upon the feedback given. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 05 September 2010 23:04:46 Greg KH wrote: > On Sun, Sep 05, 2010 at 10:12:33PM +0200, Ondrej Zary wrote: > > Ignore ADSL routers, which can have the same vendor and product IDs > > as ADSL modems but should be handled by the cx82310_eth driver. > > > > This intentionally ignores device IDs that aren't currently handled > > by cx82310_eth. There may be other device IDs that perhaps shouldn't > > be claimed by cxacru. > > > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org> > > > > --- linux-2.6.36-rc3-orig/drivers/usb/atm/cxacru.c 2010-08-29 > > 17:36:04.000000000 +0200 +++ > > linux-2.6.36-rc3/drivers/usb/atm/cxacru.c 2010-09-05 18:54:56.000000000 > > +0200 @@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive > > .tx_padding = 11, > > }; > > > > -static int cxacru_usb_probe(struct usb_interface *intf, const struct > > usb_device_id *id) -{ > > +static int cxacru_usb_probe(struct usb_interface *intf, > > + const struct usb_device_id *id) { > > Ick, what? > > Did you run this patch through scripts/checkpatch.pl? Yes - and it passed. Maybe checkpatch.pl can't handle multiple lines there.
--- linux-2.6.36-rc3-orig/drivers/usb/atm/cxacru.c 2010-08-29 17:36:04.000000000 +0200 +++ linux-2.6.36-rc3/drivers/usb/atm/cxacru.c 2010-09-05 18:54:56.000000000 +0200 @@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive .tx_padding = 11, }; -static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) -{ +static int cxacru_usb_probe(struct usb_interface *intf, + const struct usb_device_id *id) { + struct usb_device *usb_dev = interface_to_usbdev(intf); + char buf[15]; + + /* avoid ADSL routers (cx82310_eth) + * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */ + if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC + && usb_string(usb_dev, usb_dev->descriptor.iProduct, + buf, sizeof(buf)) > 0) { + if (!strcmp(buf, "USB NET CARD")) { + dev_info(&intf->dev, "ignoring cx82310_eth device\n"); + return -ENODEV; + } + } + return usbatm_usb_probe(intf, id, &cxacru_driver); }
Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver. This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru. Signed-off-by: Ondrej Zary <linux@rainbow-software.org>