OSDev.org

Erm, I'm looking at the code now and I'm wondering what the hell I'm reading. The interrupt firing early is the least of your problems. There are quite a few things here that are fundamentally wrong, such as:

- There is no such thing as a "stack" of queues! If the Link Pointer of a Queue Head has the T bit set, it marks the end of the frame. To avoid an infinite loop if the HCD makes use of bandwidth reclamation, the time taken for each transfer must be counted up, and processing must stop once the time spent exceeds the allowed maximum.
- If a packet does not complete successfully, Bochs happily advances the queue anyway.
- If the device handler returns USB_RET_ASYNC, the NAK bit is not set as it ought to be.
- If the device handler returns USB_RET_NAK, the ActLen field is set to 0x7fd.
- If a short packet is detected, all further queues will become stuck if a packet completes successfully.

Gigasoft wrote:Erm, I'm looking at the code now and I'm wondering what the hell I'm reading. The interrupt firing early is the least of your problems. There are quite a few things here that are fundamentally wrong

What code are you looking at? With the reference to USB_RET_* below, I am guessing the Bochs code. Well, I don't admit that the code is perfect. I coded it so that it would work with various modern (and not so modern) OSes, which it does just fine. Remember, Bochs assumes that all devices attached are valid and can read and write as needed. It has not reason to assume otherwise.

Gigasoft wrote:such as:
- There is no such thing as a "stack" of queues! If the Link Pointer of a Queue Head has the T bit set, it marks the end of the frame. To avoid an infinite loop if the HCD makes use of bandwidth reclamation, the time taken for each transfer must be counted up, and processing must stop once the time spent exceeds the allowed maximum.

Well, this is where you are wrong. There is to a "stack" of queues. For example, if the current Queue Head's vert pointer is pointing to another Queue Head, you now have a stack of Queues.

Gigasoft wrote:- If a packet does not complete successfully, Bochs happily advances the queue anyway.

Again, Bochs assumes that all packets will complete successfully. Why wouldn't it? Bochs is in control. If it makes it to the point where it will now transfer a packet, if the packet isn't going to transfer correct, there is more problems than just the packet transfer. Bochs returns errors for invalid requests, but if it is a valid request to a valid device, the packet *will* transfer correctly.

Gigasoft wrote:- If the device handler returns USB_RET_ASYNC, the NAK bit is not set as it ought to be.

No, the ASYNC is used differently. As far as Bochs is concerned, (and please note I did not write the async part, so I am not totally clear on the process), the transaction is going to be successful.

Gigasoft wrote:- If the device handler returns USB_RET_NAK, the ActLen field is set to 0x7fd.

(IIRC) It should be 0x7FF. I don't know where you are getting 0x7FD, but I can have a look.

Gigasoft wrote:- If a short packet is detected, all further queues will become stuck if a packet completes successfully.

Again, I did not write the ASYNC part and can't comment accurately about that. However, the code does work for all guests that we have tested with, which include various Linux Distros, Win95, Win98SE, WinXP, Win7, as well as others.

If you have additions and or changes, you are welcome to modify the source and send an update request. My email address can be found on the URL below.

Ben
http://www.fysnet.net/the_universal_serial_bus.htm

Wait what? Of course there is no stack of QHs. If the vertical pointer points to another QH, the first QH is forgotten. UHCI (on real hardware / as specified in the spec) does not return to the first QH and does not advance it ever.

For example, if the current Queue Head's vert pointer is pointing to another Queue Head, you now have a stack of Queues.

When the Element Pointer of a Queue Head points to another Queue Head, the first Queue Head is just discarded. The T bit can't be used to return from a Queue Head to the previous Queue Head. Instead, the Link Pointer directly points at the next Queue Head to execute.

Bochs returns errors for invalid requests, but if it is a valid request to a valid device, the packet *will* transfer correctly.

It correctly inactivates the TD and sets the stall bit for an invalid request, but notice that it still advances the queue, which is wrong. It must leave the Element Pointer pointing to the TD that resulted in an error. By the way, if the SETUP packet is invalid, the status returned should be 0x44 (Stalled + Timeout).

No, the ASYNC is used differently. As far as Bochs is concerned, (and please note I did not write the async part, so I am not totally clear on the process), the transaction is going to be successful.

Well, although it's going to be successful, it hasn't completed yet, so one can't just skip past it and handle the TDs that are supposed to come after it ahead of their time. The correct thing to do is to leave the Element Pointer pointing to the TD that is being deferred, and set the status to 0x88 (Active + NAK). USB_RET_NAK should result in the same thing, if it's even being used.

(IIRC) It should be 0x7FF. I don't know where you are getting 0x7FD, but I can have a look.

Sorry, I misread what it said. The ActLen field is correct, it is the Status field that is wrong (0x08 instead of 0x88) as well as the behaviour of advancing the queue.

Oh, I found another thing. Set_status clears the SPD bit for some reason.

Gigasoft wrote:

For example, if the current Queue Head's vert pointer is pointing to another Queue Head, you now have a stack of Queues.

When the Element Pointer of a Queue Head points to another Queue Head, the first Queue Head is just discarded. The T bit can't be used to return from a Queue Head to the previous Queue Head. Instead, the Link Pointer directly points at the next Queue Head to execute.

Forgive me. It has been a considerable time since I have worked with UHCI. I had to go and look. You (and Korona) are correct. Sorry.

Gigasoft wrote:

Bochs returns errors for invalid requests, but if it is a valid request to a valid device, the packet *will* transfer correctly.

It correctly inactivates the TD and sets the stall bit for an invalid request, but notice that it still advances the queue, which is wrong. It must leave the Element Pointer pointing to the TD that resulted in an error. By the way, if the SETUP packet is invalid, the status returned should be 0x44 (Stalled + Timeout).

Looks like I need to dig out my UHCI stuff again and have a look.

Gigasoft wrote:

No, the ASYNC is used differently. As far as Bochs is concerned, (and please note I did not write the async part, so I am not totally clear on the process), the transaction is going to be successful.

Well, although it's going to be successful, it hasn't completed yet, so one can't just skip past it and handle the TDs that are supposed to come after it ahead of their time. The correct thing to do is to leave the Element Pointer pointing to the TD that is being deferred, and set the status to 0x88 (Active + NAK). USB_RET_NAK should result in the same thing, if it's even being used.

As for the ASYNC stuff, I can't comment. When I originally wrote the UHCI stuff (10+ years ago), my code assumed that all TD's would be executed before the 1ms had elapsed. Nothing was deferred. This means that a TD was executed before the controller was advanced. No TD's would be executed out of sequence. Later, another Bochs developer decided to add the deferred stuff. I haven't looked at it to see exactly how it works, but I do know that it may no longer be UHCI accurate due to the advancement of the controller. It also assumes that the line of QHs point to the same string of TDs, which is an assumption, not a rule. Once the deferred stuff was added, I no longer maintained, nor looked at the UHCI code. This is something someone else will have to figure out.

Gigasoft wrote:

(IIRC) It should be 0x7FF. I don't know where you are getting 0x7FD, but I can have a look.

Sorry, I misread what it said. The ActLen field is correct, it is the Status field that is wrong (0x08 instead of 0x88) as well as the behaviour of advancing the queue.

Oh, I found another thing. Set_status clears the SPD bit for some reason.

I will have a look at this and see what we got. Again, it has been nearly 10 years since I have done any UHCI stuff.

Thanks for catching this, and I apologize for the misleading of the "stack" issue. My memory isn't what it use to be.

Ben

Gigasoft wrote:Oh, I found another thing. Set_status clears the SPD bit for some reason.

The SPD bit wasn't being set/cleared until after set_status() was called.

Anyway, I looked over the code and the change to deferred packets has really modified the way Bochs handles the UHCI. It has quite a few problems, problems that were not in the original code.

I admit, for some reason I had a "stack" within the original code, yet I see why I did now. I was stacking the two 32-bit dwords on each other so that once the Vert Pointer was done, I only had to "pop" it off the stack to get to the Horz Pointer.

Anyway, after looking over the code, there is going to have to be some major work done on it. If it was me, I would remove the deferred technique, but I don't think the Bochs developers will do that. I will have to look and see what I can do to modify the code.

The biggest thing will be to figure out how to "end the timer", i.e.: find out when there are no more transfers to do. For example, Linux has an empty set of Queues in a round robin type loop at the end of their frame. The current code just loops through it a few times and quits. I will probably have to detect if the same Queue is being executed multiple times with no active TDs.

Anyway, lots has changed, more than I thought had been done. If I get a chance, I will see what I can do to remedy the situation.

Thanks,
Ben

The SPD bit should be left unchanged, it's just a parameter. As for the async packet business, it is inherently fishy. For example, someone could start a transfer, and then decide to abort it before it has completed, and reuse the TD for a different transfer.

The deferring behaviour can stay, but it must be implemented without trying to remember information about specific TDs, and the UHCI controller needs to handle it correctly.

Gigasoft wrote:As for the async packet business, it is inherently fishy. For example, someone could start a transfer, and then decide to abort it before it has completed, and reuse the TD for a different transfer.

The deferring behaviour can stay, but it must be implemented without trying to remember information about specific TDs, and the UHCI controller needs to handle it correctly.

I am in communication with the Bochs developer on this and hope to be able to revamp it soon.

Thanks again,
Ben

Oh, found another strange thing. When I reset a port, the Connect Status Change bit becomes set. That doesn't seem right?

Gigasoft wrote:Oh, found another strange thing. When I reset a port, the Connect Status Change bit becomes set. That doesn't seem right?

On a connected port, after a reset, this bit is set on all hardware I tested with as well as the way I interpret the documentation. On an empty port, this bit should be zero. Are you seeing it as set on an empty port?