On Feb 13, 2015, at 7:29 AM, David Higgs <hig...@gmail.com> wrote:
> On Friday, February 13, 2015, Martin Pieuchot <mpieuc...@nolizard.org> wrote:
> On 13/02/15(Fri) 00:28, David Higgs wrote:
> > I guess nobody else has tried calling uhidev_get_report_async() yet. :)
> >
> > First I was getting a NULL pointer deref in the uhidev async callback.
> > Then I realized that due to USBD_NO_COPY, xfer->buffer was always
> > NULL. Next, I tried to use the DMA buffer, but I ended up in DDB in a
> > very cryptic way. I believe this is because the DMA buffer isn't
> > available when the callback is invoked.
> >
> > For the async callback to get a valid dmabuf, it needs to be invoked
> > prior to usb_freemem() in usbd_transfer_complete(). The xfer->status
> > determination would need to move up too. I'd do this myself but I
> > don't understand the logic and ordering of pipe->repeat stuff, and am
> > concerned about unintentionally breaking other devices.
> >
> > This is partially my fault, because I "tested" the original diff that
> > added the USBD_NO_COPY semantics to verify that it didn't break my
> > synchronous code paths, but hadn't yet written anything for upd(4) to
> > check the async ones.
>
> Does the diff below help?
>
> Partially but not enough. I had already figured out that I needed that to
> solve the NULL pointer dereference. See my 2nd paragraph above.
Below is the diff I’m currently running with, seeing the same problems. This
may actually be indicative of some error in my big upd(4) update - which I
didn’t include - but I’m not sure how to find out for sure because I’m not sure
how to interpret the DDB trace (hand-copied, may have errors). There were two
async requests pending when I got kicked into the debugger. Any help or
suggestions would be very welcome at this point.
Thanks.
--david
uvm_fault(0xd565f480, 0x0, 0, 1) -> e
kernel: page fault trap, code = 0
Stopped at 0:uvm_fault(0xd565f480, 0x0, 0, 1) -> e
kernel: page fault trap, code = 0
Stopped at db_read_bytes+0x17: movzbl 0(%esi,%ecx,1),%eax
ddb> trace
db_read_bytes(0,1,f375ea44,0,f375ea54) at db_read_bytes+0x17
db_get_value(0,1,0,0,d09e1ada) at db_get_value+0x38
db_disasm(0,0,d03cc470,d03cc495,d09b6b38,f375eb14,0,0,f375eb14) at
db_disasm+0x31
db_print_loc_and_inst(0,f375,eb2c,f375eb38,d03cc495,d09e1acb) at
db_print_loc_and_inst+0x3e
db_trap(6,0,58,0,f375eb74) at db_trap+0x89
kdb_trap(6,0,f375ebe4,1,e) at kdb_trap+0xcc
trap() at trap+0x2e5
— trap (number 0) —
Bad frame pointer: 0xd56641e0
0:
ddb>
Index: uhidev.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
retrieving revision 1.69
diff -u -p -r1.69 uhidev.c
--- uhidev.c 22 Jan 2015 10:27:47 -0000 1.69
+++ uhidev.c 14 Feb 2015 15:18:47 -0000
@@ -53,6 +53,7 @@
#include <dev/usb/usbdi.h>
#include <dev/usb/usbdi_util.h>
#include <dev/usb/usbdivar.h>
+#include <dev/usb/usb_mem.h>
#include <dev/usb/hid.h>
#include <dev/usb/usb_quirks.h>
@@ -747,15 +748,17 @@ void
uhidev_get_report_async_cb(struct usbd_xfer *xfer, void *priv, usbd_status err)
{
struct uhidev_async_info *info = priv;
+ char *buf;
int len = -1;
if (err == USBD_NORMAL_COMPLETION || err == USBD_SHORT_XFER) {
len = xfer->actlen;
+ buf = KERNADDR(&xfer->dmabuf, 0);
if (info->id > 0) {
len--;
- memcpy(info->data, xfer->buffer + 1, len);
+ memcpy(info->data, buf + 1, len);
} else {
- memcpy(info->data, xfer->buffer, len);
+ memcpy(info->data, buf, len);
}
}
info->callback(info->priv, info->id, info->data, len);
Index: usbdi.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.80
diff -u -p -r1.80 usbdi.c
--- usbdi.c 12 Feb 2015 05:07:52 -0000 1.80
+++ usbdi.c 14 Feb 2015 15:18:47 -0000
@@ -748,6 +748,15 @@ usb_transfer_complete(struct usbd_xfer *
memcpy(xfer->buffer, KERNADDR(&xfer->dmabuf, 0), xfer->actlen);
}
+ if (!xfer->status && xfer->actlen < xfer->length &&
+ !(xfer->flags & USBD_SHORT_XFER_OK)) {
+ DPRINTFN(-1,("usb_transfer_complete: short transfer %d<%d\n",
+ xfer->actlen, xfer->length));
+ xfer->status = USBD_SHORT_XFER;
+ }
+ if (xfer->callback)
+ xfer->callback(xfer, xfer->priv, xfer->status);
+
/* if we allocated the buffer in usbd_transfer() we free it here. */
if (xfer->rqflags & URQ_AUTO_DMABUF) {
if (!pipe->repeat) {
@@ -774,22 +783,7 @@ usb_transfer_complete(struct usbd_xfer *
[pipe->endpoint->edesc->bmAttributes & UE_XFERTYPE];
xfer->done = 1;
- if (!xfer->status && xfer->actlen < xfer->length &&
- !(xfer->flags & USBD_SHORT_XFER_OK)) {
- DPRINTFN(-1,("usb_transfer_complete: short transfer %d<%d\n",
- xfer->actlen, xfer->length));
- xfer->status = USBD_SHORT_XFER;
- }
-
- if (pipe->repeat) {
- if (xfer->callback)
- xfer->callback(xfer, xfer->priv, xfer->status);
- pipe->methods->done(xfer);
- } else {
- pipe->methods->done(xfer);
- if (xfer->callback)
- xfer->callback(xfer, xfer->priv, xfer->status);
- }
+ pipe->methods->done(xfer);
/*
* If we already got an I/O error that generally means the
