Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UHCI: do not deactivate TDs on NAKs #362

Merged
merged 1 commit into from
Oct 21, 2024
Merged

Conversation

Cacodemon345
Copy link
Contributor

@Cacodemon345 Cacodemon345 commented Oct 9, 2024

NAKs from USB devices do not deactivate TDs according to the official UHCI spec (page 22).

Reference: http://ftp.netbsd.org/pub/NetBSD/misc/blymn/uhci11d.pdf

@fysnet
Copy link
Collaborator

fysnet commented Oct 12, 2024

I think this PR is valid.

The only thing I think I would do different, is add another field to the set_status() call, sending a 1 to keep the active bit set, and a 0 to clear it. This way, if we find another reference where this is the case, a simple change of a single digit is all that is needed. Since there are only three calls to this function, and they are all within a couple of lines of each other, this addition may be overkill though.

@fysnet
Copy link
Collaborator

fysnet commented Oct 19, 2024

It's dword1 not dword2 though.

I would do something similar to:

--- uhci_core.h         2024-10-19 16:44:23.161155600 -0700
+++ uhci_core.h        2024-10-19 16:40:21.021583600 -0700
@@ -215,7 +215,7 @@
   static void uhci_timer_handler(void *);
   void uhci_timer(void);
   bool DoTransfer(Bit32u address, struct TD *);
-  void set_status(struct TD *td, bool stalled, bool data_buffer_error, bool babble,
+  void set_status(struct TD *td, bool active, bool stalled, bool data_buffer_error, bool babble,
     bool nak, bool crc_time_out, bool bitstuff_error, Bit16u act_len);

   static Bit32u read_handler(void *this_ptr, Bit32u address, unsigned io_len);
--- uhci_core.cc        2024-10-19 16:44:02.468873800 -0700
+++ uhci_core.cc       2024-10-19 16:41:08.843534900 -0700
@@ -1009,11 +1009,11 @@
     }
   }
   if (ret >= 0) {
-    set_status(td, 0, 0, 0, 0, 0, 0, len-1);
+    set_status(td, 0, 0, 0, 0, 0, 0, 0, len-1);
   } else if (ret == USB_RET_NAK) {
-    set_status(td, 0, 0, 0, 1, 0, 0, len-1); // NAK
+    set_status(td, 1, 0, 0, 0, 1, 0, 0, len-1); // NAK
   } else {
-    set_status(td, 1, 0, 0, 0, 0, 0, 0x007); // stalled
+    set_status(td, 0, 1, 0, 0, 0, 0, 0, 0x007); // stalled
   }
   remove_async_packet(&packets, p);
   return 1;
@@ -1032,13 +1032,14 @@
 }

 // If the request fails, set the stall bit ????
-void bx_uhci_core_c::set_status(struct TD *td, bool stalled, bool data_buffer_error, bool babble,
+void bx_uhci_core_c::set_status(struct TD *td, bool active, bool stalled, bool data_buffer_error, bool babble,
                              bool nak, bool crc_time_out, bool bitstuff_error, Bit16u act_len)
 {
   // clear out the bits we can modify and/or want zero
   td->dword1 &= 0xDF00F800;

   // now set the bits according to the passed param's
+  td->dword1 |= active            ? (1<<23) : 0; // active
   td->dword1 |= stalled           ? (1<<22) : 0; // stalled
   td->dword1 |= data_buffer_error ? (1<<21) : 0; // data buffer error
   td->dword1 |= babble            ? (1<<20) : 0; // babble
@@ -1050,7 +1051,6 @@
     td->dword1 &= ~((1<<28) | (1<<27));  // clear the c_err field if there was an error
 }

-
 // pci configuration space write callback handler
 void bx_uhci_core_c::pci_write_handler(Bit8u address, Bit32u value, unsigned io_len)
 {

@vruppert
Copy link
Contributor

Please update PR based on Ben's suggestion.

@Cacodemon345
Copy link
Contributor Author

Done.

@vruppert vruppert merged commit f6617f2 into bochs-emu:master Oct 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants