[RFC 1/4] Add usb_get_address and usb_set_address support

[RFC 1/4] Add usb_get_address and usb_set_address support

[Charles.Hyde]

The core USB driver message.c is missing get/set address functionality
that stops ifconfig from being able to push MAC addresses out to USB
based ethernet devices.  Without this functionality, some USB devices
stop responding to ethernet packets when using ifconfig to change MAC
addresses.  This has been tested with a Dell Universal Dock D6000.

Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
Cc: Mario Limonciello <mario.limonciello@dell.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
---
 drivers/usb/core/message.c | 59 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h        |  3 ++
 2 files changed, 62 insertions(+)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 5adf489428aa..eea775234b09 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1085,6 +1085,65 @@ int usb_clear_halt(struct usb_device *dev, int pipe)
 }
 EXPORT_SYMBOL_GPL(usb_clear_halt);
 
+/**
+ * usb_get_address - 
+ * @dev: device whose endpoint is halted
+ * @mac: buffer for containing 
+ * Context: !in_interrupt ()
+ *
+ * This will attempt to get the six byte MAC address from a USB device's
+ * ethernet controller using GET_NET_ADDRESS command.
+ *
+ * This call is synchronous, and may not be used in an interrupt context.
+ *
+ * Return: Zero on success, or else the status code returned by the
+ * underlying usb_control_msg() call.
+ */
+int usb_get_address(struct usb_device *dev, unsigned char * mac)
+{
+	int ret = -ENOMEM;
+	unsigned char *tbuf = kmalloc(256, GFP_NOIO);
+
+	if (!tbuf)
+		return -ENOMEM;
+
+	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+			USB_CDC_GET_NET_ADDRESS,
+			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			0, USB_REQ_SET_ADDRESS, tbuf, 256,
+			USB_CTRL_GET_TIMEOUT);
+	if (ret == 6)
+		memcpy(mac, tbuf, 6);
+
+	kfree(tbuf);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_get_address);
+
+/**
+ * usb_set_address - 
+ * @dev: device whose endpoint is halted
+ * @mac: desired MAC address in network address order
+ * Context: !in_interrupt ()
+ *
+ * This will attempt to set a six byte MAC address to the USB device's ethernet
+ * controller using SET_NET_ADDRESS command.
+ *
+ * This call is synchronous, and may not be used in an interrupt context.
+ *
+ * Return: Zero on success, or else the status code returned by the
+ * underlying usb_control_msg() call.
+ */
+int usb_set_address(struct usb_device *dev, unsigned char *mac)
+{
+	return usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+			USB_CDC_SET_NET_ADDRESS,
+			USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			0, USB_REQ_SET_ADDRESS, mac, 6,
+			USB_CTRL_SET_TIMEOUT);
+}
+EXPORT_SYMBOL_GPL(usb_set_address);
+
 static int create_intf_ep_devs(struct usb_interface *intf)
 {
 	struct usb_device *udev = interface_to_usbdev(intf);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index e87826e23d59..862c979d9821 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1806,6 +1806,9 @@ static inline int usb_get_ptm_status(struct usb_device *dev, void *data)
 extern int usb_string(struct usb_device *dev, int index,
 	char *buf, size_t size);
 
+extern int usb_get_address(struct usb_device *dev, unsigned char * mac);
+extern int usb_set_address(struct usb_device *dev, unsigned char * mac);
+
 /* wrappers that also update important state inside usbcore */
 extern int usb_clear_halt(struct usb_device *dev, int pipe);
 extern int usb_reset_configuration(struct usb_device *dev);
-- 
2.20.1

Re: [RFC 1/4] Add usb_get_address and usb_set_address support

[Greg KH]

On Tue, Aug 20, 2019 at 10:18:42PM +0000, Charles.Hyde@dellteam.com wrote:
> The core USB driver message.c is missing get/set address functionality
> that stops ifconfig from being able to push MAC addresses out to USB
> based ethernet devices.  Without this functionality, some USB devices
> stop responding to ethernet packets when using ifconfig to change MAC
> addresses.  This has been tested with a Dell Universal Dock D6000.
> 
> Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
> Cc: Mario Limonciello <mario.limonciello@dell.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> ---
>  drivers/usb/core/message.c | 59 ++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h        |  3 ++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 5adf489428aa..eea775234b09 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1085,6 +1085,65 @@ int usb_clear_halt(struct usb_device *dev, int pipe)
>  }
>  EXPORT_SYMBOL_GPL(usb_clear_halt);
>  
> +/**
> + * usb_get_address - 

trailing whitespace?

No description of what the function does?

> + * @dev: device whose endpoint is halted
> + * @mac: buffer for containing 

Trailing whitespace?

> + * Context: !in_interrupt ()
> + *
> + * This will attempt to get the six byte MAC address from a USB device's
> + * ethernet controller using GET_NET_ADDRESS command.
> + *
> + * This call is synchronous, and may not be used in an interrupt context.
> + *
> + * Return: Zero on success, or else the status code returned by the
> + * underlying usb_control_msg() call.
> + */
> +int usb_get_address(struct usb_device *dev, unsigned char * mac)
> +{
> +	int ret = -ENOMEM;
> +	unsigned char *tbuf = kmalloc(256, GFP_NOIO);
> +
> +	if (!tbuf)
> +		return -ENOMEM;
> +
> +	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> +			USB_CDC_GET_NET_ADDRESS,
> +			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +			0, USB_REQ_SET_ADDRESS, tbuf, 256,
> +			USB_CTRL_GET_TIMEOUT);
> +	if (ret == 6)
> +		memcpy(mac, tbuf, 6);
> +
> +	kfree(tbuf);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_get_address);

This is a VERY cdc-net-specific function.  It is not a "generic" USB
function at all.  Why does it belong in the USB core?  Shouldn't it live
in the code that handles the other cdc-net-specific logic?

thanks,

greg k-h

Re: [RFC 1/4] Add usb_get_address and usb_set_address support

[Charles.Hyde]

<snipped>
>
> This is a VERY cdc-net-specific function.  It is not a "generic" USB
> function at all.  Why does it belong in the USB core?  Shouldn't it live
> in the code that handles the other cdc-net-specific logic?
>
> thanks,
>
> greg k-h


Thank you for this feedback, Greg.  I was not sure about adding this to message.c, because of the USB_CDC_GET_NET_ADDRESS.  I had found references to SET_ADDRESS in the USB protocol at https://wiki.osdev.org/Universal_Serial_Bus#USB_Protocol.  If one wanted a generic USB function for SET_ADDRESS, to be used for both sending a MAC address and receiving one, how would you suggest this be implemented?  This is a legit question because I am curious.

Your feedback led to moving the functionality into cdc_ncm.c for today's testing, and removing all changes from messages.c, usb.h, usbnet.c, and usbnet.h.  This may be where I end up long term, but I would like to learn if there is a possible solution that could live in message.c and be callable from other USB-to-Ethernet aware drivers.

Thank you again,
Charles Hyde

Re: [RFC 1/4] Add usb_get_address and usb_set_address support

[Oliver Neukum]

Am Mittwoch, den 21.08.2019, 23:35 +0000 schrieb
Charles.Hyde@dellteam.com:
> <snipped>
> > 
> > This is a VERY cdc-net-specific function.  It is not a "generic" USB
> > function at all.  Why does it belong in the USB core?  Shouldn't it live
> > in the code that handles the other cdc-net-specific logic?
> > 
> > thanks,
> > 
> > greg k-h
> 
> 
> Thank you for this feedback, Greg.  I was not sure about adding this to message.c, because of the USB_CDC_GET_NET_ADDRESS.  I had found references to SET_ADDRESS in the USB protocol at https://wiki.osdev.org/Universal_Serial_Bus#USB_Protocol.  If one wanted a generic USB function for SET_ADDRESS, to be used for both sending a MAC address and receiving one, how would you suggest this be implemented?  This is a legit question because I am curious.

Your implementation was, except for missing error handling, usable.
The problem is where you put it. CDC messages exist only for CDC
devices. Now it is true that there is no generic CDC driver.
Creating a module just for that would cost more memory than it saves
in most cases.
But MACs are confined to network devices. Hence the functionality
can be put into usbnet. It should not be put into any individual
driver, so that every network driver can use it without duplication.

> Your feedback led to moving the functionality into cdc_ncm.c for today's testing, and removing all changes from messages.c, usb.h, usbnet.c, and usbnet.h.  This may be where I end up long term, but I would like to learn if there is a possible solution that could live in message.c and be callable from other USB-to-Ethernet aware drivers.

All those drivers use usbnet. Hence there it should be.

	Regards
		Oliver

RE: [RFC 1/4] Add usb_get_address and usb_set_address support

[Charles.Hyde]

> > <snipped>
> > >
> > > This is a VERY cdc-net-specific function.  It is not a "generic" USB
> > > function at all.  Why does it belong in the USB core?  Shouldn't it
> > > live in the code that handles the other cdc-net-specific logic?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> >
> > Thank you for this feedback, Greg.  I was not sure about adding this to
> message.c, because of the USB_CDC_GET_NET_ADDRESS.  I had found
> references to SET_ADDRESS in the USB protocol at
> https://wiki.osdev.org/Universal_Serial_Bus#USB_Protocol.  If one wanted a
> generic USB function for SET_ADDRESS, to be used for both sending a MAC
> address and receiving one, how would you suggest this be implemented?  This
> is a legit question because I am curious.
> 
> Your implementation was, except for missing error handling, usable.
> The problem is where you put it. CDC messages exist only for CDC devices. Now
> it is true that there is no generic CDC driver.
> Creating a module just for that would cost more memory than it saves in most
> cases.
> But MACs are confined to network devices. Hence the functionality can be put
> into usbnet. It should not be put into any individual driver, so that every
> network driver can use it without duplication.
> 
> > Your feedback led to moving the functionality into cdc_ncm.c for today's
> testing, and removing all changes from messages.c, usb.h, usbnet.c, and
> usbnet.h.  This may be where I end up long term, but I would like to learn if
> there is a possible solution that could live in message.c and be callable from
> other USB-to-Ethernet aware drivers.
> 
> All those drivers use usbnet. Hence there it should be.
> 
> 	Regards
> 		Oliver


Some of the drivers in drivers/net/usb/ do call functions in drivers/net/usb/usbnet, but not all.  As Greg pointed out, the USB change I developed is cdc specific, so putting it into usbnet would raise the same concerns Greg mentioned.  Leaving my newest implementation in cdc_ncm.c will be most appropriate, as it also fits with what other drivers in this folder have done.  My original code was rather short sighted, at best.

Charles

Re: [RFC 1/4] Add usb_get_address and usb_set_address support

[Greg KH]

On Tue, Aug 20, 2019 at 10:18:42PM +0000, Charles.Hyde@dellteam.com wrote:
> +int usb_get_address(struct usb_device *dev, unsigned char * mac)
> +{
> +	int ret = -ENOMEM;
> +	unsigned char *tbuf = kmalloc(256, GFP_NOIO);

On a technical level, why are you asking for 256 bytes here, and in the
control message, yet assuming you will only get 6 back for a correct
message?  Shouldn't you be only asking for 6 bytes?

> +
> +	if (!tbuf)
> +		return -ENOMEM;
> +
> +	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> +			USB_CDC_GET_NET_ADDRESS,
> +			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +			0, USB_REQ_SET_ADDRESS, tbuf, 256,
> +			USB_CTRL_GET_TIMEOUT);
> +	if (ret == 6)
> +		memcpy(mac, tbuf, 6);
> +
> +	kfree(tbuf);
> +	return ret;

So if 100 is returned by the device (not likely, but let's say 7), then
you return 7 bytes, yet you did not copy the data into the pointer given
to you.

SHouldn't you report a real error for when this happens (hint, it will.)

thanks,

greg k-h

Re: [RFC 1/4] Add usb_get_address and usb_set_address support

[Andrew Lunn]

On Tue, Aug 20, 2019 at 10:18:42PM +0000, Charles.Hyde@dellteam.com wrote:
> The core USB driver message.c is missing get/set address functionality
> that stops ifconfig from being able to push MAC addresses out to USB
> based ethernet devices.  Without this functionality, some USB devices
> stop responding to ethernet packets when using ifconfig to change MAC
> addresses.

Hi Charles

ifconfig has been deprecated for years, maybe a decade. Please
reference the current tools, iproute2.

	  Andrew

Re: [RFC 1/4] Add usb_get_address and usb_set_address support

[Charles.Hyde]

>> The core USB driver message.c is missing get/set address functionality
>> that stops ifconfig from being able to push MAC addresses out to USB
>> based ethernet devices.  Without this functionality, some USB devices
>> stop responding to ethernet packets when using ifconfig to change MAC
>> addresses.
>
> Hi Charles
>
> ifconfig has been deprecated for years, maybe a decade. Please
> reference the current tools, iproute2.
>
>           Andrew


Thank you.  I would, but that is not available on the system under test, while ifconfig is.

Charles Hyde

Re: [RFC 1/4] Add usb_get_address and usb_set_address support

[Oliver Neukum]

Am Dienstag, den 20.08.2019, 22:18 +0000 schrieb
Charles.Hyde@dellteam.com:
> The core USB driver message.c is missing get/set address functionality

This should go into usbnet. The CDC parser is where it is because
it is needed for serial and network devices. As serial devices
do not have a MAC, this can go into usbnet.

> that stops ifconfig from being able to push MAC addresses out to USB
> based ethernet devices.  Without this functionality, some USB devices
> stop responding to ethernet packets when using ifconfig to change MAC
> addresses.  This has been tested with a Dell Universal Dock D6000.
> 
> Signed-off-by: Charles Hyde <charles.hyde@dellteam.com>
> Cc: Mario Limonciello <mario.limonciello@dell.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> ---
>  drivers/usb/core/message.c | 59 ++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h        |  3 ++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 5adf489428aa..eea775234b09 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1085,6 +1085,65 @@ int usb_clear_halt(struct usb_device *dev, int pipe)
>  }
>  EXPORT_SYMBOL_GPL(usb_clear_halt);
>  
> +/**
> + * usb_get_address - 
> + * @dev: device whose endpoint is halted

Which endpoint?

> + * @mac: buffer for containing 
> + * Context: !in_interrupt ()
> + *
> + * This will attempt to get the six byte MAC address from a USB device's
> + * ethernet controller using GET_NET_ADDRESS command.
> + *
> + * This call is synchronous, and may not be used in an interrupt context.
> + *
> + * Return: Zero on success, or else the status code returned by the

Well, I am afraid it will return 6 on success.

> + * underlying usb_control_msg() call.
> + */
> +int usb_get_address(struct usb_device *dev, unsigned char * mac)
> +{
> +	int ret = -ENOMEM;

Initialization is unnecessary here.

> +	unsigned char *tbuf = kmalloc(256, GFP_NOIO);

If you intentionally picked a safety margin of 42 times, this
is cool. Otherwise it is a litttle much.

> +
> +	if (!tbuf)
> +		return -ENOMEM;
> +
> +	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> +			USB_CDC_GET_NET_ADDRESS,
> +			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +			0, USB_REQ_SET_ADDRESS, tbuf, 256,
> +			USB_CTRL_GET_TIMEOUT);
> +	if (ret == 6)
> +		memcpy(mac, tbuf, 6);

You cannot ignore the case of devices sending more or less than 6
bytes.

	Regards
		Oliver