[patch 00/28] BKL removal queued patches

Thomas Gleixner · External Since: May 14, 2006 Posts: 944

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx.RemoveThis@linutronix.de>
Cc: Martin Schwidefsky <schwidefsky.RemoveThis@de.ibm.com>
---
arch/s390/crypto/prng.c | 2 --
1 file changed, 2 deletions(-)

Index: linux-2.6-tip/arch/s390/crypto/prng.c
===================================================================
--- linux-2.6-tip.orig/arch/s390/crypto/prng.c
+++ linux-2.6-tip/arch/s390/crypto/prng.c
@@ -6,7 +6,6 @@
#include <linux/fs.h>
#include <linux/init.h>
#include <linux/kernel.h>
-#include <linux/smp_lock.h>
#include <linux/miscdevice.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
@@ -49,7 +48,6 @@ static unsigned char parm_block[32] = {

static int prng_open(struct inode *inode, struct file *file)
{
- cycle_kernel_lock();
return nonseekable_open(inode, file);
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Thomas Gleixner · External Since: May 14, 2006 Posts: 944

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx.TakeThisOut@linutronix.de>
Cc: Geert Uytterhoeven <geert.TakeThisOut@linux-m68k.org>
Cc: Dmitry Torokhov <dtor.TakeThisOut@mail.ru>
---
drivers/input/misc/hp_sdc_rtc.c | 2 --
1 file changed, 2 deletions(-)

Index: linux-2.6-tip/drivers/input/misc/hp_sdc_rtc.c
===================================================================
--- linux-2.6-tip.orig/drivers/input/misc/hp_sdc_rtc.c
+++ linux-2.6-tip/drivers/input/misc/hp_sdc_rtc.c
@@ -35,7 +35,6 @@

#include <linux/hp_sdc.h>
#include <linux/errno.h>
-#include <linux/smp_lock.h>
#include <linux/types.h>
#include <linux/init.h>
#include <linux/module.h>
@@ -409,7 +408,6 @@ static unsigned int hp_sdc_rtc_poll(stru

static int hp_sdc_rtc_open(struct inode *inode, struct file *file)
{
- cycle_kernel_lock();
return 0;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Thomas Gleixner · External Since: May 14, 2006 Posts: 944

The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
the BKL pushdown and it still uses the locked ioctl.

The BKL serialization in this driver is more than obscure and
definitely does not cover all possible corner cases. Protect the
access to the hardware with a local mutex and get rid of BKL and
locked ioctl.

Signed-off-by: Thomas Gleixner <tglx RemoveThis @linutronix.de>
Cc: Benjamin Herrenschmidt <benh RemoveThis @kernel.crashing.org>
Cc: linuxppc-dev RemoveThis @ozlabs.org
---
drivers/macintosh/ans-lcd.c | 45 +++++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 17 deletions(-)

Index: linux-2.6-tip/drivers/macintosh/ans-lcd.c
===================================================================
--- linux-2.6-tip.orig/drivers/macintosh/ans-lcd.c
+++ linux-2.6-tip/drivers/macintosh/ans-lcd.c
@@ -3,7 +3,6 @@
*/

#include <linux/types.h>
-#include <linux/smp_lock.h>
#include <linux/errno.h>
#include <linux/kernel.h>
#include <linux/miscdevice.h>
@@ -26,6 +25,7 @@
static unsigned long anslcd_short_delay = 80;
static unsigned long anslcd_long_delay = 3280;
static volatile unsigned char __iomem *anslcd_ptr;
+static DEFINE_MUTEX(anslcd_mutex);

#undef DEBUG

@@ -65,26 +65,31 @@ anslcd_write( struct file * file, const

if (!access_ok(VERIFY_READ, buf, count))
return -EFAULT;
+
+ mutex_lock(&anslcd_mutex);
for ( i = *ppos; count > 0; ++i, ++p, --count )
{
char c;
__get_user(c, p);
anslcd_write_byte_data( c );
}
+ mutex_unlock(&anslcd_mutex);
*ppos = i;
return p - buf;
}

-static int
-anslcd_ioctl( struct inode * inode, struct file * file,
- unsigned int cmd, unsigned long arg )
+static long
+anslcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
char ch, __user *temp;
+ long ret = 0;

#ifdef DEBUG
printk(KERN_DEBUG "LCD: ioctl(%d,%d)\n",cmd,arg);
#endif

+ mutex_lock(&anslcd_mutex);
+
switch ( cmd )
{
case ANSLCD_CLEAR:
@@ -93,7 +98,7 @@ anslcd_ioctl( struct inode * inode, stru
anslcd_write_byte_ctrl ( 0x06 );
anslcd_write_byte_ctrl ( 0x01 );
anslcd_write_byte_ctrl ( 0x02 );
- return 0;
+ break;
case ANSLCD_SENDCTRL:
temp = (char __user *) arg;
__get_user(ch, temp);
@@ -101,33 +106,37 @@ anslcd_ioctl( struct inode * inode, stru
anslcd_write_byte_ctrl ( ch );
__get_user(ch, temp);
}
- return 0;
+ break;
case ANSLCD_SETSHORTDELAY:
if (!capable(CAP_SYS_ADMIN))
- return -EACCES;
- anslcd_short_delay=arg;
- return 0;
+ ret =-EACCES;
+ else
+ anslcd_short_delay=arg;
+ break;
case ANSLCD_SETLONGDELAY:
if (!capable(CAP_SYS_ADMIN))
- return -EACCES;
- anslcd_long_delay=arg;
- return 0;
+ ret = -EACCES;
+ else
+ anslcd_long_delay=arg;
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
}
+
+ mutex_unlock(&anslcd_mutex);
+ return ret;
}

static int
anslcd_open( struct inode * inode, struct file * file )
{
- cycle_kernel_lock();
return 0;
}

const struct file_operations anslcd_fops = {
- .write = anslcd_write,
- .ioctl = anslcd_ioctl,
- .open = anslcd_open,
+ .write = anslcd_write,
+ .unlocked_ioctl = anslcd_ioctl,
+ .open = anslcd_open,
};

static struct miscdevice anslcd_dev = {
@@ -168,6 +177,7 @@ anslcd_init(void)
printk(KERN_DEBUG "LCD: init\n");
#endif

+ mutex_lock(&anslcd_mutex);
anslcd_write_byte_ctrl ( 0x38 );
anslcd_write_byte_ctrl ( 0x0c );
anslcd_write_byte_ctrl ( 0x06 );
@@ -176,6 +186,7 @@ anslcd_init(void)
for(a=0;a<80;a++) {
anslcd_write_byte_data(anslcd_logo[a]);
}
+ mutex_unlock(&anslcd_mutex);
return 0;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Thomas Gleixner · External Since: May 14, 2006 Posts: 944

Push lock/unlock_kernel() into do_sysctl() and remove it from all call
sites of do_sysctl().

Signed-off-by: Thomas Gleixner <tglx DeleteThis @linutronix.de>
Cc: Tony Luck <tony.luck DeleteThis @intel.com>
Cc: Ralf Baechle <ralf DeleteThis @linux-mips.org>
Cc: Benjamin Herrenschmidt <benh DeleteThis @kernel.crashing.org>
Cc: Martin Schwidefsky <schwidefsky DeleteThis @de.ibm.com>
Cc: "David S. Miller" <davem DeleteThis @davemloft.net>

---
arch/ia64/ia32/sys_ia32.c | 2 --
arch/mips/kernel/linux32.c | 2 --
arch/parisc/kernel/sys_parisc32.c | 2 --
arch/powerpc/kernel/sys_ppc32.c | 2 --
arch/s390/kernel/compat_linux.c | 2 --
arch/sparc/kernel/sys_sparc32.c | 2 --
arch/x86/ia32/sys_ia32.c | 2 --
kernel/sysctl.c | 6 ++++--
8 files changed, 4 insertions(+), 16 deletions(-)

Index: linux-2.6-tip/arch/ia64/ia32/sys_ia32.c
===================================================================
--- linux-2.6-tip.orig/arch/ia64/ia32/sys_ia32.c
+++ linux-2.6-tip/arch/ia64/ia32/sys_ia32.c
@@ -1670,10 +1670,8 @@ sys32_sysctl (struct sysctl32 __user *ar
return -EFAULT;

set_fs(KERNEL_DS);
- lock_kernel();
ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *) &oldlen,
newvalp, (size_t) a32.newlen);
- unlock_kernel();
set_fs(old_fs);

if (oldvalp && put_user (oldlen, (int __user *) compat_ptr(a32.oldlenp)))
Index: linux-2.6-tip/arch/mips/kernel/linux32.c
===================================================================
--- linux-2.6-tip.orig/arch/mips/kernel/linux32.c
+++ linux-2.6-tip/arch/mips/kernel/linux32.c
@@ -302,10 +302,8 @@ SYSCALL_DEFINE1(32_sysctl, struct sysctl
oldlenp = (size_t __user *)addr;
}

- lock_kernel();
error = do_sysctl((int __user *)A(tmp.name), tmp.nlen, (void __user *)A(tmp.oldval),
oldlenp, (void __user *)A(tmp.newval), tmp.newlen);
- unlock_kernel();
if (oldlenp) {
if (!error) {
if (get_user(oldlen, (size_t __user *)addr) ||
Index: linux-2.6-tip/arch/parisc/kernel/sys_parisc32.c
===================================================================
--- linux-2.6-tip.orig/arch/parisc/kernel/sys_parisc32.c
+++ linux-2.6-tip/arch/parisc/kernel/sys_parisc32.c
@@ -137,11 +137,9 @@ asmlinkage long sys32_sysctl(struct __sy
oldlenp = (size_t *)addr;
}

- lock_kernel();
error = do_sysctl((int __user *)(u64)tmp.name, tmp.nlen,
(void __user *)(u64)tmp.oldval, oldlenp,
(void __user *)(u64)tmp.newval, tmp.newlen);
- unlock_kernel();
if (oldlenp) {
if (!error) {
if (get_user(oldlen, (size_t *)addr)) {
Index: linux-2.6-tip/arch/powerpc/kernel/sys_ppc32.c
===================================================================
--- linux-2.6-tip.orig/arch/powerpc/kernel/sys_ppc32.c
+++ linux-2.6-tip/arch/powerpc/kernel/sys_ppc32.c
@@ -555,11 +555,9 @@ asmlinkage long compat_sys_sysctl(struct
return -EFAULT;
}

- lock_kernel();
error = do_sysctl(compat_ptr(tmp.name), tmp.nlen,
compat_ptr(tmp.oldval), oldlenp,
compat_ptr(tmp.newval), tmp.newlen);
- unlock_kernel();
if (oldlenp) {
if (!error) {
if (get_user(oldlen, oldlenp) ||
Index: linux-2.6-tip/arch/s390/kernel/compat_linux.c
===================================================================
--- linux-2.6-tip.orig/arch/s390/kernel/compat_linux.c
+++ linux-2.6-tip/arch/s390/kernel/compat_linux.c
@@ -562,10 +562,8 @@ asmlinkage long sys32_sysctl(struct __sy
oldlenp = (size_t __user *)addr;
}

- lock_kernel();
error = do_sysctl(compat_ptr(tmp.name), tmp.nlen, compat_ptr(tmp.oldval),
oldlenp, compat_ptr(tmp.newval), tmp.newlen);
- unlock_kernel();
if (oldlenp) {
if (!error) {
if (get_user(oldlen, (size_t __user *)addr) ||
Index: linux-2.6-tip/arch/sparc/kernel/sys_sparc32.c
===================================================================
--- linux-2.6-tip.orig/arch/sparc/kernel/sys_sparc32.c
+++ linux-2.6-tip/arch/sparc/kernel/sys_sparc32.c
@@ -627,14 +627,12 @@ asmlinkage long sys32_sysctl(struct __sy
oldlenp = (size_t __user *)addr;
}

- lock_kernel();
error = do_sysctl((int __user *)(unsigned long) tmp.name,
tmp.nlen,
(void __user *)(unsigned long) tmp.oldval,
oldlenp,
(void __user *)(unsigned long) tmp.newval,
tmp.newlen);
- unlock_kernel();
if (oldlenp) {
if (!error) {
if (get_user(oldlen, (size_t __user *)addr) ||
Index: linux-2.6-tip/arch/x86/ia32/sys_ia32.c
===================================================================
--- linux-2.6-tip.orig/arch/x86/ia32/sys_ia32.c
+++ linux-2.6-tip/arch/x86/ia32/sys_ia32.c
@@ -477,10 +477,8 @@ asmlinkage long sys32_sysctl(struct sysc
return -EFAULT;

set_fs(KERNEL_DS);
- lock_kernel();
ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *)&oldlen,
newvalp, (size_t) a32.newlen);
- unlock_kernel();
set_fs(old_fs);

if (oldvalp && put_user(oldlen, (int __user *)compat_ptr(a32.oldlenp)))
Index: linux-2.6-tip/kernel/sysctl.c
===================================================================
--- linux-2.6-tip.orig/kernel/sysctl.c
+++ linux-2.6-tip/kernel/sysctl.c
@@ -1848,6 +1848,8 @@ int do_sysctl(int __user *name, int nlen
return -EFAULT;
}

+ lock_kernel();
+
for (head = sysctl_head_next(NULL); head;
head = sysctl_head_next(head)) {
error = parse_table(name, nlen, oldval, oldlenp,
@@ -1858,6 +1860,8 @@ int do_sysctl(int __user *name, int nlen
break;
}
}
+
+ unlock_kernel();
return error;
}

@@ -1873,10 +1877,8 @@ SYSCALL_DEFINE1(sysctl, struct __sysctl_
if (error)
goto out;

- lock_kernel();
error = do_sysctl(tmp.name, tmp.nlen, tmp.oldval, tmp.oldlenp,
tmp.newval, tmp.newlen);
- unlock_kernel();
out:
return error;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Frederic Weisbecker · External Since: Jan 17, 2009 Posts: 267

On Sat, Oct 10, 2009 at 03:35:24PM -0000, Thomas Gleixner wrote:
> pm_qos_power_open got its lock_kernel() calls from the open() pushdown. A
> look at the code shows that the only global resources accessed are
> pm_qos_array and "name". pm_qos_array doesn't change (things pointed to
> therein do change, but they are atomics and/or are protected by
> pm_qos_lock). Accesses to "name" are totally unprotected with or without
> the BKL; that will be fixed shortly. The BKL is not helpful here; take it
> out.
>
> Signed-off-by: Jonathan Corbet <corbet DeleteThis @lwn.net>
> Cc: Mark Gross <mgross DeleteThis @linux.intel.com>
> Signed-off-by: Thomas Gleixner <tglx DeleteThis @linutronix.de>

Reviewed-by: Frederic Weisbecker <fweisbec DeleteThis @gmail.com>

>
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index dfdec52..d96b83e 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -29,7 +29,6 @@
>
> #include <linux/pm_qos_params.h>
> #include <linux/sched.h>
> -#include <linux/smp_lock.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> #include <linux/time.h>
> @@ -352,20 +351,15 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
> int ret;
> long pm_qos_class;
>
> - lock_kernel();
> pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
> if (pm_qos_class >= 0) {
> filp->private_data = (void *)pm_qos_class;
> sprintf(name, "process_%d", current->pid);
> ret = pm_qos_add_requirement(pm_qos_class, name,
> PM_QOS_DEFAULT_VALUE);
> - if (ret >= 0) {
> - unlock_kernel();
> + if (ret >= 0)
> return 0;
> - }
> }
> - unlock_kernel();
> -
> return -EPERM;
> }
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jean Delvare · External Since: May 16, 2006 Posts: 284

Hi Thomas,

On Sat, 10 Oct 2009 15:37:17 -0000, Thomas Gleixner wrote:
> The BKL is held over a kmalloc so cannot protect anything beyond that.
> The two calls before the kmalloc have their own locking.
> Improve device open function by removing the now unnecessary ret variable
>
> Signed-off-by: Vincent Sanders <vince.TakeThisOut@simtec.co.uk>
> LKML-Reference: <1255175172-2666-1-git-send-email-vince.TakeThisOut@simtec.co.uk>
> Cc: Jean Delvare <khali.TakeThisOut@linux-fr.org>
> Signed-off-by: Thomas Gleixner <tglx.TakeThisOut@linutronix.de>
> ---
> drivers/i2c/i2c-dev.c | 22 ++++++----------------
> 1 file changed, 6 insertions(+), 16 deletions(-)
>
> Index: linux-2.6-tip/drivers/i2c/i2c-dev.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/i2c/i2c-dev.c
> +++ linux-2.6-tip/drivers/i2c/i2c-dev.c
> @@ -34,7 +34,6 @@
> #include <linux/list.h>
> #include <linux/i2c.h>
> #include <linux/i2c-dev.h>
> -#include <linux/smp_lock.h>
> #include <linux/jiffies.h>
> #include <asm/uaccess.h>
>
> @@ -445,20 +444,14 @@ static int i2cdev_open(struct inode *ino
> struct i2c_client *client;
> struct i2c_adapter *adap;
> struct i2c_dev *i2c_dev;
> - int ret = 0;
>
> - lock_kernel();
> i2c_dev = i2c_dev_get_by_minor(minor);
> - if (!i2c_dev) {
> - ret = -ENODEV;
> - goto out;
> - }
> + if (!i2c_dev)
> + return -ENODEV;
>
> adap = i2c_get_adapter(i2c_dev->adap->nr);
> - if (!adap) {
> - ret = -ENODEV;
> - goto out;
> - }
> + if (!adap)
> + return -ENODEV;
>
> /* This creates an anonymous i2c_client, which may later be
> * pointed to some address using I2C_SLAVE or I2C_SLAVE_FORCE.
> @@ -470,8 +463,7 @@ static int i2cdev_open(struct inode *ino
> client = kzalloc(sizeof(*client), GFP_KERNEL);
> if (!client) {
> i2c_put_adapter(adap);
> - ret = -ENOMEM;
> - goto out;
> + return -ENOMEM;
> }
> snprintf(client->name, I2C_NAME_SIZE, "i2c-dev %d", adap->nr);
> client->driver = &i2cdev_driver;
> @@ -479,9 +471,7 @@ static int i2cdev_open(struct inode *ino
> client->adapter = adap;
> file->private_data = client;
>
> -out:
> - unlock_kernel();
> - return ret;
> + return 0;
> }
>
> static int i2cdev_release(struct inode *inode, struct file *file)
>

Looks good to me:

Acked-by: Jean Delvare <khali.TakeThisOut@linux-fr.org>

Do you want me to pick this patch in my i2c tree, or will you take care
of pushing it upstream? If you're not going to push it quickly, I'd
prefer the first solution, to avoid conflicts.

(As a side note, i2c-dev would deserve a partial rewrite as it doesn't
integrate into the standard device driver model, and certainly has a
number of race conditions. But this is well beyond the scope of your
patch.)

Thanks,
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Thomas Gleixner · External Since: May 14, 2006 Posts: 944

Jean,

On Sat, 10 Oct 2009, Jean Delvare wrote:
> Looks good to me:
>
> Acked-by: Jean Delvare <khali RemoveThis @linux-fr.org>
>
> Do you want me to pick this patch in my i2c tree, or will you take care
> of pushing it upstream? If you're not going to push it quickly, I'd
> prefer the first solution, to avoid conflicts.

I'm targeting for .33, but please pick it up. I drop it from my queue
then.

> (As a side note, i2c-dev would deserve a partial rewrite as it doesn't
> integrate into the standard device driver model, and certainly has a
> number of race conditions. But this is well beyond the scope of your
> patch.)

Yeah, noticed that this code needs some care, but my current dosis of
brain wreckage with the BKL stuff is enough Smile

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jean Delvare · External Since: May 16, 2006 Posts: 284

On Sat, 10 Oct 2009 19:09:45 +0200 (CEST), Thomas Gleixner wrote:
> Jean,
>
> On Sat, 10 Oct 2009, Jean Delvare wrote:
> > Looks good to me:
> >
> > Acked-by: Jean Delvare <khali.TakeThisOut@linux-fr.org>
> >
> > Do you want me to pick this patch in my i2c tree, or will you take care
> > of pushing it upstream? If you're not going to push it quickly, I'd
> > prefer the first solution, to avoid conflicts.
>
> I'm targeting for .33, but please pick it up. I drop it from my queue
> then.

Will do. I guess I can assume that the patch is from Vincent Sanders,
not you, despite the lack of From: header?

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Thomas Gleixner · External Since: May 14, 2006 Posts: 944

Jean,

On Sat, 10 Oct 2009, Jean Delvare wrote:

> On Sat, 10 Oct 2009 19:09:45 +0200 (CEST), Thomas Gleixner wrote:
> > Jean,
> >
> > On Sat, 10 Oct 2009, Jean Delvare wrote:
> > > Looks good to me:
> > >
> > > Acked-by: Jean Delvare <khali RemoveThis @linux-fr.org>
> > >
> > > Do you want me to pick this patch in my i2c tree, or will you take care
> > > of pushing it upstream? If you're not going to push it quickly, I'd
> > > prefer the first solution, to avoid conflicts.
> >
> > I'm targeting for .33, but please pick it up. I drop it from my queue
> > then.
>
> Will do. I guess I can assume that the patch is from Vincent Sanders,
> not you, despite the lack of From: header?

Duh, yes. Missed that.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Peter Zijlstra · External Since: Jun 06, 2007 Posts: 205

On Sat, 2009-10-10 at 20:10 +0200, Thomas Gleixner wrote:
>
> > Will do. I guess I can assume that the patch is from Vincent Sanders,
> > not you, despite the lack of From: header?
>
> Duh, yes. Missed that.

I'm still wanting to fix quilt mail to not mess that up... maybe I
should sit down and try and grok that stuff again.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

John Kacur · External Since: Jul 23, 2009 Posts: 23

On Sat, 10 Oct 2009, Thomas Gleixner wrote:

> Hi all,
>
> I'm sending out the pending BKL removal patches which I collected so
> far to avoid duplicate work and to solicit review/comments.
>
> If nobody objects I'd like to keep them in one git branch which is
> going to be included into linux-next.
>
> Thanks,
>
> tglx

Seems like a great idea to me. I'm applying them all to a private
git-branch against the latest from Linus to avoid duplicate work.

Please let us know when there is a tip branch we can work with.

John
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Thomas Gleixner · External Since: May 14, 2006 Posts: 944

On Sat, 10 Oct 2009, Peter Zijlstra wrote:

> On Sat, 2009-10-10 at 20:10 +0200, Thomas Gleixner wrote:
> >
> > > Will do. I guess I can assume that the patch is from Vincent Sanders,
> > > not you, despite the lack of From: header?
> >
> > Duh, yes. Missed that.
>
> I'm still wanting to fix quilt mail to not mess that up... maybe I
> should sit down and try and grok that stuff again.

IIRC it worked some time ago. That's why I did not notice.

Thanks,

tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

John Kacur · External Since: Jul 23, 2009 Posts: 23

On Sat, 10 Oct 2009, Thomas Gleixner wrote:

> "name" is a poor name for a file-global variable. It was used in three
> different functions, with no mutual exclusion. But it's just a tiny,
> temporary string; let's just move it onto the stack in the functions that
> need it. Also use snprintf() just in case.
>
> Signed-off-by: Jonathan Corbet <corbet.TakeThisOut@lwn.net>
> Cc: Mark Gross <mgross.TakeThisOut@linux.intel.com>
> Reviewed-by: Frederic Weisbecker <fweisbec.TakeThisOut@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx.TakeThisOut@linutronix.de>
>
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index d96b83e..3db49b9 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -343,18 +343,18 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
>
> -#define PID_NAME_LEN sizeof("process_1234567890")
> -static char name[PID_NAME_LEN];
> +#define PID_NAME_LEN 32

Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890"
which is 19, an attempt to show what the maximum string size would be. If
a system were configured to enlarge the maximum PID from 32767 to 4194303
that would still only be 7 digits, so "process_1234567" - which is 16
digits with the newline would enough.

So, I suggest you change that to
#define PID_NAME_LEN sizeof("process_1234567")

Other than that, Reviewed-by: John Kacur <jkacur.TakeThisOut@redhat.com>

>
> static int pm_qos_power_open(struct inode *inode, struct file *filp)
> {
> int ret;
> long pm_qos_class;
> + char name[PID_NAME_LEN];
>
> pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
> if (pm_qos_class >= 0) {
> filp->private_data = (void *)pm_qos_class;
> - sprintf(name, "process_%d", current->pid);
> + snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
> ret = pm_qos_add_requirement(pm_qos_class, name,
> PM_QOS_DEFAULT_VALUE);
> if (ret >= 0)
> @@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
> static int pm_qos_power_release(struct inode *inode, struct file *filp)
> {
> int pm_qos_class;
> + char name[PID_NAME_LEN];
>
> pm_qos_class = (long)filp->private_data;
> - sprintf(name, "process_%d", current->pid);
> + snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
> pm_qos_remove_requirement(pm_qos_class, name);
>
> return 0;
> @@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> {
> s32 value;
> int pm_qos_class;
> + char name[PID_NAME_LEN];
>
> pm_qos_class = (long)filp->private_data;
> if (count != sizeof(s32))
> return -EINVAL;
> if (copy_from_user(&value, buf, sizeof(s32)))
> return -EFAULT;
> - sprintf(name, "process_%d", current->pid);
> + snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
> pm_qos_update_requirement(pm_qos_class, name, value);
>
> return sizeof(s32);
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jonathan Corbet · External Since: Oct 13, 2006 Posts: 22

On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
John Kacur <jkacur.RemoveThis@redhat.com> wrote:

> Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890"
> which is 19, an attempt to show what the maximum string size would be. If
> a system were configured to enlarge the maximum PID from 32767 to 4194303
> that would still only be 7 digits, so "process_1234567" - which is 16
> digits with the newline would enough.
>
> So, I suggest you change that to
> #define PID_NAME_LEN sizeof("process_1234567")

....which works great until somebody enables 64-bit process IDs... Smile

We're talking about 20 bytes of stack space in an almost-never-called
function. I honestly don't think it's worth worrying about, but if
somebody wants to tweak it, I'll not complain.

(Thanks for looking at the patch).

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Peter Zijlstra · External Since: Jun 06, 2007 Posts: 205

On Sat, 2009-10-10 at 14:03 -0600, Jonathan Corbet wrote:
> On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
> John Kacur <jkacur.TakeThisOut@redhat.com> wrote:
>
> > Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890"
> > which is 19, an attempt to show what the maximum string size would be. If
> > a system were configured to enlarge the maximum PID from 32767 to 4194303
> > that would still only be 7 digits, so "process_1234567" - which is 16
> > digits with the newline would enough.
> >
> > So, I suggest you change that to
> > #define PID_NAME_LEN sizeof("process_1234567")
>
> ....which works great until somebody enables 64-bit process IDs... Smile

PID/TIDs are limited to 2^29, raising it above that will break things
like futexes. Raising it above 2^32 will break heaps of userspace.

That said, 512M tasks still seems like a lot, but if history is
something to go by we'll eventually run out...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

John Kacur · External Since: Jul 23, 2009 Posts: 23

On Sat, 10 Oct 2009, Jonathan Corbet wrote:

> On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
> John Kacur <jkacur.TakeThisOut@redhat.com> wrote:
>
> > Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890"
> > which is 19, an attempt to show what the maximum string size would be. If
> > a system were configured to enlarge the maximum PID from 32767 to 4194303
> > that would still only be 7 digits, so "process_1234567" - which is 16
> > digits with the newline would enough.
> >
> > So, I suggest you change that to
> > #define PID_NAME_LEN sizeof("process_1234567")
>
> ...which works great until somebody enables 64-bit process IDs... Smile

>
> We're talking about 20 bytes of stack space in an almost-never-called
> function. I honestly don't think it's worth worrying about, but if
> somebody wants to tweak it, I'll not complain.
>
> (Thanks for looking at the patch).
>

It was a minor nit at best! My point was less about the stack space than
the readability - which you could argue is a personal style choice here.
There is nothing else to criticize in these patches. Smile

Thomas, I reviewed all 28 patches and applied them.

Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

John Kacur · External Since: Jul 23, 2009 Posts: 23

On Sat, 10 Oct 2009, Thomas Gleixner wrote:

> The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
> the BKL pushdown and it still uses the locked ioctl.
>
> The BKL serialization in this driver is more than obscure and
> definitely does not cover all possible corner cases. Protect the
> access to the hardware with a local mutex and get rid of BKL and
> locked ioctl.
>
> Signed-off-by: Thomas Gleixner <tglx.RemoveThis@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh.RemoveThis@kernel.crashing.org>
> Cc: linuxppc-dev.RemoveThis@ozlabs.org
> ---
> drivers/macintosh/ans-lcd.c | 45 +++++++++++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 17 deletions(-)
>
> Index: linux-2.6-tip/drivers/macintosh/ans-lcd.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/macintosh/ans-lcd.c
> +++ linux-2.6-tip/drivers/macintosh/ans-lcd.c
> @@ -3,7 +3,6 @@
> */
>
> #include <linux/types.h>
> -#include <linux/smp_lock.h>
> #include <linux/errno.h>
> #include <linux/kernel.h>
> #include <linux/miscdevice.h>
> @@ -26,6 +25,7 @@
> static unsigned long anslcd_short_delay = 80;
> static unsigned long anslcd_long_delay = 3280;
> static volatile unsigned char __iomem *anslcd_ptr;
> +static DEFINE_MUTEX(anslcd_mutex);
>
> #undef DEBUG
>
> @@ -65,26 +65,31 @@ anslcd_write( struct file * file, const
>
> if (!access_ok(VERIFY_READ, buf, count))
> return -EFAULT;
> +
> + mutex_lock(&anslcd_mutex);
> for ( i = *ppos; count > 0; ++i, ++p, --count )
> {
> char c;
> __get_user(c, p);
> anslcd_write_byte_data( c );
> }
> + mutex_unlock(&anslcd_mutex);
> *ppos = i;
> return p - buf;
> }
>
> -static int
> -anslcd_ioctl( struct inode * inode, struct file * file,
> - unsigned int cmd, unsigned long arg )
> +static long
> +anslcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> char ch, __user *temp;
> + long ret = 0;
>
> #ifdef DEBUG
> printk(KERN_DEBUG "LCD: ioctl(%d,%d)\n",cmd,arg);
> #endif
>
> + mutex_lock(&anslcd_mutex);
> +
> switch ( cmd )
> {
> case ANSLCD_CLEAR:
> @@ -93,7 +98,7 @@ anslcd_ioctl( struct inode * inode, stru
> anslcd_write_byte_ctrl ( 0x06 );
> anslcd_write_byte_ctrl ( 0x01 );
> anslcd_write_byte_ctrl ( 0x02 );
> - return 0;
> + break;
> case ANSLCD_SENDCTRL:
> temp = (char __user *) arg;
> __get_user(ch, temp);
> @@ -101,33 +106,37 @@ anslcd_ioctl( struct inode * inode, stru
> anslcd_write_byte_ctrl ( ch );
> __get_user(ch, temp);
> }
> - return 0;
> + break;
> case ANSLCD_SETSHORTDELAY:
> if (!capable(CAP_SYS_ADMIN))
> - return -EACCES;
> - anslcd_short_delay=arg;
> - return 0;
> + ret =-EACCES;
> + else
> + anslcd_short_delay=arg;
> + break;
> case ANSLCD_SETLONGDELAY:
> if (!capable(CAP_SYS_ADMIN))
> - return -EACCES;
> - anslcd_long_delay=arg;
> - return 0;
> + ret = -EACCES;
> + else
> + anslcd_long_delay=arg;
> + break;
> default:
> - return -EINVAL;
> + ret = -EINVAL;
> }
> +
> + mutex_unlock(&anslcd_mutex);
> + return ret;
> }
>
> static int
> anslcd_open( struct inode * inode, struct file * file )
> {
> - cycle_kernel_lock();
> return 0;
> }
>
> const struct file_operations anslcd_fops = {
> - .write = anslcd_write,
> - .ioctl = anslcd_ioctl,
> - .open = anslcd_open,
> + .write = anslcd_write,
> + .unlocked_ioctl = anslcd_ioctl,
> + .open = anslcd_open,
> };
>
> static struct miscdevice anslcd_dev = {
> @@ -168,6 +177,7 @@ anslcd_init(void)
> printk(KERN_DEBUG "LCD: init\n");
> #endif
>
> + mutex_lock(&anslcd_mutex);
> anslcd_write_byte_ctrl ( 0x38 );
> anslcd_write_byte_ctrl ( 0x0c );
> anslcd_write_byte_ctrl ( 0x06 );
> @@ -176,6 +186,7 @@ anslcd_init(void)
> for(a=0;a<80;a++) {
> anslcd_write_byte_data(anslcd_logo[a]);
> }
> + mutex_unlock(&anslcd_mutex);
> return 0;
> }
>
>
>
>

There were 4 checkpatch errors on this patch, all of the type
ERROR: spaces required around that '=' (ctx:WxO)
#1466: FILE: drivers/macintosh/ans-lcd.c:112:
+ ret =-EACCES;

Cheers

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Alan Cox · External Since: Sep 11, 2004 Posts: 1608

> There were 4 checkpatch errors on this patch, all of the type
> ERROR: spaces required around that '=' (ctx:WxO)
> #1466: FILE: drivers/macintosh/ans-lcd.c:112:
> + ret =-EACCES;

Here's a suggestion. If a few spaces bug you that much then instead of
complaining about it and posting checkpatch results deal with the file
itself.

Wait until the patch goes in and send a follow up patch that fixes up the
file to fit codingstyle. There's no point whining about the bits a patch
touches when the file wasn't in that format before, but if you've got
nothing better to do then doing a pass over the whole file *is* useful.

(Plus it gets a patch to your name Wink

)

Checkpatch whines on files that simple don't follow style are usually
best ignored because they make the file formatting less internally
consistent.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

John Kacur · External Since: Jul 23, 2009 Posts: 23

On Sun, 11 Oct 2009, Alan Cox wrote:

> > There were 4 checkpatch errors on this patch, all of the type
> > ERROR: spaces required around that '=' (ctx:WxO)
> > #1466: FILE: drivers/macintosh/ans-lcd.c:112:
> > + ret =-EACCES;
>
> Here's a suggestion. If a few spaces bug you that much then instead of
> complaining about it and posting checkpatch results deal with the file
> itself.
>
> Wait until the patch goes in and send a follow up patch that fixes up the
> file to fit codingstyle. There's no point whining about the bits a patch
> touches when the file wasn't in that format before, but if you've got
> nothing better to do then doing a pass over the whole file *is* useful.
>
> (Plus it gets a patch to your name Wink

)
>
> Checkpatch whines on files that simple don't follow style are usually
> best ignored because they make the file formatting less internally
> consistent.
>
Thanks Alan, I was sincerely debatting whether to send this because I know
that checkpatch can be annoying - but on the other hand, I thought it
prudent to run it since I was claiming to have reviewed all of those
patches.

I like your suggestion though - next time, I won't send the mail, since
since the folks submitting these patches are more than capable of checking
that kind of thing themselves, and if I feel it's important enough, I'll
follow up with a trivial style patch.

Cheers!

John
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Frederic Weisbecker · External Since: Jan 17, 2009 Posts: 267

On Sun, Oct 11, 2009 at 11:08:10PM +0200, Frederic Weisbecker wrote:
> On Sun, Oct 11, 2009 at 09:31:40PM +0200, Arnd Bergmann wrote:
> > On Saturday 10 October 2009, Thomas Gleixner wrote:
> > > There is nothing to protect inside nvram_llseek(), the file
> > > offset doesn't need to be protected and nvram_len is only
> > > initialized from an __init path.
> > >
> > > It's safe to remove the big kernel lock there.
> > >
> >
> > The generic_nvram driver still uses ->ioctl instead of ->unlocked_ioctl.
> > I guess it would be helpful to change that in the same series, so we
> > don't get the BKL back as soon as someone does a pushdown into the
> > remaining ioctl functions.
> >
> > Arnd <><
>
>
> Right!
> I'll add that in a second patch.
>
> I've completely forgotten this ioctl/unlocked_ioctl thing.

BTW, I was focusing on the lock_kernel() callsites in the kernel which
are around 626 (I've excluded reiserfs)

Now I'm adding the ioctl() sites too:

git-grep "\.ioctl *=" | grep -P "^\S+\.c" | wc -l
452

Hehe Smile

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/