Hi Konstantin, There is one compilation error and one link error on IA64. See below.
Konstantin Baydarov wrote: > On Fri, 30 Nov 2007 12:11:05 -0800 (PST) > Aaron Young <[EMAIL PROTECTED]> wrote: > >>> On Fri, 30 Nov 2007 08:43:07 -0800 (PST) >>> Aaron Young <[EMAIL PROTECTED]> wrote: >>> >>>>> >>>>> >>>>> Looking better! More comments: >>>>> >>>>> 1. I don't understand the need for the call to >>>>> kdb_uhci_keyboard_clear(). It's only called if >>>>> kdb_usb_keyboard_attach() fails and in that case we >>>>> didn't add the urb to kdb_usb_kbds[]. So, I don't >>>>> see the need to clear it out of kdb_usb_kbds[]... >>>>> (I'm probably missing something here). >>> - Now I call kdb_uhci_keyboard_clear() if usb_submit_urb() failed, >>> that is called after kdb_usb_keyboard_attach(). >> Why not just call kdb_usb_keyboard_detach()? > Got rid from kdb_uhci_keyboard_clear(). > >>>>> 2. I probably would have made kdb_uhci_submit_urb() return >>>>> the newly created kdb_urb (extra arg) on success and not >>>>> call kdb_usb_keyboard_attach() directly from within. Then you >>>>> can just make a single call to kdb_usb_keyboard_attach() for >>>>> all three cases (OHCI, EHCI and UHCI) out of hid_probe(). But, >>>>> not a *BIG* deal... >>> - We should first fill kdb_usb_kbds[] before call of >>> usb_submit_urb() because uhci_urb_enqueue() checks kdb_usb_kbds[] >>> to find out if KDB URB is enqueued. So kdb_usb_keyboard_attach() >>> should be called in the "middle" of the function >>> kdb_uhci_submit_urb(). >> OK.. >> >>>>> 3. We'll have to get updates to ia64 kdba_io.c as well, >>>>> otherwise it will result in compile errors on ia64. i.e. >>>>> kdb_usb_keyboard_attach() will have a number of args mismatch >>>>> and all the routines you added to kdba_io_32.c and kdba_io_64.c >>>>> will not be there. >>> - Can't find any ia64 file... >> linux/arch/ia64/kdb/kdba_io.c > Added arch/ia64/kdb/kdba_io.c. > >> Maybe it's part of a different KDB patch or something? Jay? >> >>>>> Does hotplug/hotunplug of the keyboards work? >>>>> Does it correctly remove the KDB URBs on hotunplug? >>> - kdb_usb_keyboard_detach() is never called on my PC. So I tested >>> only 1 case: attaching keyboard after kernel boot - works OK. >> Should get called out of hid_disconnect() when a >> keyboard is unplugged. If you are using a usb1.1 >> hub, it may not work - I've seen this problem with >> USB1.1 hub. MIght want to try plugging/unplugging the keyboard >> directly into the chassis port if so. > Fixed hotplug/hotunplug. The problem was in my patch - UHCI URB should be > unlinked from endpoin queue. > >> BTW - do you have a USB2.0 hub? The EHCI KDB code has yet to >> be tested on x86 and I'd be curious if it worked or not (hope it >> does)... >> >> -Aaron > No, I don't have USB2.0 hub. > > Aaron, here is incremental patch against my last patch > (http://oss.sgi.com/archives/kdb/2007-11/msg00039.html): > > Index: linux-2.6.24-rc2/arch/ia64/kdb/kdba_io.c > =================================================================== > --- linux-2.6.24-rc2.orig/arch/ia64/kdb/kdba_io.c > +++ linux-2.6.24-rc2/arch/ia64/kdb/kdba_io.c > @@ -65,12 +65,120 @@ static unsigned char kdb_usb_keycode[256 > 150,158,159,128,136,177,178,176,142,152,173,140 > }; > > +/* Check if URB is managed by KDB code */ > +int kdb_uhci_keyboard_urb(struct urb *urb) > +{ > + int i; > + > + for (i = 0; i < KDB_USB_NUM_KEYBOARDS; i++) { > + if (kdb_usb_kbds[i].urb && kdb_usb_kbds[i].urb == urb) > + return i; > + } > + return -1; > +} > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_urb); > + > +/* Check if UHCI QH is managed by KDB code */ > +int kdb_uhci_keyboard_check_uhci_qh(struct uhci_qh *qh) > +{ > + int i; > + > + for (i = 0; i < KDB_USB_NUM_KEYBOARDS; i++) { > + if (kdb_usb_kbds[i].urb && kdb_usb_kbds[i].qh == qh) > + return i; > + } > + return -1; > +} > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_check_uhci_qh); > + > +/* Set UHCI QH using URB pointer */ > +int kdb_uhci_keyboard_set_qh(struct urb *urb, struct uhci_qh *qh) > +{ > + int i; > + > + i = kdb_uhci_keyboard_urb(urb); > + if (i != -1) > + kdb_usb_kbds[i].qh = qh; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_set_qh); > + > +/* Get UHCI QH using URB pointer */ > +struct uhci_qh *kdb_uhci_keyboard_get_qh(struct urb *urb) > +{ > + int i; > + > + i = kdb_uhci_keyboard_urb(urb); > + if (i != -1) > + return kdb_usb_kbds[i].qh; > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_get_qh); > + > +/* Set UHCI hid_event using URB pointer */ > +int kdb_uhci_keyboard_set_hid_event(struct urb *urb, int hid_event) > +{ > + int i; > + > + i = kdb_uhci_keyboard_urb(urb); > + if (i != -1) > + kdb_usb_kbds[i].kdb_hid_event = hid_event; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_set_hid_event); > + > +/* Get UHCI hid_event using URB pointer */ > +int kdb_uhci_keyboard_get_hid_event(struct urb *urb) > +{ > + int i; > + > + i = kdb_uhci_keyboard_urb(urb); > + if (i != -1) > + return kdb_usb_kbds[i].kdb_hid_event; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_get_hid_event); > + > +/* Set UHCI hid_event using UHCI QH pointer */ > +int kdb_uhci_keyboard_set_hid_event_qh(struct uhci_qh *qh, int hid_event) > +{ > + int i; > + > + for (i = 0; i < KDB_USB_NUM_KEYBOARDS; i++) { > + if (kdb_usb_kbds[i].urb && kdb_usb_kbds[i].qh == qh){ > + kdb_usb_kbds[i].kdb_hid_event = hid_event; > + return i; > + } > + } > + return -1; > +} > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_set_hid_event_qh); > + > +/* Get UHCI hid_event using UHCI QH pointer */ > +int kdb_uhci_keyboard_get_hid_event_qh(struct uhci_qh *qh) > +{ > + int i; > + > + for (i = 0; i < KDB_USB_NUM_KEYBOARDS; i++) { > + if (kdb_usb_kbds[i].urb && kdb_usb_kbds[i].qh == qh) > + return kdb_usb_kbds[i].kdb_hid_event; > + } > + > + return -1; > +} > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_get_hid_event_qh); > + > /* > * kdb_usb_keyboard_attach() > * Attach a USB keyboard to kdb. > */ > int > -kdb_usb_keyboard_attach(struct urb *urb, unsigned char *buffer, void > *poll_func) > +kdb_usb_keyboard_attach(struct urb *urb, unsigned char *buffer, > + void *poll_func, void *compl_func, struct urb *hid_urb) > { > int i; > int rc = -1; > @@ -92,6 +200,8 @@ kdb_usb_keyboard_attach(struct urb *urb, > kdb_usb_kbds[i].urb = urb; > kdb_usb_kbds[i].buffer = buffer; > kdb_usb_kbds[i].poll_func = poll_func; > + kdb_usb_kbds[i].urb_complete = compl_func; > + kdb_usb_kbds[i].hid_urb = hid_urb; > > rc = 0; /* success */ > > @@ -111,6 +221,7 @@ kdb_usb_keyboard_detach(struct urb *urb) > { > int i; > int rc = -1; > + extern void kdb_unlink_uhci_qh(struct urb *urb, struct uhci_qh *qh); > > if (kdb_no_usb) > return 0; > @@ -122,14 +233,21 @@ kdb_usb_keyboard_detach(struct urb *urb) > */ > > for (i = 0; i < KDB_USB_NUM_KEYBOARDS; i++) { > - if (kdb_usb_kbds[i].urb != urb) > - continue; > + if (kdb_usb_kbds[i].qh) { > + /* UHCI keyboard */ > + if (kdb_usb_kbds[i].hid_urb != urb) > + continue; > + kdb_unlink_uhci_qh(kdb_usb_kbds[i].urb, > kdb_usb_kbds[i].qh); > + } else > + if (kdb_usb_kbds[i].urb != urb) > + continue; > > /* found it, clear the index */ > kdb_usb_kbds[i].urb = NULL; > kdb_usb_kbds[i].buffer = NULL; > kdb_usb_kbds[i].poll_func = NULL; > kdb_usb_kbds[i].caps_lock = 0; > + kdb_usb_kbds[i].hid_urb = NULL; > > rc = 0; /* success */ > > @@ -152,6 +270,9 @@ get_usb_char(void) > int ret; > unsigned char keycode, spec; > extern u_short plain_map[], shift_map[], ctrl_map[]; > + int ret_key = -1, i2, j, max; > + > + ret = -1; > > if (kdb_no_usb) > return -1; > @@ -179,15 +300,24 @@ get_usb_char(void) > if (ret < 0) /* error or no characters, try the next kbd */ > continue; > > + /* If 2 keys was pressed simultaneously, > + * both keycodes will be in buffer. > + * Last pressed key will be last non > + * zero byte. > + */ > + for (j=0; j<4; j++){ > + if (!kdb_usb_kbds[i].buffer[2+j]) > + break; > + } > + /* Last pressed key */ > + max = j + 1; > + > spec = kdb_usb_kbds[i].buffer[0]; > - keycode = kdb_usb_kbds[i].buffer[2]; > + keycode = kdb_usb_kbds[i].buffer[max]; > kdb_usb_kbds[i].buffer[0] = (char)0; > kdb_usb_kbds[i].buffer[2] = (char)0; > > - if(kdb_usb_kbds[i].buffer[3]) { > - kdb_usb_kbds[i].buffer[3] = (char)0; > - continue; > - } > + ret_key = -1; > > /* A normal key is pressed, decode it */ > if(keycode) > @@ -199,10 +329,12 @@ get_usb_char(void) > { > case 0x2: > case 0x20: /* Shift */ > - return shift_map[keycode]; > + ret_key = shift_map[keycode]; > + break; > case 0x1: > case 0x10: /* Ctrl */ > - return ctrl_map[keycode]; > + ret_key = ctrl_map[keycode]; > + break; > case 0x4: > case 0x40: /* Alt */ > break; > @@ -211,31 +343,47 @@ get_usb_char(void) > switch(keycode) > { > case 0x1C: /* Enter */ > - return 13; > + ret_key = 13; > + break A ';' is missing in the above line. > > case 0x3A: /* Capslock */ > kdb_usb_kbds[i].caps_lock = > !(kdb_usb_kbds[i].caps_lock); > break; > case 0x0E: /* Backspace */ > - return 8; > + ret_key = 8; > + break; > case 0x0F: /* TAB */ > - return 9; > + ret_key = 9; > + break; > case 0x77: /* Pause */ > break ; > default: > if(!kdb_usb_kbds[i].caps_lock) { > - return plain_map[keycode]; > + ret_key = plain_map[keycode]; > + break; > } > else { > - return shift_map[keycode]; > + ret_key = shift_map[keycode]; > + break; > } > } > } > + > + /* Key was pressed, return keycode */ > + break; > } > > - /* no chars were returned from any of the USB keyboards */ > + for(i2=0; i2<8; i2++){ > + if (kdb_usb_kbds[i2].buffer) > + for(j=0; j<8; j++){ > + kdb_usb_kbds[i2].buffer[j] = (char)0; > + } > + } > > - return -1; > + if (kdb_usb_kbds[i].urb_complete) > + (*kdb_usb_kbds[i].urb_complete)((struct urb > *)kdb_usb_kbds[i].urb); > + > + return ret_key; > } > #endif /* CONFIG_KDB_USB */ > > Index: linux-2.6.24-rc2/drivers/hid/usbhid/hid-core.c > =================================================================== > --- linux-2.6.24-rc2.orig/drivers/hid/usbhid/hid-core.c > +++ linux-2.6.24-rc2/drivers/hid/usbhid/hid-core.c > @@ -992,13 +992,15 @@ static inline void kdb_usb_fill_int_urb > > static int kdb_uhci_submit_urb(struct usb_interface *intf, unsigned int > usbhid_bufsize, struct urb *hid_inurb) > { > - struct urb *kdb_urb = NULL; > + struct urb *kdb_urb; > unsigned char *kdb_buffer; > dma_addr_t uhci_inbuf_dma; > int ret = -1; > extern void * usb_hcd_get_kdb_poll_func(struct usb_device *udev); > extern void * usb_hcd_get_kdb_completion_func(struct usb_device *udev); > > + kdb_urb = NULL; > + kdb_buffer = NULL; > if (!(kdb_buffer = usb_buffer_alloc(hid_inurb->dev, > usbhid_bufsize, GFP_ATOMIC, > &uhci_inbuf_dma))) > @@ -1020,6 +1022,7 @@ static int kdb_uhci_submit_urb(struct us > (kdb_urb)->transfer_dma = uhci_inbuf_dma; > (kdb_urb)->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > > + > ret = kdb_usb_keyboard_attach(kdb_urb, kdb_buffer, > usb_hcd_get_kdb_poll_func(interface_to_usbdev(intf)), > usb_hcd_get_kdb_completion_func(interface_to_usbdev(intf)), > hid_inurb); > @@ -1028,14 +1031,24 @@ static int kdb_uhci_submit_urb(struct us > goto out; > > if (usb_submit_urb(kdb_urb, GFP_ATOMIC)){ > - kdb_uhci_keyboard_clear(kdb_urb); > + /* We don't know exactly if UHCI KDB QH was allocated, > + * so if kdb_usb_keyboard_detach(hid_inurb) failed > + * kdb_usb_keyboard_detach(kdb_urb) should be called. > + */ > + kdb_usb_keyboard_detach(hid_inurb); > + kdb_usb_keyboard_detach(kdb_urb); > goto out; > } > + /* Remove KDB special URB from endpoin queue to > + * prevent hang during hid_disconnect(). > + */ > + list_del(&(kdb_urb->urb_list)); > > ret = 0; > return ret; > out: > /* Some Error Cleanup */ > + ret = -1; > printk("KDB: Error, UHCI Keyboard HID won't work!\n"); > > if (kdb_buffer) > Index: linux-2.6.24-rc2/arch/x86/kdb/kdba_io_32.c > =================================================================== > --- linux-2.6.24-rc2.orig/arch/x86/kdb/kdba_io_32.c > +++ linux-2.6.24-rc2/arch/x86/kdb/kdba_io_32.c > @@ -79,24 +79,7 @@ int kdb_uhci_keyboard_check_uhci_qh(stru > } > return -1; > } > -EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_chech_uhci_qh); > - > -/* Clear keyboar entry */ > -int kdb_uhci_keyboard_clear(struct urb *urb) > -{ > - int i; > - > - i = kdb_uhci_keyboard_urb(urb); > - if (i != -1){ > - kdb_usb_kbds[i].urb = NULL; > - kdb_usb_kbds[i].buffer = NULL; > - kdb_usb_kbds[i].poll_func = NULL; > - kdb_usb_kbds[i].urb_complete = NULL; > - } > - > - return 0; > -} > -EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_clear); > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_check_uhci_qh); > > /* Set UHCI QH using URB pointer */ > int kdb_uhci_keyboard_set_qh(struct urb *urb, struct uhci_qh *qh) > @@ -228,6 +211,7 @@ kdb_usb_keyboard_detach(struct urb *urb) > { > int i; > int rc = -1; > + extern void kdb_unlink_uhci_qh(struct urb *urb, struct uhci_qh *qh); > > if (kdb_no_usb) > return 0; > @@ -243,6 +227,7 @@ kdb_usb_keyboard_detach(struct urb *urb) > /* UHCI keyboard */ > if (kdb_usb_kbds[i].hid_urb != urb) > continue; > + kdb_unlink_uhci_qh(kdb_usb_kbds[i].urb, > kdb_usb_kbds[i].qh); > } else > if (kdb_usb_kbds[i].urb != urb) > continue; > Index: linux-2.6.24-rc2/arch/x86/kdb/kdba_io_64.c > =================================================================== > --- linux-2.6.24-rc2.orig/arch/x86/kdb/kdba_io_64.c > +++ linux-2.6.24-rc2/arch/x86/kdb/kdba_io_64.c > @@ -79,24 +79,7 @@ int kdb_uhci_keyboard_check_uhci_qh(stru > } > return -1; > } > -EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_chech_uhci_qh); > - > -/* Clear keyboar entry */ > -int kdb_uhci_keyboard_clear(struct urb *urb) > -{ > - int i; > - > - i = kdb_uhci_keyboard_urb(urb); > - if (i != -1){ > - kdb_usb_kbds[i].urb = NULL; > - kdb_usb_kbds[i].buffer = NULL; > - kdb_usb_kbds[i].poll_func = NULL; > - kdb_usb_kbds[i].urb_complete = NULL; > - } > - > - return 0; > -} > -EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_clear); > +EXPORT_SYMBOL_GPL (kdb_uhci_keyboard_check_uhci_qh); > > /* Set UHCI QH using URB pointer */ > int kdb_uhci_keyboard_set_qh(struct urb *urb, struct uhci_qh *qh) > @@ -228,6 +211,7 @@ kdb_usb_keyboard_detach(struct urb *urb) > { > int i; > int rc = -1; > + extern void kdb_unlink_uhci_qh(struct urb *urb, struct uhci_qh *qh); > > if (kdb_no_usb) > return 0; > @@ -243,6 +227,7 @@ kdb_usb_keyboard_detach(struct urb *urb) > /* UHCI keyboard */ > if (kdb_usb_kbds[i].hid_urb != urb) > continue; > + kdb_unlink_uhci_qh(kdb_usb_kbds[i].urb, > kdb_usb_kbds[i].qh); > } else > if (kdb_usb_kbds[i].urb != urb) > continue; > Index: linux-2.6.24-rc2/drivers/usb/host/uhci-q.c > =================================================================== > --- linux-2.6.24-rc2.orig/drivers/usb/host/uhci-q.c > +++ linux-2.6.24-rc2/drivers/usb/host/uhci-q.c > @@ -27,7 +27,7 @@ > */ > #ifdef CONFIG_KDB_USB > /* KDB HID QH, managed by KDB code */ > -extern int kdb_uhci_keyboard_chech_uhci_qh(struct uhci_qh *qh); > +extern int kdb_uhci_keyboard_check_uhci_qh(struct uhci_qh *qh); > extern int kdb_uhci_keyboard_set_qh(struct urb *urb, struct uhci_qh *qh); > extern struct uhci_qh *kdb_uhci_keyboard_get_qh(struct urb *urb); > extern int kdb_uhci_keyboard_set_hid_event(struct urb *urb, int hid_event); > @@ -1728,7 +1728,7 @@ static int uhci_advance_check(struct uhc > > #ifdef CONFIG_KDB_USB > /* Don't manage KDB QH */ > - if(kdb_uhci_keyboard_chech_uhci_qh(qh) != -1){ > + if(kdb_uhci_keyboard_check_uhci_qh(qh) != -1){ > ret = 0; > goto done; > } > @@ -1827,7 +1827,7 @@ rescan: > > #ifdef CONFIG_KDB_USB > /* Don't manage KDB QH */ > - if(kdb_uhci_keyboard_chech_uhci_qh(qh) != -1) > + if(kdb_uhci_keyboard_check_uhci_qh(qh) != -1) > continue; > #endif > if (uhci_advance_check(uhci, qh)) { > Index: linux-2.6.24-rc2/drivers/usb/host/uhci-hcd.c > =================================================================== > --- linux-2.6.24-rc2.orig/drivers/usb/host/uhci-hcd.c > +++ linux-2.6.24-rc2/drivers/usb/host/uhci-hcd.c > @@ -434,6 +434,21 @@ static irqreturn_t uhci_irq(struct usb_h > return IRQ_HANDLED; > } > > +/* Unlink KDB QH from hardware and software scheduler */ Since CONFIG_USB_UHCI_HCD can be a module, the following routine needs to be exported. Also, the new routine should be enclosed inside the CONFIG_KDB_USB. Thanks, - jay > +void kdb_unlink_uhci_qh(struct urb *urb, struct uhci_qh *qh) > +{ > + unsigned long flags; > + struct uhci_hcd *uhci; > + > + uhci = (struct uhci_hcd *) hcd_to_uhci(bus_to_hcd(urb->dev->bus)); > + > + spin_lock_irqsave(&uhci->lock, flags); > + unlink_interrupt(NULL, qh); > + list_del(&(qh->node)); > + spin_unlock_irqrestore(&uhci->lock, flags); > + > +} > + > #ifdef CONFIG_KDB_USB > static int > uhci_kdb_poll_char(struct urb *urb) > Index: linux-2.6.24-rc2/include/linux/kdb.h > =================================================================== > --- linux-2.6.24-rc2.orig/include/linux/kdb.h > +++ linux-2.6.24-rc2/include/linux/kdb.h > @@ -144,7 +144,6 @@ extern void smp_kdb_stop(void); > #include <linux/usb.h> > > extern int kdb_uhci_keyboard_urb(struct urb *urb); > -extern int kdb_uhci_keyboard_clear(struct urb *urb); > extern int kdb_usb_keyboard_alloc(void); > > extern int kdb_usb_keyboard_attach(struct urb *urb, unsigned char *buffer, > void *poll_func, void *compl_func, struct urb *hid_urb); > --------------------------- > Use http://oss.sgi.com/ecartis to modify your settings or to unsubscribe. --------------------------- Use http://oss.sgi.com/ecartis to modify your settings or to unsubscribe.
