USB Support

Hi,

As many of you know, I am working on USB support. I hope some of you can have a look at this patch. It is not finished yet, but I hope to get some comments anyways so it can be committed sooner. Please test it, comment on the interfaces and generic structure of the code. But if you stop bugs or weird code, please tell me!

Features:

- UHCI support, tested on an Intel chipset and on QEmu

- Generic USB interfaces

- USB test command ("usb") that can be used to show device information, will be renamed and cleaned up.

- USB Mass Storage Support

- Broken OHCI support, it works in QEMu but I haven't tested on real hardware. Please do not review ohci.c, it won't be committed but I included it now in case someone wants to test it anyways :-)

Some bugs that are still present:

- --enable-grub-emu-usb does not work properly. grub_emu_SOURCES += ... doesn't seem to work properly. I have no idea how to fix this, I do not want to learn ruby just to make such simple change. Help is appreciated.

- Endianess fixes. Unfortunately I didn't fix endianess everywhere and need to check this carefully... :-) This will be done.

- I marked some weird or ugly things with XXX in my code, no need to check this. Some might be even committed.

- There are some things I will have to check again. Perhaps I should even review some parts of the standards to see if I indeed implemented this as described. Initially I might have misinterpreted something which I do understand now.

- This code wants to be tested ;-)

--
Marco

2008-08-27 Marco Gerards <[email protected]>

* Makefile.in (enable_grub_emu_usb): New variable.
* conf/i386-pc.rmk (grub_emu_SOURCES): Add disk/scsi.c'. (grub_emu_SOURCES) [grub_emu_SOURCES]: Add disk/usbms.c', util/usb.c', bus/usb/usb.c' and commands/usbtest.c'. (grub_emu_LDFLAGS): Add $(LIBUSB)'. (pkglib_MODULES): Add usb.mod', uhci.mod', ohci.mod', usbtest.mod' and `usbms.mod'.
(usb_mod_SOURCES, usb_mod_CFLAGS, usb_mod_LDFLAGS) (usbtest_mod_SOURCES, usbtest_mod_CFLAGS, usbtest_mod_LDFLAGS) (uhci_mod_SOURCES, uhci_mod_CFLAGS, uhci_mod_LDFLAGS, (ohci_mod_SOURCES, ohci_mod_CFLAGS, ohci_mod_LDFLAGS) (usbms_mod_SOURCES, usbms_mod_CFLAGS, usbms_mod_LDFLAGS): New variables.

* disk/usbms.c: New file.

* include/grub/usb.h: Likewise.

* include/grub/usbtrans.h: Likewise.

* include/grub/usbdesc.h: Likewise.

* bus/usb/usbtrans.c: Likewise.

* bus/usb/ohci.c: Likewise.

* bus/usb/uhci.c: Likewise.

* bus/usb/usbhub.c: Likewise.

* bus/usb/usb.c: Likewise.

* commands/usbtest.c: Likewise.

* util/usb.c: Likewise.

* include/grub/err.h (grub_err_t): Add `GRUB_ERR_IO'.

* configure.ac: Test for libusb presence.

* util/grub-emu.c (main) [HAVE_LIBUSB_H]: Call `grub_libusb_init'.

Index: Makefile.in
===================================================================
@@ -92,6 +92,7 @@ UNIFONT_HEX = @UNIFONT_HEX@
 
 # Options.
 enable_grub_emu = @enable_grub_emu@
+enable_grub_emu_usb = @enable_grub_emu_usb@
 enable_grub_fstest = @enable_grub_fstest@
 enable_grub_pe2elf = @enable_grub_pe2elf@
 enable_lzo = @enable_lzo@

Index: configure.ac
===================================================================
@@ -367,6 +367,9 @@ AC_ARG_ENABLE([mm-debug], 
 AC_ARG_ENABLE([grub-emu],
 	      [AS_HELP_STRING([--enable-grub-emu],
                              [build and install the `grub-emu' debugging utility])])
+AC_ARG_ENABLE([grub-emu-usb],
+	      [AS_HELP_STRING([--enable-grub-emu-usb],
+                             [build and install the `grub-emu' debugging utility with USB support])])
 [if [ x"$enable_grub_emu" = xyes ]; then
   # Check for curses libraries.]
   AC_CHECK_LIB([ncurses], [wgetch], [LIBCURSES="-lncurses"],

@@ -379,8 +382,20 @@ AC_ARG_ENABLE([grub-emu],
     [AC_CHECK_HEADERS([ncurses.h], [],
       [AC_CHECK_HEADERS([curses.h], [],
 	[AC_MSG_ERROR([(n)curses header files are required to build `grub-emu'])])])])
+
+  [if [ x"$enable_grub_emu_usb" = xyes ]; then
+    # Check for libusb libraries.]
+    AC_CHECK_LIB([usb], [usb_claim_interface], [LIBUSB="-lusb"],
+      [AC_MSG_ERROR([libusb libraries are required to build `grub-emu' with USB support])])
+    AC_SUBST([LIBUSB])
+
+    [# Check for headers.]
+    AC_CHECK_HEADERS([usb.h], [],
+      [AC_MSG_ERROR([libusb header file is required to build `grub-emu' with USB support])])
+  [fi]
 [fi]
 AC_SUBST([enable_grub_emu])
+AC_SUBST([enable_grub_emu_usb])
 
 AC_ARG_ENABLE([grub-fstest],
 	      [AS_HELP_STRING([--enable-grub-fstest],

Index: include/grub/err.h
===================================================================
@@ -52,7 +52,8 @@ typedef enum
     GRUB_ERR_SYMLINK_LOOP,
     GRUB_ERR_BAD_GZIP_DATA,
     GRUB_ERR_MENU,
-    GRUB_ERR_TIMEOUT
+    GRUB_ERR_TIMEOUT,
+    GRUB_ERR_IO
   }
 grub_err_t;
 

Index: config.h.in
===================================================================
@@ -73,6 +73,9 @@
 /* Define to 1 if you have the <unistd.h> header file. */
 #undef HAVE_UNISTD_H
 
+/* Define to 1 if you have the <usb.h> header file. */
+#undef HAVE_USB_H
+
 /* Define to 1 if you enable memory manager debugging. */
 #undef MM_DEBUG
 

Index: util/grub-emu.c
===================================================================

Part of the diff is not shown here. View the raw message.

Am Mittwoch, den 27.08.2008, 21:56 +0200 schrieb Marco Gerards:

Hello Marco,

not that big as you might have expected, but maybe not that bad start. I try to get more used to this :)

;)

Index: conf/i386-pc.rmk
===================================================================

@@ -140,14 +140,21 @@ grub_emu_SOURCES = commands/boot.c comma
fs/ufs.c fs/xfs.c fs/afs.c \
\
util/console.c util/hostfs.c util/grub-emu.c util/misc.c \
- util/biosdisk.c util/getroot.c \
+ util/biosdisk.c util/getroot.c \

Janitor work? :)

@@ -165,7 +172,8 @@ pkglib_MODULES = biosdisk.mod _chain.mod
vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \
videotest.mod play.mod bitmap.mod tga.mod cpuid.mod serial.mod \
ata.mod vga.mod memdisk.mod jpeg.mod png.mod pci.mod lspci.mod \
- aout.mod _bsd.mod bsd.mod pxe.mod pxecmd.mod datetime.mod date.mod \
+ aout.mod _bsd.mod bsd.mod usb.mod uhci.mod ohci.mod usbtest.mod usbms.mod \
+ pxe.mod pxecmd.mod datetime.mod date.mod \
datehook.mod

I think datehook.mod feels more comfortable if it would be along with datetime.mod and date.mod

Index: disk/usbms.c
===================================================================

+ int usb_iterate (grub_usb_device_t usbdev)
+ {

+      grub_usb_err_t err;
+      struct grub_usb_desc_device *descdev = &usbdev->descdev;
+      int i;

+
+ if (descdev->class != 0 || descdev->subclass || descdev->protocol != 0)
+ return 0;

Please be consistent, != 0 isn't needed anyway.

+ grub_usb_iterate (usb_iterate);
+}
+
+

Weird `' character which seems not to make sense.

+
+static int
+grub_usbms_iterate (int (*hook) (const char *name, int luns))

+ /* XXX: Magic and check this code. */
+ if (status.status == 2)

A structure named status with a field status looks a bit funny, but I don't have a suggestion for you :(

+static struct grub_scsi_dev grub_usbms_dev =
+ {
+ .name = "usb",

Shouldn't this be "usbms" ?

+    .iterate = grub_usbms_iterate,
+    .open = grub_usbms_open,
+    .close = grub_usbms_close,
+    .read = grub_usbms_read,
+    .write = grub_usbms_write

+ };

+grub_usb_err_t
+grub_usb_root_hub (grub_usb_controller_t controller); +
+
Again weird `'
+/* XXX: All handled by libusb for now. */ +struct grub_usb_controller_dev

+struct grub_usb_controller
+{
+ /* The underlying USB Host Controller device. */
+ grub_usb_controller_dev_t dev;
+
+ /* Data used by the USB Host Controller Driver. */
+ void *data;
+};
+

Again.

+
+struct grub_usb_interface
+{
+ struct grub_usb_desc_if *descif;
+
+ struct grub_usb_desc_endp *descendp;
+};

+};
+
+

Again.

+
+typedef enum
+ {
+typedef struct grub_usb_transfer *grub_usb_transfer_t; +
+

Again.

+#define GRUB_USB_REQTYPE_IN (1 << 7)
+#define GRUB_USB_REQTYPE_OUT (0 << 7)

Index: bus/usb/uhci.c
===================================================================

+ fail:
+ if (u)
+ {
+ grub_free ((void *) u->qh);
+ grub_free (u->framelist);
+ }
+ grub_free (u);

util/misc.c: grub_free () doestn't check *ptr before calling free () But better util/misc.c get's changed then your code and bus/usb/uhci isn't compiled for grub-emu anyway :)

+static int
+grub_uhci_hubports (grub_usb_controller_t dev __attribute__((unused))) +{
+ /* The root hub has exactly two ports. */
+ return 2;
+}
+
+

Again weird `' character.

+static struct grub_usb_controller_dev usb_controller = +{

+  .name = "uhci",
+  .iterate = grub_uhci_iterate,
+  .transfer = grub_uhci_transfer,
+  .hubports = grub_uhci_hubports,
+  .portstatus = grub_uhci_portstatus,
+  .detect_dev = grub_uhci_detect_dev

+};

Index: bus/usb/usbhub.c
===================================================================

+ return dev;
+}
+
+

Again.

+static grub_err_t
+grub_usb_add_hub (grub_usb_device_t dev)
+{

Index: bus/usb/usb.c
===================================================================

+ return 0;
+}
+#endif
+
+

And again weird `'

+grub_usb_err_t
+grub_usb_clear_halt (grub_usb_device_t dev, int endpoint) +{

Index: commands/usbtest.c
===================================================================

+static const char *usb_classes[] =
+ {

+    "",
+    "Audio",
+    "Communication Interface",
+    "HID",
+    "",

Does this empty string makes sense?

+    "Physical",
+    "Image",
+    "Printer",
+    "Mass Storage",
+    "Hub",
+    "Data Interface",
+    "Smart Card",
+    "Content Security",
+    "Video"

+ };

Index: util/usb.c
===================================================================

+#include <config.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <usb.h>
+#include <grub/usb.h>
+#include <grub/dl.h>
+
+

Again weird `'

+grub_err_t
+grub_libusb_fini (void)
+{
+ return 0;
+}
+
+

Again.

Felix Zielcke

Actually that is a form feed character, which the GNU coding standard recommends to be used to break up source code at logical places:

Please use formfeed characters (control-L) to divide the program into pages at logical places (but not within a function). It does not matter just how long the pages are, since they do not have to fit on a printed page. The formfeeds should appear alone on lines by themselves.
(http://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting)

But the C standard says it's OK to call free() with a NULL pointer.

Regards,
Colin

Am Sonntag, den 31.08.2008, 08:59 -0700 schrieb Colin D Bennett:

Hello Colin,

Yep Marco already told me on IRC that this was with intent :)

Thanks for the copy&paste and even the link to it.
I still find it weird and I even wasn't able yet to produce them in Emacs.
Control-L is the wrong key combination for it ;)

Felix Zielcke

Try C-q C-L.

Cheers,
Colin