Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: "Bjørn Mork" <bjorn@mork.no>
To: <Charles.Hyde@dellteam.com>
Cc: <oliver@neukum.org>, <rjw@rjwysocki.net>, <lenb@kernel.org>,
	<Mario.Limonciello@dell.com>, <chip.programmer@gmail.com>,
	<nic_swsd@realtek.com>, <linux-usb@vger.kernel.org>,
	<linux-acpi@vger.kernel.org>
Subject: Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
Date: Fri, 06 Sep 2019 11:02:52 +0200
Message-ID: <87zhjhssoz.fsf@miraculix.mork.no> (raw)
In-Reply-To: <1567717318990.49322@Dellteam.com> (Charles Hyde's message of "Thu, 5 Sep 2019 21:01:59 +0000")

<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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05 21:01 Charles.Hyde
2019-09-06  9:02 ` Bjørn Mork [this message]
2019-09-06 18:00   ` Charles.Hyde
2019-09-06 18:15     ` Alan Stern
2019-09-06 18:19       ` Charles.Hyde
2019-09-06 20:07         ` Bjørn Mork
2019-09-06 20:20           ` Charles.Hyde
2019-09-06 20:39             ` Bjørn Mork
2019-09-06 21:00               ` Charles.Hyde
  -- strict thread matches above, loose matches on Subject: below --
2019-08-30 19:37 Charles.Hyde
2019-09-02  9:43 ` Oliver Neukum
2019-09-03 16:45   ` Charles.Hyde

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87zhjhssoz.fsf@miraculix.mork.no \
    --to=bjorn@mork.no \
    --cc=Charles.Hyde@dellteam.com \
    --cc=Mario.Limonciello@dell.com \
    --cc=chip.programmer@gmail.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=nic_swsd@realtek.com \
    --cc=oliver@neukum.org \
    --cc=rjw@rjwysocki.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git