On Fri, Jan 17 2014, Robert Baldyga wrote:
> @@ -623,8 +644,55 @@ static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req)
> }
> }
>
> -static ssize_t ffs_epfile_io(struct file *file,
> - char __user *buf, size_t len, int read)
> +static void ffs_user_copy_worker(struct work_struct *work)
> +{
> + size_t len = 0;
Nit: This would be better named “pos” since it's a position in
a io_data->buf buffer.
> + int i = 0;
> + int ret;
> +
> + struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
> + work);
> + ret = io_data->len;
> +
> + use_mm(io_data->mm);
> + for (i = 0; i < io_data->nr_segs; i++) {
> + if (unlikely(copy_to_user(io_data->iovec[i].iov_base,
> + &io_data->buf[len],
Nit: IMO this is a bit confusing way of writing “io_data->buf + len”.
I wouldn't mention it if you cared about pointing to a single character
at position len, but here you just move the pointer of the buffer to
position len.
> + io_data->iovec[i].iov_len))) {
> + ret = -EFAULT;
> + break;
> + }
> + len += io_data->iovec[i].iov_len;
> + }
> + unuse_mm(io_data->mm);
> +
> + aio_complete(io_data->kiocb, ret, ret);
> +
> + kfree(io_data->iovec);
> + kfree(io_data->buf);
> + kfree(io_data);
> +}
> @@ -667,15 +735,32 @@ static ssize_t ffs_epfile_io(struct file *file,
> * Controller may require buffer size to be aligned to
> * maxpacketsize of an out endpoint.
> */
> - data_len = read ? usb_ep_align_maybe(gadget, ep->ep, len) : len;
> + data_len = io_data->read ? usb_ep_align_maybe(gadget, ep->ep,
> + io_data->len) :
> + io_data->len;
Nit: The following is easier to read IMO:
data_len = io_data->read ?
usb_ep_align_maybe(gadget, ep->ep, io_data->len) :
io_data->len;
>
> data = kmalloc(data_len, GFP_KERNEL);
> if (unlikely(!data))
> return -ENOMEM;
> -
> - if (!read && unlikely(copy_from_user(data, buf, len))) {
> - ret = -EFAULT;
> - goto error;
> + if (io_data->aio && !io_data->read) {
> + int i;
> + size_t len = 0;
> + for (i = 0; i < io_data->nr_segs; i++) {
> + if (unlikely(copy_from_user(&data[len],
> + io_data->iovec[i].iov_base,
> + io_data->iovec[i].iov_len) != 0)) {
Nit: != 0 is not necessary here.
> + ret = -EFAULT;
> + goto error;
> + }
> + len += io_data->iovec[i].iov_len;
> + }
> + } else {
> + if (!io_data->read &&
> + unlikely(__copy_from_user(data, io_data->buf,
> + io_data->len))) {
> + ret = -EFAULT;
> + goto error;
> + }
> }
> }
> @@ -741,17 +858,31 @@ static ssize_t
> ffs_epfile_write(struct file *file, const char __user *buf, size_t len,
> loff_t *ptr)
> {
> + struct ffs_io_data io_data;
> +
> ENTER();
>
> - return ffs_epfile_io(file, (char __user *)buf, len, 0);
> + io_data.aio = 0;
> + io_data.read = 0;
Use “false” since those are now bools. Same for the rest of the code.
> + io_data.buf = (char * __user)buf;
> + io_data.len = len;
> +
> + return ffs_epfile_io(file, &io_data);
> }
> @@ -770,6 +901,80 @@ ffs_epfile_open(struct inode *inode, struct file *file)
> return 0;
> }
>
> +static int ffs_aio_cancel(struct kiocb *kiocb)
> +{
> + struct ffs_io_data *io_data = kiocb->private;
> + struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
> + int value;
> +
> + ENTER();
> +
> + spin_lock_irq(&epfile->ffs->eps_lock);
> +
> + if (likely(io_data && io_data->ep && io_data->req))
> + value = usb_ep_dequeue(io_data->ep, io_data->req);
> + else
> + value = -EINVAL;
> +
Nit: Trailing whitespace.
> + usb_ep_free_request(io_data->ep, io_data->req);
> +
> + spin_unlock_irq(&epfile->ffs->eps_lock);
> +
> + return value;
> +}
> +
> +static ssize_t ffs_epfile_aio_write(struct kiocb *kiocb,
> + const struct iovec *iovec,
> + unsigned long nr_segs, loff_t loff)
> +{
> + struct ffs_io_data *io_data;
> +
> + ENTER();
> +
> + io_data = kmalloc(sizeof(struct ffs_io_data), GFP_KERNEL);
io_data = kmalloc(sizeof(*io_data), GFP_KERNEL);
And you need to check if allocation succeeded.
> + io_data->aio = 1;
> + io_data->read = 0;
> + io_data->kiocb = kiocb;
> + io_data->iovec = iovec;
> + io_data->nr_segs = nr_segs;
> + io_data->len = kiocb->ki_nbytes;
> + io_data->mm = current->mm;
> +
> + kiocb->private = io_data;
> +
> + kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
> +
> + return ffs_epfile_io(kiocb->ki_filp, io_data);
> +}
> +
> +static ssize_t ffs_epfile_aio_read(struct kiocb *kiocb,
> + const struct iovec *iovec,
> + unsigned long nr_segs, loff_t loff)
> +{
> + struct ffs_io_data *io_data;
> + struct iovec *iovec_copy;
> +
> + ENTER();
> +
> + iovec_copy = kmalloc(sizeof(struct iovec)*nr_segs, GFP_KERNEL);
> + memcpy(iovec_copy, iovec, sizeof(struct iovec)*nr_segs);
> +
> + io_data = kmalloc(sizeof(struct ffs_io_data), GFP_KERNEL);
iovec_copy = kmalloc_array(nr_segs, sizeof(*iovec_copy), GFP_KERNEL);
memcpy(iovec_copy, iovec, sizeof(struct iovec)*nr_segs);
io_data = kmalloc(sizeof(*io_data), GFP_KERNEL);
Plus you need to check whether allocation succeeded.
> + io_data->aio = 1;
> + io_data->read = 1;
> + io_data->kiocb = kiocb;
> + io_data->iovec = iovec_copy;
> + io_data->nr_segs = nr_segs;
> + io_data->len = kiocb->ki_nbytes;
> + io_data->mm = current->mm;
> +
> + kiocb->private = io_data;
> +
> + kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
> +
> + return ffs_epfile_io(kiocb->ki_filp, io_data);
> +}
> +
> static int
> ffs_epfile_release(struct inode *inode, struct file *file)
> {
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
Attachment:
signature.asc
Description: PGP signature
