| Message ID | 1567717318990.49322@Dellteam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] net: cdc_ncm: add get/set ethernet address functions | expand |
<Charles.Hyde@dellteam.com> writes: > +static int cdc_ncm_get_ethernet_address(struct usbnet *dev, > + struct cdc_ncm_ctx *ctx) Is this function called anywhere? Shouldn't it replace the usbnet_get_ethernet_addr() call in cdc_ncm_bind_common()? But do note that cdc_ncm_bind_common() is shared with cdc_mbim and huawei_cdc_ncm, and that NCM specific code therefore must be conditional. That's why the usbnet_get_ethernet_addr() call is currently wrapped in 'if (ctx->ether_desc) {}'. You should definitely not try to do USB_CDC_GET_NET_ADDRESS or USB_CDC_SET_NET_ADDRESS on MBIM. I don't know about the Huawei firmwares. But I believe it's best to be careful and avoid these requests there too. Unless you have a number of different Huawei devices with assorted firmware revisions for testing. CDC NCM compliance is obviously not a requirement for their vendor specific dialect. > +{ > + int ret; > + char *buf; > + > + buf = kmalloc(ETH_ALEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; usbnet_read_cmd() will kmalloc() yet another buffer, so this is completely pointless. Just use the stack for the 6 byte buffer. Or let it write directly to dev->net->dev_addr, since you will fall back to usbnet_get_ethernet_addr() anyway if the request fails. > + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS, > + USB_DIR_IN | USB_TYPE_CLASS > + | USB_RECIP_INTERFACE, 0, > + USB_REQ_SET_ADDRESS, buf, ETH_ALEN); Where did that USB_REQ_SET_ADDRESS come from? Did you just look up an arbutrary macro that happened to match your device config? How do you expect this to work with a generic NCM device? Or even your own device, when the next firmware revision moves the function to a different interface? It's nice to have USB_CDC_GET_NET_ADDRESS and USB_CDC_GET_NET_ADDRESS implemented, but there are a large number of NCM devices. You should probably test the code with one or two other than your own. Note that few, if any, NCM devices are spec compliant. You should expect at least one of them to do something really stupid when the see this optional and unexpected request. I think it would be wise to avoid sending unsupported requests more than once to a device. > + if (ret == ETH_ALEN) { > + memcpy(dev->net->dev_addr, buf, ETH_ALEN); > + ret = 0; /* success */ > + } else { > + ret = usbnet_get_ethernet_addr(dev, > + ctx->ether_desc->iMACAddress); > + } > + kfree(buf); > + return ret; If you passed dev->net->dev_addr above, then this could be simplified to if (ret == ETH_ALEN) return 0; return usbnet_get_ethernet_addr(dev,..); > + > +/* Provide method to push MAC address to the USB device's ethernet controller. > + * If the device does not support CDC_SET_ADDRESS, there is no harm and we > + * proceed as before. > + */ > +static int cdc_ncm_set_ethernet_address(struct usbnet *dev, > + struct sockaddr *addr) > +{ > + int ret; > + char *buf; > + > + buf = kmalloc(ETH_ALEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + memcpy(buf, addr->sa_data, ETH_ALEN); > + ret = usbnet_write_cmd(dev, USB_CDC_SET_NET_ADDRESS, > + USB_DIR_OUT | USB_TYPE_CLASS > + | USB_RECIP_INTERFACE, 0, > + USB_REQ_SET_ADDRESS, buf, ETH_ALEN); Same comments as above. Bjørn
<snipped> > > + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS, > > + USB_DIR_IN | USB_TYPE_CLASS > > + | USB_RECIP_INTERFACE, 0, > > + USB_REQ_SET_ADDRESS, buf, ETH_ALEN); > > Where did that USB_REQ_SET_ADDRESS come from? Did you just look up an > arbutrary macro that happened to match your device config? How do you > expect this to work with a generic NCM device? Or even your own device, > when the next firmware revision moves the function to a different interface? <snipped> https://wiki.osdev.org/Universal_Serial_Bus#SET_ADDRESS https://www.usb.org/document-library/network-control-model-devices-specification-v10-and-errata-and-adopters-agreement Download and view the NCM specification v1.0 (Errata 1), dated November 24, 2010. See table 6.2 on page 30. Also see sections 6.2.2 and 6.2.3 on page 32. USB_REQ_SET_ADDRESS came from include/uapi/linux/usb/ch9.h. This matches the SET_ADDRESS definition from the osdev wiki page, so I used it because the name and numeric values both matched. It further matches what the Windows driver issues.
On Fri, 6 Sep 2019 Charles.Hyde@dellteam.com wrote: > <snipped> > > > + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS, > > > + USB_DIR_IN | USB_TYPE_CLASS > > > + | USB_RECIP_INTERFACE, 0, > > > + USB_REQ_SET_ADDRESS, buf, ETH_ALEN); > > > > Where did that USB_REQ_SET_ADDRESS come from? Did you just look up an > > arbutrary macro that happened to match your device config? How do you > > expect this to work with a generic NCM device? Or even your own device, > > when the next firmware revision moves the function to a different interface? > <snipped> > > https://wiki.osdev.org/Universal_Serial_Bus#SET_ADDRESS > > https://www.usb.org/document-library/network-control-model-devices-specification-v10-and-errata-and-adopters-agreement > Download and view the NCM specification v1.0 (Errata 1), dated November 24, 2010. See table 6.2 on page 30. Also see sections 6.2.2 and 6.2.3 on page 32. > > USB_REQ_SET_ADDRESS came from include/uapi/linux/usb/ch9.h. This matches the SET_ADDRESS definition from the osdev wiki page, so I used it because the name and numeric values both matched. It further matches what the Windows driver issues. The names and values may match, but the meanings do not. USB_REQ_SET_ADDRESS refers to a USB bus address, not a MAC address. Alan Stern
> > <snipped> > > > > + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS, > > > > + USB_DIR_IN | USB_TYPE_CLASS > > > > + | USB_RECIP_INTERFACE, 0, > > > > + USB_REQ_SET_ADDRESS, buf, ETH_ALEN); > > > > > > Where did that USB_REQ_SET_ADDRESS come from? Did you just look up > > > an arbutrary macro that happened to match your device config? How > > > do you expect this to work with a generic NCM device? Or even your > > > own device, when the next firmware revision moves the function to a > different interface? > > <snipped> > > > > https://wiki.osdev.org/Universal_Serial_Bus#SET_ADDRESS > > > > https://www.usb.org/document-library/network-control-model-devices-spe > > cification-v10-and-errata-and-adopters-agreement > > Download and view the NCM specification v1.0 (Errata 1), dated November > 24, 2010. See table 6.2 on page 30. Also see sections 6.2.2 and 6.2.3 on page > 32. > > > > USB_REQ_SET_ADDRESS came from include/uapi/linux/usb/ch9.h. This > matches the SET_ADDRESS definition from the osdev wiki page, so I used it > because the name and numeric values both matched. It further matches what > the Windows driver issues. > > The names and values may match, but the meanings do not. > USB_REQ_SET_ADDRESS refers to a USB bus address, not a MAC address. > > Alan Stern What better suggestion do folks have, instead of using USB_REQ_SET_ADDRESS? Thank you, Charles
<Charles.Hyde@dellteam.com> writes:
> What better suggestion do folks have, instead of using USB_REQ_SET_ADDRESS?
The spec is clear: wIndex is supposed to be 'NCM Communications
Interface'. That's how you address a specific NCM function (a USB
device can have more than one...), and that's what you'll see in all the
other interface specific class requests in this driver. You don't have
to look hard to find examples.
Bjørn
> > What better suggestion do folks have, instead of using > USB_REQ_SET_ADDRESS? > > The spec is clear: wIndex is supposed to be 'NCM Communications Interface'. > That's how you address a specific NCM function (a USB device can have more > than one...), and that's what you'll see in all the other interface specific class > requests in this driver. You don't have to look hard to find examples. > > > Bjørn I have presented what works, with the v3 patch series. Since you obviously do not like what does work, I will ask you to provide clear and concise code for what you believe ought to work. Mind you, the code I have provided sends the exact same USB message as I traced with Wireshark on my Windows system. If you can provide good working code that replicates what I have provided, I would be thrilled. Thank you, Charles
<Charles.Hyde@dellteam.com> writes: >> > What better suggestion do folks have, instead of using >> USB_REQ_SET_ADDRESS? >> >> The spec is clear: wIndex is supposed to be 'NCM Communications Interface'. >> That's how you address a specific NCM function (a USB device can have more >> than one...), and that's what you'll see in all the other interface specific class >> requests in this driver. You don't have to look hard to find examples. >> >> >> Bjørn > > > I have presented what works, with the v3 patch series. Sure. It will work iff your NCM function has a control interface numbered 5. Most NCM functions do not. > Mind you, the code I have provided sends the exact same USB message as > I traced with Wireshark on my Windows system. Snooping on communcation with one specific device is not a good way to figure out dynamic content. wIndex cannot be a constant. It depends on the device configuration. >If you can provide good working code that replicates what I have >provided, I would be thrilled. There is working control request code a few lines up in the driver. I didn't think it was too much to ask that you looked it up. But I can copy an example here too: static int cdc_ncm_init(struct usbnet *dev) { struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; int err; err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS, USB_TYPE_CLASS | USB_DIR_IN |USB_RECIP_INTERFACE, 0, iface_no, &ctx->ncm_parm, sizeof(ctx->ncm_parm)); ,, You'll obviously have to replace USB_CDC_GET_NTB_PARAMETERS with USB_CDC_GET_NET_ADDRESS, &ctx->ncm_parm with buf, and sizeof(ctx->ncm_parm) with ETH_ALEN. Bjørn
<snipped> > static int cdc_ncm_init(struct usbnet *dev) { > struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; > u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; > int err; > > err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS, > USB_TYPE_CLASS | USB_DIR_IN > |USB_RECIP_INTERFACE, > 0, iface_no, &ctx->ncm_parm, > sizeof(ctx->ncm_parm)); > ,, > > You'll obviously have to replace USB_CDC_GET_NTB_PARAMETERS with > USB_CDC_GET_NET_ADDRESS, &ctx->ncm_parm with buf, and > sizeof(ctx->ncm_parm) with ETH_ALEN. > > > Bjørn Not everything is obvious to those who do not live and breathe USB. This has been an experience. Is this snippet what you have in mind? Will iface_no be correct? If not, then what do you suggest? /* Provide method to push MAC address to the USB device's ethernet controller. * If the device does not support CDC_SET_ADDRESS, there is no harm and we * proceed as before. */ static int cdc_ncm_set_ethernet_address(struct usbnet *dev, struct sockaddr *addr) { int ret; struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; ret = usbnet_write_cmd(dev, USB_CDC_SET_NET_ADDRESS, USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE, 0, iface_no, addr->sa_data, ETH_ALEN); if (ret == ETH_ALEN) ret = 0; /* success */ else if (ret < 0) dev_dbg(&dev->udev->dev, "bad MAC address put, %d\n", ret); return ret; }
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 50c05d0f44cb..85093579612f 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -750,6 +750,78 @@ int cdc_ncm_change_mtu(struct net_device *net, int new_mtu) } EXPORT_SYMBOL_GPL(cdc_ncm_change_mtu); +/* Provide method to get MAC address from the USB device's ethernet controller. + * If the device supports CDC_GET_ADDRESS, we should receive just six bytes. + * Otherwise, use the prior method by asking for the descriptor. + */ +static int cdc_ncm_get_ethernet_address(struct usbnet *dev, + struct cdc_ncm_ctx *ctx) +{ + int ret; + char *buf; + + buf = kmalloc(ETH_ALEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + ret = usbnet_read_cmd(dev, USB_CDC_GET_NET_ADDRESS, + USB_DIR_IN | USB_TYPE_CLASS + | USB_RECIP_INTERFACE, 0, + USB_REQ_SET_ADDRESS, buf, ETH_ALEN); + if (ret == ETH_ALEN) { + memcpy(dev->net->dev_addr, buf, ETH_ALEN); + ret = 0; /* success */ + } else { + ret = usbnet_get_ethernet_addr(dev, + ctx->ether_desc->iMACAddress); + } + kfree(buf); + return ret; +} + +/* Provide method to push MAC address to the USB device's ethernet controller. + * If the device does not support CDC_SET_ADDRESS, there is no harm and we + * proceed as before. + */ +static int cdc_ncm_set_ethernet_address(struct usbnet *dev, + struct sockaddr *addr) +{ + int ret; + char *buf; + + buf = kmalloc(ETH_ALEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + memcpy(buf, addr->sa_data, ETH_ALEN); + ret = usbnet_write_cmd(dev, USB_CDC_SET_NET_ADDRESS, + USB_DIR_OUT | USB_TYPE_CLASS + | USB_RECIP_INTERFACE, 0, + USB_REQ_SET_ADDRESS, buf, ETH_ALEN); + if (ret == ETH_ALEN) + ret = 0; /* success */ + else if (ret < 0) + dev_dbg(&dev->udev->dev, "bad MAC address put, %d\n", ret); + + kfree(buf); + return ret; +} + +/* Provide method to push MAC address to the USB device's ethernet controller. + */ +int cdc_ncm_set_mac_addr(struct net_device *net, void *p) +{ + struct usbnet *dev = netdev_priv(net); + + /* Try to push the MAC address out to the device. Ignore any errors, + * to be compatible with prior versions of this source. + */ + cdc_ncm_set_ethernet_address(dev, (struct sockaddr *)p); + + return eth_mac_addr(net, p); +} +EXPORT_SYMBOL_GPL(cdc_ncm_set_mac_addr); + static const struct net_device_ops cdc_ncm_netdev_ops = { .ndo_open = usbnet_open, .ndo_stop = usbnet_stop, @@ -757,7 +829,7 @@ static const struct net_device_ops cdc_ncm_netdev_ops = { .ndo_tx_timeout = usbnet_tx_timeout, .ndo_get_stats64 = usbnet_get_stats64, .ndo_change_mtu = cdc_ncm_change_mtu, - .ndo_set_mac_address = eth_mac_addr, + .ndo_set_mac_address = cdc_ncm_set_mac_addr, .ndo_validate_addr = eth_validate_addr, };
This patch adds support for pushing a MAC address out to USB based ethernet controllers driven by cdc_ncm. With this change, ifconfig can now set the device's MAC address. For example, the Dell Universal Dock D6000 is driven by cdc_ncm. The D6000 can now have its MAC address set by ifconfig, as it can be done in Windows. This was tested with a D6000 using ifconfig on an x86 based chromebook, where iproute2 is not available. Signed-off-by: Charles Hyde <charles.hyde@dellteam.com> Cc: Mario Limonciello <mario.limonciello@dell.com> Cc: chip.programmer@gmail.com Cc: Oliver Neukum <oliver@neukum.org> Cc: linux-usb@vger.kernel.org --- drivers/net/usb/cdc_ncm.c | 74 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-)