[PATCH] cdc-ether: clean packet filter upon probe

[PATCH] cdc-ether: clean packet filter upon probe

[Oliver Neukum]

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:
-- 
1.8.4.5

Re: [PATCH] cdc-ether: clean packet filter upon probe

[David Miller]

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.

Re: [PATCH] cdc-ether: clean packet filter upon probe

[Bjørn Mork]

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

Re: [PATCH] cdc-ether: clean packet filter upon probe

[Oliver Neukum]

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

Re: [PATCH] cdc-ether: clean packet filter upon probe

[Bjørn Mork]

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

Re: [PATCH] cdc-ether: clean packet filter upon probe

[Oliver Neukum]

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

Re: [PATCH] cdc-ether: clean packet filter upon probe

[Bjørn Mork]

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

Re: [PATCH] cdc-ether: clean packet filter upon probe

[Oliver Neukum]

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

[PATCH] cdc-ether: clean packet filter upon probe

[Oliver Neukum]

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..ef8f8f8 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,
+			1000 /* out of thin air */
+		);
 	return 0;
 
 bad_desc:
-- 
1.8.4.5

Re: [PATCH] cdc-ether: clean packet filter upon probe

[David Miller]

From: Oliver Neukum <oneukum@suse.de>
Date: Thu, 24 Jul 2014 13:56:29 +0200

> +			1000 /* out of thin air */

Just use USB_CTRL_SET_TIMEOUT if you can't come up with a specific
reason to use another value.

Thanks.

Re: [PATCH] cdc-ether: clean packet filter upon probe

[Oliver Neukum]

On Sun, 2014-07-27 at 22:30 -0700, David Miller wrote:
> From: Oliver Neukum <oneukum@suse.de>
> Date: Thu, 24 Jul 2014 13:56:29 +0200
> 
> > +			1000 /* out of thin air */
> 
> Just use USB_CTRL_SET_TIMEOUT if you can't come up with a specific
> reason to use another value.
> 
> Thanks.

Certainly

	Sorry
		Oliver