Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions

["Bjørn Mork" <bjorn@mork.no>]

<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