[USBUHCI_NEW] Bring-in the USB UHCI miniport driver created by Vadim Galyant. #245

AmineKhaldi · 2017-12-30T20:08:24Z

No description provided.

ThFabba · 2018-01-12T20:22:52Z

drivers/usb/usbuhci_new/hardware.h

+  };
+  USHORT AsUSHORT;
+} UHCI_PCI_LEGSUP;
+


This file could use some C_ASSERTs

HBelusca · 2018-01-12T20:28:36Z

Can we wait for few days until I'm able to finish to review this PR? :)

ThFabba · 2018-01-12T20:41:03Z

drivers/usb/usbuhci_new/roothub.c

+                IN PUSHORT Status)
+{
+    DPRINT("UhciRHGetStatus: ...\n");
+    *Status = 1;


Constantify.

ThFabba · 2018-01-12T20:43:24Z

drivers/usb/usbuhci_new/roothub.c

+VOID
+NTAPI
+UhciRHGetRootHubData(IN PVOID uhciExtension,
+                     IN PVOID rootHubData)


This function should use PUSBPORT_ROOT_HUB_DATA directly. I'll push a change to fix this in usbport and usbohci shortly.

I will fix this at the last minute or after fixes in usbport, so that the builder does not give errors.

Sounds good. Let me know if you want me to get something into master before this is merged.

ThFabba · 2018-01-12T20:43:51Z

drivers/usb/usbuhci_new/roothub.c

+    //DPRINT("UhciRHGetPortStatus: ...\n");
+
+    BaseRegister = UhciExtension->BaseRegister;
+    PortControlRegister = &BaseRegister->PortControl[Port-1].AsUSHORT;


We'll want a Port > 0 assert again here.

ThFabba · 2018-01-12T20:45:12Z

drivers/usb/usbuhci_new/roothub.c

+        portStatus.Suspend = FALSE;
+    }
+
+    // FIXME HcFlavor in usbport


Sounds fixable :p

First we need HcFlavor support in usbport.
This is for VIA controllers.

ThFabba · 2018-01-12T20:45:39Z

drivers/usb/usbuhci_new/roothub.c

+    //if (UhciExtension->HcFlavor == UHCI_Piix4)
+    if (TRUE)
+    {
+        portStatus.OverCurrent = PortControl.Reserved2 & 1;


Intel use it bits, but not documented in UHCI 1.1 spec.

ThFabba · 2018-01-12T20:46:16Z

drivers/usb/usbuhci_new/roothub.c

+    portStatus.CurrentConnectStatus = PortControl.CurrentConnectStatus;
+    portStatus.PortEnabledDisabled = PortControl.PortEnabledDisabled;
+
+    if (PortControl.Suspend == TRUE &&


I think we generally use 0/1 rather than FALSE/TRUE for bitfields.

ThFabba · 2018-01-12T20:47:19Z

drivers/usb/usbuhci_new/roothub.c

+    portStatus.LowSpeedDeviceAttached = PortControl.LowSpeedDevice;
+    portChange.ConnectStatusChange = PortControl.ConnectStatusChange;
+
+    port = 1 << (Port - 1);


portBit or something would be more descriptive.

ThFabba · 2018-01-12T20:49:01Z

drivers/usb/usbuhci_new/roothub.c

+    DPRINT("UhciRHPortResetComplete: ...\n");
+
+    BaseRegister = UhciExtension->BaseRegister;
+    Port = *(PUSHORT)pPort - 1;


ASSERT, possibly with another added variable to avoid all the casting

Port = *(PUSHORT)pPort; ASSERT(Port != 0); PortControlRegister = &BaseRegister->PortControl[Port - 1].AsUSHORT;

ThFabba · 2018-01-12T20:51:05Z

drivers/usb/usbuhci_new/roothub.c

+        {
+            continue;
+        }
+


You can stop the while loop here, since all the paths below use return, so will never loop.
E.g.:

while (UhciHardwarePresent(UhciExtension)) { PortControl.AsUSHORT = READ_PORT_USHORT(PortControlRegister); if (PortControl.PortReset == 0) { break; } }

ThFabba · 2018-01-12T20:52:30Z

drivers/usb/usbuhci_new/roothub.c

+    DPRINT("UhciRHSetFeaturePortReset: ...\n");
+
+    ResetPortMask = UhciExtension->ResetPortMask;
+    port = 1 << (Port - 1);


portBit or similar

ThFabba · 2018-01-12T20:52:52Z

I've only just started, so yes.

ThFabba · 2018-01-12T20:54:21Z

drivers/usb/usbuhci_new/roothub.c

+
+VOID
+NTAPI
+UhciRHSetFeaturePortResetWorker(IN PUHCI_EXTENSION UhciExtension,


I'm curious why this is its own function. It only seems to be called in once place.

It use also UhciVia...(), but it not implemented.

ThFabba · 2018-01-12T20:57:43Z

drivers/usb/usbuhci_new/roothub.c

+    PortControlRegister = (PUSHORT)&BaseRegister->PortControl[Port - 1];
+    PortControl.AsUSHORT = READ_PORT_USHORT(PortControlRegister);
+
+    if (PortControl.ConnectStatusChange == TRUE)


Why do you check whether the bit is set here, but not in UhciRHClearFeaturePortEnableChange?

Why does it need to be checked in UhciRHClearFeaturePortEnableChange?

Why does it need to be checked here? My point is that these two functions basically do the same thing, just with different bits. So it seems odd that one has the check and one does not.

It makes no sense to do a check in UhciRHClearFeaturePortEnableChange(). For UhciRHClearFeaturePortEnableChange() such a case is possible when we call USBH_SyncClearPortStatus() in USBH_StartHubFdoDevice().

ThFabba · 2018-01-12T20:59:06Z

drivers/usb/usbuhci_new/roothub.c

+NTAPI
+UhciRHDisableIrq(IN PVOID uhciExtension)
+{
+    //DPRINT("UhciRHDisableIrq: \n");


I thought these were pretty important. Is UHCI special in that it doesn't need them?

ThFabba · 2018-01-12T20:59:41Z

drivers/usb/usbuhci_new/usbuhci.c

+    while (TD)
+    {
+        TD->HwTD.Token.DataToggle = (TD->HwTD.Token.DataToggle == FALSE);
+        DataToggle = (DataToggle == FALSE);


DataToggle = !DataToggle; seems more intuitive to me but YMMV

I agree, you variant looks easier.

ThFabba · 2018-01-12T21:00:52Z

drivers/usb/usbuhci_new/usbuhci.h

+  UHCI_TD HwTD;
+  /* Software */
+  USB_DEFAULT_PIPE_SETUP_PACKET SetupPacket;
+  ULONG_PTR PhysicalAddress;


I'm guessing this structure doesn't change its size depending on whether you run a 32 or 64 bit OS. If it's always 32 bits, ULONG is the right choice.

Earlier I already got rid of ULONG_PTR, but sent a commit to the "usb_wip" branch. By the way, in usbport there are also several changes. I will work on this after the "Usbport transaction translator" so as not to get confused.

ThFabba · 2018-01-12T21:06:10Z

drivers/usb/usbuhci_new/usbuhci.c

+
+    if (InterlockedIncrement(&UhciExtension->LockFrameList) != 1)
+    {
+        InterlockedDecrement(&UhciExtension->LockFrameList);


This lock seems odd. It protects code rather than data, which is a red flag.
If a second thread can get here and bail out while the first thread is already done walking the list but hasn't released the lock yet, the second cleanup would do nothing, even though there may be work to be done. Is this acceptable for this function (i.e. it just needs a "best effort" approach)? That might deserve a comment if so.

This is clearing the list for isochronous transfers. This is also not implemented yet, so it may look strange. I can make them as a comment, so as not to forget about them. Or?

A comment sounds fine to me.

ThFabba · 2018-01-12T21:11:06Z

drivers/usb/usbuhci_new/usbuhci.c

+    NewFrameNumber = UhciGet32BitFrameNumber(UhciExtension);
+    OldFrameNumber = UhciExtension->FrameNumber;
+
+    if ((NewFrameNumber - OldFrameNumber) < UHCI_FRAME_LIST_MAX_ENTRIES &&


Do frame numbers roll over or are they monotonously increasing? If the former, you may get an integer overflow here; if the latter, ASSERT(NewFrameNumber >= OldFrameNumber) above

Yes, in theory there may be an overflow here. Since MAX_ULONG == 0xFFFFFFFF == 4,294,967,295, and in NewFrameNumber are stored milliseconds (1 USB frame == 1 millisecond), then dividing by 1000 we get ~ 4,294,967 seconds. Dividing by (60 * 60 * 24), we get ~ 50 days. Thus, approximately 50 days after switching on the computer, we can lose isochronous transmissions (usually an audio signal).

vgalnt · 2018-01-12T22:11:40Z

drivers/usb/usbuhci_new/usbuhci.c

@@ -772,6 +773,8 @@ UhciStopController(IN PVOID uhciExtension,
            DbgBreakPoint();
            break;
        }
+
+        Command.AsUSHORT = READ_PORT_USHORT(CommandReg);
    }


I had a slightly different variant:
-- while (((UHCI_USB_COMMAND)READ_PORT_USHORT(CommandReg)).HcReset == 1)
++ while (Command.AsUSHORT = READ_PORT_USHORT(CommandReg),
++ Command.HcReset == 1)

Feel free to overwrite mine, I just wanted to have the build green.

ThFabba · 2018-01-12T22:22:13Z

drivers/usb/usbuhci_new/usbuhci.h

+
+#define UHCI_ENDPOINT_FLAG_HALTED           1
+#define UHCI_ENDPOINT_FLAG_RESERVED         2
+#define UHCI_ENDPOINT_FLAG_CONTROLL_OR_ISO  4


ThFabba · 2018-01-12T22:23:50Z

drivers/usb/usbuhci_new/usbuhci.c

+                              IN PUSBPORT_ENDPOINT_REQUIREMENTS EndpointRequirements)
+{
+    ULONG TransferType;
+    ULONG TdCont;


ThFabba · 2018-01-12T22:25:50Z

drivers/usb/usbuhci_new/usbuhci.c

+    {
+        DPRINT1("UhciTakeControlHC: MP_STATUS_ERROR\n");
+        MpStatus = MP_STATUS_ERROR;
+    }


Should the function return here? Is there other error handling missing?

No. Here it should be. It ok.

ThFabba · 2018-01-13T15:50:53Z

drivers/usb/usbuhci_new/usbuhci.c

+    HcStatus.AsUSHORT = READ_PORT_USHORT(StatusRegister);
+    DPRINT("UhciTakeControlHC: HcStatus.AsUSHORT - %04X\n", HcStatus.AsUSHORT);
+
+    if (HcStatus.HcHalted == 0)


while (HcStatus.HcHalted == 0) ... if (CurrentTime ...) break; seems more intuitive than if-do-while.

HBelusca · 2018-01-15T14:14:30Z

drivers/usb/usbuhci_new/usbuhci.c

@@ -81,7 +81,7 @@ UhciCleanupFrameListEntry(IN PUHCI_EXTENSION UhciExtension,
                          IN ULONG Index)
 {
    PUHCI_HC_RESOURCES UhciResources;
-    ULONG_PTR PhysicalAddress;
+    ULONG PhysicalAddress;


Ah? so the physical address can never be greater than 0xffffffff ? Or should one use ULONGLONG instead?

Yes, all these controller types only support 32 bit DMA.

See comment in HalAllocateCommonBuffer() - is called from USBPORT_AllocateCommonBuffer().

HBelusca · 2018-01-15T14:15:59Z

drivers/usb/usbuhci_new/usbuhci.h

@@ -34,6 +34,7 @@ extern USBPORT_REGISTRATION_PACKET RegPacket;
 typedef struct _UHCI_ENDPOINT *PUHCI_ENDPOINT;
 typedef struct _UHCI_TRANSFER *PUHCI_TRANSFER;

+#if !defined(_M_X64)


Isn't it possible to have the #if !defined (_M_X64) for the new/modified fields inside the structure itself?

_M_X64 is not a thing by the way. There's _M_IX86 and _M_AMD64

I took _M_X64 because it is already used in usbport.

Oh yeah, _M_X64 is actually a synonym, I didn't know that. This is okay then.

HBelusca · 2018-01-15T14:16:37Z

drivers/usb/usbuhci_new/usbuhci.c

@@ -291,30 +291,31 @@ UhciQueryEndpointRequirements(IN PVOID uhciExtension,
            TdCount = 2 * UHCI_MAX_ISO_TD_COUNT;

            EndpointRequirements->HeaderBufferSize = 0 + // Iso queue is have not Queue Heads


"is have not..." --> "does not have ..."

HBelusca · 2018-01-15T14:21:24Z

drivers/usb/usbuhci_new/usbuhci.h

+  ULONG PhysicalAddress;
+  ULONG Flags;
+  struct _UHCI_HCD_TD * NextHcdTD;
+  _ANONYMOUS_UNION union {


You can move the #if !defined (M_X64) inside just for this union, and the ULONG Padded[x]; that is after.

Ok, I chahged. It turned out very human-readable (how do you like) :).
Btw, I fixed ULONG Padded[15] instead ULONG Padded[9], thank you.

HBelusca · 2018-01-15T14:22:23Z

drivers/usb/usbuhci_new/usbuhci.h

+
+#define UHCI_FRAME_LIST_POINTER_VALID      (0 << 0) 
+#define UHCI_FRAME_LIST_POINTER_TERMINATE  (1 << 0) 
+#define UHCI_FRAME_LIST_POINTER_TD         (0 << 1) 


I'm not sure I understand what "0 << 1" should do: isn't it equal to 0 ?

It means "bit 1 has the value 0", to contrast with (1 << 1). So yes it's 0, but it's an annotated 0 :p

HBelusca · 2018-01-15T14:27:09Z

drivers/usb/usbuhci_new/roothub.c

+
+    for (ix = 0; ix < 10; ++ix)
+    {
+        KeStallExecutionProcessor(50);


Just a general question for me to understand something: calling KeStallExecutionProcessor (that really stalls the CPU) instead of just calling e.g. KeDelayExecutionThread, is to be very sure that nobody else could mess with values in IO ports?

It's generally used for short waits, either because you're at a high IRQL and can't invoke the scheduler, or to avoid the scheduling overhead (your next timeslice may come in several ms, i.e. much much later than 50µs)

Callers of KeDelayExecutionThread must be running at IRQL <= APC_LEVEL.
Callers of KeStallExecutionProcessor can be running at any IRQL.

Thanks for the info!

HBelusca · 2018-01-15T14:30:04Z

drivers/usb/usbuhci_new/roothub.c

+    BaseRegister = UhciExtension->BaseRegister;
+
+    Port = *(PUSHORT)pPort;
+    ASSERT(Port);


It looks like port numbers are 1-based. Is it so because of the specifications? Or is it because in your code port number "0" has a special meaning?

We using bitmask (UhciExtension->ResetPortMask,...)
And in specifications use port - requests with "The port number must be a valid port number for that hub, greater than zero."

HBelusca · 2018-01-15T14:32:03Z

drivers/usb/usbuhci_new/roothub.c

+NTAPI
+UhciRHDisableIrq(IN PVOID uhciExtension)
+{
+    return;


Not implemented? Or should this function do nothing?

This 2 functions for UHCI not used (do nothing).

Ok. Maybe you can add a comment in the code to say it's on purpose?

…ListEntry() with correct.

…Endpoint.

ThFabba self-requested a review December 30, 2017 20:24

ThFabba self-assigned this Dec 30, 2017

AmineKhaldi force-pushed the usbuhci_new branch from 06ff876 to af43913 Compare December 30, 2017 20:41

ThFabba reviewed Jan 12, 2018

View reviewed changes

vgalnt reviewed Jan 12, 2018

View reviewed changes

ThFabba reviewed Jan 12, 2018

View reviewed changes

ThFabba reviewed Jan 13, 2018

View reviewed changes

HBelusca reviewed Jan 15, 2018

View reviewed changes

vgalnt force-pushed the usbuhci_new branch from 131a06e to 9049054 Compare January 16, 2018 02:50

vgalnt added 17 commits August 20, 2018 17:50

[USBUHCI] Add check "Port != 0".

6a2ef17

[USBUHCI] Use 0|1 instead FALSE|TRUE for bitfields.

1366d55

[USBUHCI] Renaming "port" => "PortBit".

0d4492a

[USBUHCI] Change ASSERTs.

96a2694

[USBUHCI] Delete DPRINTs.

f829761

[USBUHCI] Simplify the inversion of the DataToggle.

8ed86e2

[USBUHCI] Correction of spelling errors.

dd99a44

[USBUHCI] Change comments.

5f5f295

[USBUHCI] Correcting UhciCleanupFrameListEntry() (add comment).

04ceea2

[USBUHCI] Simplify cycle in UhciTakeControlHC().

f7c48eb

[USBUHCI] Simplify cycle in UhciRHPortResetComplete().

8f62a1e

[USBUHCI] Simplify code ("!= 0").

756f92b

[USBUHCI] Fixing: replace UhciCleanupFrameList() and UhciCleanupFrame…

77fec73

…ListEntry() with correct.

[USBUHCI] Small cosmetic changes.

dedca24

[USBUHCI] Corrections for x64.

24d74e5

[USBUHCI] Using #if !defined(_M_X64) inside struct.

0608094

[USBUHCI] Add comments.

c3f4e5a

ThFabba force-pushed the usbuhci_new branch from 21d59d2 to c3f4e5a Compare August 20, 2018 15:51

ThFabba added 10 commits August 20, 2018 17:51

[USBUHCI_NEW] Fix some copypasta'd DPRINTs

f47d153

[USBUHCI_NEW] Move special-case code into its check in UhciPollNonIso…

e590d12

…Endpoint.

[USBUHCI_NEW] Move code out of loop in UhciPollNonIsoEndpoint.

95fbf29

[USBUHCI_NEW] Reduce code duplication in UhciPollNonIsoEndpoint.

4fd10fd

[USBUHCI_NEW] Use a loop condition in UhciPollNonIsoEndpoint.

4808399

[USBUHCI_NEW] Fix timeout handling in UhciStopController.

ba89acd

[USBUHCI_NEW] Avoid a goto in UhciProcessDoneNonIsoTD.

b8a9501

[USBUHCI_NEW] Simplify UhciAllocateTD.

bd3b403

[USBUHCI_NEW] Avoid using pointers for physical addresses

626acb5

[USB] Keep usbuhci_new disabled for now.

a6112ec

ThFabba merged commit a1a65e9 into reactos:master Sep 1, 2018

HBelusca removed the review pending For PRs undergoing review. label Jan 11, 2022

		@@ -291,30 +291,31 @@ UhciQueryEndpointRequirements(IN PVOID uhciExtension,
		TdCount = 2 * UHCI_MAX_ISO_TD_COUNT;

		EndpointRequirements->HeaderBufferSize = 0 + // Iso queue is have not Queue Heads

[USBUHCI_NEW] Bring-in the USB UHCI miniport driver created by Vadim Galyant. #245

[USBUHCI_NEW] Bring-in the USB UHCI miniport driver created by Vadim Galyant. #245

Conversation

AmineKhaldi commented Dec 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HBelusca commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThFabba Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThFabba Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThFabba commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThFabba Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgalnt Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThFabba Jan 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThFabba Jan 12, 2018 •

edited

ThFabba Jan 12, 2018 •

edited

ThFabba Jan 12, 2018 •

edited

vgalnt Jan 12, 2018 •

edited

ThFabba Jan 15, 2018 •

edited