2010-02-02 14:00:05

by Alessio Igor Bogani

Subject: [PATCH] USB: ftdi_sio: Remove BKL

Seems to me that BKL don't protect nothing here anymore. So I would want suggest to remove it.

Signed-off-by: Alessio Igor Bogani <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Johan Hovold <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Daniel Mack <[email protected]>
Cc: Andrew Morton <[email protected]>
---
drivers/usb/serial/ftdi_sio.c | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 216f187..592dc5f 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -33,7 +33,6 @@
#include <linux/errno.h>
#include <linux/init.h>
#include <linux/slab.h>
-#include <linux/smp_lock.h>
#include <linux/tty.h>
#include <linux/tty_driver.h>
#include <linux/tty_flip.h>
@@ -1210,7 +1209,6 @@ static int set_serial_info(struct tty_struct *tty,
if (copy_from_user(&new_serial, newinfo, sizeof(new_serial)))
return -EFAULT;

- lock_kernel();
old_priv = *priv;

/* Do error checking and permission checking */
@@ -1218,7 +1216,6 @@ static int set_serial_info(struct tty_struct *tty,
if (!capable(CAP_SYS_ADMIN)) {
if (((new_serial.flags & ~ASYNC_USR_MASK) !=
(priv->flags & ~ASYNC_USR_MASK))) {
- unlock_kernel();
return -EPERM;
}
priv->flags = ((priv->flags & ~ASYNC_USR_MASK) |
@@ -1229,7 +1226,6 @@ static int set_serial_info(struct tty_struct *tty,

if ((new_serial.baud_base != priv->baud_base) &&
(new_serial.baud_base < 9600)) {
- unlock_kernel();
return -EINVAL;
}

@@ -1259,11 +1255,8 @@ check_and_exit:
(priv->flags & ASYNC_SPD_MASK)) ||
(((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) &&
(old_priv.custom_divisor != priv->custom_divisor))) {
- unlock_kernel();
change_speed(tty, port);
}
- else
- unlock_kernel();
return 0;

} /* set_serial_info */
--
1.6.3.3

2010-02-02 14:12:49

by Alan

Subject: Re: [PATCH] USB: ftdi_sio: Remove BKL

On Tue, 2 Feb 2010 14:58:14 +0100
Alessio Igor Bogani <[email protected]> wrote:

> Seems to me that BKL don't protect nothing here anymore. So I would want suggest to remove it.

What now stops parallel calls to the config ioctl making a nasty mess ?

The current locking is broken certainly, but removing it is not the fix
(the speed locking is broken too on that path I notice)

Alan

2010-02-02 15:18:49

by Alessio Igor Bogani

Subject: [PATCH] USB: ftdi_sio: Replace BKL with a mutex

As Alan Cox have pinpointed the driver still required protection against
parallels calls to the config ioctl(). If lock is still necessary the use of
BKL is abused here. So replace BKL with a more convenient mutex.

Signed-off-by: Alessio Igor Bogani <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Johan Hovold <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Daniel Mack <[email protected]>
Cc: Andrew Morton <[email protected]>
---
drivers/usb/serial/ftdi_sio.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 216f187..23d3de7 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -33,12 +33,12 @@
#include <linux/errno.h>
#include <linux/init.h>
#include <linux/slab.h>
-#include <linux/smp_lock.h>
#include <linux/tty.h>
#include <linux/tty_driver.h>
#include <linux/tty_flip.h>
#include <linux/module.h>
#include <linux/spinlock.h>
+#include <linux/mutex.h>
#include <linux/uaccess.h>
#include <linux/usb.h>
#include <linux/serial.h>
@@ -92,6 +92,7 @@ struct ftdi_private {
unsigned long tx_outstanding_bytes;
unsigned long tx_outstanding_urbs;
unsigned short max_packet_size;
+ struct mutex cfg_lock; /* Avoid mess by parallel calls of config ioctl() */
};

/* struct ftdi_sio_quirk is used by devices requiring special attention. */
@@ -1210,7 +1211,7 @@ static int set_serial_info(struct tty_struct *tty,
if (copy_from_user(&new_serial, newinfo, sizeof(new_serial)))
return -EFAULT;

- lock_kernel();
+ mutex_lock(&priv->cfg_lock);
old_priv = *priv;

/* Do error checking and permission checking */
@@ -1218,7 +1219,7 @@ static int set_serial_info(struct tty_struct *tty,
if (!capable(CAP_SYS_ADMIN)) {
if (((new_serial.flags & ~ASYNC_USR_MASK) !=
(priv->flags & ~ASYNC_USR_MASK))) {
- unlock_kernel();
+ mutex_unlock(&priv->cfg_lock);
return -EPERM;
}
priv->flags = ((priv->flags & ~ASYNC_USR_MASK) |
@@ -1229,7 +1230,7 @@ static int set_serial_info(struct tty_struct *tty,

if ((new_serial.baud_base != priv->baud_base) &&
(new_serial.baud_base < 9600)) {
- unlock_kernel();
+ mutex_unlock(&priv->cfg_lock);
return -EINVAL;
}

@@ -1259,11 +1260,11 @@ check_and_exit:
(priv->flags & ASYNC_SPD_MASK)) ||
(((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) &&
(old_priv.custom_divisor != priv->custom_divisor))) {
- unlock_kernel();
+ mutex_unlock(&priv->cfg_lock);
change_speed(tty, port);
}
else
- unlock_kernel();
+ mutex_unlock(&priv->cfg_lock);
return 0;

} /* set_serial_info */
@@ -1532,6 +1533,7 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)

kref_init(&priv->kref);
spin_lock_init(&priv->tx_lock);
+ mutex_init(&priv->cfg_lock);
init_waitqueue_head(&priv->delta_msr_wait);
/* This will push the characters through immediately rather
than queue a task to deliver them */
--
1.6.3.3