On Sat, 29 Oct 2005, Joshua Wise wrote:
> I understand that it'll probably not be ready for a commit now, but it's a
> lot cleaner than it used to be. Can someone run a pair of eyes over it and
> tell me if anything is glaringly preventing it from going into the tree?
Here are a few comments, mixed in with your code.
> #define DBUFMAX 65536
This is pretty big for a kernel buffer. You ought to be able to
use something smaller.
> static unsigned buflen = 2048;
> static unsigned qlen = 8;
It's not clear that these values are the best. It depends on the
characteristics of the transfers you expect.
> static const struct usb_device_descriptor
> device_desc = {
...
> .bMaxPacketSize0 = EP0_MAXPACKET,
For portability, this value cannot be predefined. You have to get
it from the controller driver.
> static void gchar_complete (struct usb_ep *ep, struct usb_request *req)
> {
> struct gchar_dev *dev = ep->driver_data;
> int status = req->status;
>
> switch (status) {
>
> case 0: /* normal completion? */
> if (ep == dev->out_ep) {
This is an unnecessary test. What other endpoint could it be?
> /* it looks like you've got a packet! */
> req->zero = (req->actual < req->length);
There's no point setting req->zero for an OUT transfer.
> req->length = req->actual;
>
> spin_lock(&dev->buflock);
> if ((dev->databuflen+req->actual) < DBUFMAX) {
You might as well copy all the data that will fit. Is there any
reason to leave out an entire packet if it's a few bytes too long?
> memcpy(&dev->databuf[dev->databuflen],
> req->buf, req->actual);
> dev->databuflen += req->actual;
> }
> spin_unlock(&dev->buflock);
> wake_up_interruptible(&dev->wait);
>
> /* queue the buffer for some later OUT packet */
> req->length = buflen;
> status = usb_ep_queue (dev->out_ep, req, GFP_ATOMIC);
> if (status == 0)
> return;
>
> ERROR (dev, "complete: failed to requeue request -->
> %d\n", status);
> return;
> }
This next section is all dead code.
> /* queue the buffer for some later OUT packet */
> req->length = buflen;
> status = usb_ep_queue (dev->out_ep, req, GFP_ATOMIC);
>
> ERROR (dev, "complete: invalid endpoint --> %s (%d/%d)\n",
> ep->name, req->actual, req->length);
> if (status == 0)
> return;
> /* FALLTHROUGH */
What is it doing here? Looks like it something got left out or moved
to the wrong spot.
> static ssize_t read_gchar(struct file* file, char* buf, size_t count, loff_t
> *ppos)
Isn't buf supposed to be __user? And ditto for the write_gchar routine?
> struct gchar_dev *dev = (struct gchar_dev *)file->private_data;
> ssize_t readcnt;
> DECLARE_WAITQUEUE(wait, current);
>
> add_wait_queue(&dev->wait, &wait);
> current->state = TASK_INTERRUPTIBLE;
>
> do {
> if (dev->databuflen != 0)
> break;
>
> if (file->f_flags & O_NONBLOCK)
> {
> readcnt = -EAGAIN;
> goto out;
> }
>
> if (!dev->configured)
> return -ENXIO;
Whoops! You're still in TASK_INTERRUPTIBLE.
> schedule();
> } while (1);
>
> spin_lock_irq(&dev->buflock);
> readcnt = (count > dev->databuflen) ? dev->databuflen : count;
> memcpy(buf, dev->databuf, readcnt);
Shouldn't this be copy_from_user?
> memmove(dev->databuf, dev->databuf+readcnt,
> dev->databuflen-readcnt);
> dev->databuflen -= readcnt;
> spin_unlock_irq(&dev->buflock);
> static ssize_t write_gchar(struct file* file, const char* buf, size_t count,
> loff_t* ppos)
> {
> struct gchar_dev *dev = (struct gchar_dev *)file->private_data;
> struct usb_request *req;
> int result;
>
> req = alloc_ep_req (dev->in_ep, count);
>
> if (!dev)
> return 0;
>
> if (!dev->configured)
> return 0;
Looks like these tests should go before the alloc_ep_req call. On the
other hand, I don't see how dev could ever be NULL.
> static loff_t null_lseek(struct file * file, loff_t offset, int orig)
> {
> return file->f_pos = 0;
> }
Isn't there a standard kernel routine to do this for you?
Alan Stern
-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course
Free Certification Exam for All Training Attendees Through End of 2005
Visit http://www.jboss.com/services/certification for more information
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
