[patch 22/28] macintosh: Remove BKL from ans-lcd

[patch 22/28] macintosh: Remove BKL from ans-lcd

[Thomas Gleixner]

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@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@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;
 }
 

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[John Kacur]

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@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@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

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[Alan Cox]

> 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 ;))

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

Alan

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[John Kacur]

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 ;))
> 
> 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

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[Benjamin Herrenschmidt]

On Sat, 2009-10-10 at 15:37 +0000, Thomas Gleixner wrote:
> plain text document attachment (drivers-mac-ans-lcd-remove-bkl.patch)
> 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@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@ozlabs.org
> ---

While I -do- have an ANS ... it's rusting in the back of my garage and I
really don't have time nor space to set it up and get it back to booting
shape :-) (It's a pretty huge thing)

Patch looks good, so if it builds...

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Cheers,
Ben.

[PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

>From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Wed, 21 Oct 2009 23:01:12 +0200
Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

Now that we've removed the BKL here, let's explicitly set lleek to no_llseek

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/macintosh/ans-lcd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
index 4ae8ec9..a1a1bde 100644
--- a/drivers/macintosh/ans-lcd.c
+++ b/drivers/macintosh/ans-lcd.c
@@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
 	.write		= anslcd_write,
 	.unlocked_ioctl	= anslcd_ioctl,
 	.open		= anslcd_open,
+	.llseedk	= no_llseek,
 };
 
 static struct miscdevice anslcd_dev = {
-- 
1.6.0.6

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:07:18PM +0200, John Kacur wrote:
> From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Wed, 21 Oct 2009 23:01:12 +0200
> Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd
> 
> Now that we've removed the BKL here, let's explicitly set lleek to no_llseek
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  drivers/macintosh/ans-lcd.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
> index 4ae8ec9..a1a1bde 100644
> --- a/drivers/macintosh/ans-lcd.c
> +++ b/drivers/macintosh/ans-lcd.c
> @@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
>  	.write		= anslcd_write,
>  	.unlocked_ioctl	= anslcd_ioctl,
>  	.open		= anslcd_open,
> +	.llseedk	= no_llseek,


llseedk? :)


Should we better pushdown default_llseek to every to every
file operations that don't implement llseek?
I don't know how many of them don't implement llseek() though.

That said we can't continue anymore with this default attribution
of default_llseek() on new fops.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:07:18PM +0200, John Kacur wrote:
> > From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Wed, 21 Oct 2009 23:01:12 +0200
> > Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd
> > 
> > Now that we've removed the BKL here, let's explicitly set lleek to no_llseek
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> > ---
> >  drivers/macintosh/ans-lcd.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
> > index 4ae8ec9..a1a1bde 100644
> > --- a/drivers/macintosh/ans-lcd.c
> > +++ b/drivers/macintosh/ans-lcd.c
> > @@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
> >  	.write		= anslcd_write,
> >  	.unlocked_ioctl	= anslcd_ioctl,
> >  	.open		= anslcd_open,
> > +	.llseedk	= no_llseek,
> 
> 
> llseedk? :)
> 
> 
> Should we better pushdown default_llseek to every to every
> file operations that don't implement llseek?
> I don't know how many of them don't implement llseek() though.
> 
> That said we can't continue anymore with this default attribution
> of default_llseek() on new fops.
> 

If you don't explicitly set it to no_llseek, you automatically get the
default_llseek, which uses the BKL. So if your driver doesn't need it, it 
is best to explicitly set it to no_llseek.

There is also a generic_file_llseek_unlocked, somewhat analogous to the 
unlocked_ioctls that you can use if you don't need to provide a full 
llseek yourself.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:33:17PM +0200, John Kacur wrote:
> > Should we better pushdown default_llseek to every to every
> > file operations that don't implement llseek?
> > I don't know how many of them don't implement llseek() though.
> > 
> > That said we can't continue anymore with this default attribution
> > of default_llseek() on new fops.
> > 
> 
> If you don't explicitly set it to no_llseek, you automatically get the
> default_llseek, which uses the BKL. So if your driver doesn't need it, it 
> is best to explicitly set it to no_llseek.


Sure, that's the right thing to do.

 
> There is also a generic_file_llseek_unlocked, somewhat analogous to the 
> unlocked_ioctls that you can use if you don't need to provide a full 
> llseek yourself.


No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
depending on the context is the right thing to do.

What I'm wondering about concerns the future code that will have
no llsek() implemented in their fops.

We can't continue to use default_llseek() for future code unless we
want to continue these post reviews and fixes forever.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:33:17PM +0200, John Kacur wrote:
> > > Should we better pushdown default_llseek to every to every
> > > file operations that don't implement llseek?
> > > I don't know how many of them don't implement llseek() though.
> > > 
> > > That said we can't continue anymore with this default attribution
> > > of default_llseek() on new fops.
> > > 
> > 
> > If you don't explicitly set it to no_llseek, you automatically get the
> > default_llseek, which uses the BKL. So if your driver doesn't need it, it 
> > is best to explicitly set it to no_llseek.
> 
> 
> Sure, that's the right thing to do.
> 
>  
> > There is also a generic_file_llseek_unlocked, somewhat analogous to the 
> > unlocked_ioctls that you can use if you don't need to provide a full 
> > llseek yourself.
> 
> 
> No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
> depending on the context is the right thing to do.
> 
> What I'm wondering about concerns the future code that will have
> no llsek() implemented in their fops.
> 
> We can't continue to use default_llseek() for future code unless we
> want to continue these post reviews and fixes forever.
> 

I'm thinking that the simplier approach, would be to make the 
default_llseek the unlocked one. Then you only have to audit the drivers 
that have the BKL - ie the ones we are auditing anyway, and explicitly set 
them to the bkl locked llseek.

There might be a hidden interaction though between the non-unlocked 
variety of ioctls and default llseek though.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:53:21PM +0200, John Kacur wrote:
> > No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
> > depending on the context is the right thing to do.
> > 
> > What I'm wondering about concerns the future code that will have
> > no llsek() implemented in their fops.
> > 
> > We can't continue to use default_llseek() for future code unless we
> > want to continue these post reviews and fixes forever.
> > 
> 
> I'm thinking that the simplier approach, would be to make the 
> default_llseek the unlocked one. Then you only have to audit the drivers 
> that have the BKL - ie the ones we are auditing anyway, and explicitly set 
> them to the bkl locked llseek.
> 
> There might be a hidden interaction though between the non-unlocked 
> variety of ioctls and default llseek though.


I fear that won't work because the bkl in default_llseek() does not
only synchronizes with others uses of the bkl in a driver, it also
synchronizes lseek() itself.

As an example offset change is not atomic. This is a long long, so
updating its value is not atomic in 32 bits archs.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Arnd Bergmann]

On Thursday 22 October 2009, Frederic Weisbecker wrote:
> > I'm thinking that the simplier approach, would be to make the 
> > default_llseek the unlocked one. Then you only have to audit the drivers 
> > that have the BKL - ie the ones we are auditing anyway, and explicitly set 
> > them to the bkl locked llseek.
> > 
> > There might be a hidden interaction though between the non-unlocked 
> > variety of ioctls and default llseek though.
> 
> 
> I fear that won't work because the bkl in default_llseek() does not
> only synchronizes with others uses of the bkl in a driver, it also
> synchronizes lseek() itself.
> 
> As an example offset change is not atomic. This is a long long, so
> updating its value is not atomic in 32 bits archs.

A late follow-up on this one:

I looked at what places in the code manipulate file->f_pos directly
and found that almost all the uses in driver code are broken because
they don't take any locks. Most of them are in driver specific
lseek operations. Others are in read/write methods and are even
more broken because the change gets immediately overwritten by
vfs_read/vfs_write when the driver method returns.

IMHO, we should complete the pushdown into all ioctl methods
that John and Thomas have started working on, fixing the lseek
methods in those files we touch.

When that is done, all interaction between default_llseek and
driver locking has to be with explicit calls to lock_kernel
in those drivers, so we can reasonably well script the search
for drivers needing the BKL in llseek: everyhing that
 a) defines file_operations without an llseek function,
 b) touches f_pos somewhere, and
 c) calls lock_kernel() somewhere
That should only be a small number and when they are fixed,
we can remove default_llseek and instead call generic_file_llseek
for any file operation without a separate llseek method.

	Arnd <><

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Christoph Hellwig]

On Mon, Nov 02, 2009 at 04:51:52PM +0100, Arnd Bergmann wrote:
> I looked at what places in the code manipulate file->f_pos directly
> and found that almost all the uses in driver code are broken because
> they don't take any locks. Most of them are in driver specific
> lseek operations. Others are in read/write methods and are even
> more broken because the change gets immediately overwritten by
> vfs_read/vfs_write when the driver method returns.
> 
> IMHO, we should complete the pushdown into all ioctl methods
> that John and Thomas have started working on, fixing the lseek
> methods in those files we touch.
> 
> When that is done, all interaction between default_llseek and
> driver locking has to be with explicit calls to lock_kernel
> in those drivers, so we can reasonably well script the search
> for drivers needing the BKL in llseek: everyhing that
>  a) defines file_operations without an llseek function,
>  b) touches f_pos somewhere, and
>  c) calls lock_kernel() somewhere
> That should only be a small number and when they are fixed,
> we can remove default_llseek and instead call generic_file_llseek
> for any file operation without a separate llseek method.

As mentioned before making generic_file_llseek the new default is
probably a bad idea.  The majority of our file_operations instances
don't actually support seeking, so no_llseek should become the new
default if you spend some effort on converting things.  Anything that
wants to allow seeking will have to set a llseek method.  This also
mirrors what we do for other file operations.  None of the major ones
has a non-trivial default, it's either silently succeeding for a
selected few like open or release or returning an error for operatings
that actually do something like read and write.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Arnd Bergmann]

On Monday 16 November 2009, Christoph Hellwig wrote:
> As mentioned before making generic_file_llseek the new default is
> probably a bad idea.  The majority of our file_operations instances
> don't actually support seeking, so no_llseek should become the new
> default if you spend some effort on converting things.  Anything that
> wants to allow seeking will have to set a llseek method.  This also
> mirrors what we do for other file operations.  None of the major ones
> has a non-trivial default, it's either silently succeeding for a
> selected few like open or release or returning an error for operatings
> that actually do something like read and write.

Ok, good point.

Do you think we should also prevent pread/pwrite for devices without
an llseek operation, like nonseekable_open does? I guess that would
be consistent.

Then there is the point that (I forgot who) brought up that changing
code to do no_llseek is actually an ABI change. Even if the file
position is never used anywhere, some random user application might
expect a chardev not to return an error when its llseek method is
called, resulting in regressions.

	Arnd <><