The UHCI implementation is possibly flawed

[toluschr]

Hi,

I had a look at your UHCI/USB stack as a reference for my own kernel. While it is overall well written, I think I found some flaws with the implementation.

For reference, I'm used the following command:

qemu-system-i386 -serial mon:stdio -kernel kernel/kernel -drive file=panicos.img,format=raw,if=virtio \
	-smp 2 -m 128 -net none -rtc base=localtime \
	-usb -device usb-host,vendorid=0x0403,productid=0x6001

1. Off by one error in usb/transfer.c:50

After appending the out packet (USB_PACKET_OUT) to the packets buffer, pid will not get incremented. The UHCI Controller will therefore send only pid - 1 packets:

	int toggle = 1;
	int pid = 1;
	for (int i = 0; i < size; i += 8) {
		packets[pid].type = USB_PACKET_IN;
		packets[pid].maxlen = 8;
		packets[pid].buffer = payload + i;
		packets[pid].toggle = toggle;
		toggle = toggle ? 0 : 1;
		pid++;
	}

	packets[pid].type = USB_PACKET_OUT;
	packets[pid].maxlen = 0x800;
	packets[pid].buffer = (void*)0x80000000;
	packets[pid].toggle = 1;
        // pid++ missing here

2. Missing error checks in uhci.c:108

After sending a transfer, USBSTS may hold the value 0x3 per specification:

Bit	Description
1	USB Error Interrupt. The Host Controller sets this bit to 1 when completion of a USB transaction results in an error condition (e.g., error counter underflow). If the TD on which the error interrupt occurred also had its IOC bit set, both this bit and Bit 0 are set.

This signals both an interrupt and an error interrupt. However, the current UHCI implementation only checks the interrupt bit and ignores the error interrupt bit. This means it will report success, even if an error occurred:

	while (!(inw(dev->iobase + UHCI_USBSTS) & 1)) {
	}

	uhci_stop(dev);
	return USB_STATUS_OK; // No error check

3. UHCI does not properly wait for a reply in usb.c:58

After adding pid++ (from 1.), the UHCI controller will, for whatever reason, still report an erroneous execution. I believe this has something to do with the queue layout, but I'm not quite sure. If I change it, the UHCI controller signals no interrupt at all. Anyways, the UHCI implementation does not properly wait for a reply.

As you can see from the screenshot below, the usb_set_address function gets executed before the reply in usb_get_device_descriptor was received:

You can also see that even though the error bit was set, the device sent a proper reply anyways.

4. USB gets stuck in kernel/driver/usb/usb.c:74

After setting the device address, usb_get_configuration_descriptor hangs.

	}

	uint8_t configuration_value;
	if (usb_get_configuration_descriptor(usbdev, 0, &configuration_value) != 0) { // hangs here
		cprintf("[usb] GetConfigurationDescriptor on port %d failed\n", port);
	}
	for (unsigned int i = 0; i < usbdev->num_interface; i++) {

Needless to say, I'm yet to find the bug in my own implementation and am starting to get frustrated with myself.