Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [RFC 1/4] Add usb_get_address and usb_set_address support
@ 2019-08-20 22:18 Charles.Hyde
  2019-08-20 22:26 ` Greg KH
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Charles.Hyde @ 2019-08-20 22:18 UTC (permalink / raw)
  To: linux-usb, linux-acpi; +Cc: gregkh, Mario.Limonciello, oliver, netdev, nic_swsd

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

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

* Re: [RFC 1/4] Add usb_get_address and usb_set_address support
  2019-08-20 22:18 [RFC 1/4] Add usb_get_address and usb_set_address support Charles.Hyde
@ 2019-08-20 22:26 ` Greg KH
  2019-08-21 23:35   ` Charles.Hyde
  2019-08-20 22:28 ` Greg KH
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2019-08-20 22:26 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: linux-usb, linux-acpi, Mario.Limonciello, oliver, netdev, nic_swsd

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

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

* Re: [RFC 1/4] Add usb_get_address and usb_set_address support
  2019-08-20 22:18 [RFC 1/4] Add usb_get_address and usb_set_address support Charles.Hyde
  2019-08-20 22:26 ` Greg KH
@ 2019-08-20 22:28 ` Greg KH
  2019-08-21  1:22 ` Andrew Lunn
  2019-08-21  9:08 ` Oliver Neukum
  3 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2019-08-20 22:28 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: linux-usb, linux-acpi, Mario.Limonciello, oliver, netdev, nic_swsd

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

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

* Re: [RFC 1/4] Add usb_get_address and usb_set_address support
  2019-08-20 22:18 [RFC 1/4] Add usb_get_address and usb_set_address support Charles.Hyde
  2019-08-20 22:26 ` Greg KH
  2019-08-20 22:28 ` Greg KH
@ 2019-08-21  1:22 ` Andrew Lunn
  2019-08-21 23:45   ` Charles.Hyde
  2019-08-21  9:08 ` Oliver Neukum
  3 siblings, 1 reply; 9+ messages in thread
From: Andrew Lunn @ 2019-08-21  1:22 UTC (permalink / raw)
  To: Charles.Hyde
  Cc: linux-usb, linux-acpi, gregkh, Mario.Limonciello, oliver, netdev,
	nic_swsd

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

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

* Re: [RFC 1/4] Add usb_get_address and usb_set_address support
  2019-08-20 22:18 [RFC 1/4] Add usb_get_address and usb_set_address support Charles.Hyde
                   ` (2 preceding siblings ...)
  2019-08-21  1:22 ` Andrew Lunn
@ 2019-08-21  9:08 ` Oliver Neukum
  3 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2019-08-21  9:08 UTC (permalink / raw)
  To: Charles.Hyde, linux-acpi, linux-usb
  Cc: Mario.Limonciello, gregkh, nic_swsd, netdev

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


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

* Re: [RFC 1/4] Add usb_get_address and usb_set_address support
  2019-08-20 22:26 ` Greg KH
@ 2019-08-21 23:35   ` Charles.Hyde
  2019-08-22  8:08     ` Oliver Neukum
  0 siblings, 1 reply; 9+ messages in thread
From: Charles.Hyde @ 2019-08-21 23:35 UTC (permalink / raw)
  To: gregkh; +Cc: linux-usb, linux-acpi, Mario.Limonciello, oliver, netdev, nic_swsd

<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


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

* Re: [RFC 1/4] Add usb_get_address and usb_set_address support
  2019-08-21  1:22 ` Andrew Lunn
@ 2019-08-21 23:45   ` Charles.Hyde
  0 siblings, 0 replies; 9+ messages in thread
From: Charles.Hyde @ 2019-08-21 23:45 UTC (permalink / raw)
  To: andrew
  Cc: linux-usb, linux-acpi, gregkh, Mario.Limonciello, oliver, netdev,
	nic_swsd

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

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

* Re: [RFC 1/4] Add usb_get_address and usb_set_address support
  2019-08-21 23:35   ` Charles.Hyde
@ 2019-08-22  8:08     ` Oliver Neukum
  2019-08-22 17:14       ` Charles.Hyde
  0 siblings, 1 reply; 9+ messages in thread
From: Oliver Neukum @ 2019-08-22  8:08 UTC (permalink / raw)
  To: Charles.Hyde, gregkh
  Cc: Mario.Limonciello, nic_swsd, linux-acpi, linux-usb, netdev

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


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

* RE: [RFC 1/4] Add usb_get_address and usb_set_address support
  2019-08-22  8:08     ` Oliver Neukum
@ 2019-08-22 17:14       ` Charles.Hyde
  0 siblings, 0 replies; 9+ messages in thread
From: Charles.Hyde @ 2019-08-22 17:14 UTC (permalink / raw)
  To: oneukum, gregkh
  Cc: Mario.Limonciello, nic_swsd, linux-acpi, linux-usb, netdev

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

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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-20 22:18 [RFC 1/4] Add usb_get_address and usb_set_address support Charles.Hyde
2019-08-20 22:26 ` Greg KH
2019-08-21 23:35   ` Charles.Hyde
2019-08-22  8:08     ` Oliver Neukum
2019-08-22 17:14       ` Charles.Hyde
2019-08-20 22:28 ` Greg KH
2019-08-21  1:22 ` Andrew Lunn
2019-08-21 23:45   ` Charles.Hyde
2019-08-21  9:08 ` Oliver Neukum

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/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-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

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


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