subsys: usb: host: support for usb host cdc-ecm class#100289
subsys: usb: host: support for usb host cdc-ecm class#100289D-Veda wants to merge 3 commits intozephyrproject-rtos:mainfrom
Conversation
|
Hello @D-Veda, and thank you very much for your first pull request to the Zephyr project! |
| @@ -0,0 +1,1157 @@ | |||
| /* | |||
There was a problem hiding this comment.
this is a ethernet driver, so it belongs in drivers/ethernet/. I know there are already others there, but these should probably be moved too. I don't like ethernet drivers and its api be scattered all over the tree. (should be limited to the ethernet and wifi drivers and the net subsystem)
There was a problem hiding this comment.
It seems like a bit of discussion would help to decide:
-
Where USB classes (host/device) that implement a driver API are implemented. This would imply most USB classes move, or it would be an exception just for CDC-ECM.
-
Whether source in
drivers/are allowed to call functions fromsubsystem/. I've not seen anything in the doc but suspect this could interest several parties to review which model to use.
How about pursuing with the implementation details in the meantime that the location of tes code is discussed?
Thank you for the review!
There was a problem hiding this comment.
Drivers are allowed to use subsystems.
For example there is the random subsystem subsys/random/, it implements /include/zephyr/random/random.h, which includes sys_rand_get(). Btw the random subsystem uses the entropy_driver_api, if that is selected to be the source of randomness.
For the ethernet drivers, we have a common dt prop zephyr,random-mac-address if that is set, the ethernet mac address is randomly generated with sys_rand_get() during the init of the ethernet driver.
Other example are sd cards in zephyr. (that might be similar enough to usb, as it also has a sd subsystem above the sdhc drivers and api)
The sd card or disk (dt compatible zephyr,sdmmc-disk, drivers/disk/sdmmc_subsys.c) implements the disk api (zephyr/drivers/disk.h) and uses the SD subsystem (subsys/sd) and that uses the sdhc drivers (include/zephyr/drivers/sdhc.h).
We also have drivers for a SDIO wifi card in tree, which uses the sd subsystem it is in
drivers/wifi/infineon/airoc_whd_hal_sdio.c and not in subsys/sd.
Even bigger: We have a crypto device api and one of the drivers is zephyr/drivers/crypto/crypto_mbedtls_shim.c and it is using mbedTLS, which even is a separate module.
Drivers can use anything what they need.
Another thing is control and ensuring quality. There is a reason we have Driver Areas in the maintainers.yml and that in addition to the vendor specific areas for their drivers.
For this PR I wasn't added because I'm the ethernet maintainer, no it was just because I'm also a collaborator for DHCP in the networking subsystem and it's sample got changed.
How about pursuing with the implementation details in the meantime that the location of tes code is discussed?
Sure
There was a problem hiding this comment.
Thank you for your overview of driver <-> subsystem dependencies. I think I heard this "subsystem/driver" split about a particular driver, not as a general guideline.
For this PR I wasn't added because I'm the ethernet maintainer, no it was just because I'm also a collaborator for DHCP in the networking subsystem and it's sample got changed.
Regardless the solution, there would need to make sure that MAINTAINERS.yml pings correctly everyone involved.
There was a problem hiding this comment.
@jfischer-no can you please share your thoughts on the location of this driver.
There was a problem hiding this comment.
All the USB host class drivers that use USBH class driver API and implement a USB class specification or other very well known standard must be placed in subsys/usb/host/class. Similar to what we do with USB device support.
Also, considering that there is not even an Ethernet controller driver API, the complaints and claims here are absolutely unfounded and inappropriate.
The commit usb: host: class: cdc_ecm: Add CDC-ECM host class driver implementation places correctly a new USB host CDC ECM driver in subsys/usb/host/class.
There was a problem hiding this comment.
All the USB host class drivers are drivers and as such should not be in subsys/* but instead in drivers/*
|
Architecture WG meeting:
|
7f51652 to
e67d72b
Compare
| @@ -0,0 +1,1157 @@ | |||
| /* | |||
There was a problem hiding this comment.
All the USB host class drivers are drivers and as such should not be in subsys/* but instead in drivers/*
| max_segment_size = ctx->max_segment_size; | ||
| k_mutex_unlock(&ctx->mutex); | ||
|
|
||
| total_len = net_buf_frags_len(buf); | ||
| if (total_len > max_segment_size || total_len > CONFIG_USBH_CDC_ECM_DATA_BUF_POOL_SIZE) { | ||
| return -EMSGSIZE; | ||
| } | ||
|
|
||
| tx_buf = net_buf_alloc(&usbh_cdc_ecm_data_pool, K_NO_WAIT); | ||
| if (tx_buf == NULL) { | ||
| LOG_WRN("failed to allocate data buffer for transmitting"); | ||
| return -ENOMEM; | ||
| } | ||
|
|
||
| if (net_buf_linearize(tx_buf->data, net_buf_tailroom(tx_buf), buf, 0, total_len) != | ||
| total_len) { | ||
| LOG_ERR("data linearization failed for transmitting"); | ||
| ret = -EIO; | ||
| goto cleanup; | ||
| } | ||
|
|
||
| net_buf_add(tx_buf, total_len); | ||
|
|
||
| ret = usbh_cdc_ecm_start_data_out_xfer(ctx, tx_buf); | ||
| if (ret != 0) { | ||
| goto cleanup; | ||
| } | ||
|
|
||
| return 0; | ||
|
|
||
| cleanup: | ||
| if (tx_buf != NULL) { | ||
| net_buf_unref(tx_buf); | ||
| } | ||
|
|
There was a problem hiding this comment.
leave it for now.
But for the non-cachable memory, these host controller drivers should instead add cache management functions in the future
josuah
left a comment
There was a problem hiding this comment.
Sorry for this very incomplete review as I am short on time today.
#99097 (comment) does not seem to be interested in modifying his PR to make it closer to this one. Would you be interested in trying to rebase your PR on top of his content?
If finding modifications needed to #99097, they could then be submitted as review comments to his PR.
josuah
left a comment
There was a problem hiding this comment.
Some various feedback around queued_xfers.list, which can hopefully be avoided.
e67d72b to
2561c10
Compare
2561c10 to
01dfed3
Compare
|
The latest CDC-ECM implementation has improved the following issues
|
01dfed3 to
5822aa9
Compare
| static void carrier_work_handler(struct k_work *work) | ||
| { | ||
| struct k_work_delayable *dwork = k_work_delayable_from_work(work); | ||
| struct carrier_work_ctx *const carrier_work = | ||
| CONTAINER_OF(dwork, struct carrier_work_ctx, dwork); | ||
| struct cdc_ecm_host_data *const host_data = | ||
| CONTAINER_OF(carrier_work, struct cdc_ecm_host_data, carrier_work); | ||
| int state; | ||
|
|
||
| state = atomic_set(&carrier_work->pending_state, CDC_ECM_DEVICE_WORK_CARRIER_STATE_NONE); | ||
|
|
||
| if (!atomic_test_bit(&host_data->flags, CDC_ECM_DEVICE_FLAG_CONNECTED) || | ||
| !atomic_test_bit(&host_data->flags, CDC_ECM_DEVICE_FLAG_FORWARDING)) { | ||
| return; | ||
| } | ||
|
|
||
| switch (state) { | ||
| case CDC_ECM_DEVICE_WORK_CARRIER_STATE_ON: | ||
| LOG_DBG("Carrier ON (deferred)"); | ||
| net_eth_carrier_on(host_data->eth_iface); | ||
| break; | ||
|
|
||
| case CDC_ECM_DEVICE_WORK_CARRIER_STATE_OFF: | ||
| LOG_DBG("Carrier OFF (deferred)"); | ||
| net_eth_carrier_off(host_data->eth_iface); | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
why is this in a work queue?
net_eth_carrier_on and net_eth_carrier_off should be called directly
There was a problem hiding this comment.
Here’s the issue: if a network device supports the ETHERNET_HW_FILTERING feature, and the USB host’s interrupt IN callback in the USB subsystem receives a link-switching notification from the USB device, then if net_eth_carrier_on() is called directly, the network subsystem will directly invoke the code in the ETHERNET_CONFIG_TYPE_FILTER section of .set_config and this code, in turn, calls the USB host’s control transfer function usbh_req_setup(). Thus, the overall call logic is equivalent to triggering a USB control transfer within the USB transfer callback. However, since the usbh_req_setup() is a synchronous API, the thread containing the USB callback will remain blocked until the synchronous usbh_req_setup() API completes, and since the callback triggered by usbh_req_setup is also in the current coroutine, this causes the usbh_req_setup() triggered within the transfer callback to always return a timeout.
@josuah Is it possible to improve the implementation of usbh_req_setup() to support calling it from within a USB transfer callback? If not, is there a better solution here?
There was a problem hiding this comment.
Have you tested it?
because that is the implementation of net_eth_carrier_on and net_eth_carrier_off:
zephyr/subsys/net/l2/ethernet/ethernet.c
Lines 895 to 912 in c855ede
the network subsystem itself uses a workqueue for the real action
There was a problem hiding this comment.
there is a difference between net_eth_carrier_on and net_if_carrier_on
There was a problem hiding this comment.
I checked it locally, directly call can work without issue. Now the work queue is removed. Thanks for the reminder.
| if (is_broadcast) { | ||
| atomic_add(&host_data->stats[USB_CDC_ECM_STAT_BROADCAST_BYTES_RCV], buf_len); | ||
| atomic_inc(&host_data->stats[USB_CDC_ECM_STAT_BROADCAST_FRAMES_RCV]); | ||
| } else if (is_multicast) { | ||
| atomic_add(&host_data->stats[USB_CDC_ECM_STAT_MULTICAST_BYTES_RCV], buf_len); | ||
| atomic_inc(&host_data->stats[USB_CDC_ECM_STAT_MULTICAST_FRAMES_RCV]); | ||
| } else { | ||
| atomic_add(&host_data->stats[USB_CDC_ECM_STAT_DIRECTED_BYTES_RCV], buf_len); | ||
| atomic_inc(&host_data->stats[USB_CDC_ECM_STAT_DIRECTED_FRAMES_RCV]); | ||
| } | ||
| #endif | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
is this needed we already collect these stats in
zephyr/subsys/net/l2/ethernet/ethernet.c
Lines 167 to 182 in 7d0ba33
| } | ||
|
|
||
| #if defined(CONFIG_NET_STATISTICS_ETHERNET) | ||
| static struct net_stats_eth *eth_get_stats(const struct device *dev, struct net_if *iface) |
There was a problem hiding this comment.
the returned struct net_stats_eth pointer is not only for reading, but also for saving. take a look at subsys/net/l2/ethernet/eth_stats.h
your export_statistics works only for reading.
you need to provide a mutable struct net_stats_eth here.
also don't do double work, please check wich fields are already filled by the networking subsystem.
| #if defined(CONFIG_NET_STATISTICS_ETHERNET) | ||
| if (buf_len > 0) { | ||
| atomic_inc(&host_data->stats[USB_CDC_ECM_STAT_XMIT_OK]); | ||
|
|
||
| if (is_broadcast) { | ||
| atomic_add(&host_data->stats[USB_CDC_ECM_STAT_BROADCAST_BYTES_XMIT], | ||
| buf_len); | ||
| atomic_inc(&host_data->stats[USB_CDC_ECM_STAT_BROADCAST_FRAMES_XMIT]); | ||
| } else if (is_multicast) { | ||
| atomic_add(&host_data->stats[USB_CDC_ECM_STAT_MULTICAST_BYTES_XMIT], | ||
| buf_len); | ||
| atomic_inc(&host_data->stats[USB_CDC_ECM_STAT_MULTICAST_FRAMES_XMIT]); | ||
| } else { | ||
| atomic_add(&host_data->stats[USB_CDC_ECM_STAT_DIRECTED_BYTES_XMIT], | ||
| buf_len); | ||
| atomic_inc(&host_data->stats[USB_CDC_ECM_STAT_DIRECTED_FRAMES_XMIT]); | ||
| } | ||
| } | ||
| #endif |
|
Just to mention, not every optional feature needs to be in the initial PR, for example statistics are not needed for networking to work. it is a nice to have and can be added in a later PR. If you really want it in here, put it in its own commit. |
5822aa9 to
9aac218
Compare
The statistics part feature is moved out now. |
9aac218 to
e5acb05
Compare
Updates: - Add Ethernet Statistics Feature Selector marcos - Add CONNECTION_SPEED_CHANGE notification macro - Add Ethernet Power Management Pattern activation macros Signed-off-by: Dv Alan <zhangyang.shen@nxp.com>
Add 4 functions to handle USB string descriptor operations: - usbh_req_desc_str() to retrieve USB string descriptors from device - usbh_desc_is_valid_string() to validate string descriptor type - usbh_desc_get_supported_langs() to get supported languages list - usbh_desc_str_utfle16_to_ascii() to convert UTF-16LE string to ASCII Co-authored-by: Johann Fischer <johann.fischer@nordicsemi.no> Signed-off-by: Dv Alan <zhangyang.shen@nxp.com>
Implements USB CDC-ECM standard protocol and integrates USB-to-Ethernet devices as network interfaces into Zephyr network stack Co-authored-by: Santhosh Charles <santhosh@linumiz.com> Signed-off-by: Dv Alan <zhangyang.shen@nxp.com>
|



Dependencies:
This implementation contains all commits from #94590 and in addition: