Skip to content

Conversation

josuah
Copy link
Contributor

@josuah josuah commented Aug 14, 2025

Downstream:

This is a very small, incremental change to help downstream pull requests use a shared base.

I was thinking of doing several such small pull requests with just a few commits, to progressively integrate the code of other pull requests:

Here is how I tested it:

west build -t run -b native_sim samples/subsys/usb/shell/ \
-DEXTRA_CONF_FILE=virtual.conf \
-DDTC_OVERLAY_FILE=virtual.overlay \
-DEXTRA_CONF_FILE=device_and_host_prj.conf

Then in another terminal, connect to the console, such as using picocom /dev/pts/0.

Make the struct name match the device naming for ease of use, although
slightly longer name. Propagate the change to the subsystem, includes,
tests and samples.

Signed-off-by: Josuah Demangeon <[email protected]>
@josuah josuah added the area: USB Universal Serial Bus label Aug 14, 2025
@josuah josuah marked this pull request as ready for review August 14, 2025 12:57
@zephyrbot zephyrbot added the area: Samples Samples label Aug 14, 2025
@josuah josuah changed the title Start refactoring the Host class API USB Host: integrate class API [1] Aug 17, 2025
@josuah josuah changed the title USB Host: integrate class API [1] USB Host: integrate class API [1: types] Aug 17, 2025

udev->cfg_desc = k_heap_alloc(&usb_device_heap,
cfg_desc.wTotalLength,
cfg_desc.wTotalLength + sizeof(struct usb_desc_header),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the intention? I remember doing the same, but then I dropped it in favor of just using wTotalLength.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to add a nil descriptor at the end so that places like this can be sure to never go past the end of the array:

while ((dhp->bDescriptorType != 0 || dhp->bLength != 0) && (void *)dhp < desc_end) {

Maybe the code above can be modified instead? Such as testing for (void *)dhp < desc_end only and not test for dhp->bDescriptorType != 0 || dhp->bLength != 0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the concept of nil descriptor. I think the descriptor sets for classes should just end with NULL pointer, and the check here should be fine with (void *)dhp < desc_end condition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But of course, this needs to detect bLength == 0 and fail (if it is within wTotalLength) because the data is tainted (coming from external untrusted source).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you I understand the difference now, I am dropping the commit.

Copy link
Contributor Author

@josuah josuah Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with this to check dhp before it is de-referenced:

-while ((dhp->bDescriptorType != 0 || dhp->bLength != 0) && (void *)dhp < desc_end) {
+while ((void *)dhp < desc_end && (dhp->bDescriptorType != 0 || dhp->bLength != 0)) {

*
* @return true if USB host is in enabled, false otherwise
*/
static inline bool usbd_is_enabled(const struct usbh_context *const uhs_ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/usbd/usbh

However, I am not sure how it could be used. In my approach, I do not explicitly register any class drivers, I simply place them in an iterable section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am removing it then, thank you

*
* @return true if USB host is in enabled, false otherwise
*/
static inline bool usbd_is_initialized(const struct usbh_context *const uhs_ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/usbd/usbh

@josuah josuah force-pushed the pr_usb_host_class_api branch from 2a7863a to 4b8314c Compare September 3, 2025 12:13
@zephyrbot zephyrbot requested a review from JarmouniA September 3, 2025 12:14
In the while loop parsing descriptors, check that the descriptor is past
the end just before dereferencing it to check if it is seemingly valid.

Signed-off-by: Josuah Demangeon <[email protected]>
Comment on lines 20 to 31
static inline bool usbh_is_initialized(const struct usbh_context *const uhs_ctx)
{
return uhs_ctx->status.initialized;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, perhaps I was not clear. I think it would be better to remove this commit from the PR. It is not used anywhere. I will comment on your second PR once this one has been merged.

@josuah josuah force-pushed the pr_usb_host_class_api branch 3 times, most recently from d78e394 to d769180 Compare September 4, 2025 15:48
Josuah Demangeon added 2 commits September 4, 2025 15:49
Hide the mutex lock details inside a wrapper call, like done for the
device stack API. This is not used for the per-device lock of the host
stack which use a different mutex lock.

Signed-off-by: Josuah Demangeon <[email protected]>
Update the USB shell sample to the new syntax. This is based on tests
done with native_sim, along with the virtual.conf, virtual.overlay and
device_and_host_prj.conf extra config files.

Signed-off-by: Josuah Demangeon <[email protected]>
@josuah josuah force-pushed the pr_usb_host_class_api branch from d769180 to 8a40c80 Compare September 4, 2025 15:49
Copy link

sonarqubecloud bot commented Sep 4, 2025

@josuah
Copy link
Contributor Author

josuah commented Sep 4, 2025

Well it works much better when testing things before pushing...

@josuah josuah requested a review from jfischer-no September 4, 2025 16:24
@kartben kartben merged commit 190cde1 into zephyrproject-rtos:main Sep 5, 2025
26 checks passed
struct sys_bitarray *addr_ba;
};

#define USBH_CONTROLLER_DEFINE(device_name, uhc_dev) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please consider adding doxygen docs at some point in the future :) thanks!

@josuah josuah deleted the pr_usb_host_class_api branch September 7, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Samples Samples area: USB Universal Serial Bus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants