On Friday 08 January 2010 20:56:16 you wrote:
> On Fri, Jan 08, 2010 at 08:07:51PM +0100, Stefan Brüns wrote:
> > get rid of temporary iso frame descriptor (isopkt)
>
> Why? I don't think you can rely on access_ok, copy_from_user() is
> "safer" from what I remember.
You are right on this one, copy_{from,to}_user takes paging into account.
> > + if(uurb->type != USBDEVFS_URB_TYPE_ISO) {
> > + as = alloc_async(0);
> > + if (!as)
> > + return -ENOMEM;
> > + }
>
> Why allocate one if we aren't going to use it?
Hm, dont understand this comment. /as/ will be used, except for error cases.
> Also, can you run your patch through 'scripts/checkpatch.pl' to catch
> the minor formatting issues?
done
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
phone: +49 241 53809034 mobile: +49 151 50412019
From edee1b56def24fa4142b789320d3e0578c4653b5 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Stefan=20Br=C3=BCns?= <stefan.bruens@xxxxxxxxxxxxxx>
Date: Fri, 8 Jan 2010 19:49:08 +0100
Subject: [PATCH] usbdevfs: allocate async early, to allow some cleanup
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
- only set urb fields if applicable for respective urb type
- traverse iso pkt descriptor only once, free it immediately when
it is no longer needed.
Signed-off-by: Stefan Brüns <stefan.bruens@xxxxxxxxxxxxxx>
---
drivers/usb/core/devio.c | 55 +++++++++++++++++++++++----------------------
1 files changed, 28 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 6e8bcdf..6d26c9f 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1061,6 +1061,11 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
}
if (!ep)
return -ENOENT;
+ if (uurb->type != USBDEVFS_URB_TYPE_ISO) {
+ as = alloc_async(0);
+ if (!as)
+ return -ENOMEM;
+ }
switch(uurb->type) {
case USBDEVFS_URB_TYPE_CONTROL:
if (!usb_endpoint_xfer_control(&ep->desc))
@@ -1071,20 +1076,23 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
uurb->buffer_length > (8 + MAX_USBFS_BUFFER_SIZE))
return -EINVAL;
dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
- if (!dr)
+ if (!dr) {
+ free_async(as);
return -ENOMEM;
+ }
+ as->urb->setup_packet = (unsigned char *)dr;
if (copy_from_user(dr, uurb->buffer, 8)) {
- kfree(dr);
+ free_async(as);
return -EFAULT;
}
if (uurb->buffer_length < (le16_to_cpup(&dr->wLength) + 8)) {
- kfree(dr);
+ free_async(as);
return -EINVAL;
}
ret = check_ctrlrecip(ps, dr->bRequestType,
le16_to_cpup(&dr->wIndex));
if (ret) {
- kfree(dr);
+ free_async(as);
return ret;
}
uurb->number_of_packets = 0;
@@ -1126,21 +1134,31 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
kfree(isopkt);
return -EFAULT;
}
+ as = alloc_async(uurb->number_of_packets);
+ if (!as)
+ return -ENOMEM;
for (totlen = u = 0; u < uurb->number_of_packets; u++) {
/* arbitrary limit,
* sufficient for USB 2.0 high-bandwidth iso */
if (isopkt[u].length > 8192) {
kfree(isopkt);
+ free_async(as);
return -EINVAL;
}
+ as->urb->iso_frame_desc[u].offset = totlen;
+ as->urb->iso_frame_desc[u].length = isopkt[u].length;
totlen += isopkt[u].length;
}
/* 3072 * 64 microframes */
if (totlen > 196608) {
kfree(isopkt);
+ free_async(as);
return -EINVAL;
}
+ kfree(isopkt);
uurb->buffer_length = totlen;
+ as->urb->start_frame = uurb->start_frame;
+ as->urb->interval = 1 << min(15, ep->desc.bInterval - 1);
break;
case USBDEVFS_URB_TYPE_INTERRUPT:
@@ -1149,6 +1167,11 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
return -EINVAL;
if (uurb->buffer_length > MAX_USBFS_BUFFER_SIZE)
return -EINVAL;
+ if (ps->dev->speed == USB_SPEED_HIGH)
+ as->urb->interval = 1 <<
+ min(15, ep->desc.bInterval - 1);
+ else
+ as->urb->interval = ep->desc.bInterval;
break;
default:
@@ -1157,22 +1180,13 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
if (uurb->buffer_length > 0 &&
!access_ok(is_in ? VERIFY_WRITE : VERIFY_READ,
uurb->buffer, uurb->buffer_length)) {
- kfree(isopkt);
- kfree(dr);
+ free_async(as);
return -EFAULT;
}
- as = alloc_async(uurb->number_of_packets);
- if (!as) {
- kfree(isopkt);
- kfree(dr);
- return -ENOMEM;
- }
if (uurb->buffer_length > 0) {
as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
GFP_KERNEL);
if (!as->urb->transfer_buffer) {
- kfree(isopkt);
- kfree(dr);
free_async(as);
return -ENOMEM;
}
@@ -1200,22 +1214,9 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
as->urb->transfer_flags = u;
as->urb->transfer_buffer_length = uurb->buffer_length;
- as->urb->setup_packet = (unsigned char *)dr;
- as->urb->start_frame = uurb->start_frame;
as->urb->number_of_packets = uurb->number_of_packets;
- if (uurb->type == USBDEVFS_URB_TYPE_ISO ||
- ps->dev->speed == USB_SPEED_HIGH)
- as->urb->interval = 1 << min(15, ep->desc.bInterval - 1);
- else
- as->urb->interval = ep->desc.bInterval;
as->urb->context = as;
as->urb->complete = async_completed;
- for (totlen = u = 0; u < uurb->number_of_packets; u++) {
- as->urb->iso_frame_desc[u].offset = totlen;
- as->urb->iso_frame_desc[u].length = isopkt[u].length;
- totlen += isopkt[u].length;
- }
- kfree(isopkt);
as->ps = ps;
as->userurb = arg;
if (is_in && uurb->buffer_length > 0)
--
1.5.6
