subsys: usb: host: Add USB Host CDC ECM Class Support for Ethernet

[santhosh-c-c]

USB Host: Add CDC ECM class support

This PR introduces the USB CDC ECM (Ethernet Control Model) class support
to the Zephyr USB host subsystem, enabling Ethernet functionality for
USB hosts.

Changes since draft:

• Rebased on merged dependencies USB Host: integrate class API [2: helpers] #94590
• Subsys: Optimized High-Speed device initialization by defaulting
  bMaxPacketSize0 to 64 per USB 2.0 spec (85df60a)

Dependency:

• usb: host: fix HS device enumeration and interface probing #106802 (usb: host: fix HS device enumeration and interface probing) carries equivalent and additional USBH core fixes needed for HS device interop. The bMaxPacketSize0 commit in this PR overlaps with the first commit of 106802 and will be dropped from this PR once 106802 merges.

Signed-off-by: Santhosh Charles santhosh@linumiz.com

[josuah]

Thank you for this implementation!
I took note of the bugfixes you noticed and am importing them in the API2 PR you are based upon.

[josuah]

@MarkWangChinese is this related to your ongoing effort for implementing ECM?

[MarkWangChinese]

@MarkWangChinese is this related to your ongoing effort for implementing ECM?

Thanks @josuah

Hi @santhosh-c-c As the comments #95661 (comment), our side (NXP) is trying to enable the USB Host ECM too, our plan is creating the formal pr before the end of December, we will do basic function test too. Could you tell me your plan? let's align to meet both our requirements. Thanks.

[MarkWangChinese]

Our code has been tested on the hardware (MIMXRT1064-EVK), and we are currently waiting for the dependency to be merged first, namely this PR: #94590. Once that merge is complete, we will proceed with merging our ECM host class support.

An internal review is ongoing and we are looking for any features that should be added. This internal review should not take more than a week. If you would like, we can discuss any additional features that need to be added so we can align with your requirements.

Thanks @santhosh-c-c I think our side can co-work on this pr. For example: the current codes doesn't send CDC ECM class specific control transfer, the mac address is not got from the attached device's descriptor. Our side can add commits to your pr and do test on our side too. Is it OK?
BYW, Could you please share your test flow? how did you test it on MIMXRT1064-EVK, what's your building cmd. Thanks.

[sonarqubecloud]

Quality Gate passed

Issues
24 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.2% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[carlescufi]

@santhosh-c-c dependencies should now be merged. Please take out of draft and rebase on top of main.

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time.

[santhosh-c-c]

Hello @jfischer-no @josuah @MarkWangChinese Pinging this PR for review as discussed in the other thread #95661 (comment). I'm standing by to address any comments or requested changes to help get this core support merged.

[josuah]

It seems like this PR is smaller, simpler which makes it easier to integrate.

With #100289 already going on, this means that some deduplication needs to happen, and using the current PR as a basis for #100289 is not feasible given the difference of structure.

Do you think it would be possible to refactor this PR to make it closer to #100289 while also applying your subjective improvements?

For instance, every time something is imported with changes applied, also propose the changes in #100289 as a review.

[josuah]

It is possible to merge some commits from this PR outside of CDC ECM context.

It is possible to integrate some commits from #100289 into this current PR.

This directly contributes to progress on CDC ECM host support.

Thank you for the efforts and time on this.

[santhosh-c-c]

It seems like this PR is smaller, simpler which makes it easier to integrate.

With #100289 already going on, this means that some deduplication needs to happen, and using the current PR as a basis for #100289 is not feasible given the difference of structure.

Do you think it would be possible to refactor this PR to make it closer to #100289 while also applying your subjective improvements?

For instance, every time something is imported with changes applied, also propose the changes in #100289 as a review.

Thanks @josuah for the input. A few clarifications

1. On refactoring this PR closer to subsys: usb: host: support for usb host cdc-ecm class #100289: the structural difference is intentional. This PR keeps the implementation minimal and focused on the core ECM data path. subsys: usb: host: support for usb host cdc-ecm class #100289 layers significantly more on top (multicast filter, packet filter bitmap, hw stats, link speed, async xfer queue with cleanup list). Refactoring this PR to mirror that structure would essentially turn it into a duplicate of subsys: usb: host: support for usb host cdc-ecm class #100289 and defeat the purpose of having a small, reviewable base.

2. On commits outside the CDC ECM context: agreed. The usb: host: set default HS bMaxPacketSize0 to 64 commit from this PR is a generic USBH core fix unrelated to ECM and is already being addressed independently in usb: host: fix HS device enumeration and interface probing #106802 (along with two other HS enumeration/probing fixes from the same testing effort). I will drop that commit from this PR once usb: host: fix HS device enumeration and interface probing #106802 lands.

3. On integrating commits from subsys: usb: host: support for usb host cdc-ecm class #100289 into this PR: this is the part of your suggestion I want to act on directly. The two non-driver commits in subsys: usb: host: support for usb host cdc-ecm class #100289 are useful regardless of which PR lands the ECM driver, the usb: class: cdc: Add ECM related macros header additions, and the usb: host: ch9: Add string descriptor related request helper function ch9/desc helpers. I plan to pull these in (with credit) and replace the inline string parsing in this PR. That brings the two PRs structurally closer without merging the feature scope.

4. On the overall direction: my preference remains landing a minimal core first (this PR), with subsys: usb: host: support for usb host cdc-ecm class #100289 rebasing the additional features on top. This keeps each PR focused and easier to review but I would like @jfischer-no's call on which path is preferred before I proceed.

@jfischer-no could you weigh in on the preferred merge strategy for ECM host support?

[josuah]

Thank you for clarifying.

Would you be interested in helping #100289 rebase on top of this? For instance by indicating the parts that would need to be changed?

[santhosh-c-c]

Would you be interested in helping #100289 rebase on top of this? For instance by indicating the parts that would need to be changed?

Happy to help @josuah Here is how I see the migration mapping. Splitting into phases by dependency.

Phase A: Independent PRs (both drivers benefit):

• ECM Macros: Extract the usb_cdc.h additions from subsys: usb: host: support for usb host cdc-ecm class #100289 and land as a standalone PR.
• CH9 Helpers: Extract the string descriptor helpers (usbh_req_desc_str, usbh_desc_str_utf16le_to_ascii, etc.) from subsys: usb: host: support for usb host cdc-ecm class #100289. Once merged, this PR (subsys: usb: host: Add USB Host CDC ECM Class Support for Ethernet #99097) will rebase to use them in cdc_ecm_fetch_mac, replacing the manual UTF-16LE parsing.

Phase B: Merge Minimal ECM Core (#99097):

• Lands with probe, single-pass descriptor parsing, MAC fetch, intr-in carrier tracking, bulk in/out, and initial SET_ETHERNET_PACKET_FILTER.

Phase C: Incremental Feature PRs (ported from #100289):
To keep reviews manageable, #100289’s driver commit becomes follow-up PRs on top of #99097:

1. Switch to memslab-pooled xfer cb_priv + queued_xfers list.
2. Dynamic packet filter, Multicast filter, Hardware statistics.
3. CONNECTION_SPEED_CHANGE notification + dynamic link speed.
4. Concurrent TX queue (sema + DATA_TX_CONCURRENT_NUM).
5. Disconnect xfer cleanup (drain queued_xfers).

Reconciliation Details:

• Descriptor Parser: subsys: usb: host: support for usb host cdc-ecm class #100289 strictly requires HEADER_FUNC_DESC. This PR is permissive (handles real-world devices missing the HEADER). I lean toward keeping it permissive, but open to discussion.
• Iface Bring-up: subsys: usb: host: support for usb host cdc-ecm class #100289 correctly defers usbh_device_interface_set until eth_iface start (admin-up). I currently do it at probe, but plan to absorb NXP's approach here before the rebase.
• TX Path: Addressed in Phase C (4) migrating from my mutex single-xfer to NXP's sema concurrent model for better throughput.

@jfischer-no @MarkWangChinese Let me know your thoughts.

[josuah]

1. Switch to memslab-pooled xfer cb_priv + queued_xfers list.

I have the impression that this can be avoided altogether.

The thing is: when removing a class from the stack, all associated transfers also need to be removed, and the way to do this is to call dequeue() on each, but there is no way to get the list of all the transfers that are queued to a particular class.

This could be done at the stack level maybe, as then it is a lot less effort for each and every class (implemented with an array for UVC for instance).

zephyr/subsys/usb/host/class/usbh_uvc.c

Line 116 in 44853cf

struct uhc_transfer *video_transfer[CONFIG_USBH_VIDEO_CONCURRENT_TRANSFERS];

Without this, transfers de-allocation upon class removal is not being handled?

[MarkWangChinese]

1. Switch to memslab-pooled xfer cb_priv + queued_xfers list.

I have the impression that this can be avoided altogether.

The thing is: when removing a class from the stack, all associated transfers also need to be removed, and the way to do this is to call dequeue() on each, but there is no way to get the list of all the transfers that are queued to a particular class.

This could be done at the stack level maybe, as then it is a lot less effort for each and every class (implemented with an array for UVC for instance).

zephyr/subsys/usb/host/class/usbh_uvc.c

Line 116 in 44853cf

struct uhc_transfer *video_transfer[CONFIG_USBH_VIDEO_CONCURRENT_TRANSFERS];

Without this, transfers de-allocation upon class removal is not being handled?

Hi @josuah For disconnecting, the controller will callback all the xfers that are enqueued. For the case of video: stooping the stream https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/usb/host/class/usbh_uvc.c#L3022. I think it is the controller driver interface issue, we needs one controller interface api to release the resource of "dev + endpoint" if it is no longer used, then the related usb bandwidth can be released. If needed, we can talk here #101159. Thanks.

[santhosh-c-c]

Hi @josuah For disconnecting, the controller will callback all the xfers that are enqueued. For the case of video: stooping the stream https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/usb/host/class/usbh_uvc.c#L3022. I think it is the controller driver interface issue, we needs one controller interface api to release the resource of "dev + endpoint" if it is no longer used, then the related usb bandwidth can be released. If needed, we can talk here #101159. Thanks.

@josuah - Based on Mark's explanation, does the host stack actually guarantee that removed() runs before the callbacks (EP_REQUEST) for cancelled transfers? Since they use different message queues (usbh_bus_msgq and usbh_msgq) processed by separate threads, I don't see anything ensuring that specific order.

[josuah]

I think this is not guaranteed: there is a thread for endpoint events and other events, they are split in different queues there:

zephyr/subsys/usb/host/usbh_core.c

Lines 33 to 45 in 603fa48

	static int usbh_event_carrier(const struct device *dev,
	const struct uhc_event *const event)
	{
	int err;
	if (event->type == UHC_EVT_EP_REQUEST) {
	err = k_msgq_put(&usbh_msgq, event, K_NO_WAIT);
	} else {
	err = k_msgq_put(&usbh_bus_msgq, event, K_NO_WAIT);
	}
	return err;
	}

That suggests full concurrency, and events such as "remove this device and all the classes" might need to go through mechanism like "atomically remove all endpoints from all queues and ask drivers they processed/errored-out the ongoing buffer.

[santhosh-c-c]

I think this is not guaranteed: there is a thread for endpoint events and other events, they are split in different queues there:

Thanks for the clarification @josuah

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud