| Message ID | 1406537796-20156-1-git-send-email-oneukum@suse.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Oliver Neukum <oneukum@suse.de> Date: Mon, 28 Jul 2014 10:56:36 +0200 > There are devices that don't do reset all the way. So the packet filter should > be set to a sane initial value. Failure to do so leads to intermittent failures > of DHCP on some systems under some conditions. > > Signed-off-by: Oliver Neukum <oneukum@suse.de> Applied. -- 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
Oliver Neukum <oneukum@suse.de> writes: > There are devices that don't do reset all the way. So the packet filter should > be set to a sane initial value. Failure to do so leads to intermittent failures > of DHCP on some systems under some conditions. > > Signed-off-by: Oliver Neukum <oneukum@suse.de> > --- > drivers/net/usb/cdc_ether.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c > index 9ea4bfe..2a32d91 100644 > --- a/drivers/net/usb/cdc_ether.c > +++ b/drivers/net/usb/cdc_ether.c > @@ -341,6 +341,22 @@ next_desc: > usb_driver_release_interface(driver, info->data); > return -ENODEV; > } > + > + /* Some devices don't initialise properly. In particular > + * the packet filter is not reset. There are devices that > + * don't do reset all the way. So the packet filter should > + * be set to a sane initial value. > + */ > + usb_control_msg(dev->udev, > + usb_sndctrlpipe(dev->udev, 0), > + USB_CDC_SET_ETHERNET_PACKET_FILTER, > + USB_TYPE_CLASS | USB_RECIP_INTERFACE, > + USB_CDC_PACKET_TYPE_ALL_MULTICAST | USB_CDC_PACKET_TYPE_DIRECTED | USB_CDC_PACKET_TYPE_BROADCAST, > + intf->cur_altsetting->desc.bInterfaceNumber, > + NULL, > + 0, > + USB_CTRL_SET_TIMEOUT > + ); > return 0; > > bad_desc: Hmm, the usbnet_generic_cdc_bind() function is used by the vendor specific zaurus and rndis_host drivers as well. Are we sure the USB_CDC_SET_ETHERNET_PACKET_FILTER message is supported by all devices supported by these drivers? Bjørn -- 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 Thu, 2014-08-14 at 13:11 +0200, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.de> writes: > > > There are devices that don't do reset all the way. So the packet filter should > > be set to a sane initial value. Failure to do so leads to intermittent failures > > of DHCP on some systems under some conditions. > > > > Signed-off-by: Oliver Neukum <oneukum@suse.de> > > --- > > drivers/net/usb/cdc_ether.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c > > index 9ea4bfe..2a32d91 100644 > > --- a/drivers/net/usb/cdc_ether.c > > +++ b/drivers/net/usb/cdc_ether.c > > @@ -341,6 +341,22 @@ next_desc: > > usb_driver_release_interface(driver, info->data); > > return -ENODEV; > > } > > + > > + /* Some devices don't initialise properly. In particular > > + * the packet filter is not reset. There are devices that > > + * don't do reset all the way. So the packet filter should > > + * be set to a sane initial value. > > + */ > > + usb_control_msg(dev->udev, > > + usb_sndctrlpipe(dev->udev, 0), > > + USB_CDC_SET_ETHERNET_PACKET_FILTER, > > + USB_TYPE_CLASS | USB_RECIP_INTERFACE, > > + USB_CDC_PACKET_TYPE_ALL_MULTICAST | USB_CDC_PACKET_TYPE_DIRECTED | USB_CDC_PACKET_TYPE_BROADCAST, > > + intf->cur_altsetting->desc.bInterfaceNumber, > > + NULL, > > + 0, > > + USB_CTRL_SET_TIMEOUT > > + ); > > return 0; > > > > bad_desc: > > Hmm, the usbnet_generic_cdc_bind() function is used by the vendor > specific zaurus and rndis_host drivers as well. Are we sure the > USB_CDC_SET_ETHERNET_PACKET_FILTER message is supported by all devices > supported by these drivers? Support for this request is mandatory. Regards Oliver -- 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
Oliver Neukum <oneukum@suse.de> writes: > On Thu, 2014-08-14 at 13:11 +0200, Bjørn Mork wrote: >> Oliver Neukum <oneukum@suse.de> writes: >> >> > There are devices that don't do reset all the way. So the packet filter should >> > be set to a sane initial value. Failure to do so leads to intermittent failures >> > of DHCP on some systems under some conditions. >> > >> > Signed-off-by: Oliver Neukum <oneukum@suse.de> >> > --- >> > drivers/net/usb/cdc_ether.c | 16 ++++++++++++++++ >> > 1 file changed, 16 insertions(+) >> > >> > diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c >> > index 9ea4bfe..2a32d91 100644 >> > --- a/drivers/net/usb/cdc_ether.c >> > +++ b/drivers/net/usb/cdc_ether.c >> > @@ -341,6 +341,22 @@ next_desc: >> > usb_driver_release_interface(driver, info->data); >> > return -ENODEV; >> > } >> > + >> > + /* Some devices don't initialise properly. In particular >> > + * the packet filter is not reset. There are devices that >> > + * don't do reset all the way. So the packet filter should >> > + * be set to a sane initial value. >> > + */ >> > + usb_control_msg(dev->udev, >> > + usb_sndctrlpipe(dev->udev, 0), >> > + USB_CDC_SET_ETHERNET_PACKET_FILTER, >> > + USB_TYPE_CLASS | USB_RECIP_INTERFACE, >> > + USB_CDC_PACKET_TYPE_ALL_MULTICAST | USB_CDC_PACKET_TYPE_DIRECTED | USB_CDC_PACKET_TYPE_BROADCAST, >> > + intf->cur_altsetting->desc.bInterfaceNumber, >> > + NULL, >> > + 0, >> > + USB_CTRL_SET_TIMEOUT >> > + ); >> > return 0; >> > >> > bad_desc: >> >> Hmm, the usbnet_generic_cdc_bind() function is used by the vendor >> specific zaurus and rndis_host drivers as well. Are we sure the >> USB_CDC_SET_ETHERNET_PACKET_FILTER message is supported by all devices >> supported by these drivers? > > Support for this request is mandatory. Yes, there is no problem for standard conforming ECM devices. I fully agree there. But standard conformance is unfortunately not something we can expect. My question was regarding the non-conforming devices, like the ones supported by th zaurus and rndis_host drivers. These drivers are made solely because the supported devices do *not* conform to the ECM spec. If you know they support the request, then fine. But I do not think we can assume that they do without testing it, despite what the ECM spec says. Bjørn -- 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 Fri, 2014-08-15 at 09:13 +0200, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.de> writes: > > > On Thu, 2014-08-14 at 13:11 +0200, Bjørn Mork wrote: > > Support for this request is mandatory. > > Yes, there is no problem for standard conforming ECM devices. I fully > agree there. > > But standard conformance is unfortunately not something we can expect. > My question was regarding the non-conforming devices, like the ones > supported by th zaurus and rndis_host drivers. These drivers are made You have a point. Yet another flag? Do you see an alternative? Regards Oliver -- 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
Oliver Neukum <oneukum@suse.de> writes: > On Fri, 2014-08-15 at 09:13 +0200, Bjørn Mork wrote: >> Oliver Neukum <oneukum@suse.de> writes: >> >> > On Thu, 2014-08-14 at 13:11 +0200, Bjørn Mork wrote: > >> > Support for this request is mandatory. >> >> Yes, there is no problem for standard conforming ECM devices. I fully >> agree there. >> >> But standard conformance is unfortunately not something we can expect. >> My question was regarding the non-conforming devices, like the ones >> supported by th zaurus and rndis_host drivers. These drivers are made > > You have a point. Yet another flag? Do you see an alternative? Note that I don't know whether this is a real problem or not. I don't believe I have any zaurus or rndis_host devices. But how about just splitting the .bind functions for true ECM devices and the others, letting the zaurus and rndis_host driver continue to use usbnet_cdc_bind with no filter setting? Maybe define a new unexported .bind in cdc_ether for this purpose, moving the ECM specific initialization there? Maybe something along this (instead of the change to usbnet_generic_cdc_bind)?: static int usbnet_cdc_ecm_bind(struct usbnet *dev, struct usb_interface *intf) { int rv = usbnet_cdc_bind(dev, intf); if (rv < 0) return rv; /* Some devices don't initialise properly. In particular * the packet filter is not reset. There are devices that * don't do reset all the way. So the packet filter should * be set to a sane initial value. */ rv = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), USB_CDC_SET_ETHERNET_PACKET_FILTER, USB_TYPE_CLASS | USB_RECIP_INTERFACE, USB_CDC_PACKET_TYPE_ALL_MULTICAST | USB_CDC_PACKET_TYPE_DIRECTED | USB_CDC_PACKET_TYPE_BROADCAST, intf->cur_altsetting->desc.bInterfaceNumber, NULL, 0, USB_CTRL_SET_TIMEOUT); if (rv < 0) return rv; return 0; } .. static const struct driver_info cdc_info = { .description = "CDC Ethernet Device", .flags = FLAG_ETHER | FLAG_POINTTOPOINT, .bind = usbnet_cdc_ecm_bind, .unbind = usbnet_cdc_unbind, .status = usbnet_cdc_status, .manage_power = usbnet_manage_power, }; Bjørn -- 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 Mon, 2014-08-25 at 10:39 +0200, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.de> writes: > > On Fri, 2014-08-15 at 09:13 +0200, Bjørn Mork wrote: > >> Oliver Neukum <oneukum@suse.de> writes: > >> > >> > On Thu, 2014-08-14 at 13:11 +0200, Bjørn Mork wrote: > > > >> > Support for this request is mandatory. > >> > >> Yes, there is no problem for standard conforming ECM devices. I fully > >> agree there. > >> > >> But standard conformance is unfortunately not something we can expect. > >> My question was regarding the non-conforming devices, like the ones > >> supported by th zaurus and rndis_host drivers. These drivers are made > > > > You have a point. Yet another flag? Do you see an alternative? > > Note that I don't know whether this is a real problem or not. I don't > believe I have any zaurus or rndis_host devices. > > But how about just splitting the .bind functions for true ECM devices > and the others, letting the zaurus and rndis_host driver continue to use > usbnet_cdc_bind with no filter setting? Maybe define a new unexported > .bind in cdc_ether for this purpose, moving the ECM specific > initialization there? > > Maybe something along this (instead of the change to > usbnet_generic_cdc_bind)?: Yes, that looks superior. Regards Oliver -- 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
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c index 9ea4bfe..2a32d91 100644 --- a/drivers/net/usb/cdc_ether.c +++ b/drivers/net/usb/cdc_ether.c @@ -341,6 +341,22 @@ next_desc: usb_driver_release_interface(driver, info->data); return -ENODEV; } + + /* Some devices don't initialise properly. In particular + * the packet filter is not reset. There are devices that + * don't do reset all the way. So the packet filter should + * be set to a sane initial value. + */ + usb_control_msg(dev->udev, + usb_sndctrlpipe(dev->udev, 0), + USB_CDC_SET_ETHERNET_PACKET_FILTER, + USB_TYPE_CLASS | USB_RECIP_INTERFACE, + USB_CDC_PACKET_TYPE_ALL_MULTICAST | USB_CDC_PACKET_TYPE_DIRECTED | USB_CDC_PACKET_TYPE_BROADCAST, + intf->cur_altsetting->desc.bInterfaceNumber, + NULL, + 0, + USB_CTRL_SET_TIMEOUT + ); return 0; bad_desc:
There are devices that don't do reset all the way. So the packet filter should be set to a sane initial value. Failure to do so leads to intermittent failures of DHCP on some systems under some conditions. Signed-off-by: Oliver Neukum <oneukum@suse.de> --- drivers/net/usb/cdc_ether.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)