Review the USB 3.0 stack #21
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also, it is just not possible to make generic transfer abortion. So the current semantics of endpoint unregistering is also aborting the pending transfer. As it is not yet implemented in XHCI, and the stub in UHCI is missing the magic, it breaks offlining interrupt devices, such as mouse. When finishing this commit, I came across the fact we need some more synchronization above device. Guard can protect internal structures, but it cannot synchronize multiple calls to offline, or offline & removal between each other - they both need to allow driver to unregister endpoints, and as such must release the guard.
Traversing loop looking for finished xhci transfer is now only 1 loop traversal in average case. Added reset timer which resets the endpoint after there is no action for a given period.
Previousy, we abused the fact new fibrils are spawned for handling notifications, so we could afford blocking the event handler. We were told this is a subject to change and we should stop doing it. This commit removes the abuse, but newly requires event handlers not to block waiting for another event (e.g. commands do wait for events). To quickly detect this situation, deadlock detection was added. This commit breaks current functionality. Our current job is to identify processes which do block and have them moved to separate fibril / spawn fibril for the process alone.
There are two new generic mechanisms, which are used in only one instance. First one is a simple wrapper for roothub event handlers that should run in separate fibril. The usage can be seen at line 337, for example. The mechanism just passes arguments to a newly created fibril. The second is a bit more complex, and its purpose is to broadcast roothub events to all fibrils that could wait for it. It's used to wait for port reset complete at USB 2 ports. See rh_port_reset_sync. Sorry for naming both mechanisms "rh_event", which could create a confusion.
Redistributed code between device_remove(), _removed() and _gone() in order to minimize redundancy and improve efficient design.
Renamed polling synchronization primitives with the same convention as in `usbhub`. Added some documentation.
le-jzr
reviewed
Feb 21, 2018
| dir, buffer, transfer_size, -1, last); | ||
| td_init(&ehci_batch->tds[td_current], | ||
| last ? 0 : dma_buffer_phys(&ehci_batch->ehci_dma_buffer, | ||
| &ehci_batch->tds[td_current + 1]), |
There was a problem hiding this comment.
Nit: The last tab should be four spaces instead.
There was a problem hiding this comment.
Well, it's a continuation of a line continuation (more refactoring would certainly help!), so IMO it shall be 8 spaces.
There was a problem hiding this comment.
A continuation line does not have a continuation line of its own with a different indent. One big line can have multiple continuation lines that are each indented the same - by four spaces.
There was a problem hiding this comment.
Ok, my comment is not entirely clear if you don't have visible whitespace or a custom tab width. The line has three tab indents and four spaces indentation. It should be two tabs and eight spaces. IMO the extra level of continuation is correct, because the nested continuation continues a nested syntactic construct. Flat four space continution would harm readability. Or, you could use intermediate variables. That would probably be best.
There was a problem hiding this comment.
This not however how it is supposed to be used.
le-jzr
reviewed
Feb 22, 2018
| void td_init(td_t *instance, const td_t *next, | ||
| usb_direction_t direction, const void *buffer, size_t size, int toggle, | ||
| bool ioc) | ||
| void td_init(td_t *instance, uintptr_t next_phys, uintptr_t buffer, |
There was a problem hiding this comment.
Why has the type of buffer changed?
If it's a physical address (or other kind of different address space), it would be more readable if this used a typedef describing what kind of pointer it is.
There was a problem hiding this comment.
Previously, it was a virtual pointer to a generic data, now it's a physical one. uintptr_t is used as physical pointer all over the project...
There was a problem hiding this comment.
In the future, physical addresses will be able to have different width than virtual addresses (for example when we support PAE on 32-bit systems), but IMO it's out of scope for this project and will need a global change later.
There was a problem hiding this comment.
I see. Yes, if you use physical pointers a lot, it wouldn't make sense to change just this instance. And it would also need to be consistent with the rest of the system (a special type in /abi?). So yeah, out of scope.
le-jzr
reviewed
Feb 22, 2018
| /* TDs must be 32-byte aligned */ | ||
| PADD32 [3]; | ||
|
|
||
| } __attribute__((packed)) td_t; |
There was a problem hiding this comment.
Does this need packed? This attribute should only be used if the structure or its members are misaligned in memory.
There was a problem hiding this comment.
Ad PADD32 macro.. this is an example of quite ugly preprocessor magic that I'd rather be avoided.
There was a problem hiding this comment.
It shall not need it. But it won't do any harm either, and for me packed is a kind of hint that this structure is expected to stay exactly as it was written.
There was a problem hiding this comment.
@jxsvoboda then why it is used everywhere else? :) If you want new code to stop using it, a slight deprecation note would do do the trick. Otherwise i rather like this one, because it shows that the padding must be there and it's not of any interest for reader, because it's not touched by the code. And it's very self-explanatory.
There was a problem hiding this comment.
First of all, the older code can be and often is wrong too or could suffer from things we didn't know at the time. So avoiding better practices because the old code does not follow them is IMO itself not a good practice.
Secondly, I think when you have a packed structure, it literally tells the compiler that the alignment of the structure is 1 (besides also packing it). There is a different attribute for alignment, which can specify a minimal alignment (if greater than the alignment inferred from the structure's members). But maybe the comment is wrong and should rather say "TDs must be padded to 32-bytes?"
There was a problem hiding this comment.
I am not sure, but don't aligned(32) and packed contradict each other? When you use both, what does alignof() say?
There was a problem hiding this comment.
No, they don't:
typedef struct foo {
char pad;
unsigned long long foo;
} __attribute__((packed,aligned(32))) foo_t;
int main() {
printf("%zu %zu", sizeof(foo_t), &((foo_t *) NULL)->foo); // 32 1
}
There was a problem hiding this comment.
I think it'd be nice to have HW_MEM_STRUCT(size, align) macro, that would hide the exact attributes. It can even add a static assert check suggested by @le-jzr. Both size and alignment are usually stated in the docs, so it's easy to reference the information in a comment.
It can also change per architecture/compiler if need be.
@Aearsis why is sizeof(foo_t) == 32? in the above example. Was it supposed to be alignof() ?
There was a problem hiding this comment.
I can't see how to make the macro also add an static assert, without being way too magic :)
@jvesely alignof is C++ specific, but sizeof() here is correct. That was actually the initial motivation - the structure has to be padded to align at the end in order to allow arrays to be created - because array offsets are just syntax sugar for pointer arithmetics, and pointer arithmetics uses size.
There was a problem hiding this comment.
@Aearsis There is _Alignof since C11.
le-jzr
reviewed
Feb 22, 2018
uspace/drv/bus/usb/ohci/hc.c
Outdated
| } | ||
|
|
||
| if ((err = ohci_transfer_batch_prepare(ohci_batch))) | ||
| return err; |
There was a problem hiding this comment.
No, it's few lines below. OHCI is designed so wrong, that it needed to lock the endpoint before the transfer is prepared. In short: it has to reuse the last TD of previous transfer as the first one.
But thank's for the comment, because we overcome this and forgot that now it's not needed :)
There was a problem hiding this comment.
I think this comment was re. the error path on line 313.
There was a problem hiding this comment.
Oh, of course you're right then. Will be solved by moving out of the critical section.
jermar
commented
Feb 24, 2018
There was a problem hiding this comment.
I added some comments, to the individual changes.
Noticed that so far we have completely ignored caches during DMA operations (see #720 in trac).
There are two mechanisms that are not USB specific and should be moved somewhere else or be prefixed by usb_: DMA buffer management and joinable fibrils.
Some of the code does not follow our coding style, for examples lines are sometimes over 80 columns, wrong indentation
uspace/drv/bus/usb/usbhub/port.h
Outdated
| @@ -1,6 +1,7 @@ | |||
| /* | |||
| * Copyright (c) 2011 Vojtech Horky | |||
| * Copyright (c) 2011 Jan Vesely | |||
| * Copyright (c) 2017 Ondra Hlavaty | |||
uspace/drv/bus/usb/xhci/debug.c
Outdated
| usb_log_debug("Extended capability %s", xhci_ec_str_id(id)); | ||
|
|
||
| switch (id) { | ||
| case XHCI_EC_SUPPORTED_PROTOCOL: |
There was a problem hiding this comment.
The case should not be indented.
uspace/drv/bus/usb/xhci/isoch.c
Outdated
| @@ -0,0 +1,667 @@ | |||
| /* | |||
| * Copyright (c) 2017 HelUSB3 team | |||
There was a problem hiding this comment.
Please state the actual authors here.
uspace/drv/bus/usb/xhci/isoch.c
Outdated
| const xhci_trb_completion_code_t completion_code = TRB_COMPLETION_CODE(*trb); | ||
|
|
||
| switch (completion_code) { | ||
| case XHCI_TRBC_RING_OVERRUN: |
There was a problem hiding this comment.
case should not be indented
uspace/drv/bus/usb/xhci/isoch.h
Outdated
| @@ -0,0 +1,131 @@ | |||
| /* | |||
| * Copyright (c) 2017 HelUSB3 team | |||
There was a problem hiding this comment.
Please state the actual authors here.
| @@ -0,0 +1,248 @@ | |||
| /* | |||
There was a problem hiding this comment.
If this is not a generic dma_ mechanism, please prefix it with usb_
There was a problem hiding this comment.
Actually, it is a bit wider idea which we think shall be adopted by libdrv. But that's for separate discussion.
uspace/lib/usb/src/port.c
Outdated
| @@ -0,0 +1,255 @@ | |||
| /* | |||
| * Copyright (c) 2018 HelenOS xHCI team | |||
There was a problem hiding this comment.
Please state the names of the actual authors here.
uspace/lib/usb/include/usb/port.h
Outdated
| @@ -0,0 +1,110 @@ | |||
| /* | |||
| * Copyright (c) 2018 HelenOS xHCI team | |||
There was a problem hiding this comment.
Please state the name of the actual authors here.
| int hc_get_hub_desc(device_t *, usb_hub_descriptor_header_t *); | ||
| int hc_device_explore(device_t *); | ||
|
|
||
| /** Joinable fibril */ |
There was a problem hiding this comment.
This should be imo part of the stock fibril support in libc.
There was a problem hiding this comment.
Ideally by making fibril joinable by default.
There was a problem hiding this comment.
Yeah, we think so. But we like to introduce such changes in our subtree first, they can be moved easily later.
There was a problem hiding this comment.
I see, but there is really no benefit in hiding this in your subtree when you have your own branch. See the note about self-contained code here: http://www.helenos.org/wiki/StudentTips#Implementation
There was a problem hiding this comment.
OK, let's move it to libc. I don't think we shall change the behavior of fibril (making fid_t a structure with former fid_t, mutex, condvar etc.). This is more an "async task", though the term "task" shall probably not be used to avoid confusion. How do you think it should be called?
There was a problem hiding this comment.
I wouldn't mind if the fibrils themselves were joinable. If this is not feasible for some reason, I'd move support for your joinable fibrils next to the ordinary fibrils in libc and unify the functionality later. As for the possible name, how about jibrils? Just kiddig :-)
There was a problem hiding this comment.
The only way how to make fibrils joinable is making fibril_t reference-counted. If that is feasible, we can make fibrils joinable on-demand before starting them. The current usage would be preserved, and code that would want to join their fibrils would have to increase the refcount, join and decrease the refcount again. (This has even the neat imaginary benefit of allowing more fibrils to join on one :) ). Counterexamples, opinions, comments, acks?
There was a problem hiding this comment.
I think the general concensus the last time we discussed fibrils is that we want to move towards a more generic threading API, and deprecate any use of fibrils that doesn't work with the standard APIs. That means we should go with the traditional join()/detach() scheme, to avoid future headaches. We'll probably also need a new creation function, and leave the current one as detached-by-default, to avoid changes to existing code. I think we'll eventually drop the fibril API entirely, in favor of C11's <threads.h>, so there's no point in fussing with it too much.
There was a problem hiding this comment.
In other words, essentialy reference counting with default value 2, and fibril_create calling fibril_create_joinable and dropping the reference? That could work. Shall I prepare it?
There was a problem hiding this comment.
Yeah, something like that. Please do, thanks! :)
| } | ||
| } | ||
|
|
||
| typedef struct joinable_fibril { |
There was a problem hiding this comment.
We need to move this to libc or make fibrils joinable by default.
The data was generated by a script, guided manually. If you feel your name is missing somewhere, please add it! The semi-automated process was roughly: 1) Changes per file and author (limited to our team) were counted 2) Trivial numbers were thrown away 3) Authors were sorted by lines added to file 4) All previous copyrights were replaced by the newly generated one 5) Hunks changing only year were discarded It seems that a lot of my copyrights were added. It is due to me being both sticking my nose everywhere and lazy to update the copyright right away :)
|
I successfully tested this on my desktop machine, where I was able to use USB keyboard plugged into OHCI and XHCI controllers. The stack detected a USB mass-storage attached vie EHCI. This pretty much fixes the stack on this machine to a large degree (some ports seem not to be powered). I was not that lucky on my Lenovo x220 laptop, where USB hid devices connected via EHCI would be discovered only when the laptop was docked. Even then the usbhid driver would report stalled polls, so the keyboard / mouse was unusable. Also tested in QEMU where I could successfully use a USB mouse attached via all USB controllers. |
|
Here is what tools/ccheck.sh found in the files touched by this merge (some of this might have existed before though): |
|
@jermar Tried to fix some of the cstyle errors, and it complaints about a space after asterisk on e.g.:
The line defines a constant pointer. I really don't think we shall remove the space, but I want to make sure. What is the preffered way of writing these declarations? |
|
@Aearsis I think you can leave this one as is. |
|
@Aearsis There were more serious violations of the coding style, such as wrong indentation, lines to long and not making spaces in essential places. |
le-jzr
approved these changes
Feb 27, 2018
| @@ -116,7 +120,7 @@ errno_t usbmid_spawn_interface_child(usb_device_t *parent, | |||
| * The interface number shall provide uniqueness while the | |||
| * class name something humanly understandable. | |||
| */ | |||
| int ret = asprintf(&child_name, "%s%hhu", | |||
| errno_t ret = asprintf(&child_name, "%s%hhu", | |||
| usb_debug_str_buffer(buffer, buffer_size, 0)); | ||
|
|
||
| if (hid_dev->max_input_report_size >= buffer_size) { | ||
| /*! @todo This should probably be atomic. */ |
There was a problem hiding this comment.
I'm not even sure this is our code, but sure we'll change it :)
| #define uint8_t_le2host(n) (n) | ||
| #define host2uint8_t_be(n) (n) | ||
| #define host2uint8_t_le(n) (n) | ||
| #define host2uint8_t_le(n) (n) |
Hi, let's do some preliminary review.
@jvesely, it would be interesting to have your high-level review of the overall architecture changes
@vhotspur, ^