From: Stefani Seibold <[email protected]>
This is a fix for the USB skeleton driver to bring it in shape.
- Code cleanup
- Eliminated dead code
- Handle a non blocking read without blocking
- Fix a small race condition
- Introduced fsync
- Single user mode
The patch is against linux 3.5.0 commit eea5b5510fc5545d15b69da8e485a7424ae388cf
Hope this helps
Signed-off-by: Stefani Seibold <[email protected]>
From: Stefani Seibold <[email protected]>
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 0616f23..efda3a5 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -114,10 +114,11 @@ static int skel_open(struct inode *inode, struct file *file)
retval = usb_autopm_get_interface(interface);
if (retval)
- goto out_err;
+ goto exit2;
/* save our object in the file's private structure */
file->private_data = dev;
+exit2:
mutex_unlock(&dev->io_mutex);
exit:
--
1.7.8.6
From: Stefani Seibold <[email protected]>
- Check for disconnect
- Fix nameing of the return value
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index e4cb88a..dc95129 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -136,21 +136,25 @@ static void skel_draw_down(struct usb_skel *dev)
static int skel_flush(struct file *file, fl_owner_t id)
{
struct usb_skel *dev = file->private_data;
- int res;
+ int retval;
/* wait for io to stop */
mutex_lock(&dev->io_mutex);
+ if (!dev->interface) { /* disconnect() was called */
+ retval = -ENODEV;
+ goto exit;
+ }
skel_draw_down(dev);
/* read out errors, leave subsequent opens a clean slate */
spin_lock_irq(&dev->err_lock);
- res = dev->errors ? (dev->errors == -EPIPE ? -EPIPE : -EIO) : 0;
+ retval = dev->errors ? (dev->errors == -EPIPE ? -EPIPE : -EIO) : 0;
dev->errors = 0;
spin_unlock_irq(&dev->err_lock);
-
+exit:
mutex_unlock(&dev->io_mutex);
- return res;
+ return retval;
}
static void skel_read_bulk_callback(struct urb *urb)
--
1.7.8.6
From: Stefani Seibold <[email protected]>
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 12 +++---------
1 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 7fc9621..2ca2530 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -82,17 +82,11 @@ static int skel_open(struct inode *inode, struct file *file)
{
struct usb_skel *dev;
struct usb_interface *interface;
- int subminor;
- int retval = 0;
-
- subminor = iminor(inode);
+ int retval;
- interface = usb_find_interface(&skel_driver, subminor);
- if (!interface) {
- pr_err("%s - error, can't find device for minor %d\n",
- __func__, subminor);
+ interface = usb_find_interface(&skel_driver, iminor(inode));
+ if (!interface)
return -ENODEV;
- }
dev = usb_get_intfdata(interface);
if (!dev)
--
1.7.8.6
From: Stefani Seibold <[email protected]>
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 2ca2530..e4cb88a 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -225,9 +225,14 @@ static ssize_t skel_read(struct file *file, char *buffer, size_t count,
return 0;
/* no concurrent readers */
- retval = mutex_lock_interruptible(&dev->io_mutex);
- if (retval < 0)
- return retval;
+ if (file->f_flags & O_NONBLOCK) {
+ if (!mutex_trylock(&dev->io_mutex))
+ return -EAGAIN;
+ } else {
+ retval = mutex_lock_interruptible(&dev->io_mutex);
+ if (retval < 0)
+ return retval;
+ }
if (!dev->interface) { /* disconnect() was called */
retval = -ENODEV;
--
1.7.8.6
From: Stefani Seibold <[email protected]>
Place the skel_draw_down() function at the rigth place to remove
the forward declaration.
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 21 ++++++++++-----------
1 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 92cdfca..7fc9621 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -67,7 +67,6 @@ struct usb_skel {
#define to_skel_dev(d) container_of(d, struct usb_skel, kref)
static struct usb_driver skel_driver;
-static void skel_draw_down(struct usb_skel *dev);
static void skel_delete(struct kref *kref)
{
@@ -130,6 +129,16 @@ static int skel_release(struct inode *inode, struct file *file)
return 0;
}
+static void skel_draw_down(struct usb_skel *dev)
+{
+ int time;
+
+ time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
+ if (!time)
+ usb_kill_anchored_urbs(&dev->submitted);
+ usb_kill_urb(dev->bulk_in_urb);
+}
+
static int skel_flush(struct file *file, fl_owner_t id)
{
struct usb_skel *dev = file->private_data;
@@ -586,16 +595,6 @@ static void skel_disconnect(struct usb_interface *interface)
dev_info(&interface->dev, "USB Skeleton #%d now disconnected", minor);
}
-static void skel_draw_down(struct usb_skel *dev)
-{
- int time;
-
- time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
- if (!time)
- usb_kill_anchored_urbs(&dev->submitted);
- usb_kill_urb(dev->bulk_in_urb);
-}
-
static int skel_suspend(struct usb_interface *intf, pm_message_t message)
{
struct usb_skel *dev = usb_get_intfdata(intf);
--
1.7.8.6
From: Stefani Seibold <[email protected]>
All code which use processed_urb can be eliminated.
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 12 ------------
1 files changed, 0 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 7385aa8..92cdfca 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -59,7 +59,6 @@ struct usb_skel {
__u8 bulk_out_endpointAddr; /* the address of the bulk out endpoint */
int errors; /* the last request tanked */
bool ongoing_read; /* a read is going on */
- bool processed_urb; /* indicates we haven't processed the urb */
spinlock_t err_lock; /* lock for errors */
struct kref kref;
struct mutex io_mutex; /* synchronize I/O with disconnect */
@@ -256,17 +255,6 @@ retry:
* we must finish now
*/
dev->bulk_in_copied = 0;
- dev->processed_urb = true;
- }
-
- if (!dev->processed_urb) {
- /*
- * the URB hasn't been processed
- * do it now
- */
- wait_for_completion(&dev->bulk_in_completion);
- dev->bulk_in_copied = 0;
- dev->processed_urb = true;
}
/* errors must be reported */
--
1.7.8.6
From: Stefani Seibold <[email protected]>
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index dc95129..61f4ac8 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -133,7 +133,7 @@ static void skel_draw_down(struct usb_skel *dev)
usb_kill_urb(dev->bulk_in_urb);
}
-static int skel_flush(struct file *file, fl_owner_t id)
+static int skel_fsync(struct file *file, loff_t start, loff_t end, int datasync)
{
struct usb_skel *dev = file->private_data;
int retval;
@@ -157,6 +157,11 @@ exit:
return retval;
}
+static int skel_flush(struct file *file, fl_owner_t id)
+{
+ return skel_fsync(file, 0, LLONG_MAX, 0);
+}
+
static void skel_read_bulk_callback(struct urb *urb)
{
struct usb_skel *dev;
@@ -470,6 +475,7 @@ static const struct file_operations skel_fops = {
.write = skel_write,
.open = skel_open,
.release = skel_release,
+ .fsync = skel_fsync,
.flush = skel_flush,
.llseek = noop_llseek,
};
--
1.7.8.6
From: Stefani Seibold <[email protected]>
The io_mutex must not acquired since a disconnect waits in usb_deregister_dev()
due the already locked minor_rwsem in the usb_open() function
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 61f4ac8..6482acb 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -95,11 +95,11 @@ static int skel_open(struct inode *inode, struct file *file)
/* increment our usage count for the device */
kref_get(&dev->kref);
- /* lock the device to allow correctly handling errors
- * in resumption */
- mutex_lock(&dev->io_mutex);
+ /*
+ * must be not locked since disconnect waits in usb_deregister_dev()
+ * due the already locked minor_rwsem in the usb_open() function
+ */
retval = usb_autopm_get_interface(interface);
- mutex_unlock(&dev->io_mutex);
/* save our object in the file's private structure */
if (!retval)
--
1.7.8.6
From: Stefani Seibold <[email protected]>
- consistent nameing
- more compact style
- remove unneeded code
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 119 +++++++++++++++++---------------------------
1 files changed, 46 insertions(+), 73 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index efda3a5..7385aa8 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -22,7 +22,6 @@
#include <linux/usb.h>
#include <linux/mutex.h>
-
/* Define these values to match your devices */
#define USB_SKEL_VENDOR_ID 0xfff0
#define USB_SKEL_PRODUCT_ID 0xfff0
@@ -34,7 +33,6 @@ static const struct usb_device_id skel_table[] = {
};
MODULE_DEVICE_TABLE(usb, skel_table);
-
/* Get a minor range for your devices from the usb maintainer */
#define USB_SKEL_MINOR_BASE 192
@@ -95,15 +93,12 @@ static int skel_open(struct inode *inode, struct file *file)
if (!interface) {
pr_err("%s - error, can't find device for minor %d\n",
__func__, subminor);
- retval = -ENODEV;
- goto exit;
+ return -ENODEV;
}
dev = usb_get_intfdata(interface);
- if (!dev) {
- retval = -ENODEV;
- goto exit;
- }
+ if (!dev)
+ return -ENODEV;
/* increment our usage count for the device */
kref_get(&dev->kref);
@@ -111,27 +106,19 @@ static int skel_open(struct inode *inode, struct file *file)
/* lock the device to allow correctly handling errors
* in resumption */
mutex_lock(&dev->io_mutex);
-
retval = usb_autopm_get_interface(interface);
- if (retval)
- goto exit2;
+ mutex_unlock(&dev->io_mutex);
/* save our object in the file's private structure */
- file->private_data = dev;
-exit2:
- mutex_unlock(&dev->io_mutex);
+ if (!retval)
+ file->private_data = dev;
-exit:
return retval;
}
static int skel_release(struct inode *inode, struct file *file)
{
- struct usb_skel *dev;
-
- dev = file->private_data;
- if (dev == NULL)
- return -ENODEV;
+ struct usb_skel *dev = file->private_data;
/* allow the device to be autosuspended */
mutex_lock(&dev->io_mutex);
@@ -146,13 +133,9 @@ static int skel_release(struct inode *inode, struct file *file)
static int skel_flush(struct file *file, fl_owner_t id)
{
- struct usb_skel *dev;
+ struct usb_skel *dev = file->private_data;
int res;
- dev = file->private_data;
- if (dev == NULL)
- return -ENODEV;
-
/* wait for io to stop */
mutex_lock(&dev->io_mutex);
skel_draw_down(dev);
@@ -188,7 +171,7 @@ static void skel_read_bulk_callback(struct urb *urb)
} else {
dev->bulk_in_filled = urb->actual_length;
}
- dev->ongoing_read = 0;
+ dev->ongoing_read = false;
spin_unlock(&dev->err_lock);
complete(&dev->bulk_in_completion);
@@ -196,7 +179,7 @@ static void skel_read_bulk_callback(struct urb *urb)
static int skel_do_read_io(struct usb_skel *dev, size_t count)
{
- int rv;
+ int retval;
/* prepare a read */
usb_fill_bulk_urb(dev->bulk_in_urb,
@@ -209,45 +192,43 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
dev);
/* tell everybody to leave the URB alone */
spin_lock_irq(&dev->err_lock);
- dev->ongoing_read = 1;
+ dev->ongoing_read = true;
spin_unlock_irq(&dev->err_lock);
/* do it */
- rv = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
- if (rv < 0) {
+ retval = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
+ if (retval < 0) {
dev_err(&dev->interface->dev,
"%s - failed submitting read urb, error %d\n",
- __func__, rv);
+ __func__, retval);
dev->bulk_in_filled = 0;
- rv = (rv == -ENOMEM) ? rv : -EIO;
+ retval = (retval == -ENOMEM) ? retval : -EIO;
spin_lock_irq(&dev->err_lock);
- dev->ongoing_read = 0;
+ dev->ongoing_read = false;
spin_unlock_irq(&dev->err_lock);
}
- return rv;
+ return retval;
}
static ssize_t skel_read(struct file *file, char *buffer, size_t count,
loff_t *ppos)
{
- struct usb_skel *dev;
- int rv;
+ struct usb_skel *dev = file->private_data;
+ int retval;
bool ongoing_io;
- dev = file->private_data;
-
/* if we cannot read at all, return EOF */
if (!dev->bulk_in_urb || !count)
return 0;
/* no concurrent readers */
- rv = mutex_lock_interruptible(&dev->io_mutex);
- if (rv < 0)
- return rv;
+ retval = mutex_lock_interruptible(&dev->io_mutex);
+ if (retval < 0)
+ return retval;
if (!dev->interface) { /* disconnect() was called */
- rv = -ENODEV;
+ retval = -ENODEV;
goto exit;
}
@@ -260,22 +241,22 @@ retry:
if (ongoing_io) {
/* nonblocking IO shall not wait */
if (file->f_flags & O_NONBLOCK) {
- rv = -EAGAIN;
+ retval = -EAGAIN;
goto exit;
}
/*
* IO may take forever
* hence wait in an interruptible state
*/
- rv = wait_for_completion_interruptible(&dev->bulk_in_completion);
- if (rv < 0)
+ retval = wait_for_completion_interruptible(&dev->bulk_in_completion);
+ if (retval < 0)
goto exit;
/*
* by waiting we also semiprocessed the urb
* we must finish now
*/
dev->bulk_in_copied = 0;
- dev->processed_urb = 1;
+ dev->processed_urb = true;
}
if (!dev->processed_urb) {
@@ -285,16 +266,16 @@ retry:
*/
wait_for_completion(&dev->bulk_in_completion);
dev->bulk_in_copied = 0;
- dev->processed_urb = 1;
+ dev->processed_urb = true;
}
/* errors must be reported */
- rv = dev->errors;
- if (rv < 0) {
+ retval = dev->errors;
+ if (retval < 0) {
/* any error is reported once */
dev->errors = 0;
/* to preserve notifications about reset */
- rv = (rv == -EPIPE) ? rv : -EIO;
+ retval = (retval == -EPIPE) ? retval : -EIO;
/* no data to deliver */
dev->bulk_in_filled = 0;
/* report it */
@@ -316,8 +297,8 @@ retry:
* all data has been used
* actual IO needs to be done
*/
- rv = skel_do_read_io(dev, count);
- if (rv < 0)
+ retval = skel_do_read_io(dev, count);
+ if (retval < 0)
goto exit;
else
goto retry;
@@ -330,9 +311,9 @@ retry:
if (copy_to_user(buffer,
dev->bulk_in_buffer + dev->bulk_in_copied,
chunk))
- rv = -EFAULT;
+ retval = -EFAULT;
else
- rv = chunk;
+ retval = chunk;
dev->bulk_in_copied += chunk;
@@ -344,16 +325,16 @@ retry:
skel_do_read_io(dev, count - chunk);
} else {
/* no data in the buffer */
- rv = skel_do_read_io(dev, count);
- if (rv < 0)
+ retval = skel_do_read_io(dev, count);
+ if (retval < 0)
goto exit;
else if (!(file->f_flags & O_NONBLOCK))
goto retry;
- rv = -EAGAIN;
+ retval = -EAGAIN;
}
exit:
mutex_unlock(&dev->io_mutex);
- return rv;
+ return retval;
}
static void skel_write_bulk_callback(struct urb *urb)
@@ -385,32 +366,26 @@ static void skel_write_bulk_callback(struct urb *urb)
static ssize_t skel_write(struct file *file, const char *user_buffer,
size_t count, loff_t *ppos)
{
- struct usb_skel *dev;
+ struct usb_skel *dev = file->private_data;
int retval = 0;
struct urb *urb = NULL;
char *buf = NULL;
size_t writesize = min(count, (size_t)MAX_TRANSFER);
- dev = file->private_data;
-
/* verify that we actually have some data to write */
- if (count == 0)
- goto exit;
+ if (!count)
+ return 0;
/*
* limit the number of URBs in flight to stop a user from using up all
* RAM
*/
if (!(file->f_flags & O_NONBLOCK)) {
- if (down_interruptible(&dev->limit_sem)) {
- retval = -ERESTARTSYS;
- goto exit;
- }
+ if (down_interruptible(&dev->limit_sem))
+ return -ERESTARTSYS;
} else {
- if (down_trylock(&dev->limit_sem)) {
- retval = -EAGAIN;
- goto exit;
- }
+ if (down_trylock(&dev->limit_sem))
+ return -EAGAIN;
}
spin_lock_irq(&dev->err_lock);
@@ -475,7 +450,6 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
*/
usb_free_urb(urb);
-
return writesize;
error_unanchor:
@@ -487,7 +461,6 @@ error:
}
up(&dev->limit_sem);
-exit:
return retval;
}
--
1.7.8.6
From: Stefani Seibold <[email protected]>
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 456c9a8..1e36782 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -1,7 +1,8 @@
/*
- * USB Skeleton driver - 2.2
+ * USB Skeleton driver - 2.3
*
* Copyright (C) 2001-2004 Greg Kroah-Hartman ([email protected])
+ * Fixes and cleanup by Stefani Seibold ([email protected])
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
--
1.7.8.6
From: Stefani Seibold <[email protected]>
Most USB devices can only used in a single usage mode. This patch prevents a
reopening on an already opened device.
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 551fb51..456c9a8 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -59,6 +59,7 @@ struct usb_skel {
__u8 bulk_out_endpointAddr; /* the address of the bulk out endpoint */
int errors; /* the last request tanked */
bool ongoing_read; /* a read is going on */
+ bool in_use; /* in use flag */
spinlock_t err_lock; /* lock for errors */
struct kref kref;
struct mutex io_mutex; /* synchronize I/O with disconnect */
@@ -92,17 +93,28 @@ static int skel_open(struct inode *inode, struct file *file)
if (!dev)
return -ENODEV;
+ mutex_lock(&dev->io_mutex);
+ if (dev->in_use) {
+ mutex_unlock(&dev->io_mutex);
+ return -EBUSY;
+ }
+
/*
* must be not locked since a disconnect waits in usb_deregister_dev()
* due the already locked minor_rwsem in the usb_open() function
*/
retval = usb_autopm_get_interface(interface);
- if (!retval)
+ if (!retval) {
+ mutex_unlock(&dev->io_mutex);
return retval;
+ }
/* increment our usage count for the device */
kref_get(&dev->kref);
+ dev->in_use = true;
+ mutex_unlock(&dev->io_mutex);
+
/* save our object in the file's private structure */
file->private_data = dev;
@@ -117,6 +129,7 @@ static int skel_release(struct inode *inode, struct file *file)
mutex_lock(&dev->io_mutex);
if (dev->interface)
usb_autopm_put_interface(dev->interface);
+ dev->in_use = false;
mutex_unlock(&dev->io_mutex);
/* decrement the count on our device */
@@ -517,6 +530,7 @@ static int skel_probe(struct usb_interface *interface,
dev->udev = usb_get_dev(interface_to_usbdev(interface));
dev->interface = interface;
+ dev->in_use = false;
/* set up the endpoint information */
/* use only the first bulk-in and bulk-out endpoints */
--
1.7.8.6
From: Stefani Seibold <[email protected]>
Access to members of dev->interface without holding the io_mutex lock
could result in a NULL pointer access, since disconnect will do this
as a marker for disconnect.
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 6482acb..0a1ab0b 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -96,7 +96,7 @@ static int skel_open(struct inode *inode, struct file *file)
kref_get(&dev->kref);
/*
- * must be not locked since disconnect waits in usb_deregister_dev()
+ * must be not locked since a disconnect waits in usb_deregister_dev()
* due the already locked minor_rwsem in the usb_open() function
*/
retval = usb_autopm_get_interface(interface);
@@ -441,13 +441,14 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
/* send the data out the bulk port */
retval = usb_submit_urb(urb, GFP_KERNEL);
- mutex_unlock(&dev->io_mutex);
if (retval) {
dev_err(&dev->interface->dev,
"%s - failed submitting write urb, error %d\n",
__func__, retval);
+ mutex_unlock(&dev->io_mutex);
goto error_unanchor;
}
+ mutex_unlock(&dev->io_mutex);
/*
* release our reference to this urb, the USB core will eventually free
--
1.7.8.6
From: Stefani Seibold <[email protected]>
Increment the kref at last, so we need no extra error path for decrement
them in a case of error..
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 0a1ab0b..551fb51 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -92,18 +92,19 @@ static int skel_open(struct inode *inode, struct file *file)
if (!dev)
return -ENODEV;
- /* increment our usage count for the device */
- kref_get(&dev->kref);
-
/*
* must be not locked since a disconnect waits in usb_deregister_dev()
* due the already locked minor_rwsem in the usb_open() function
*/
retval = usb_autopm_get_interface(interface);
+ if (!retval)
+ return retval;
+
+ /* increment our usage count for the device */
+ kref_get(&dev->kref);
/* save our object in the file's private structure */
- if (!retval)
- file->private_data = dev;
+ file->private_data = dev;
return retval;
}
--
1.7.8.6
[email protected] writes:
> @@ -95,15 +93,12 @@ static int skel_open(struct inode *inode, struct file *file)
> if (!interface) {
> pr_err("%s - error, can't find device for minor %d\n",
> __func__, subminor);
> - retval = -ENODEV;
> - goto exit;
> + return -ENODEV;
> }
This may save you a line, but that line was there for a reason...
Using a common exit path for errors makes it easier to keep unlocking,
deallocation and other cleanups correct. Although you *can* do that
change now, you introduce future bugs here. Someone adding a lock
before this will now have to go through all the error paths to ensure
that they unlock before exiting.
See "Chapter 7: Centralized exiting of functions" in
Documentation/CodingStyle.
Most of this patch consists of this kind of bogus changes. I won't
comment on the rest of them.
Focus on creating a *good* example. Compacting the code is not
necessarily improving the code...
> /* verify that we actually have some data to write */
> - if (count == 0)
> - goto exit;
> + if (!count)
> + return 0;
zero-testing is discussed over and over again, and is a matter of
taste. But I fail to see how changing it can be part of a cleanup. It
just changes the flavour to suit another taste. What's the reason for
doing that?
Bjørn
Another bad example you do not want to teach anyone. Remove the version
number instead.
Bjørn
Am Donnerstag, den 07.06.2012, 11:06 +0200 schrieb Bj?rn Mork:
> [email protected] writes:
>
> > @@ -95,15 +93,12 @@ static int skel_open(struct inode *inode, struct file *file)
> > if (!interface) {
> > pr_err("%s - error, can't find device for minor %d\n",
> > __func__, subminor);
> > - retval = -ENODEV;
> > - goto exit;
> > + return -ENODEV;
> > }
>
>
> This may save you a line, but that line was there for a reason...
>
> Using a common exit path for errors makes it easier to keep unlocking,
> deallocation and other cleanups correct. Although you *can* do that
> change now, you introduce future bugs here. Someone adding a lock
> before this will now have to go through all the error paths to ensure
> that they unlock before exiting.
>
> See "Chapter 7: Centralized exiting of functions" in
> Documentation/CodingStyle.
>
If it is necessary... I get alway ten different complains from six
developers. Developer A says do it in this way, developer B do it in the
other way.
> Focus on creating a *good* example. Compacting the code is not
> necessarily improving the code...
>
Compacting improves since it will make the code more readable.
>
>
> > /* verify that we actually have some data to write */
> > - if (count == 0)
> > - goto exit;
> > + if (!count)
> > + return 0;
>
> zero-testing is discussed over and over again, and is a matter of
> taste. But I fail to see how changing it can be part of a cleanup. It
> just changes the flavour to suit another taste. What's the reason for
> doing that?
>
Consistency - there are a lot places in the driver skeleton handling
this in the same way.
Stefani Seibold <[email protected]> writes:
> If it is necessary...
So, why is it necessary for you to change this code *from* the style
recommended by CodingStyle and LDD3?
Quoting from LDD3:
"Error recovery is sometimes best handled with the goto statement. We
normally hate to use goto, but in our opinion, this is one situation
where it is useful. Careful use of goto in error situations can
eliminate a great deal of complicated, highly-indented, "structured"
logic. Thus, in the kernel, goto is often used as shown here to deal
with errors."
> Compacting improves since it will make the code more readable.
No, it does not. As pointed out, instead of having to follow a single
exit path from each function, your changes makes it necessary to follow
n exit paths. That does not make the code more readable, and it
contradicts both CodingStyle and LDD3.
Note that I am not stating in any way that those documents contain
absolute truths and that you cannot write your own driver the way you
like. I do however find it extremely strange that you insist on
changing a coding example to be inconsistent with those documents.
Regardless of whether you agree with them or not, you must see that such
inconsistent guidelines will be a problem for anyone trying to use this
code for learning?
Bjørn
Am Donnerstag, 7. Juni 2012, 10:20:33 schrieb [email protected]:
> From: Stefani Seibold <[email protected]>
>
> All code which use processed_urb can be eliminated.
If you eliminate it, how do you balance the completion?
Regards
Oliver
>
> Signed-off-by: Stefani Seibold <[email protected]>
> ---
> drivers/usb/usb-skeleton.c | 12 ------------
> 1 files changed, 0 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index 7385aa8..92cdfca 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -59,7 +59,6 @@ struct usb_skel {
> __u8 bulk_out_endpointAddr; /* the address of the bulk out endpoint */
> int errors; /* the last request tanked */
> bool ongoing_read; /* a read is going on */
> - bool processed_urb; /* indicates we haven't processed the urb */
> spinlock_t err_lock; /* lock for errors */
> struct kref kref;
> struct mutex io_mutex; /* synchronize I/O with disconnect */
> @@ -256,17 +255,6 @@ retry:
> * we must finish now
> */
> dev->bulk_in_copied = 0;
> - dev->processed_urb = true;
> - }
> -
> - if (!dev->processed_urb) {
> - /*
> - * the URB hasn't been processed
> - * do it now
> - */
> - wait_for_completion(&dev->bulk_in_completion);
> - dev->bulk_in_copied = 0;
> - dev->processed_urb = true;
> }
>
> /* errors must be reported */
> --
> 1.7.8.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Am Donnerstag, den 07.06.2012, 17:04 +0200 schrieb Oliver Neukum:
> >
> > All code which use processed_urb can be eliminated.
>
> If you eliminate it, how do you balance the completion?
>
> Regards
> Oliver
This was dead code, the process_urb was never used, there was no code
which set the value to false.
The completion is handled in the skel_read() function by the "if
(ongoing_io)" path.
Greetings,
Steffi
On Thu, Jun 07, 2012 at 11:09:52AM +0200, Bj?rn Mork wrote:
> Another bad example you do not want to teach anyone. Remove the version
> number instead.
I agree, the version number should just be deleted.
thanks,
greg k-h
On Thu, Jun 07, 2012 at 10:20:32AM +0200, [email protected] wrote:
> From: Stefani Seibold <[email protected]>
>
> - consistent nameing
> - more compact style
> - remove unneeded code
>
> Signed-off-by: Stefani Seibold <[email protected]>
> ---
> drivers/usb/usb-skeleton.c | 119 +++++++++++++++++---------------------------
> 1 files changed, 46 insertions(+), 73 deletions(-)
Your subject: needs to be fixed for all of these, as it does not
describe what part of the kernel is being modified.
A better example would be to use a prefix of:
Subject: [PATCH XX/13] USB: skeleton:
for all of these patches, so the shortlog entry is correct.
Care to redo this series, based on the feedback you've gotten, and
resend?
thanks,
greg k-h
On Thu, Jun 07, 2012 at 12:49:49PM +0200, Bj?rn Mork wrote:
> Stefani Seibold <[email protected]> writes:
>
> > If it is necessary...
>
> So, why is it necessary for you to change this code *from* the style
> recommended by CodingStyle and LDD3?
>
> Quoting from LDD3:
>
> "Error recovery is sometimes best handled with the goto statement. We
> normally hate to use goto, but in our opinion, this is one situation
> where it is useful. Careful use of goto in error situations can
> eliminate a great deal of complicated, highly-indented, "structured"
> logic. Thus, in the kernel, goto is often used as shown here to deal
> with errors."
>
> > Compacting improves since it will make the code more readable.
>
> No, it does not. As pointed out, instead of having to follow a single
> exit path from each function, your changes makes it necessary to follow
> n exit paths. That does not make the code more readable, and it
> contradicts both CodingStyle and LDD3.
>
> Note that I am not stating in any way that those documents contain
> absolute truths and that you cannot write your own driver the way you
> like. I do however find it extremely strange that you insist on
> changing a coding example to be inconsistent with those documents.
> Regardless of whether you agree with them or not, you must see that such
> inconsistent guidelines will be a problem for anyone trying to use this
> code for learning?
I totally agree, the original style should be preserved.
thanks,
greg k-h
On Thu, Jun 07, 2012 at 10:20:31AM +0200, [email protected] wrote:
> From: Stefani Seibold <[email protected]>
>
> Signed-off-by: Stefani Seibold <[email protected]>
> ---
> drivers/usb/usb-skeleton.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index 0616f23..efda3a5 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -114,10 +114,11 @@ static int skel_open(struct inode *inode, struct file *file)
>
> retval = usb_autopm_get_interface(interface);
> if (retval)
> - goto out_err;
> + goto exit2;
>
> /* save our object in the file's private structure */
> file->private_data = dev;
> +exit2:
> mutex_unlock(&dev->io_mutex);
That's not so much as a "wrong label" as, "fix unlock bug on error
path", right?
thanks,
greg k-h
On Thu, Jun 07, 2012 at 10:20:42AM +0200, [email protected] wrote:
> From: Stefani Seibold <[email protected]>
>
> Most USB devices can only used in a single usage mode. This patch prevents a
> reopening on an already opened device.
That's not true at all, most devices work just fine with multiple opens,
it just depends on the type of device you have.
thanks,
greg k-h
Am Dienstag, den 12.06.2012, 18:02 -0700 schrieb Greg KH:
> On Thu, Jun 07, 2012 at 10:20:32AM +0200, [email protected] wrote:
> > From: Stefani Seibold <[email protected]>
> >
> > - consistent nameing
> > - more compact style
> > - remove unneeded code
> >
> > Signed-off-by: Stefani Seibold <[email protected]>
> > ---
> > drivers/usb/usb-skeleton.c | 119 +++++++++++++++++---------------------------
> > 1 files changed, 46 insertions(+), 73 deletions(-)
>
> Your subject: needs to be fixed for all of these, as it does not
> describe what part of the kernel is being modified.
>
> A better example would be to use a prefix of:
> Subject: [PATCH XX/13] USB: skeleton:
> for all of these patches, so the shortlog entry is correct.
>
> Care to redo this series, based on the feedback you've gotten, and
> resend?
>
Yes, but i till take some time due my move to a new house.
Greetings,
Stefani