Finally, I made time to review this code!
On Monday 25 June 2007, Craig W. Nadler wrote:
> --- a/Documentation/usb/gadget_printer.txt 1969-12-31 19:00:00.000000000
> -0500
> +++ b/Documentation/usb/gadget_printer.txt 2007-06-26 00:03:59.000000000
> -0400
> @@ -0,0 +1,498 @@
> + ...
> +
> +void
> +usage(const char *option) /* I - Option string or NULL */
> +{
> + if (option)
> + fprintf(stderr, "prn_example: Unknown option \"%s\"!\n", option);
What's this "tabstop=2" stuff?? This is Linux! :)
Seriously ... even in example code, let's stick to the
CodingStyle guidelines. (I think Lindent should fix
most of this for you, other than maybe too-long lines.)
> --- a/drivers/usb/gadget/printer.c 1969-12-31 19:00:00.000000000 -0500
> +++ b/drivers/usb/gadget/printer.c 2007-06-26 00:36:08.000000000 -0400
> @@ -0,0 +1,1736 @@
> + ...
> +
> +
> +#define DEBUG 1
No ... I submitted a patch (now in Greg's queue) to provide
CONFIG_USB_GADGET_DEBUG which will automagically #define DEBUG
when appropriate. Don't force this on in drivers.
I fired this driver up, and noticed it was spamming my syslog
with debug data even though CONFIG_USB_GADGET_DEBUG wasn't set...
> +#define DRIVER_DESC "Printer Gadget"
> +#define DRIVER_VERSION "0.2.0 06/04/2007"
Version strings ... generally frowned on, since they
end up not meaning anything. I'd be happier if you just
included included a date code.
> +#define GADGET_GET_PRINTER_STATUS _IOR('P', 1, unsigned char)
> +#define GADGET_SET_PRINTER_STATUS _IOWR('P', 2, unsigned char)
These need to show up in a header file that gets exported;
maybe <linux/usb/g_printer.h> would be suitable. It'd be
useful to have constants for the printer status values too
(I'm assuming that makes sense.)
> +static struct class *usb_gadget_class;
Do you really need a usb_gadget class? There can only
ever be one instance on a given system, and none of the
other drivers have one ... I think you should just remove
this.
Similarly for the printer class, although maybe udev/mdev
expect a class before they'll make a device node. If so,
you'd only need a printer class.
> +/* Thanks to NetChip Technologies for donating this product ID.
> + */
> +#define PRINTER_VENDOR_NUM 0x0525 /* NetChip */
> +#define PRINTER_PRODUCT_NUM 0xa4a8 /* Linux-USB Printer Gadget */
Actually that ID was one of a bunch that was assigned to me,
and I just delegated it to this driver. I didn't have a record
that I had already done that...
> +#define xprintk(d, level, fmt, args...) \
> + printk(level "%s: " fmt, DRIVER_DESC, ## args)
> +
> +#ifdef DEBUG
> +#undef DEBUG
> +#define DEBUG(dev, fmt, args...) \
> + xprintk(dev, KERN_DEBUG, fmt, ## args)
> +#else
> +#define DEBUG(dev, fmt, args...) \
> + do { } while (0)
> +#endif /* DEBUG */
> +
> +#ifdef VERBOSE
> +#define VDEBUG(dev, fmt, args...) \
> + xprintk(dev, KERN_DEBUG, fmt, ## args)
> +#else
> +#define VDEBUG(dev, fmt, args...) \
> + xprintk(dev, KERN_DEBUG, fmt, ## args)
> +#endif /* DEBUG */
> +
> +#define ERROR(dev, fmt, args...) \
> + xprintk(dev, KERN_ERR, fmt, ## args)
> +#define WARN(dev, fmt, args...) \
> + xprintk(dev, KERN_WARNING, fmt, ## args)
> +#define INFO(dev, fmt, args...) \
> + xprintk(dev, KERN_INFO, fmt, ## args)
I realize I'm partly to blame for this stuff, but could you just
switch away from DEBUG(...) to DBG()/VDBG() so that DEBUG isn't
overloaded?
Some folk would prefer that this use straight driver model stuff,
but in this case no device is truly appropriate (and there can only
be a single gadget driver). So I'm OK with not using dev_dbg() and
friends in gadget drivers.
> +static struct usb_request *
> +printer_req_alloc (struct usb_ep *ep, unsigned size, gfp_t gfp_flags)
Oh, and the "use whitespace for readability" style -- "func (...)" -- is
now deprecated in favor of "save bytes" style -- "func(...)". So please
convert this whole driver over to that style.
> +{
> + struct usb_request *req;
> +
> + req = usb_ep_alloc_request (ep, gfp_flags);
> + if (!req)
> + return NULL;
> +
> + req->buf = usb_ep_alloc_buffer (ep, size, &req->dma, gfp_flags);
Get rid of the ep_{alloc,free}_buffer() calls ... they're going
away. You can just use kmalloc/kfree; you don't need dma-coherent
buffers.
> +
> +static int
> +printer_open (struct inode *inode, struct file *fd)
> +{
> + struct printer_dev *dev;
> + unsigned long flags;
> + int ret = -EBUSY;
> +
> +
Extra whitespace; just one blank line there ... you do that
in a lot of places.
> + dev = container_of (inode->i_cdev, struct printer_dev, printer_cdev);
> +
> + if (dev == NULL) {
This test can never succeed ... don't bother testing for this!
> + printk(KERN_ERR "printer_open: NULL device pointer\n");
> + return -EIO;
> + }
> +
> + spin_lock_irqsave (&dev->lock, flags);
> +
> + if (!dev->printer_cdev_open) {
> + dev->printer_cdev_open = 1;
> + fd->private_data = dev;
> + ret = 0;
> + }
... I'd change the status in that "if nyet open" block, too.
This looks wrong...
> +
> + /* Change printer status to show that the printer is on-line. */
> + dev->printer_status |= PRINTER_SELECTED;
> +
> + spin_unlock_irqrestore (&dev->lock, flags);
> +
> + DEBUG (dev, "printer_open returned %x\n", ret);
> + printk(KERN_INFO "printer_open: returned %x\n", ret);
Another thing you do a lot here: inappropriate KERN_INFO messaging.
> +
> + return ret;
> +}
> +
> +
> +static int
> +printer_close (struct inode *inode, struct file *fd)
> +{
> + struct printer_dev *dev;
> + unsigned long flags;
> +
> + dev = container_of (inode->i_cdev, struct printer_dev, printer_cdev);
Mildly surprised this isn't dev = fd->private_data, like elsewhere...
> +
> + if (dev == NULL) {
Again, can't-happen, don't-bother-testing-this-case.
> + printk(KERN_ERR "printer_close: NULL device pointer\n");
> + return -EIO;
> + }
> +
> + /* Change printer status to show that the printer is off-line. */
> + dev->printer_status &= ~PRINTER_SELECTED;
> +
> + spin_lock_irqsave (&dev->lock, flags);
Surely you should spinlock before you change the state that's protected
by that spinlock! And also null out fd->private_data, so this is the
complete reverse of the open() logic above?
> +
> + dev->printer_cdev_open = 0;
> +
> + spin_unlock_irqrestore (&dev->lock, flags);
> +
> + DEBUG (dev, "printer_close\n");
> + printk(KERN_INFO "printer_close\n");
> +
> + return 0;
> +}
> +
> +
> +static ssize_t
> +printer_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
> +{
> + struct printer_dev *dev = fd->private_data;
> + unsigned long flags;
> + size_t size;
> + size_t bytes_copied;
> + struct usb_request *req;
> + struct usb_request *current_rx_req;
> + size_t current_rx_bytes;
> + u8 *current_rx_buf;
> +
> + if (dev == NULL) {
> + printk(KERN_ERR "printer_read: NULL device pointer\n");
Can't happen, right?
> + return -EIO;
> + }
> +
> + if (len == 0)
> + return -EINVAL;
> +
> + DEBUG (dev, "printer_read trying to read %d bytes\n", (int)len);
> +
> + spin_lock (&dev->lock_printer_io);
Wouldn't that lock more naturally be a mutex ... ?
> + spin_lock_irqsave (&dev->lock, flags);
> +
> + /* Check if a printer reset happens while we have interrupts on */
I don't follow the comment. IRQs are disabled here...
> + dev->reset_printer = 0;
> +
> + while (likely (!list_empty (&dev->rx_reqs))) {
> + int error;
> +
> + req = container_of (dev->rx_reqs.next,
> + struct usb_request, list);
> + list_del_init (&req->list);
> +
> + req->length = USB_BUFSIZE;
If "len" (parameter) is smaller than USB_BUFSIZE, you'll issue
reads for more than that? Seems odd. Especially since the
strategy used here has no comment, explaining what's going on...
> + req->complete = rx_complete;
> +
> + error = usb_ep_queue (dev->out_ep, req, GFP_ATOMIC);
> + if (error) {
> + DEBUG (dev, "rx submit --> %d\n", error);
> + list_add (&req->list, &dev->rx_reqs);
> + break;
> + } else {
> + list_add (&req->list, &dev->rx_reqs_active);
> + }
> + }
> +
> + bytes_copied = 0;
> + current_rx_req = dev->current_rx_req;
> + current_rx_bytes = dev->current_rx_bytes;
> + current_rx_buf = dev->current_rx_buf;
> + dev->current_rx_req = NULL;
> + dev->current_rx_bytes = 0;
> + dev->current_rx_buf = NULL;
> +
These next two loops look strangely structured. I'd expect
something more like
do {
if (data ready)
copy to userspace
if (done reading)
break;
if (nonblocking)
setup -EAGAIN and break;
sleep till more data arrives
} while (not reset && rx buffers are queued);
As it is, if just one buffer is ready, you won't ever sleep;
even when the file descriptor isn't nonblocking ... so the
read should be sleeping until "len" bytes have been read,
rather than just until some byte becomes available.
Related: I don't know the USB printer spec, but is this a
case where short packets should terminate the application
level read -- or not? I suspect the latter ... if that's
the case, it's worth a comment since that's relatively
unusual (in the USB drivers I've worked with).
> + /* Check if there is any data in the read buffers */
> + if ((current_rx_bytes == 0) &&
> + (likely (list_empty (&dev->rx_buffers)))) {
> + /* Turn interrupts back on before sleeping. */
> + spin_unlock_irqrestore (&dev->lock, flags);
> +
> + /*
> + * If no data is available check if this is a NON-Blocking
> + * call or not.
> + */
> + if (fd->f_flags & (O_NONBLOCK|O_NDELAY)) {
> + spin_unlock (&dev->lock_printer_io);
> + return -EAGAIN;
> + }
> +
> + /* Sleep until data is available */
> + wait_event_interruptible(dev->rx_wait,
> + (likely (!list_empty (&dev->rx_buffers))));
> + spin_lock_irqsave (&dev->lock, flags);
> + }
> +
> + while ((current_rx_bytes || likely (!list_empty (&dev->rx_buffers)))
> + && len) {
> + if (current_rx_bytes == 0) {
> + req = container_of (dev->rx_buffers.next,
> + struct usb_request, list);
> + list_del_init (&req->list);
> +
> + if (req->actual && req->buf) {
> + current_rx_req = req;
> + current_rx_bytes = req->actual;
> + current_rx_buf = req->buf;
This whole "current_rx_*" thing is confusing to me.
In the sense that it seems to obfuscate things. What
I think is probably going on is:
* There might be bytes left over from a previous read, at
the tail end of a buffer at the head of dev->rx_buffers
... so dev->current_rx_{req,buf} are superfluous, but
dev->current_rx_buf indicates leftover bytes.
* You've got to copy out of the "current" buffer, and have
local variables recording the request, its buffer, and
its contents ... named to match the state in the preceding
case
Please you really don't need so many confusing variables ... !? :)
> + } else {
> + list_add (&req->list, &dev->rx_reqs);
> + continue;
> + }
> + }
> +
> + /* Don't leave irqs off while doing memory copies */
> + spin_unlock_irqrestore (&dev->lock, flags);
> +
> + if (len > current_rx_bytes)
> + size = current_rx_bytes;
> + else
> + size = len;
> +
> + size -= copy_to_user (buf, current_rx_buf, size);
> + bytes_copied += size;
> + len -= size;
> + buf += size;
> +
> + spin_lock_irqsave (&dev->lock, flags);
> +
> + /* We've disconnected or reset free the req and buffer */
> + if (dev->reset_printer) {
> + printer_req_free (dev->out_ep, current_rx_req);
> + spin_unlock_irqrestore (&dev->lock, flags);
> + spin_unlock (&dev->lock_printer_io);
As a rule, one always returns the number of bytes that have been
read, and THEN flags an EOF later. Of course, you clobbered that
EOF/reset status first thing (which seems incorrect).
> + return -EAGAIN;
> + }
> +
> + if (size < current_rx_bytes) {
> + current_rx_bytes -= size;
> + current_rx_buf += size;
> + } else {
> + list_add (¤t_rx_req->list, &dev->rx_reqs);
> + current_rx_bytes = 0;
> + current_rx_buf = NULL;
> + current_rx_req = NULL;
> + }
> + }
> +
> + dev->current_rx_req = current_rx_req;
> + dev->current_rx_bytes = current_rx_bytes;
> + dev->current_rx_buf = current_rx_buf;
> +
> + spin_unlock_irqrestore (&dev->lock, flags);
> + spin_unlock (&dev->lock_printer_io);
> +
> + DEBUG (dev, "printer_read returned %d bytes\n", (int)bytes_copied);
> +
> + if (bytes_copied) {
> + return bytes_copied;
Style-wise, one prefers to avoid braces if there's only
one statement in the "if" or "else" branch.
> + } else {
> + return -EAGAIN;
> + }
> +}
> +
> +
> +static ssize_t
> +printer_write (struct file *fd, const char __user *buf, size_t len,
> + loff_t *ptr)
> +{
> + ...
Similar comments as on the read side.
> +static unsigned int
> +printer_poll (struct file *fd, poll_table *wait)
> +{
> + struct printer_dev *dev = fd->private_data;
> + unsigned long flags;
> + int status = 0;
> +
> + if (dev == NULL) {
> + printk(KERN_ERR "printer_poll: NULL device pointer\n");
> + return -EIO;
> + }
> +
I'll assume this is correct; I've not looked at poll() stuff
in ages.
> + poll_wait(fd, &dev->rx_wait, wait);
> + poll_wait(fd, &dev->tx_wait, wait);
> +
> + spin_lock_irqsave (&dev->lock, flags);
> + if (likely (!list_empty (&dev->tx_reqs))) {
> + status |= POLLOUT | POLLWRNORM;
> + }
> + if (likely (!list_empty (&dev->rx_buffers))) {
> + status |= POLLIN | POLLRDNORM;
> + }
> + spin_unlock_irqrestore (&dev->lock, flags);
> +
> + return status;
> +}
> +
> +
> +
> +static int
> +printer_ioctl (struct inode *inode, struct file *fd, unsigned int code,
> + unsigned long arg)
> +{
> + struct printer_dev *dev = fd->private_data;
> + unsigned long flags;
> + int status = 0;
> +
> + if (dev == NULL) {
> + printk(KERN_ERR "printer_ioctl: NULL device pointer\n");
can't happen, don't printk ...
> + return -EIO;
> + }
> +
> + printk(KERN_INFO "printer_ioctl: cmd=0x%4.4x, arg=%lu\n", code, arg);
> +
Maybe pr_debug(), but never pr_info() !!!
> + /* handle ioctls */
> +
> + spin_lock_irqsave (&dev->lock, flags);
> +
> + switch (code) {
> + case GADGET_GET_PRINTER_STATUS:
> + status = (int)dev->printer_status;
> + break;
> + case GADGET_SET_PRINTER_STATUS:
> + dev->printer_status = (u8)arg;
> + break;
> + default:
> + /* could not handle ioctl */
> + printk(KERN_ERR "printer_ioctl: ERROR cmd=0x%4.4x"
> + "is not supported\n", code);
This printk isn't appropriate; maybe pr_debug, but it's not
a KERN_ERR that someone needs to sit up and pay attention to.
> + status = -ENOIOCTLCMD;
> + }
> +
> + spin_unlock_irqrestore (&dev->lock, flags);
> +
> + return status;
> +}
> +
> +
> +/* change our operational config. must agree with the code
> + * that returns config descriptors, and altsetting code.
> + */
> +static int
> +printer_set_config (struct printer_dev *dev, unsigned number)
> +{
> + int result = 0;
> + struct usb_gadget *gadget = dev->gadget;
> +
> + if (gadget_is_sa1100 (gadget)
> + && dev->config) {
> + /* tx fifo is full, but we can't clear it...*/
> + INFO (dev, "can't change configurations\n");
> + return -ESPIPE;
Simpler to just refuse to run on sa1100, failing bind() ... in any case,
you don't *know* that fifo is full here!!
> +/* Change our operational Interface. */
> +static int
> +set_interface (struct printer_dev *dev, unsigned number)
> +{
> + int result = 0;
> +
> + if (gadget_is_sa1100 (dev->gadget)
> + && dev->interface < 0) {
> + /* tx fifo is full, but we can't clear it...*/
> + INFO (dev, "can't change interfaces\n");
> + return -ESPIPE;
As above...
> +static void printer_soft_reset (struct printer_dev *dev)
> +{
> + struct usb_request *req;
> +
> + INFO (dev, "Received Printer Reset Request\n");
> +
> + if (usb_ep_disable (dev->in_ep))
> + DEBUG (dev, "Failed to disable USB in_ep\n");
> + if (usb_ep_disable (dev->out_ep))
> + DEBUG (dev, "Failed to disable USB out_ep\n");
> +
> + if (dev->current_rx_req != NULL) {
> + list_add (&dev->current_rx_req->list, &dev->rx_reqs);
> + dev->current_rx_req = NULL;
> + }
> + dev->current_rx_bytes = 0;
> + dev->current_rx_buf = NULL;
> + dev->reset_printer = 1;
> +
> + while (likely (!(list_empty (&dev->rx_buffers)))) {
> + req = container_of (dev->rx_buffers.next, struct usb_request,
> + list);
> + list_del_init (&req->list);
> + list_add (&req->list, &dev->rx_reqs);
> + }
> +
> + while (likely (!(list_empty (&dev->rx_reqs_active)))) {
> + req = container_of (dev->rx_buffers.next, struct usb_request,
> + list);
> + if (usb_ep_dequeue (dev->in_ep, req))
> + DEBUG (dev, "Failed to dequeue in req %p\n", req);
Shouldn't the ep_disable() calls above have aborted all pending
transfers? So if there are still active requests, that's just a
bug to report, not something to clean up...
> +
> + list_del_init (&req->list);
> + list_add (&req->list, &dev->rx_reqs);
> + }
> +
> + while (likely (!(list_empty (&dev->tx_reqs_active)))) {
likewise...
> + req = container_of (dev->tx_reqs_active.next,
> + struct usb_request, list);
> + if (usb_ep_dequeue (dev->out_ep, req))
> + DEBUG (dev, "Failed to dequeue in req %p\n", req);
> +
> + list_del_init (&req->list);
> + list_add (&req->list, &dev->tx_reqs);
> + }
> +
> + if (usb_ep_enable (dev->in_ep, dev->in))
> + DEBUG (dev, "Failed to enable USB in_ep\n");
> + if (usb_ep_enable (dev->out_ep, dev->out))
> + DEBUG (dev, "Failed to enable USB out_ep\n");
> +
> + wake_up_interruptible(&dev->tx_wait);
> + wake_up_interruptible(&dev->tx_flush_wait);
> +}
> +
> +
> +static void
> +printer_unbind (struct usb_gadget *gadget)
> +{
> + struct printer_dev *dev = get_gadget_data (gadget);
> + struct usb_request *req;
> +
> +
> + DEBUG (dev, "%s\n", __FUNCTION__);
> +
> + /* Remove sysfs files */
> + device_destroy(usb_gadget_class, g_printer_devno);
> +
> + /* Remove Character Device */
> + cdev_del(&dev->printer_cdev);
> +
> + /* we've already been disconnected ... no i/o is active */
> + /* Free all memory for this driver. */
> + while (likely (!list_empty (&dev->tx_reqs_active))) {
If no I/O is active, then you don't need this dequeue() loop.
You could report an error if that list is non-empty though.
(And likewise on the RX side.)
> +static void
> +printer_suspend (struct usb_gadget *gadget)
> +{
> + struct printer_dev *dev = get_gadget_data (gadget);
> +
> + DEBUG (dev, "suspend\n");
> + dev->suspended = 1;
> +}
> +
> +static void
> +printer_resume (struct usb_gadget *gadget)
> +{
> + struct printer_dev *dev = get_gadget_data (gadget);
> +
> + DEBUG (dev, "resume\n");
> + dev->suspended = 0;
> +}
If you're not going to do anything with that state, there's no
point in having suspend()/resume() methods ... ;)
> +MODULE_DESCRIPTION (DRIVER_DESC);
> +MODULE_AUTHOR ("Craig Nadler, David Brownell");
I'm not an author of this code; please don't put my name there.
> +MODULE_LICENSE ("GPL");
> +
> +
> +static int __init init (void)
> +{
> + int status;
> +
> + usb_gadget_class = class_create(THIS_MODULE, "usb_gadget");
> + if (IS_ERR(usb_gadget_class)) {
> + status = PTR_ERR(usb_gadget_class);
> + ERROR (dev, "unable to create usb_gadget class %d\n", status);
> + return status;
> + }
Again, you don't need a usb_gadget class. Or for that matter,
one for the printer.
> +
> + status = alloc_chrdev_region (&g_printer_devno, 0, 2,
> + "USB printer gadget");
You don't need a region, you just need a single device ... this
would IMO be better done in the bind() routine, and undone in
the unbind() routine.
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
