| Message ID | 1566339522507.45056@Dellteam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support into cdc_ncm for MAC address pass through | expand |
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
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
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
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
<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
>> 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
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
> > <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
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);
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(+)