Re: [PATCH RFC 1/2] rust: usb: add endpoint abstraction

On Mon Aug 3, 2026 at 4:23 PM EEST, Greg Kroah-Hartman wrote:
> On Mon, Aug 03, 2026 at 03:46:44PM +0300, Alrexandru Radovici wrote:
>> On Sun Aug 2, 2026 at 11:39 AM EEST, Greg Kroah-Hartman wrote:
>> > On Sat, Aug 01, 2026 at 03:01:07AM +0300, Alexandru Radovici wrote:
>> >> Add an abstraction for `struct usb_host_endpoint`, together with the
>> >> accessors needed to reach one: `AlternateSetting` wrapping
>> >> `struct usb_host_interface`, `Interface::alternate_settings()` and
>> >> `Interface::current_alternate_setting()`, and `Device::control_endpoint()`
>> >> for the default control endpoint, which no interface descriptor lists.
>> >
>> > Why?  USB drivers shouldn't be messing with usb_host_endpoint structures
>> > for the most part, what user do you have for this?
>> 
>> The more I think of this, I think you are right. `HostEndpoint`'s accessor
>> methods are only used for debug, as the `kernel` crate can access
>> the actual `usb_host_endpoint` underneeth. For debug purposes, we should
>> just derive the `Debug` trait instead.
>
> Great, if it's even really needed.  Let's see how that works out, as I
> don't know what you want to provide for debugging.
>
>> >> `HostEndpoint` is generic over two sealed marker traits,
>> >> `EndpointDirection` and `EndpointTransferType`, whose implementors are
>> >> 1-ZSTs held in `PhantomData`. An endpoint borrowed from an alternate
>> >> setting starts out generic in both; `as_in()`, `as_out()` and
>> >> `as_control()` check the descriptor once and return a reference
>> >> carrying the corresponding marker, so a function taking
>> >> `&HostEndpoint<In, Bulk>` needs no check of its own. The type is
>> >> `#[repr(transparent)]` over the C struct and the markers are
>> >> zero-sized, so the refinement costs nothing and a slice of endpoints
>> >> can be borrowed directly from the C array.
>> >> 
>> >> Control endpoints get a distinct `Bidirectional` marker rather than an
>> >> IN or OUT one. A control transfer takes its direction from bit 7 of the
>> >> setup packet's bmRequestType, and USB 2.0 section 9.6.6 defines the
>> >> corresponding bit of bEndpointAddress as ignored for control endpoints.
>> >> `as_in()` and `as_out()` are not implemented for `Bidirectional`, making
>> >> calling them a compile error rather than a misleading result.
>> >
>> > Don't over-think USB endpoints, they are "just" a pipe that contain a
>> > numbering scheme that the USB core uses.  Is that what you are trying to
>> > create here?  What are you trying to "enforce" here that the C code does
>> > not already do?
>> 
>> My USB knowledge is limited, so I hope I am not saying something
>> stupid here. My understanding is that drivers should not expect
>> interfaces to map the same endpoints (numbers) every time.
>
> Why not?  Well, they can, or can not, depending on the device, and the
> driver knows this.  For some drivers, a specific endpoint will _ALWAYS_
> be a specific number, while for others, they are dynamically determined.
> It depends on the device/protocol being used.
>
>> A driver should expect an interface to expose a certain number of
>> endpoints, each one with a certain type, but the actual number of each
>> exposed endpoint is not to be considered hardcoded. This means that drivers
>> should anyway iterate over the endpoints to discover the numbers
>> of the required endpoints.
>
> Again, sometimes, but not always.  What a driver SHOULD always do is
> verify that the device is providing the specific number and types of
> endpoints that it is expecting at probe time and call the core to "find"
> the expected endpoints that are present.  In the C api we do that with
> the usb_find_common_endpoints() or the usb_check_bulk_endpoints() type
> functions.
I suggest adding a structure and functions like:

	struct CommonEndpoints {
		bulk_in: Option<HostEndpoint<In, Bulk>>,
		...
	}

	pub fn find_common_endpoints(&self) -> CommonEndpoints;

I can add them in the v2 series of patches, but as there is no
user yet, not sure if I should.
>
>> My idea is to leaverage Rust's type system to prevent users from supplying
>> the wrong endpoint type at compile time rather then at runtime. By making
>> the `HostEndpoint` its own Rust type with no public constructor,
>> users will be forced to iterate the endpoints to discover the correct
>> number for each endpoint that they require. Once they have it, users
>> can hold to the reference as long as the interface is valid.
>
> Having a reference is great, but really, these are things that you
> should just call the core for and get a reference back.  No need for the
> special encoding logic, see how "simple" the C code is for this please.

I started looking closer at the C API, I think I have a better understanding
of what you mean, I need to think a little more about this. The v2 set of
patches will not include these changes yet.

>> By adding the `Dir` and `Type` generic markers, suplying the wrong endpoint
>> to a function will be caught at compile time rather than at runtime. This
>> should hopefully shorthen the debug work needed for a driver, as some of
>> the errors become impossible.
>
> functions should be taking any "type" of endpoint as this will be
> checked when the USB core actually submits the data to the device, so no
> driver will get very far if all is not correct.  No real need to attempt
> to provide many different types and check it all in the api as the api
> needs to handle all endpoint types, right?
I still think that this can be checked at compile time and should be.

>> As endpoint 0 is always provided and basically _almost hardcoded_``, I added
>> the `control_endpoint` function. 
>
> That's great, but again, we "know" what that endpoint type is, and it
> will be used for both read and write operations, BUT you need to specify
> it somehow which way you want that operation to happen when you make the
> API call, right?

For control endpoints, I guess this is done by the names of
the `control_message_...` functions. They both take the
same endpoint, and use it in different ways.

For Bulk and Interrupt transfers I would add API like:

	trait UsbMessageSend<Type> {
		fn message(&self, endpoint: HostEndpoint<Out, Type>, data: &[u8]) -> Result<...>;
	}

	trait UsbMessageReceive<Type> {
		fn message(&self, endpoint: HostEndpoint<In, Type>, data: &mut [u8]) -> Result<...>;
	}

	and implement these for usb::Interface for Bulk and Interrupt.

Users will just call

	intf.message(endpoint, data)

and the compiler will select the correct function. There is no way a
user could use an endpoint in a wrong way.

This API is somehow in line with what nusb does.
https://docs.rs/nusb/latest/nusb/

I am not sure yet how to implement this for Isochornous transfers.

> I would recommend actually porting/writing a USB driver using the apis
> while you are attempting to make these bindings, as I think a lot of
> these issues will fall out automatically when doing so.  USB really
> isn't that complicated, it's just a dumb and slow "pipe" that for every
> message, is triggered by a host request, no matter which way the data is
> flowing.

I am porting usbsevseg.c and I'll send the v2 version of these patches that
include all the infrastructure needed to port the driver.

> thanks,
>
> greg k-h