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.
>
OK, I figured out my issue - the crazy DDB backtrace is produced when you
execute a NULL callback.
It still doesn’t seem legal for the callback to access DMA buffer contents
after they are “freed”. I assume this won’t work in all cases (host
controllers / architectures / cache behaviors), but I don’t experience any
problems in my i386 VM. I tried reordering parts of usbd_transfer_complete(),
but DIAGNOSTIC code became very unhappy with the results.
Fortunately, the diff below doesn’t touch that code path and just fixes the
uhidev layer. My async upd(4) changes will be forthcoming in a different
thread.
Thanks.
--david
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 20 Feb 2015 02:27:29 -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);
@@ -803,7 +806,7 @@ uhidev_get_report_async(struct uhidev_so
USETW(req.wIndex, sc->sc_ifaceno);
USETW(req.wLength, len);
- if (usbd_request_async(xfer, &req, priv, uhidev_get_report_async_cb)) {
+ if (usbd_request_async(xfer, &req, info, uhidev_get_report_async_cb)) {
free(info, M_TEMP, sizeof(*info));
actlen = -1;
}
