Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
@ 2019-08-30 19:37 Charles.Hyde
  2019-09-02  9:43 ` Oliver Neukum
  0 siblings, 1 reply; 12+ messages in thread
From: Charles.Hyde @ 2019-08-30 19:37 UTC (permalink / raw)
  To: oliver, rjw, lenb; +Cc: linux-usb, linux-acpi, nic_swsd, Mario.Limonciello

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: 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(-)

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,
 };
 
-- 
2.20.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-08-30 19:37 [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions Charles.Hyde
@ 2019-09-02  9:43 ` Oliver Neukum
  2019-09-03 16:45   ` Charles.Hyde
  0 siblings, 1 reply; 12+ messages in thread
From: Oliver Neukum @ 2019-09-02  9:43 UTC (permalink / raw)
  To: Charles.Hyde, lenb, rjw
  Cc: Mario.Limonciello, nic_swsd, linux-acpi, linux-usb

Am Freitag, den 30.08.2019, 19:37 +0000 schrieb
Charles.Hyde@dellteam.com:
> 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.

> +/* 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);

You are throwing away error reports.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-02  9:43 ` Oliver Neukum
@ 2019-09-03 16:45   ` Charles.Hyde
  0 siblings, 0 replies; 12+ messages in thread
From: Charles.Hyde @ 2019-09-03 16:45 UTC (permalink / raw)
  To: oneukum, lenb, rjw; +Cc: Mario.Limonciello, nic_swsd, linux-acpi, linux-usb

> > 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.
> 
> > +/* 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);
> 
> You are throwing away error reports.
> 
> 	Regards
> 		Oliver


My intent is limit the changes needed to support setting the MAC address., thus any potential source code errors.  I have not changed the current behavior of the driver with respect to error processing.  The existing source does not check for error return values, therefore my changes also do not check error return values.

Charles

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-06 20:39             ` Bjørn Mork
@ 2019-09-06 21:00               ` Charles.Hyde
  0 siblings, 0 replies; 12+ messages in thread
From: Charles.Hyde @ 2019-09-06 21:00 UTC (permalink / raw)
  To: bjorn
  Cc: stern, oliver, rjw, lenb, Mario.Limonciello, chip.programmer,
	nic_swsd, linux-usb, linux-acpi

<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;
}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-06 20:20           ` Charles.Hyde
@ 2019-09-06 20:39             ` Bjørn Mork
  2019-09-06 21:00               ` Charles.Hyde
  0 siblings, 1 reply; 12+ messages in thread
From: Bjørn Mork @ 2019-09-06 20:39 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: stern, oliver, rjw, lenb, Mario.Limonciello, chip.programmer,
	nic_swsd, linux-usb, linux-acpi

<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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-06 20:07         ` Bjørn Mork
@ 2019-09-06 20:20           ` Charles.Hyde
  2019-09-06 20:39             ` Bjørn Mork
  0 siblings, 1 reply; 12+ messages in thread
From: Charles.Hyde @ 2019-09-06 20:20 UTC (permalink / raw)
  To: bjorn
  Cc: stern, oliver, rjw, lenb, Mario.Limonciello, chip.programmer,
	nic_swsd, linux-usb, linux-acpi

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-06 18:19       ` Charles.Hyde
@ 2019-09-06 20:07         ` Bjørn Mork
  2019-09-06 20:20           ` Charles.Hyde
  0 siblings, 1 reply; 12+ messages in thread
From: Bjørn Mork @ 2019-09-06 20:07 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: stern, oliver, rjw, lenb, Mario.Limonciello, chip.programmer,
	nic_swsd, linux-usb, linux-acpi

<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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-06 18:15     ` Alan Stern
@ 2019-09-06 18:19       ` Charles.Hyde
  2019-09-06 20:07         ` Bjørn Mork
  0 siblings, 1 reply; 12+ messages in thread
From: Charles.Hyde @ 2019-09-06 18:19 UTC (permalink / raw)
  To: stern
  Cc: bjorn, oliver, rjw, lenb, Mario.Limonciello, chip.programmer,
	nic_swsd, linux-usb, linux-acpi

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-06 18:00   ` Charles.Hyde
@ 2019-09-06 18:15     ` Alan Stern
  2019-09-06 18:19       ` Charles.Hyde
  0 siblings, 1 reply; 12+ messages in thread
From: Alan Stern @ 2019-09-06 18:15 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: bjorn, oliver, rjw, lenb, Mario.Limonciello, chip.programmer,
	nic_swsd, linux-usb, linux-acpi

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-06  9:02 ` Bjørn Mork
@ 2019-09-06 18:00   ` Charles.Hyde
  2019-09-06 18:15     ` Alan Stern
  0 siblings, 1 reply; 12+ messages in thread
From: Charles.Hyde @ 2019-09-06 18:00 UTC (permalink / raw)
  To: bjorn
  Cc: oliver, rjw, lenb, Mario.Limonciello, chip.programmer, nic_swsd,
	linux-usb, linux-acpi

<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.

-- 
Charles

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
  2019-09-05 21:01 Charles.Hyde
@ 2019-09-06  9:02 ` Bjørn Mork
  2019-09-06 18:00   ` Charles.Hyde
  0 siblings, 1 reply; 12+ messages in thread
From: Bjørn Mork @ 2019-09-06  9:02 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: oliver, rjw, lenb, Mario.Limonciello, chip.programmer, nic_swsd,
	linux-usb, linux-acpi

<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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions
@ 2019-09-05 21:01 Charles.Hyde
  2019-09-06  9:02 ` Bjørn Mork
  0 siblings, 1 reply; 12+ messages in thread
From: Charles.Hyde @ 2019-09-05 21:01 UTC (permalink / raw)
  To: oliver, rjw, lenb
  Cc: Mario.Limonciello, chip.programmer, nic_swsd, linux-usb, linux-acpi

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(-)

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,
 };
 
-- 
2.20.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-30 19:37 [PATCH 1/3] net: cdc_ncm: add get/set ethernet address functions Charles.Hyde
2019-09-02  9:43 ` Oliver Neukum
2019-09-03 16:45   ` Charles.Hyde
2019-09-05 21:01 Charles.Hyde
2019-09-06  9:02 ` Bjørn Mork
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

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 linux-acpi@archiver.kernel.org
	public-inbox-index linux-acpi


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