[PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[John Kacur]

>From b5fefbe4ab8783a0299953b0869cf2af24160875 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 18 Oct 2009 23:49:49 +0200
Subject: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

The BKL is in this function because of the BKL pushdown
(see commit f8f2c79d594463427f7114cedb1555110d547d89)

It is not needed here because the mutex_lock sonypi_device.lock
provides the necessary locking.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/char/sonypi.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/char/sonypi.c b/drivers/char/sonypi.c
index 8c262aa..f64600b 100644
--- a/drivers/char/sonypi.c
+++ b/drivers/char/sonypi.c
@@ -50,7 +50,6 @@
 #include <linux/err.h>
 #include <linux/kfifo.h>
 #include <linux/platform_device.h>
-#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -902,14 +901,13 @@ static int sonypi_misc_release(struct inode *inode, struct file *file)
 
 static int sonypi_misc_open(struct inode *inode, struct file *file)
 {
-	lock_kernel();
 	mutex_lock(&sonypi_device.lock);
 	/* Flush input queue on first open */
 	if (!sonypi_device.open_count)
 		kfifo_reset(sonypi_device.fifo);
 	sonypi_device.open_count++;
 	mutex_unlock(&sonypi_device.lock);
-	unlock_kernel();
+
 	return 0;
 }
 
-- 
1.6.0.6

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Arnd Bergmann]

On Sunday 18 October 2009, John Kacur wrote:
> The BKL is in this function because of the BKL pushdown
> (see commit f8f2c79d594463427f7114cedb1555110d547d89)
> 
> It is not needed here because the mutex_lock sonypi_device.lock
> provides the necessary locking.

The driver still uses the BKL in the ioctl function, which can
probably be removed at the same time.

	Arnd <><

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[John Kacur]

On Mon, 19 Oct 2009, Arnd Bergmann wrote:

> On Sunday 18 October 2009, John Kacur wrote:
> > The BKL is in this function because of the BKL pushdown
> > (see commit f8f2c79d594463427f7114cedb1555110d547d89)
> > 
> > It is not needed here because the mutex_lock sonypi_device.lock
> > provides the necessary locking.
> 
> The driver still uses the BKL in the ioctl function, which can
> probably be removed at the same time.
> 
> 	Arnd <><
> 

How does this look? (Version 2 of the patch follows)

>From efd07cfcd021b4438d83d383ab81f9b35cb41eb9 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 18 Oct 2009 23:49:49 +0200
Subject: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

The BKL is in this function because of the BKL pushdown
(see commit f8f2c79d594463427f7114cedb1555110d547d89)

It is not needed here because the mutex_lock sonypi_device.lock
provides the necessary locking.

sonpi_misc_ioctl can be converted to unlocked ioctls since it relies on
its own locking (the mutex sonypi_device.lock) and not the bkl

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/char/sonypi.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/char/sonypi.c b/drivers/char/sonypi.c
index 8c262aa..593cbb5 100644
--- a/drivers/char/sonypi.c
+++ b/drivers/char/sonypi.c
@@ -50,7 +50,6 @@
 #include <linux/err.h>
 #include <linux/kfifo.h>
 #include <linux/platform_device.h>
-#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -902,14 +901,13 @@ static int sonypi_misc_release(struct inode *inode, struct file *file)
 
 static int sonypi_misc_open(struct inode *inode, struct file *file)
 {
-	lock_kernel();
 	mutex_lock(&sonypi_device.lock);
 	/* Flush input queue on first open */
 	if (!sonypi_device.open_count)
 		kfifo_reset(sonypi_device.fifo);
 	sonypi_device.open_count++;
 	mutex_unlock(&sonypi_device.lock);
-	unlock_kernel();
+
 	return 0;
 }
 
@@ -951,10 +949,10 @@ static unsigned int sonypi_misc_poll(struct file *file, poll_table *wait)
 	return 0;
 }
 
-static int sonypi_misc_ioctl(struct inode *ip, struct file *fp,
+static long sonypi_misc_ioctl(struct file *fp,
 			     unsigned int cmd, unsigned long arg)
 {
-	int ret = 0;
+	long ret = 0;
 	void __user *argp = (void __user *)arg;
 	u8 val8;
 	u16 val16;
@@ -1070,7 +1068,7 @@ static const struct file_operations sonypi_misc_fops = {
 	.open		= sonypi_misc_open,
 	.release	= sonypi_misc_release,
 	.fasync		= sonypi_misc_fasync,
-	.ioctl		= sonypi_misc_ioctl,
+	.unlocked_ioctl	= sonypi_misc_ioctl,
 };
 
 static struct miscdevice sonypi_misc_device = {
-- 
1.6.0.6

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Arnd Bergmann]

On Monday 19 October 2009, John Kacur wrote:
> How does this look? (Version 2 of the patch follows)

Looks good now.

> From efd07cfcd021b4438d83d383ab81f9b35cb41eb9 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Sun, 18 Oct 2009 23:49:49 +0200
> Subject: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open
> 
> The BKL is in this function because of the BKL pushdown
> (see commit f8f2c79d594463427f7114cedb1555110d547d89)
> 
> It is not needed here because the mutex_lock sonypi_device.lock
> provides the necessary locking.
> 
> sonpi_misc_ioctl can be converted to unlocked ioctls since it relies on
> its own locking (the mutex sonypi_device.lock) and not the bkl
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Arnd Bergmann]

On Tuesday 20 October 2009, Arnd Bergmann wrote:
> On Monday 19 October 2009, John Kacur wrote:
> > How does this look? (Version 2 of the patch follows)
> 
> Looks good now.
> 

A bit of background:

Doing only one of the two conversions is a correct patch as well
of course, I just want to make sure you don't have to go through all
the same files again once someone does a blind pushdown into the ioctl
and llseek functions, so once you prove that a specific driver doesn't
need the BKL, please always make sure to remove it from all three places.

I fear that the llseek part will get interesting as well, just because
we call default_llseek instead of no_ll by default currently.
It might be a good idea to add one of .llseek=no_llseek or 
.llseek=generic_file_llseek in any file_operations that you prove
to not require the BKL.

	Arnd <><

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Mattia Dongili]

On Tue, Oct 20, 2009 at 12:00:24AM +0200, Arnd Bergmann wrote:
> On Monday 19 October 2009, John Kacur wrote:
> > How does this look? (Version 2 of the patch follows)
> 
> Looks good now.

Looks alright to me too.

> > From efd07cfcd021b4438d83d383ab81f9b35cb41eb9 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Sun, 18 Oct 2009 23:49:49 +0200
> > Subject: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open
> > 
> > The BKL is in this function because of the BKL pushdown
> > (see commit f8f2c79d594463427f7114cedb1555110d547d89)
> > 
> > It is not needed here because the mutex_lock sonypi_device.lock
> > provides the necessary locking.
> > 
> > sonpi_misc_ioctl can be converted to unlocked ioctls since it relies on
> > its own locking (the mutex sonypi_device.lock) and not the bkl
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Mattia Dongili <malattia@linux.it>
-- 
mattia
:wq!

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[John Kacur]

On Tue, 20 Oct 2009, Arnd Bergmann wrote:

> On Tuesday 20 October 2009, Arnd Bergmann wrote:
> > On Monday 19 October 2009, John Kacur wrote:
> > > How does this look? (Version 2 of the patch follows)
> > 
> > Looks good now.
> > 
> 
> A bit of background:
> 
> Doing only one of the two conversions is a correct patch as well
> of course, I just want to make sure you don't have to go through all
> the same files again once someone does a blind pushdown into the ioctl
> and llseek functions, so once you prove that a specific driver doesn't
> need the BKL, please always make sure to remove it from all three places.
> 
> I fear that the llseek part will get interesting as well, just because
> we call default_llseek instead of no_ll by default currently.
> It might be a good idea to add one of .llseek=no_llseek or 
> .llseek=generic_file_llseek in any file_operations that you prove
> to not require the BKL.
> 

Good point.

@Thomas: I'm sending this as a separate patch, but I can combine it with
the one that removes the bkl in the open and ioctl functions if you 
prefer.

>From 11e6a3b690413c3926e6db1c53a53410b5214c3d Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Wed, 21 Oct 2009 01:51:41 +0200
Subject: [PATCH] sonypi: Use non-BKL version of llseek.

The default version of llseek uses the BKL.
We have removed the use of the BKL in open and the ioctl.
Now lets remove the last use of the BKL by explictly calling the
generic unlocked llseek, under the sonypi_device.lock mutex

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

diff --git a/drivers/char/sonypi.c b/drivers/char/sonypi.c
index 593cbb5..55b08cd 100644
--- a/drivers/char/sonypi.c
+++ b/drivers/char/sonypi.c
@@ -1061,6 +1061,16 @@ static long sonypi_misc_ioctl(struct file *fp,
 	return ret;
 }
 
+static loff_t sonypi_misc_llseek(struct file *fp, loff_t offset, int origin)
+{
+	loff_t loff;
+	mutex_lock(&sonypi_device.lock);
+	loff = generic_file_llseek_unlocked(fp, offset, origin);
+	mutex_unlock(&sonypi_device.lock);
+
+	return loff;
+}
+
 static const struct file_operations sonypi_misc_fops = {
 	.owner		= THIS_MODULE,
 	.read		= sonypi_misc_read,
@@ -1069,6 +1079,7 @@ static const struct file_operations sonypi_misc_fops = {
 	.release	= sonypi_misc_release,
 	.fasync		= sonypi_misc_fasync,
 	.unlocked_ioctl	= sonypi_misc_ioctl,
+	.llseek		= sonypi_misc_llseek,
 };
 
 static struct miscdevice sonypi_misc_device = {
-- 
1.6.0.6

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Arnd Bergmann]

On Wednesday 21 October 2009, John Kacur wrote:
> From 11e6a3b690413c3926e6db1c53a53410b5214c3d Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Wed, 21 Oct 2009 01:51:41 +0200
> Subject: [PATCH] sonypi: Use non-BKL version of llseek.
> 
> The default version of llseek uses the BKL.
> We have removed the use of the BKL in open and the ioctl.
> Now lets remove the last use of the BKL by explictly calling the
> generic unlocked llseek, under the sonypi_device.lock mutex
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>

Well, for this specific case, the driver does not actually need to seek,
because the read function never looks at the position and there is no
write function. For annotation purposes, IMHO we should have a simple
'.llseek = no_llseek' in there.

In other files, the best solution may be to just point to generic_file_llseek
and make sure we hold the i_mutex when accessing the file->f_pos explicitly.

Interestingly, atomicity of updates to f_pos is currently maintained through
file_pos_write(), which does not hold any lock but assumes that a store to
an loff_t is atomic. It is not atomic in general, so concurrent lseek and
read/write operations seem to have undefined behaviour no matter what kind
of locking we have in the llseek function.

	Arnd <><

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[John Kacur]

On Wed, 21 Oct 2009, Arnd Bergmann wrote:

> On Wednesday 21 October 2009, John Kacur wrote:
> > From 11e6a3b690413c3926e6db1c53a53410b5214c3d Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Wed, 21 Oct 2009 01:51:41 +0200
> > Subject: [PATCH] sonypi: Use non-BKL version of llseek.
> > 
> > The default version of llseek uses the BKL.
> > We have removed the use of the BKL in open and the ioctl.
> > Now lets remove the last use of the BKL by explictly calling the
> > generic unlocked llseek, under the sonypi_device.lock mutex
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> 
> Well, for this specific case, the driver does not actually need to seek,
> because the read function never looks at the position and there is no
> write function. For annotation purposes, IMHO we should have a simple
> '.llseek = no_llseek' in there.
> 
> In other files, the best solution may be to just point to generic_file_llseek
> and make sure we hold the i_mutex when accessing the file->f_pos explicitly.
> 
> Interestingly, atomicity of updates to f_pos is currently maintained through
> file_pos_write(), which does not hold any lock but assumes that a store to
> an loff_t is atomic. It is not atomic in general, so concurrent lseek and
> read/write operations seem to have undefined behaviour no matter what kind
> of locking we have in the llseek function.

Thanks again Arnd for all the information you provide!

Here is the 3rd and hopefully final version of the patch. It is simple
the version that you acked, plus .llseek		= no_llseek,

>From 96872f13a510db69fbb32f9e956615cd826f8986 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 18 Oct 2009 23:49:49 +0200
Subject: [PATCH] sony_pi: Remove the BKL from open and ioctl

The BKL is in this function because of the BKL pushdown
(see commit f8f2c79d594463427f7114cedb1555110d547d89)

It is not needed here because the mutex_lock sonypi_device.lock
provides the necessary locking.

sonpi_misc_ioctl can be converted to unlocked ioctls since it relies on
its own locking (the mutex sonypi_device.lock) and not the bkl

Document that llseek is not needed by explictly setting it to no_llseek

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/char/sonypi.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/char/sonypi.c b/drivers/char/sonypi.c
index 8c262aa..3f68be3 100644
--- a/drivers/char/sonypi.c
+++ b/drivers/char/sonypi.c
@@ -50,7 +50,6 @@
 #include <linux/err.h>
 #include <linux/kfifo.h>
 #include <linux/platform_device.h>
-#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -902,14 +901,13 @@ static int sonypi_misc_release(struct inode *inode, struct file *file)
 
 static int sonypi_misc_open(struct inode *inode, struct file *file)
 {
-	lock_kernel();
 	mutex_lock(&sonypi_device.lock);
 	/* Flush input queue on first open */
 	if (!sonypi_device.open_count)
 		kfifo_reset(sonypi_device.fifo);
 	sonypi_device.open_count++;
 	mutex_unlock(&sonypi_device.lock);
-	unlock_kernel();
+
 	return 0;
 }
 
@@ -951,10 +949,10 @@ static unsigned int sonypi_misc_poll(struct file *file, poll_table *wait)
 	return 0;
 }
 
-static int sonypi_misc_ioctl(struct inode *ip, struct file *fp,
+static long sonypi_misc_ioctl(struct file *fp,
 			     unsigned int cmd, unsigned long arg)
 {
-	int ret = 0;
+	long ret = 0;
 	void __user *argp = (void __user *)arg;
 	u8 val8;
 	u16 val16;
@@ -1070,7 +1068,8 @@ static const struct file_operations sonypi_misc_fops = {
 	.open		= sonypi_misc_open,
 	.release	= sonypi_misc_release,
 	.fasync		= sonypi_misc_fasync,
-	.ioctl		= sonypi_misc_ioctl,
+	.unlocked_ioctl	= sonypi_misc_ioctl,
+	.llseek		= no_llseek,
 };
 
 static struct miscdevice sonypi_misc_device = {
-- 
1.6.0.6

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Arnd Bergmann]

On Wednesday 21 October 2009, John Kacur wrote:
> From 96872f13a510db69fbb32f9e956615cd826f8986 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Sun, 18 Oct 2009 23:49:49 +0200
> Subject: [PATCH] sony_pi: Remove the BKL from open and ioctl
> 
> The BKL is in this function because of the BKL pushdown
> (see commit f8f2c79d594463427f7114cedb1555110d547d89)
> 
> It is not needed here because the mutex_lock sonypi_device.lock
> provides the necessary locking.
> 
> sonpi_misc_ioctl can be converted to unlocked ioctls since it relies on
> its own locking (the mutex sonypi_device.lock) and not the bkl
> 
> Document that llseek is not needed by explictly setting it to no_llseek
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

This looks perfect to me now. Just a few hundred more of these,
and we're done with the drivers ;-)

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Frederic Weisbecker]

On Tue, Oct 20, 2009 at 12:08:57AM +0200, Arnd Bergmann wrote:
> On Tuesday 20 October 2009, Arnd Bergmann wrote:
> > On Monday 19 October 2009, John Kacur wrote:
> > > How does this look? (Version 2 of the patch follows)
> > 
> > Looks good now.
> > 
> 
> A bit of background:
> 
> Doing only one of the two conversions is a correct patch as well
> of course, I just want to make sure you don't have to go through all
> the same files again once someone does a blind pushdown into the ioctl
> and llseek functions, so once you prove that a specific driver doesn't
> need the BKL, please always make sure to remove it from all three places.
> 
> I fear that the llseek part will get interesting as well, just because
> we call default_llseek instead of no_ll by default currently.
> It might be a good idea to add one of .llseek=no_llseek or 
> .llseek=generic_file_llseek in any file_operations that you prove
> to not require the BKL.
> 
> 	Arnd <><


What about a pusdown of default_lseek attribution for these
fops that don't have any llseek() (and rename it to
deprecated_default_lseek() )

Because we can probably fix these fops one by one but what
about the next drivers that will have no llseek() ?

We can't attribute default_llseek() by default anymore for
further fops that are to come.

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[John Kacur]

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Tue, Oct 20, 2009 at 12:08:57AM +0200, Arnd Bergmann wrote:
> > On Tuesday 20 October 2009, Arnd Bergmann wrote:
> > > On Monday 19 October 2009, John Kacur wrote:
> > > > How does this look? (Version 2 of the patch follows)
> > > 
> > > Looks good now.
> > > 
> > 
> > A bit of background:
> > 
> > Doing only one of the two conversions is a correct patch as well
> > of course, I just want to make sure you don't have to go through all
> > the same files again once someone does a blind pushdown into the ioctl
> > and llseek functions, so once you prove that a specific driver doesn't
> > need the BKL, please always make sure to remove it from all three places.
> > 
> > I fear that the llseek part will get interesting as well, just because
> > we call default_llseek instead of no_ll by default currently.
> > It might be a good idea to add one of .llseek=no_llseek or 
> > .llseek=generic_file_llseek in any file_operations that you prove
> > to not require the BKL.
> > 
> > 	Arnd <><
> 
> 
> What about a pusdown of default_lseek attribution for these
> fops that don't have any llseek() (and rename it to
> deprecated_default_lseek() )
> 
> Because we can probably fix these fops one by one but what
> about the next drivers that will have no llseek() ?
> 
> We can't attribute default_llseek() by default anymore for
> further fops that are to come.
> 
> 

Frederic, I think it is still useful to explicity set to no_llseek, 
drivers that don't use llseek.

I also have to agree with you, that we should no longer be using a 
default_llseek that relies on the BKL.

That is a rather large effort though. All drivers that don't specify an 
llseek function, need to either set it to no_llseek, or as you are 
proposing a deprecated default_llseek that uses the bkl.

thinking of how to start this.

John

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:41:07PM +0200, John Kacur wrote:
> > What about a pusdown of default_lseek attribution for these
> > fops that don't have any llseek() (and rename it to
> > deprecated_default_lseek() )
> > 
> > Because we can probably fix these fops one by one but what
> > about the next drivers that will have no llseek() ?
> > 
> > We can't attribute default_llseek() by default anymore for
> > further fops that are to come.
> > 
> > 
> 
> Frederic, I think it is still useful to explicity set to no_llseek, 
> drivers that don't use llseek.


Yeah, I agreed.


 
> I also have to agree with you, that we should no longer be using a 
> default_llseek that relies on the BKL.
> 
> That is a rather large effort though. All drivers that don't specify an 
> llseek function, need to either set it to no_llseek, or as you are 
> proposing a deprecated default_llseek that uses the bkl.
> 
> thinking of how to start this.
> 
> John


This is a rather large effort indeed but this pushdown seems
the only way to remove default_llseek as the default llseek()
callback.

The more we wait, the more code we'll need to review and fix.

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[John Kacur]

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:41:07PM +0200, John Kacur wrote:
> > > What about a pusdown of default_lseek attribution for these
> > > fops that don't have any llseek() (and rename it to
> > > deprecated_default_lseek() )
> > > 
> > > Because we can probably fix these fops one by one but what
> > > about the next drivers that will have no llseek() ?
> > > 
> > > We can't attribute default_llseek() by default anymore for
> > > further fops that are to come.
> > > 
> > > 
> > 
> > Frederic, I think it is still useful to explicity set to no_llseek, 
> > drivers that don't use llseek.
> 
> 
> Yeah, I agreed.
> 
> 
>  
> > I also have to agree with you, that we should no longer be using a 
> > default_llseek that relies on the BKL.
> > 
> > That is a rather large effort though. All drivers that don't specify an 
> > llseek function, need to either set it to no_llseek, or as you are 
> > proposing a deprecated default_llseek that uses the bkl.
> > 
> > thinking of how to start this.
> > 
> > John
> 
> 
> This is a rather large effort indeed but this pushdown seems
> the only way to remove default_llseek as the default llseek()
> callback.
> 
> The more we wait, the more code we'll need to review and fix.
> 

Okay, I'm sure there is something wrong in this methodology, but it's late 
at night. At least for a ballpark figure, hopefully it's right.

Files that mentions "file_operations" - 
Files that mention "file_operations" and mention "llseek"
= 1172 - 596 = 572 (in my particular git repo)

So, over 550 files that need to be set to no_llseek, locked_llseek, or 
unlocked_llseek. Yikes!

[jkacur@tycho rt.linux.git]$ git-grep -l file_operations | grep -v 
Documentation | wc -l
1172
[jkacur@tycho rt.linux.git]$ git-grep -l llseek $(git-grep -l 
file_operations | grep -v Documentation) | wc -l
596

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Frederic Weisbecker]

On Thu, Oct 22, 2009 at 12:06:32AM +0200, John Kacur wrote:
> Okay, I'm sure there is something wrong in this methodology, but it's late 
> at night. At least for a ballpark figure, hopefully it's right.
> 
> Files that mentions "file_operations" - 
> Files that mention "file_operations" and mention "llseek"
> = 1172 - 596 = 572 (in my particular git repo)
> 
> So, over 550 files that need to be set to no_llseek, locked_llseek, or 
> unlocked_llseek. Yikes!
> 
> [jkacur@tycho rt.linux.git]$ git-grep -l file_operations | grep -v 
> Documentation | wc -l
> 1172
> [jkacur@tycho rt.linux.git]$ git-grep -l llseek $(git-grep -l 
> file_operations | grep -v Documentation) | wc -l
> 596
> 


So much?

Ok, a default_lseek pushdown patch wouldn't be accepted :)

Well, I guess we first need to fix the sites that explicitly use the
bkl, one by one, and after that probably propose a new locked version but
without the bkl...

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Christoph Hellwig]

On Wed, Oct 21, 2009 at 10:29:21AM +0200, Arnd Bergmann wrote:
> Well, for this specific case, the driver does not actually need to seek,
> because the read function never looks at the position and there is no
> write function. For annotation purposes, IMHO we should have a simple
> '.llseek = no_llseek' in there.

Which is normal for character drivers.  Even better you should use
nonseekable_open to also prevent pread/pwrite.

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Christoph Hellwig]

On Wed, Oct 21, 2009 at 11:31:42PM +0200, Frederic Weisbecker wrote:
> What about a pusdown of default_lseek attribution for these
> fops that don't have any llseek() (and rename it to
> deprecated_default_lseek() )
> 
> Because we can probably fix these fops one by one but what
> about the next drivers that will have no llseek() ?
> 
> We can't attribute default_llseek() by default anymore for
> further fops that are to come.

The right (although quite complicated) thing is to return -ESPIPE from
vfs_llseek if no ->llseek method is present, or even better also
disallowing pread/pwrite by default.  It'll need a quite substantial
audit and is best done by different types of inodes - S_IFIFO is easy,
SIFDIR at least has very few instances, S_IFREG usually wants a real
llseek (generic_file_llseek in most cases) and directories also need
a llseek method that takes i_mutex so it protects against namespace
operations.

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Arnd Bergmann]

On Thursday 22 October 2009, Christoph Hellwig wrote:

> The right (although quite complicated) thing is to return -ESPIPE from
> vfs_llseek if no ->llseek method is present, or even better also
> disallowing pread/pwrite by default.  It'll need a quite substantial
> audit and is best done by different types of inodes - S_IFIFO is easy,
> SIFDIR at least has very few instances, S_IFREG usually wants a real
> llseek (generic_file_llseek in most cases) and directories also need
> a llseek method that takes i_mutex so it protects against namespace
> operations.

Is it safe to assume that file_operations without a read() or write()
method also don't need llseek?

There are over 200 instances of file_operations that have a no read,
write or lseek operations and we can easily detect that in vfs_llseek,
calling no_llseek by default.

Testing for S_IFREG will not work well for debugfs, which is probably
a large number of the cases that do not want an llseek method.

	Arnd <><

Re: [PATCH] sony_pi: Remove the BKL from sonypi_misc_open

[Christoph Hellwig]

On Thu, Oct 22, 2009 at 03:50:04PM +0200, Arnd Bergmann wrote:
> Is it safe to assume that file_operations without a read() or write()
> method also don't need llseek?

There are two reasons why a driver could need llseek:

 (a) it uses the file position somewhere.  Normally that's just in
     read/write, but I wouldn't be surprised if there are drivers using
     the file position somewhere in weird ioctls.
 (b) because broken userland assumes they can seek on the file
     descriptor.  For example some versions of tar expect lseek to work
     on tape devices despite them not actually using the file position
     anywere.

So the answer to your above questions is: most likely yes, but and audit
for a) should be performed.  We can't do much about (b) except for trial
and error.  Unless there are very important applications expecting to be
able to seek I think returning the correct error is more important than
having zero change in behaviour.

> Testing for S_IFREG will not work well for debugfs, which is probably
> a large number of the cases that do not want an llseek method.

Yes.  S_IFREG should be done last, and probably the real filesystem
should be converted to always have a llseek method before tackling the
mess in the synthetic filesystems.