[PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Kristian Evensen]

The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to
determine which type of device to export. In addition, these devices export
a REST API which can also be used to control the type of device. So far, on
Linux, the devices have been seen as RNDIS or CDC Ether.

When CDC Ether is used, devices of the same type are, as with RNDIS,
exported with the same, bogus random MAC address. In addition, the devices
(at least on all firmware revisions I have found) use this MAC when sending
traffic routed from external networks. And as a final feature, the devices
sometimes export the link state incorrectly.

This patch tries to improve the handling of these devices by doing the
following:

* If a random MAC is read from device, then generate a new random MAC
address. This fix will apply to all devices using cdc_ether, but that
should be ok as we will also fix similar mistakes from other
manufacturers.

* The MF823/MF832/MF910 sometimes export cdc carrier on twice on connect
(the correct behavior is off then on). Work around this by manually setting
carrier to off if an on-notification is received and the NOCARRIER-bit is
not set.

This change will also affect all devices, but as with the MAC-fix it should
take care of similar mistakes. I tried to think of/look/test for
problems/regressions that could be introduced by this behavior, but could
not find any. However, my familiarity with this code path is not that
great, so there could be something I have overlooked.

* Add an rx_fixup-function (and a new driver info-struct) which is used by
these three devices (identified either as PID 0x1405 or 0x1408). The
rx_fixup-function replaces the destination MAC address in the skb with that
of the device. I have not seen a revision of these three device that
behaves correctly (i.e., sets the right destination MAC), so I chose not to
do any comparison with for example the known, bogus addresses.

I have tested this patch with multiple revisions of all three devices, and
they behave as expected. In other words, they all got a valid, random MAC,
the correct operational state and I can receive/sent traffic without
problems. I also tested with some other cdc_ether devices I have and did
not find any problems/regressions caused by the two general changes.

Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>
---
 drivers/net/usb/cdc_ether.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 7cba2c3..2325097 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -388,6 +388,12 @@ void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
 	case USB_CDC_NOTIFY_NETWORK_CONNECTION:
 		netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
 			  event->wValue ? "on" : "off");
+
+		/* Work-around for devices with broken off-notifications */
+		if (event->wValue &&
+		    !test_bit(__LINK_STATE_NOCARRIER, &dev->net->state))
+			usbnet_link_change(dev, 0, 0);
+
 		usbnet_link_change(dev, !!event->wValue, 0);
 		break;
 	case USB_CDC_NOTIFY_SPEED_CHANGE:	/* tx/rx rates */
@@ -428,10 +434,34 @@ int usbnet_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 		return status;
 	}
 
+	if (dev->net->dev_addr[0] & 0x02)
+		eth_hw_addr_random(dev->net);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usbnet_cdc_bind);
 
+/* Make sure packets have correct destination MAC address
+ *
+ * A firmware bug observed on some devices (ZTE MF823/831/910) is that the
+ * device sends packets with a static, bogus, random MAC address (event if
+ * device MAC address has been updated). Always set MAC address to that of the
+ * device.
+ */
+static int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+{
+	u8 num_buggy_hwaddrs;
+	u8 buggy_hwaddrs_idx = 0;
+
+	if (skb->len < ETH_HLEN || !(skb->data[0] & 0x02))
+		return 1;
+
+	skb_reset_mac_header(skb);
+	ether_addr_copy(eth_hdr(skb)->h_dest, dev->net->dev_addr);
+
+	return 1;
+}
+
 static const struct driver_info	cdc_info = {
 	.description =	"CDC Ethernet Device",
 	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT,
@@ -442,6 +472,17 @@ static const struct driver_info	cdc_info = {
 	.manage_power =	usbnet_manage_power,
 };
 
+static const struct driver_info	zte_cdc_info = {
+	.description =	"ZTE MF823/831/910 CDC Ethernet Device",
+	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT,
+	.bind =		usbnet_cdc_bind,
+	.unbind =	usbnet_cdc_unbind,
+	.status =	usbnet_cdc_status,
+	.set_rx_mode =	usbnet_cdc_update_filter,
+	.manage_power =	usbnet_manage_power,
+	.rx_fixup = usbnet_cdc_zte_rx_fixup,
+};
+
 static const struct driver_info wwan_info = {
 	.description =	"Mobile Broadband Network Device",
 	.flags =	FLAG_WWAN,
@@ -697,6 +738,18 @@ static const struct usb_device_id	products[] = {
 				      USB_CDC_PROTO_NONE),
 	.driver_info = (unsigned long)&wwan_info,
 }, {
+	/* ZTE MF823/831/910 */
+	USB_DEVICE_AND_INTERFACE_INFO(ZTE_VENDOR_ID, 0x1405, USB_CLASS_COMM,
+				      USB_CDC_SUBCLASS_ETHERNET,
+				      USB_CDC_PROTO_NONE),
+	.driver_info = (unsigned long)&zte_cdc_info,
+}, {
+	/* ZTE MF823/831/910 */
+	USB_DEVICE_AND_INTERFACE_INFO(ZTE_VENDOR_ID, 0x1408, USB_CLASS_COMM,
+				      USB_CDC_SUBCLASS_ETHERNET,
+				      USB_CDC_PROTO_NONE),
+	.driver_info = (unsigned long)&zte_cdc_info,
+}, {
 	/* Telit modules */
 	USB_VENDOR_AND_INTERFACE_INFO(0x1bc7, USB_CLASS_COMM,
 			USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
-- 
2.5.0

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-07-18 at 14:24 +0200, Kristian Evensen wrote:
> The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to
> determine which type of device to export. In addition, these devices export
> a REST API which can also be used to control the type of device. So far, on
> Linux, the devices have been seen as RNDIS or CDC Ether.
> 
> When CDC Ether is used, devices of the same type are, as with RNDIS,
> exported with the same, bogus random MAC address. In addition, the devices

Please explain. If the MAC is random, I fail to see why the host would
be any better at making up a MAC.

> (at least on all firmware revisions I have found) use this MAC when sending
> traffic routed from external networks. And as a final feature, the devices
> sometimes export the link state incorrectly.
> 
> This patch tries to improve the handling of these devices by doing the
> following:
> 
> * If a random MAC is read from device, then generate a new random MAC
> address. This fix will apply to all devices using cdc_ether, but that
> should be ok as we will also fix similar mistakes from other
> manufacturers.

I am not really happy with this.

If this is specific to the device a quirk should be used. If it is a
truly generic misdesign, it does not belong into cdc-ether. It should go
into the generic layer.

> * The MF823/MF832/MF910 sometimes export cdc carrier on twice on connect
> (the correct behavior is off then on). Work around this by manually setting
> carrier to off if an on-notification is received and the NOCARRIER-bit is
> not set.
> 
> This change will also affect all devices, but as with the MAC-fix it should
> take care of similar mistakes. I tried to think of/look/test for
> problems/regressions that could be introduced by this behavior, but could
> not find any. However, my familiarity with this code path is not that
> great, so there could be something I have overlooked.

Looks OK

> * Add an rx_fixup-function (and a new driver info-struct) which is used by
> these three devices (identified either as PID 0x1405 or 0x1408). The
> rx_fixup-function replaces the destination MAC address in the skb with that
> of the device. I have not seen a revision of these three device that
> behaves correctly (i.e., sets the right destination MAC), so I chose not to
> do any comparison with for example the known, bogus addresses.

Looks OK

	Regards
		Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Kristian Evensen]

Hi,

On Mon, Jul 18, 2016 at 3:01 PM, Oliver Neukum <oneukum@suse.com> wrote:
> On Mon, 2016-07-18 at 14:24 +0200, Kristian Evensen wrote:
>> The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to
>> determine which type of device to export. In addition, these devices export
>> a REST API which can also be used to control the type of device. So far, on
>> Linux, the devices have been seen as RNDIS or CDC Ether.
>>
>> When CDC Ether is used, devices of the same type are, as with RNDIS,
>> exported with the same, bogus random MAC address. In addition, the devices
>
> Please explain. If the MAC is random, I fail to see why the host would
> be any better at making up a MAC.

Sorry for not explaining this properly. The problem is that all
devices of the same type store the same value in iMACAddress. On all
MF831/MF910s (that I have seen) 36:4b:50:b7:ef:da is stored in this
value, while 36:4b:50:b7:ef:38 is stored on MF823. In order to ensure
that all devices on a host has a unique MAC-address, I think it is
better to generate a new random MAC on the host than to trust the
value returned from the device.

>> * If a random MAC is read from device, then generate a new random MAC
>> address. This fix will apply to all devices using cdc_ether, but that
>> should be ok as we will also fix similar mistakes from other
>> manufacturers.
>
> I am not really happy with this.
>
> If this is specific to the device a quirk should be used. If it is a
> truly generic misdesign, it does not belong into cdc-ether. It should go
> into the generic layer.

We saw the same behaviour when these devices are exposed as RNDIS and
decided to apply a generic fix instead of limiting ourself to for
example the two, known bogus MAC address. See commit
a5a18bdf7453d505783e40e47ebb84bfdd35f93b and the thread "[PATCH]
rndis_host: Set random MAC for ZTE MF910"
(http://www.spinics.net/lists/linux-usb/msg143727.html) for the
discussion.

However, I have no strong objections towards for example checking
against the two bad MAC addresses before generating a random MAC.

-Kristian

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-07-18 at 15:23 +0200, Kristian Evensen wrote:
> Hi,
> 
> On Mon, Jul 18, 2016 at 3:01 PM, Oliver Neukum <oneukum@suse.com> wrote:
> > On Mon, 2016-07-18 at 14:24 +0200, Kristian Evensen wrote:
> >> The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to
> >> determine which type of device to export. In addition, these devices export
> >> a REST API which can also be used to control the type of device. So far, on
> >> Linux, the devices have been seen as RNDIS or CDC Ether.
> >>
> >> When CDC Ether is used, devices of the same type are, as with RNDIS,
> >> exported with the same, bogus random MAC address. In addition, the devices
> >
> > Please explain. If the MAC is random, I fail to see why the host would
> > be any better at making up a MAC.
> 
> Sorry for not explaining this properly. The problem is that all
> devices of the same type store the same value in iMACAddress. On all
> MF831/MF910s (that I have seen) 36:4b:50:b7:ef:da is stored in this
> value, while 36:4b:50:b7:ef:38 is stored on MF823. In order to ensure
> that all devices on a host has a unique MAC-address, I think it is
> better to generate a new random MAC on the host than to trust the
> value returned from the device.

OK, thanks for explaining.

> >> * If a random MAC is read from device, then generate a new random MAC
> >> address. This fix will apply to all devices using cdc_ether, but that
> >> should be ok as we will also fix similar mistakes from other
> >> manufacturers.
> >
> > I am not really happy with this.
> >
> > If this is specific to the device a quirk should be used. If it is a
> > truly generic misdesign, it does not belong into cdc-ether. It should go
> > into the generic layer.
> 
> We saw the same behaviour when these devices are exposed as RNDIS and
> decided to apply a generic fix instead of limiting ourself to for
> example the two, known bogus MAC address. See commit
> a5a18bdf7453d505783e40e47ebb84bfdd35f93b and the thread "[PATCH]
> rndis_host: Set random MAC for ZTE MF910"
> (http://www.spinics.net/lists/linux-usb/msg143727.html) for the
> discussion.
> 
> However, I have no strong objections towards for example checking
> against the two bad MAC addresses before generating a random MAC.

No, you misunderstand me. I don't want quirks if we can avoid it.
But if you need to do this for rndis_host and cdc_ether and maybe other
drivers you should not be touching drivers. This belongs into the core
ethernet code. Your code is good, but you are putting it in the wrong
places.

	Regards
		Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Kristian Evensen]

On Mon, Jul 18, 2016 at 3:50 PM, Oliver Neukum <oneukum@suse.com> wrote:
> No, you misunderstand me. I don't want quirks if we can avoid it.
> But if you need to do this for rndis_host and cdc_ether and maybe other
> drivers you should not be touching drivers. This belongs into the core
> ethernet code. Your code is good, but you are putting it in the wrong
> places.

Ok, sounds good. So far, I have only seen the random MAC issue with
the three previously mentioned devices, but who knows how many else is
out there with the same error ... I don't think it should be in the
core ethernet code, at least not yet, but I agree it would make sense
to move it to for example usbnet_core(). If you agree, I can prepare a
patch for it.

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-07-18 at 16:10 +0200, Kristian Evensen wrote:
> On Mon, Jul 18, 2016 at 3:50 PM, Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org> wrote:
> > No, you misunderstand me. I don't want quirks if we can avoid it.
> > But if you need to do this for rndis_host and cdc_ether and maybe other
> > drivers you should not be touching drivers. This belongs into the core
> > ethernet code. Your code is good, but you are putting it in the wrong
> > places.
> 
> Ok, sounds good. So far, I have only seen the random MAC issue with
> the three previously mentioned devices, but who knows how many else is
> out there with the same error ... I don't think it should be in the
> core ethernet code, at least not yet, but I agree it would make sense
> to move it to for example usbnet_core(). If you agree, I can prepare a
> patch for it.

I don't see how it would be specific for a subsystem. If the patch
is correct, it belongs into the networking core.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Kristian Evensen]

On Mon, Jul 18, 2016 at 4:14 PM, Oliver Neukum <oneukum@suse.com> wrote:
>> Ok, sounds good. So far, I have only seen the random MAC issue with
>> the three previously mentioned devices, but who knows how many else is
>> out there with the same error ... I don't think it should be in the
>> core ethernet code, at least not yet, but I agree it would make sense
>> to move it to for example usbnet_core(). If you agree, I can prepare a
>> patch for it.
>
> I don't see how it would be specific for a subsystem. If the patch
> is correct, it belongs into the networking core.

To me it sounds a bit intrusive to change the networking core for
something that has so far only been seen with devices belonging to one
subsystem, but I will do it if required. I guess David M. will have an
opinion on if we should add this fix on a per-driver basis, in usbnet
or in the networking core? If we go for the latter, what would be
correct place for this piece of code, register_netdevice()?

-Kristian

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Kristian Evensen]

On Mon, Jul 18, 2016 at 4:14 PM, Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org> wrote:
>> Ok, sounds good. So far, I have only seen the random MAC issue with
>> the three previously mentioned devices, but who knows how many else is
>> out there with the same error ... I don't think it should be in the
>> core ethernet code, at least not yet, but I agree it would make sense
>> to move it to for example usbnet_core(). If you agree, I can prepare a
>> patch for it.
>
> I don't see how it would be specific for a subsystem. If the patch
> is correct, it belongs into the networking core.

I had a look at some other drivers, and I think we need to be very
careful about making setting a random MAC too generic. For example, we
might be unlucky and break the possibly_iphdr()-code/assumption in
qmi_wwan.c. And there is probably more code/assumptions like that in
the network stack.

-Kristian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-07-18 at 17:04 +0200, Kristian Evensen wrote:
> On Mon, Jul 18, 2016 at 4:14 PM, Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org> wrote:
> >> Ok, sounds good. So far, I have only seen the random MAC issue with
> >> the three previously mentioned devices, but who knows how many else is
> >> out there with the same error ... I don't think it should be in the
> >> core ethernet code, at least not yet, but I agree it would make sense
> >> to move it to for example usbnet_core(). If you agree, I can prepare a
> >> patch for it.
> >
> > I don't see how it would be specific for a subsystem. If the patch
> > is correct, it belongs into the networking core.
> 
> I had a look at some other drivers, and I think we need to be very
> careful about making setting a random MAC too generic. For example, we
> might be unlucky and break the possibly_iphdr()-code/assumption in
> qmi_wwan.c. And there is probably more code/assumptions like that in
> the network stack.

In this case please use special cases for usbnet, too.
We need a quirk.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Kristian Evensen]

On Tue, Jul 19, 2016 at 8:20 AM, Oliver Neukum <oneukum@suse.com> wrote:
>> I had a look at some other drivers, and I think we need to be very
>> careful about making setting a random MAC too generic. For example, we
>> might be unlucky and break the possibly_iphdr()-code/assumption in
>> qmi_wwan.c. And there is probably more code/assumptions like that in
>> the network stack.
>
> In this case please use special cases for usbnet, too.
> We need a quirk.

I guess I can match on the VID/PID in usbnet, but won't it be cleaner
to add a new bind() function (in cdc_ether) which matches the two PIDs
and leave usbnet as is? Or am I misunderstanding how to add this
functionality to usbnet?

-Kristian

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Tue, 2016-07-19 at 08:40 +0200, Kristian Evensen wrote:
> On Tue, Jul 19, 2016 at 8:20 AM, Oliver Neukum <oneukum@suse.com> wrote:
> >> I had a look at some other drivers, and I think we need to be very
> >> careful about making setting a random MAC too generic. For example, we
> >> might be unlucky and break the possibly_iphdr()-code/assumption in
> >> qmi_wwan.c. And there is probably more code/assumptions like that in
> >> the network stack.
> >
> > In this case please use special cases for usbnet, too.
> > We need a quirk.
> 
> I guess I can match on the VID/PID in usbnet, but won't it be cleaner
> to add a new bind() function (in cdc_ether) which matches the two PIDs
> and leave usbnet as is? Or am I misunderstanding how to add this
> functionality to usbnet?

It would be cleaner, but it seems to me that multiple quirky devices
driven by diverse drivers use those bogus MACs. If you want to catch
them at a central place, it has to be usbnet. It is a matter of taste.
I am fine with either solution.

	Regards
		Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Lars Melin]

On 2016-07-19 13:40, Kristian Evensen wrote:

> I guess I can match on the VID/PID in usbnet, but won't it be cleaner
> to add a new bind() function (in cdc_ether) which matches the two PIDs
> and leave usbnet as is? Or am I misunderstanding how to add this
> functionality to usbnet?
>

Matching on the usb id is probably not a great idea, there is more id's
than the two you have found and there is also more than two non-unique 
mac addresses.

Example:

0200FFAAAAAA  19d2:1589/1592/1595
020CE70B0102  19d2:1040/1048/1405

You can easily find them by googling them, without colon separators you
will find them in verbose lsusb listings, with colons you will find them 
in dmesg pastings.

I would probably have found more dupes if users had refrained from using 
the stupid usbdevices cmd which removes almost all interesting info from 
device listings in internet foras.

/Lars

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Kristian Evensen]

Hi Lars,

On Tue, Jul 19, 2016 at 10:30 AM, Lars Melin <larsm17@gmail.com> wrote:
> On 2016-07-19 13:40, Kristian Evensen wrote:
>
>> I guess I can match on the VID/PID in usbnet, but won't it be cleaner
>> to add a new bind() function (in cdc_ether) which matches the two PIDs
>> and leave usbnet as is? Or am I misunderstanding how to add this
>> functionality to usbnet?
>>
>
> Matching on the usb id is probably not a great idea, there is more id's
> than the two you have found and there is also more than two non-unique mac
> addresses.
>
> Example:
>
> 0200FFAAAAAA  19d2:1589/1592/1595
> 020CE70B0102  19d2:1040/1048/1405
>
> You can easily find them by googling them, without colon separators you
> will find them in verbose lsusb listings, with colons you will find them in
> dmesg pastings.
>
> I would probably have found more dupes if users had refrained from using the
> stupid usbdevices cmd which removes almost all interesting info from device
> listings in internet foras.

Thanks for letting me know. It seems that the static MAC behaviour is
the default behaviour of ZTE-devices that use cdc_ether. Unless anyone
objects, I will make the following changes for v2:

* Create a ZTE-specific bind method in cdc_ether that checks for a
random MAC and generates a new MAC if found.
* Use the ZTE-specific bind + rx fixup for all ZTE cdc_ether devices
by default, and add exceptions if we find any devices not displaying
this behaviour. I see there are already five ZTE devices with separate
entries in the products-array in cdc_ether.

-Kristian

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:
> On Mon, 2016-07-18 at 16:10 +0200, Kristian Evensen wrote:
>> On Mon, Jul 18, 2016 at 3:50 PM, Oliver Neukum <oneukum@suse.com> wrote:
>> > No, you misunderstand me. I don't want quirks if we can avoid it.
>> > But if you need to do this for rndis_host and cdc_ether and maybe other
>> > drivers you should not be touching drivers. This belongs into the core
>> > ethernet code. Your code is good, but you are putting it in the wrong
>> > places.
>> 
>> Ok, sounds good. So far, I have only seen the random MAC issue with
>> the three previously mentioned devices, but who knows how many else is
>> out there with the same error ... I don't think it should be in the
>> core ethernet code, at least not yet, but I agree it would make sense
>> to move it to for example usbnet_core(). If you agree, I can prepare a
>> patch for it.
>
> I don't see how it would be specific for a subsystem. If the patch
> is correct, it belongs into the networking core.

The bug is in the firmware implementation of the "read unique vendor
assigned mac address" functions, and should therefore be fixed there.
It cannot be put into the networking core because "read unique vendor
assigned mac address" is a hardware dependent function.  It's
implemented in each ethernet driver based of whatever interface the
firmware provides to that register.

IMHO, usbnet_get_ethernet_addr() would be the most appropriate place for
cdc_ether and other CDC drivers. And generic_rndis_bind() is the correct
place for rndis_host.

Putting this in usbnet_get_ethernet_addr() will also fix the XMM7160
based devices having an FF:FF:FF:FF:FF:FF mac address (sic).  I'm pretty
sure there are other examples too.  There is no end to the creative
crazyness of firmware engineers...

An lsusb snippet example:

    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         2 Communications
      bInterfaceSubClass     13 
      bInterfaceProtocol      0 
      iInterface              5 Sierra Wireless EM7345 4G LTE (NCM)
      CDC Header:
        bcdCDC               1.20
      CDC Union:
        bMasterInterface        0
        bSlaveInterface         1 
      CDC NCM:
        bcdNcmVersion        1.00
        bmNetworkCapabilities 0x00
      CDC Ethernet:
        iMacAddress                      6 FFFFFFFFFFFF
        bmEthernetStatistics    0x00000000
        wMaxSegmentSize               1514
        wNumberMCFilters            0x0000
        bNumberPowerFilters              0


FWIW, putting the fix in usbnet_get_ethernet_addr() will not be a
problem for qmi_wwan.  It will further fix up the resulting random
address if required.


Bjørn

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-08-08 at 14:44 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.com> writes:

> > I don't see how it would be specific for a subsystem. If the patch
> > is correct, it belongs into the networking core.
> 
> The bug is in the firmware implementation of the "read unique vendor
> assigned mac address" functions, and should therefore be fixed there.

Well, if you define the semantics of the operation in that manner, it
certainly does. That however is not given. You could also define it
as returning the MAC the hardware listens to. The difference was just
unclear when the API was defined.

> It cannot be put into the networking core because "read unique vendor
> assigned mac address" is a hardware dependent function.  It's
> implemented in each ethernet driver based of whatever interface the
> firmware provides to that register.

Again, that depends on that particular semantics.

> IMHO, usbnet_get_ethernet_addr() would be the most appropriate place for
> cdc_ether and other CDC drivers. And generic_rndis_bind() is the correct
> place for rndis_host.
> 
> Putting this in usbnet_get_ethernet_addr() will also fix the XMM7160
> based devices having an FF:FF:FF:FF:FF:FF mac address (sic).  I'm pretty
> sure there are other examples too.  There is no end to the creative
> crazyness of firmware engineers...

It is clear that that would work. No question about that.

But why fix similar issues at two different places? And what about
PCI or other cards that show the same problem?

	Regards
		Oliver

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> But why fix similar issues at two different places? And what about
> PCI or other cards that show the same problem?

I guess some sort of common helper would be nice to avoid open coding
this fix everywhere.  But you would still have to modify every driver
where it is applicable, as there is no existing common API.

Note that this doesn't include *every* ethernet driver, although there
certainly are some examples.  There are also a number of serious
vendors, providing vendor supported drivers for cards with no known
issues of this kind.

Where exactly would you like to see this implemented if it isn't going
into those specific usbnet drivers?


Bjørn

Re: [PATCH net-next] cdc_ether: Improve ZTE MF823/831/910 handling

[Oliver Neukum]

On Mon, 2016-08-08 at 20:30 +0200, Bjørn Mork wrote:
> Note that this doesn't include *every* ethernet driver, although there
> certainly are some examples.  There are also a number of serious
> vendors, providing vendor supported drivers for cards with no known
> issues of this kind.
> 
> Where exactly would you like to see this implemented if it isn't going
> into those specific usbnet drivers?

Higher up. I'd prefer a flag to tell the network core that a driver
is unsure about the MAC and the core should deal with it.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html