On 09/01/15(Fri) 12:36, David Higgs wrote:
> On Fri, Jan 9, 2015 at 9:00 AM, Martin Pieuchot <[email protected]>
> wrote:
> > As pointed out by David Higgs, uhidev report functions do a lot of
> > memory allocations and copies between buffers. This is not uncommon
> > in USB land due to way DMA buffers are handled.
> >
> > One way to reduce the number of copies is to submit a transfer with
> > the USBD_NO_COPY flag. Without it the kernel does two to three copies:
> >
> > (stack/driver array <-->) malloc'ed array <--> DMA reacheable array
> >
> > This is not limited to uhidev functions. That's also done for every
> > transfer sent through an interrupt pipe or for every control request
> > with data.
> >
> > I'd like to slowly change the stack such that USBD_NO_COPY becomes the
> > default behavior. This would first reduces the number of buffer copies
> > and then allow umass(4) to directly use the DMA reachable buffers
> > provided by the SCSI stack, which should *at least* reduce the CPU usage
> > when using USB drives.
> >
> > Here's a diff about uhidev(4) to illustrate the changes I'm talking
> > about.
> >
> > Comments, ok?
> >
>
> See inline. Reviewed to the best of my ability/eyesight via
> variable-width webmail.
> [...]
> > +
> > + if (id > 0)
> > + len++;
> > +
> > + buf = usbd_alloc_buffer(xfer, len);
> > + if (buf == NULL)
> > + return (-1);
>
> You forgot to free xfer in this error path.
Indeed!
> > @@ -586,6 +585,7 @@ usbd_status
> > usbd_clear_endpoint_stall_async(struct usbd_pipe *pipe)
> > {
> > struct usbd_device *dev = pipe->device;
> > + struct usbd_xfer *xfer;
> > usb_device_request_t req;
> > usbd_status err;
> >
> > @@ -596,7 +596,12 @@ usbd_clear_endpoint_stall_async(struct u
> > USETW(req.wValue, UF_ENDPOINT_HALT);
> > USETW(req.wIndex, pipe->endpoint->edesc->bEndpointAddress);
> > USETW(req.wLength, 0);
> > - err = usbd_do_request_async(dev, &req, 0, 0, 0);
> > +
> > + xfer = usbd_alloc_xfer(dev);
> > + if (xfer == NULL)
> > + return (USBD_NOMEM);
> > +
> > + err = usbd_request_async(xfer, &req, NULL, NULL);
>
> Missing xfer cleanup here too, I think.
Here it's fine, as soon as you call usbd_request_async() the xfer will be
freed.
Updated diff below.
Index: uhidev.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
retrieving revision 1.68
diff -u -p -r1.68 uhidev.c
--- uhidev.c 9 Jan 2015 12:09:51 -0000 1.68
+++ uhidev.c 9 Jan 2015 19:03:45 -0000
@@ -671,18 +671,30 @@ int
uhidev_set_report_async(struct uhidev_softc *sc, int type, int id, void *data,
int len)
{
+ struct usbd_xfer *xfer;
usb_device_request_t req;
- char *buf = data;
int actlen = len;
+ char *buf;
+
+ xfer = usbd_alloc_xfer(sc->sc_udev);
+ if (xfer == NULL)
+ return (-1);
+
+ if (id > 0)
+ len++;
+
+ buf = usbd_alloc_buffer(xfer, len);
+ if (buf == NULL) {
+ usbd_free_xfer(xfer);
+ return (-1);
+ }
/* Prepend the reportID. */
if (id > 0) {
- len++;
- buf = malloc(len, M_TEMP, M_NOWAIT);
- if (buf == NULL)
- return (-1);
buf[0] = id;
memcpy(buf + 1, data, len - 1);
+ } else {
+ memcpy(buf, data, len);
}
req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
@@ -691,17 +703,9 @@ uhidev_set_report_async(struct uhidev_so
USETW(req.wIndex, sc->sc_ifaceno);
USETW(req.wLength, len);
- if (usbd_do_request_async(sc->sc_udev, &req, buf, NULL, NULL))
+ if (usbd_request_async(xfer, &req, NULL, NULL))
actlen = -1;
- /*
- * Since report requests are write-only it is safe to free
- * the buffer right after submitting the transfer because
- * it won't be used afterward.
- */
- if (id > 0)
- free(buf, M_TEMP, len);
-
return (actlen);
}
@@ -750,11 +754,11 @@ uhidev_get_report_async_cb(struct usbd_x
if (info->id > 0) {
len--;
memcpy(info->data, xfer->buffer + 1, len);
+ } else {
+ memcpy(info->data, xfer->buffer, len);
}
}
info->callback(info->priv, info->id, info->data, len);
- if (info->id > 0)
- free(xfer->buffer, M_TEMP, xfer->length);
free(info, M_TEMP, sizeof(*info));
usbd_free_xfer(xfer);
}
@@ -763,42 +767,47 @@ int
uhidev_get_report_async(struct uhidev_softc *sc, int type, int id, void *data,
int len, void *priv, void (*callback)(void *, int, void *, int))
{
+ struct usbd_xfer *xfer;
usb_device_request_t req;
struct uhidev_async_info *info;
- char *buf = data;
int actlen = len;
+ char *buf;
+
+ xfer = usbd_alloc_xfer(sc->sc_udev);
+ if (xfer == NULL)
+ return (-1);
+
+ if (id > 0)
+ len++;
+
+ buf = usbd_alloc_buffer(xfer, len);
+ if (buf == NULL) {
+ usbd_free_xfer(xfer);
+ return (-1);
+ }
info = malloc(sizeof(*info), M_TEMP, M_NOWAIT);
- if (info == NULL)
+ if (info == NULL) {
+ usbd_free_xfer(xfer);
return (-1);
+ }
info->callback = callback;
info->priv = priv;
info->data = data;
info->id = id;
- if (id > 0) {
- len++;
- buf = malloc(len, M_TEMP, M_NOWAIT|M_ZERO);
- if (buf == NULL) {
- free(info, M_TEMP, sizeof(*info));
- return (-1);
- }
- }
-
req.bmRequestType = UT_READ_CLASS_INTERFACE;
req.bRequest = UR_GET_REPORT;
USETW2(req.wValue, type, id);
USETW(req.wIndex, sc->sc_ifaceno);
USETW(req.wLength, len);
- if (usbd_do_request_async(sc->sc_udev, &req, buf, priv,
- uhidev_get_report_async_cb)) {
+ if (usbd_request_async(xfer, &req, priv, uhidev_get_report_async_cb)) {
free(info, M_TEMP, sizeof(*info));
- if (id > 0)
- free(buf, M_TEMP, len);
actlen = -1;
}
+
return (actlen);
}
Index: usbdi.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.77
diff -u -p -r1.77 usbdi.c
--- usbdi.c 9 Jan 2015 12:15:48 -0000 1.77
+++ usbdi.c 9 Jan 2015 13:22:02 -0000
@@ -55,8 +55,7 @@ extern int usbdebug;
#define DPRINTFN(n,x)
#endif
-void usbd_do_request_async_cb(struct usbd_xfer *, void *,
- usbd_status);
+void usbd_request_async_cb(struct usbd_xfer *, void *, usbd_status);
void usbd_start_next(struct usbd_pipe *pipe);
usbd_status usbd_open_pipe_ival(struct usbd_interface *, u_int8_t, u_int8_t,
struct usbd_pipe **, int);
@@ -586,6 +585,7 @@ usbd_status
usbd_clear_endpoint_stall_async(struct usbd_pipe *pipe)
{
struct usbd_device *dev = pipe->device;
+ struct usbd_xfer *xfer;
usb_device_request_t req;
usbd_status err;
@@ -596,7 +596,12 @@ usbd_clear_endpoint_stall_async(struct u
USETW(req.wValue, UF_ENDPOINT_HALT);
USETW(req.wIndex, pipe->endpoint->edesc->bEndpointAddress);
USETW(req.wLength, 0);
- err = usbd_do_request_async(dev, &req, 0, 0, 0);
+
+ xfer = usbd_alloc_xfer(dev);
+ if (xfer == NULL)
+ return (USBD_NOMEM);
+
+ err = usbd_request_async(xfer, &req, NULL, NULL);
return (err);
}
@@ -949,8 +954,7 @@ usbd_do_request_flags(struct usbd_device
}
void
-usbd_do_request_async_cb(struct usbd_xfer *xfer, void *priv,
- usbd_status status)
+usbd_request_async_cb(struct usbd_xfer *xfer, void *priv, usbd_status status)
{
usbd_free_xfer(xfer);
}
@@ -960,19 +964,17 @@ usbd_do_request_async_cb(struct usbd_xfe
* Can be used from interrupt context.
*/
usbd_status
-usbd_do_request_async(struct usbd_device *dev, usb_device_request_t *req,
- void *data, void *priv, usbd_callback callback)
+usbd_request_async(struct usbd_xfer *xfer, usb_device_request_t *req,
+ void *priv, usbd_callback callback)
{
- struct usbd_xfer *xfer;
usbd_status err;
- xfer = usbd_alloc_xfer(dev);
- if (xfer == NULL)
- return (USBD_NOMEM);
if (callback == NULL)
- callback = usbd_do_request_async_cb;
- usbd_setup_default_xfer(xfer, dev, priv, USBD_DEFAULT_TIMEOUT, req,
- data, UGETW(req->wLength), 0, callback);
+ callback = usbd_request_async_cb;
+
+ usbd_setup_default_xfer(xfer, xfer->device, priv,
+ USBD_DEFAULT_TIMEOUT, req, NULL, UGETW(req->wLength),
+ USBD_NO_COPY, callback);
err = usbd_transfer(xfer);
if (err != USBD_IN_PROGRESS) {
usbd_free_xfer(xfer);
Index: usbdi.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi.h,v
retrieving revision 1.64
diff -u -p -r1.64 usbdi.h
--- usbdi.h 9 Jan 2015 12:07:50 -0000 1.64
+++ usbdi.h 9 Jan 2015 13:14:25 -0000
@@ -121,8 +121,8 @@ usbd_status usbd_open_pipe_intr(struct u
void *buffer, u_int32_t length, usbd_callback, int);
usbd_status usbd_do_request(struct usbd_device *pipe, usb_device_request_t
*req,
void *data);
-usbd_status usbd_do_request_async(struct usbd_device *pipe,
- usb_device_request_t *req, void *data, void *priv, usbd_callback callback);
+usbd_status usbd_request_async(struct usbd_xfer *xfer,
+ usb_device_request_t *req, void *priv, usbd_callback callback);
usbd_status usbd_do_request_flags(struct usbd_device *pipe,
usb_device_request_t *req, void *data, u_int16_t flags, int*, u_int32_t);
usb_interface_descriptor_t *usbd_get_interface_descriptor(
