USB: Implement USB Ethernet Device gadget (ECM protocol) #4147

finikorg · 2017-10-02T14:10:24Z

First revision of CDC ECM gadget device.

TODO: Re-use composite device headers and implement common register_function() methods

finikorg · 2017-10-02T14:13:18Z

@jfischer-phytec-iot @pfalcon Please review first revision, I am going to re-use descriptors like in 9c91652.

pfalcon · 2017-10-02T16:15:16Z

@finikorg : Thanks for the patch! I'm not sure how much of it I'll be able to review though, I mostly keep an eye on USB things because I'd like to have an USB CDC driver for ARM devices to be implemented, but I don't have enough experience doing that myself.

jfischer-no

@finikorg it all looks like WIP,
can you please open an issue and describe your work, like: - add ethernet emulation device, add CDC-ECM driver, add RNDIS driver. I guess ethernet emulator can also be used with CDC-EEM driver and it would be good to synchronize the work on it with @loicpoulain

jfischer-no · 2017-10-16T13:21:04Z

subsys/usb/class/netusb/eth_emu.c

+
+static struct eth_emu_context eth_emu_ctx;
+
+static int eth_emu_init(struct device *dev)


eth_emu does not need to depend on usb, I think it would be better to separate it from this PR and move to drivers/ethernet/, @jukkar your opinion?

I can move it to some other place if needed.

@jukkar @tbursztyka @rveerama1 could you check this virtual ethernet device? Commit: 745a7f0111b741bc735a00e90c86bae581b235fb

see also #4461 (comment)

nashif · 2017-10-16T15:52:07Z

@finikorg can you please rebase?

finikorg · 2017-10-17T07:37:31Z

@nashif Actually I am trying to rebase it now and reuse composite device framework.

finikorg · 2017-10-18T11:46:10Z

@nashif @jfischer-phytec-iot rebased to work with new interface

finikorg · 2017-10-19T12:02:47Z

@laperie Please review

jfischer-no

I looked at it quickly, will do more closely later.

jfischer-no · 2017-10-19T12:40:24Z

subsys/usb/usb_descriptor.c

@@ -59,6 +59,23 @@ struct dev_common_descriptor {
 		struct usb_ep_descriptor if1_out_ep;
 	} __packed cdc_acm_cfg;
 #endif
+#ifdef CONFIG_USB_DEVICE_NETWORK_ECM
+	struct usb_cdc_ecm_config {
+		struct usb_association_descriptor iad;


association descriptor is necessary for composite device only, see above

@jfischer-phytec-iot fixed

nashif

This commit is not increasing the debug level, it is adding more information, so please fix the commit message

nashif · 2017-10-20T12:27:55Z

This commit is not increasing the debug level, it is adding more information, so please fix the commit message

referring to


Author:     Andrei Emeltchenko <[email protected]>
AuthorDate: Wed Aug 16 16:22:59 2017 +0300
Commit:     Andrei Emeltchenko <[email protected]>
CommitDate: Fri Oct 20 11:18:17 2017 +0300

    usb: Increase debug level

    Signed-off-by: Andrei Emeltchenko <[email protected]>

nashif · 2017-10-20T12:33:08Z

then we have a few commits where you

first add Kconfig
then add sources
then incorporate those into Makefiles

Please squash them, when a source file is introduced, it had to be buildable, no need to split the commits this way.

I am referring to

2017-09-26 15:40 Andrei Emeltchenko             o usb: netusb: Add build infrastructure for USB net device
2017-10-02 13:55 Andrei Emeltchenko             o usb: netusb: Add composite device with ECM function
2017-09-26 15:30 Andrei Emeltchenko             o usb: netusb: Add ethernet emulation device
2017-09-25 13:44 Andrei Emeltchenko             o usb: netusb: Add ECM protocol handling
2017-10-02 13:40 Andrei Emeltchenko             o usb: netusb: Add RNDIS Networking device configuration
2017-10-02 13:36 Andrei Emeltchenko             o usb: netusb: Add ECM Networking Device configuration

nashif · 2017-10-20T12:35:39Z

subsys/usb/class/netusb/Kconfig

+menu "USB Device Networking support"
+	depends on USB_DEVICE_STACK
+
+config USB_DEVICE_NETWORK


what is the idea behind this kconfig option? it has no description and is automatically selected

@nashif idea here is that we implement some other functions (RNDIS, etc). Common code is built whenever network function is enabled. I have working solution with several functions where it does make sense

nashif · 2017-10-20T12:40:42Z

You just added netusb.c, so why are you refactoring it in a followup commit? Why not squash this

usb: netusb: Refactor netusb code using new interface

into where the file was introduced?

When reviewing this commit by commit, it just makes reviews very difficult, please submit less commits that are self-contained rather than bit and pieces in many more commits without a good reason.

finikorg · 2017-10-20T12:53:34Z

@nashif for some reason I cannot reply directly to comments due to this **** github interface, so I reply in my comment here. I squashed most of the commits together. I left emulation device commit separate since it may be reworked to be included to networking codebase.

finikorg · 2017-10-20T13:05:39Z

I squashed even more, only Ethernet emulation device left separate.

Add extra debug information to USB commands. Signed-off-by: Andrei Emeltchenko <[email protected]>

There is no particular reason this spot in usb_dw_tx() cannot be reached by racing threads on the same endpoint, though existing API usage in the tree is all unithreaded. The FIFO state read at the top of the function must still be true at the bottom or else the packet byte count will be corrupt. Also, as described in an existing comment, the databook has some scary-sounding warnings about access to the registers during FIFO operations, even if they "should" be on separate endpoints and unrelated. Signed-off-by: Andy Ross <[email protected]>

…oint." This reverts commit 1da0a9e. The workaround caused a severe performance penalty, and only worked for USB packets of 4-15 bytes in length (16+ byte packets weren't subject to the hardware bug). Single-byte packets (very common for cdc_acm serial port transfers) would still be duplicated sometimes. The upcoming DMA implementation does not share the performance penalty, and also is not subject to the bug for those sizes of packets (though it DOES still have a problem with single-byte packets!). Signed-off-by: Andy Ross <[email protected]>

The designware hardware in dedicated FIFO mode (which is all we support right now for lack of shared-FIFO hardware) has one hardware FIFO per IN (i.e. transmit) endpoint. But it doesn't assign them on its own, it's the drivers responsibility to populate the TxFNum field of the DIEPCTL registers with integer indices corresponding to the desired FIFO. We weren't doing that, which meant that all IN endpoints were sharing the same FIFO zero which is supposed to be dedicated to EP0 control transfers. The net effect is that sometimes outbound transfers would be corrupted, showing data from the wrong endpoint. More often that not this would leak from control transfers over to the higher-bandwidth bulk endpoints of the application, but occasionally you'd see a control transfer itself get borked and the USB device would glitch. Get this right and set the FIFOs properly. Signed-off-by: Andy Ross <[email protected]>

The Designware FIFO is filled in units of 32 bit words, but the buffer we are passed is not guaranteed to be a multiple of 4 bytes long, nor aligned on a 4-byte boundary. So in theory we are reading 0-3 bytes of unused garbage from the end of the array. That's currently benign on supported platforms with this hardware, which all support misaligned reads. But not all do. And the incoming arrival of memory protection opens the possibility that those extra bytes would cross a protection boundary and cause a crash or security bug. Do this right. (Note that this is fixed to little endian byte order. The Designware databook is frustratingly silent on the endianness it expects, but existing hardware I can see is definitely LE and I see a few spots in the Linux dwc2 driver that likewise assume LE). Signed-off-by: Andy Ross <[email protected]>

The enable and clear NAK bit can be legally written together, do it in just one write. Signed-off-by: Andy Ross <[email protected]>

Add USB device configuration for CDC ECM Signed-off-by: Andrei Emeltchenko <[email protected]>

Ethernet emulation device allows to use networking interface for interaction with USB endpoints. Signed-off-by: Andrei Emeltchenko <[email protected]>

Add composite device skeleton with ECM function implemented. Signed-off-by: Andrei Emeltchenko <[email protected]>

Add netusb and composite netusb build check configurations. Signed-off-by: Andrei Emeltchenko <[email protected]>

Signed-off-by: Andrei Emeltchenko <[email protected]>

finikorg · 2017-10-23T08:00:47Z

rebased against the latest upstream

Use documentation defined values for virtual devices MAC addresses in Zephyr and Host OS. Signed-off-by: Andrei Emeltchenko <[email protected]>

The situation when FIFO is not empty is not a bug and it is spamming console when only bugs are enabled. Signed-off-by: Andrei Emeltchenko <[email protected]>

Adding sleep before TX FIFO flash fixes splitting networking packets sent over USB endpoints making ECM broken since there is no flow control other then frame sizes. Signed-off-by: Andrei Emeltchenko <[email protected]>

jfischer-no · 2017-10-27T14:22:53Z

@finikorg Andrei, can you please move dw driver relevant commits in a separate PR. I fear that dw driver fixes here will be overlooked and someone begins to fix dw driver, thx

dw should be fine here if we merge this...

nashif · 2017-10-28T17:46:22Z

recheck

lpereira · 2017-10-31T17:08:33Z

subsys/usb/class/netusb/function_ecm.c

+
+	net_hexdump_frags("<", pkt);
+
+	if (!pkt->frags) {


If pkt->frags is NULL, the net_pkt_ll() call below will crash: it calls net_pkt_ip_data(), which dereferences pkt->frags. I could fix this, but I don't know the intent of this code here.

This is fixed in PR #4641

jli157 · 2020-05-25T17:32:26Z

@finikorg If using a device with this driver enabled on Windows, what kind of windows driver can be used to support the device?

finikorg · 2020-05-26T05:41:09Z

@finikorg If using a device with this driver enabled on Windows, what kind of windows driver can be used to support the device?

I could not make it working in Windows, it is better to use MS RNDIS.

finikorg requested a review from andyross as a code owner October 2, 2017 14:10

jfischer-no mentioned this pull request Oct 2, 2017

USB device stack and class drivers #4150

Closed

10 tasks

nashif added the area: USB Universal Serial Bus label Oct 13, 2017

jfischer-no previously requested changes Oct 16, 2017

View reviewed changes

finikorg force-pushed the netusb branch from 161c1b5 to d583557 Compare October 18, 2017 11:43

finikorg force-pushed the netusb branch from d583557 to 24f7622 Compare October 19, 2017 07:31

jfischer-no reviewed Oct 19, 2017

View reviewed changes

finikorg force-pushed the netusb branch from 0cdeae5 to 65ee036 Compare October 20, 2017 08:27

finikorg requested a review from youvedeep as a code owner October 20, 2017 08:27

nashif requested changes Oct 20, 2017

View reviewed changes

finikorg force-pushed the netusb branch from 65ee036 to f6703fa Compare October 20, 2017 12:31

nashif reviewed Oct 20, 2017

View reviewed changes

finikorg force-pushed the netusb branch from f6703fa to 111b25d Compare October 20, 2017 12:48

finikorg force-pushed the netusb branch from 111b25d to b7ad31d Compare October 20, 2017 13:04

nashif approved these changes Oct 20, 2017

View reviewed changes

finikorg and others added 4 commits October 23, 2017 10:52

usb: Add extra debug information

2c60be7

Add extra debug information to USB commands. Signed-off-by: Andrei Emeltchenko <[email protected]>

Andy Ross and others added 7 commits October 23, 2017 10:52

usb_dw_dc: Cleanup: write DIEPCTL register just once

9f16066

The enable and clear NAK bit can be legally written together, do it in just one write. Signed-off-by: Andy Ross <[email protected]>

usb: Add CDC ECM composite configuration

87869f5

Add USB device configuration for CDC ECM Signed-off-by: Andrei Emeltchenko <[email protected]>

usb: netusb: Add ethernet emulation device

3dc9b7b

Ethernet emulation device allows to use networking interface for interaction with USB endpoints. Signed-off-by: Andrei Emeltchenko <[email protected]>

usb: netusb: Add composite device with ECM function

936a1e5

Add composite device skeleton with ECM function implemented. Signed-off-by: Andrei Emeltchenko <[email protected]>

usb: Add sanity_check configuration for netusb

ab63964

Add netusb and composite netusb build check configurations. Signed-off-by: Andrei Emeltchenko <[email protected]>

net: sample: Add USB Ethernet stick configuration

6917bc7

Signed-off-by: Andrei Emeltchenko <[email protected]>

finikorg force-pushed the netusb branch from 174a965 to 6917bc7 Compare October 23, 2017 07:57

finikorg added 3 commits October 23, 2017 12:47

usb: Use RFC 7042 Documentation values for MAC strings

dd272ff

Use documentation defined values for virtual devices MAC addresses in Zephyr and Host OS. Signed-off-by: Andrei Emeltchenko <[email protected]>

usb: dw: Fix debug log specifier for FIFO not empty

7cac82a

The situation when FIFO is not empty is not a bug and it is spamming console when only bugs are enabled. Signed-off-by: Andrei Emeltchenko <[email protected]>

usb: dw: Add sleep before FIFO flash

45c9d6b

Adding sleep before TX FIFO flash fixes splitting networking packets sent over USB endpoints making ECM broken since there is no flow control other then frame sizes. Signed-off-by: Andrei Emeltchenko <[email protected]>

nashif merged commit fe9dee0 into zephyrproject-rtos:master Oct 28, 2017

finikorg deleted the netusb branch October 30, 2017 09:22

lpereira reviewed Oct 31, 2017

View reviewed changes


		static struct eth_emu_context eth_emu_ctx;

		static int eth_emu_init(struct device *dev)

USB: Implement USB Ethernet Device gadget (ECM protocol) #4147

USB: Implement USB Ethernet Device gadget (ECM protocol) #4147

Uh oh!

Conversation

finikorg commented Oct 2, 2017

Uh oh!

finikorg commented Oct 2, 2017

Uh oh!

pfalcon commented Oct 2, 2017

Uh oh!

jfischer-no left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finikorg Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nashif commented Oct 16, 2017

Uh oh!

finikorg commented Oct 17, 2017

Uh oh!

finikorg commented Oct 18, 2017

Uh oh!

finikorg commented Oct 19, 2017

Uh oh!

jfischer-no left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nashif left a comment

Choose a reason for hiding this comment

Uh oh!

nashif commented Oct 20, 2017

Uh oh!

nashif commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nashif commented Oct 20, 2017

Uh oh!

finikorg commented Oct 20, 2017

Uh oh!

finikorg commented Oct 20, 2017

Uh oh!

finikorg commented Oct 23, 2017

Uh oh!

jfischer-no commented Oct 27, 2017

Uh oh!

nashif commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jli157 commented May 25, 2020

Uh oh!

finikorg commented May 26, 2020

Uh oh!

Uh oh!

jfischer-no left a comment •

edited

Loading

finikorg Oct 20, 2017 •

edited

Loading