Commit 8702965848ed4bee27486a3e3d2ae34ebba6dd83 has push down the BKL
into uinput open function. However, there's nothing that needs locking
in there.
Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
---
drivers/input/misc/uinput.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index d3f5724..18206e1 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -34,7 +34,6 @@
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/init.h>
-#include <linux/smp_lock.h>
#include <linux/fs.h>
#include <linux/miscdevice.h>
#include <linux/uinput.h>
@@ -284,7 +283,6 @@ static int uinput_open(struct inode *inode, struct file *file)
if (!newdev)
return -ENOMEM;
- lock_kernel();
mutex_init(&newdev->mutex);
spin_lock_init(&newdev->requests_lock);
init_waitqueue_head(&newdev->requests_waitq);
@@ -292,7 +290,6 @@ static int uinput_open(struct inode *inode, struct file *file)
newdev->state = UIST_NEW_DEVICE;
file->private_data = newdev;
- unlock_kernel();
return 0;
}
--
1.6.6
On Friday 29 January 2010, Thadeu Lima de Souza Cascardo wrote:
> Commit 8702965848ed4bee27486a3e3d2ae34ebba6dd83 has push down the BKL
> into uinput open function. However, there's nothing that needs locking
> in there.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
The change looks good, but the same driver also uses the BKL in the
default_llseek function. It would be nice to get rid of that in the
same patch, e.g. by adding a ".llseek = generic_file_llseek," line
in the file_operations, or making it call nonseekable_open() if the
driver does not require seek to do anything.
Arnd
On Sat, Jan 30, 2010 at 07:41:20AM +0100, Arnd Bergmann wrote:
> On Friday 29 January 2010, Thadeu Lima de Souza Cascardo wrote:
> > Commit 8702965848ed4bee27486a3e3d2ae34ebba6dd83 has push down the BKL
> > into uinput open function. However, there's nothing that needs locking
> > in there.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
>
> The change looks good, but the same driver also uses the BKL in the
> default_llseek function. It would be nice to get rid of that in the
> same patch, e.g. by adding a ".llseek = generic_file_llseek," line
> in the file_operations, or making it call nonseekable_open() if the
> driver does not require seek to do anything.
>
I am afraid you mixed up the drivers, I don't see uinput implementing
seek...
Thanks.
--
Dmitry
On Fri, Jan 29, 2010 at 07:23:16PM -0200, Thadeu Lima de Souza Cascardo wrote:
> Commit 8702965848ed4bee27486a3e3d2ae34ebba6dd83 has push down the BKL
> into uinput open function. However, there's nothing that needs locking
> in there.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
Applied to next, thank you.
--
Dmitry
On Saturday 30 January 2010, Dmitry Torokhov wrote:
> On Sat, Jan 30, 2010 at 07:41:20AM +0100, Arnd Bergmann wrote:
> > The change looks good, but the same driver also uses the BKL in the
> > default_llseek function. It would be nice to get rid of that in the
> > same patch, e.g. by adding a ".llseek = generic_file_llseek," line
> > in the file_operations, or making it call nonseekable_open() if the
> > driver does not require seek to do anything.
> >
>
> I am afraid you mixed up the drivers, I don't see uinput implementing
> seek...
Sorry, I should have been clearer, but not implementing llseek
is the problem I was referring to: When a driver has no explicit
.llseek operation in its file operations and does not call
nonseekable_open from its open operation, the VFS layer will
implicitly use default_llseek, which takes the BKL. We're
in the process of changing drivers not to do this, one by one
so we can kill the BKL in the end.
Arnd
On Sat, Jan 30, 2010 at 10:57 PM, Arnd Bergmann <[email protected]> wrote:
> On Saturday 30 January 2010, Dmitry Torokhov wrote:
>> On Sat, Jan 30, 2010 at 07:41:20AM +0100, Arnd Bergmann wrote:
>> > The change looks good, but the same driver also uses the BKL in the
>> > default_llseek function. It would be nice to get rid of that in the
>> > same patch, e.g. by adding a ".llseek = generic_file_llseek," line
>> > in the file_operations, or making it call nonseekable_open() if the
>> > driver does not require seek to do anything.
>> >
>>
>> I am afraid you mixed up the drivers, I don't see uinput implementing
>> seek...
>
> Sorry, I should have been clearer, but not implementing llseek
> is the problem I was referring to: When a driver has no explicit
> .llseek operation in its file operations and does not call
> nonseekable_open from its open operation, the VFS layer will
> implicitly use default_llseek, which takes the BKL. We're
> in the process of changing drivers not to do this, one by one
> so we can kill the BKL in the end.
>
I know we've discussed this before, but why wouldn't the following
make more sense?
.llseek = no_llseek,
On Sunday 31 January 2010, John Kacur wrote:
> > Sorry, I should have been clearer, but not implementing llseek
> > is the problem I was referring to: When a driver has no explicit
> > .llseek operation in its file operations and does not call
> > nonseekable_open from its open operation, the VFS layer will
> > implicitly use default_llseek, which takes the BKL. We're
> > in the process of changing drivers not to do this, one by one
> > so we can kill the BKL in the end.
> >
>
> I know we've discussed this before, but why wouldn't the following
> make more sense?
> .llseek = no_llseek,
That's one of the possible solutions. Assigning it to generic_file_llseek
also gets rid of the BKL but keeps the current behaviour (calling seek
returns success without having an effect, no_llseek returns -ESPIPE),
while calling nonseekable_open has the other side-effect of making
pread/pwrite fail with -ESPIPE, which is more consistent than
only failing seek.
Arnd
On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
> On Sunday 31 January 2010, John Kacur wrote:
> > > Sorry, I should have been clearer, but not implementing llseek
> > > is the problem I was referring to: When a driver has no explicit
> > > .llseek operation in its file operations and does not call
> > > nonseekable_open from its open operation, the VFS layer will
> > > implicitly use default_llseek, which takes the BKL. We're
> > > in the process of changing drivers not to do this, one by one
> > > so we can kill the BKL in the end.
> > >
> >
> > I know we've discussed this before, but why wouldn't the following
> > make more sense?
> > .llseek = no_llseek,
>
> That's one of the possible solutions. Assigning it to generic_file_llseek
> also gets rid of the BKL but keeps the current behaviour (calling seek
> returns success without having an effect, no_llseek returns -ESPIPE),
> while calling nonseekable_open has the other side-effect of making
> pread/pwrite fail with -ESPIPE, which is more consistent than
> only failing seek.
>
OK, so how about the patch below (on top of Thadeu's patch)?
--
Dmitry
Input: uinput - use nonseekable_open
Seeking does not make sense for uinput so let's use nonseekable_open
to mark the device non-seekable.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/input/misc/uinput.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 18206e1..7089151 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
static int uinput_open(struct inode *inode, struct file *file)
{
struct uinput_device *newdev;
+ int error;
newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
if (!newdev)
@@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
file->private_data = newdev;
+ error = nonseekable_open(inode, file);
+ if (error) {
+ kfree(newdev);
+ return error;
+ }
+
return 0;
}
On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov
<[email protected]> wrote:
> On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
>> On Sunday 31 January 2010, John Kacur wrote:
>> > > Sorry, I should have been clearer, but not implementing llseek
>> > > is the problem I was referring to: When a driver has no explicit
>> > > .llseek operation in its file operations and does not call
>> > > nonseekable_open from its open operation, the VFS layer will
>> > > implicitly use default_llseek, which takes the BKL. We're
>> > > in the process of changing drivers not to do this, one by one
>> > > so we can kill the BKL in the end.
>> > >
>> >
>> > I know we've discussed this before, but why wouldn't the following
>> > make more sense?
>> > ?.llseek ? ? ? ? = no_llseek,
>>
>> That's one of the possible solutions. Assigning it to generic_file_llseek
>> also gets rid of the BKL but keeps the current behaviour (calling seek
>> returns success without having an effect, no_llseek returns -ESPIPE),
>> while calling nonseekable_open has the other side-effect of making
>> pread/pwrite fail with -ESPIPE, which is more consistent than
>> only failing seek.
>>
>
> OK, so how about the patch below (on top of Thadeu's patch)?
>
> --
> Dmitry
>
> Input: uinput - use nonseekable_open
>
> Seeking does not make sense for uinput so let's use nonseekable_open
> to mark the device non-seekable.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> ?drivers/input/misc/uinput.c | ? ?7 +++++++
> ?1 files changed, 7 insertions(+), 0 deletions(-)
>
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 18206e1..7089151 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
> ?static int uinput_open(struct inode *inode, struct file *file)
> ?{
> ? ? ? ?struct uinput_device *newdev;
> + ? ? ? int error;
>
> ? ? ? ?newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
> ? ? ? ?if (!newdev)
> @@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
>
> ? ? ? ?file->private_data = newdev;
>
> + ? ? ? error = nonseekable_open(inode, file);
> + ? ? ? if (error) {
> + ? ? ? ? ? ? ? kfree(newdev);
> + ? ? ? ? ? ? ? return error;
> + ? ? ? }
> +
> ? ? ? ?return 0;
> ?}
>
>
Hmnn, if you look at nonseekable_open() it will always return 0. I
think you can just do the following.
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 18206e1..697c0a6 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -291,7 +291,7 @@ static int uinput_open(struct inode *inode, struct file *fil
file->private_data = newdev;
- return 0;
+ return nonseekable_open(inode, file);
}
Signed-off-by: John Kacur <[email protected]>
On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <[email protected]> wrote:
> On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov
> <[email protected]> wrote:
>> On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
>>> On Sunday 31 January 2010, John Kacur wrote:
>>> > > Sorry, I should have been clearer, but not implementing llseek
>>> > > is the problem I was referring to: When a driver has no explicit
>>> > > .llseek operation in its file operations and does not call
>>> > > nonseekable_open from its open operation, the VFS layer will
>>> > > implicitly use default_llseek, which takes the BKL. We're
>>> > > in the process of changing drivers not to do this, one by one
>>> > > so we can kill the BKL in the end.
>>> > >
>>> >
>>> > I know we've discussed this before, but why wouldn't the following
>>> > make more sense?
>>> > ?.llseek ? ? ? ? = no_llseek,
>>>
>>> That's one of the possible solutions. Assigning it to generic_file_llseek
>>> also gets rid of the BKL but keeps the current behaviour (calling seek
>>> returns success without having an effect, no_llseek returns -ESPIPE),
>>> while calling nonseekable_open has the other side-effect of making
>>> pread/pwrite fail with -ESPIPE, which is more consistent than
>>> only failing seek.
>>>
>>
>> OK, so how about the patch below (on top of Thadeu's patch)?
>>
>> --
>> Dmitry
>>
>> Input: uinput - use nonseekable_open
>>
>> Seeking does not make sense for uinput so let's use nonseekable_open
>> to mark the device non-seekable.
>>
>> Signed-off-by: Dmitry Torokhov <[email protected]>
>> ---
>>
>> ?drivers/input/misc/uinput.c | ? ?7 +++++++
>> ?1 files changed, 7 insertions(+), 0 deletions(-)
>>
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index 18206e1..7089151 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
>> ?static int uinput_open(struct inode *inode, struct file *file)
>> ?{
>> ? ? ? ?struct uinput_device *newdev;
>> + ? ? ? int error;
>>
>> ? ? ? ?newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
>> ? ? ? ?if (!newdev)
>> @@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
>>
>> ? ? ? ?file->private_data = newdev;
>>
>> + ? ? ? error = nonseekable_open(inode, file);
>> + ? ? ? if (error) {
>> + ? ? ? ? ? ? ? kfree(newdev);
>> + ? ? ? ? ? ? ? return error;
>> + ? ? ? }
>> +
>> ? ? ? ?return 0;
>> ?}
>>
>>
>
> Hmnn, if you look at nonseekable_open() it will always return 0. I
> think you can just do the following.
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 18206e1..697c0a6 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -291,7 +291,7 @@ static int uinput_open(struct inode *inode, struct file *fil
>
> ? ? ? ?file->private_data = newdev;
>
> - ? ? ? return 0;
> + ? ? ? return nonseekable_open(inode, file);
> ?}
>
> Signed-off-by: John Kacur <[email protected]>
>
Btw, Thadeu Lima de Souza Cascardo should just combine that all into
one patch, no point really in making two patches out of that.
On Mon, Feb 01, 2010 at 09:27:22PM +0100, John Kacur wrote:
> On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <[email protected]> wrote:
> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov
> > <[email protected]> wrote:
> >> On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
> >>> On Sunday 31 January 2010, John Kacur wrote:
> >>> > > Sorry, I should have been clearer, but not implementing llseek
> >>> > > is the problem I was referring to: When a driver has no explicit
> >>> > > .llseek operation in its file operations and does not call
> >>> > > nonseekable_open from its open operation, the VFS layer will
> >>> > > implicitly use default_llseek, which takes the BKL. We're
> >>> > > in the process of changing drivers not to do this, one by one
> >>> > > so we can kill the BKL in the end.
> >>> > >
> >>> >
> >>> > I know we've discussed this before, but why wouldn't the following
> >>> > make more sense?
> >>> > .llseek = no_llseek,
> >>>
> >>> That's one of the possible solutions. Assigning it to generic_file_llseek
> >>> also gets rid of the BKL but keeps the current behaviour (calling seek
> >>> returns success without having an effect, no_llseek returns -ESPIPE),
> >>> while calling nonseekable_open has the other side-effect of making
> >>> pread/pwrite fail with -ESPIPE, which is more consistent than
> >>> only failing seek.
> >>>
> >>
> >> OK, so how about the patch below (on top of Thadeu's patch)?
> >>
> >> --
> >> Dmitry
> >>
> >> Input: uinput - use nonseekable_open
> >>
> >> Seeking does not make sense for uinput so let's use nonseekable_open
> >> to mark the device non-seekable.
> >>
> >> Signed-off-by: Dmitry Torokhov <[email protected]>
> >> ---
> >>
> >> drivers/input/misc/uinput.c | 7 +++++++
> >> 1 files changed, 7 insertions(+), 0 deletions(-)
> >>
> >>
> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> >> index 18206e1..7089151 100644
> >> --- a/drivers/input/misc/uinput.c
> >> +++ b/drivers/input/misc/uinput.c
> >> @@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
> >> static int uinput_open(struct inode *inode, struct file *file)
> >> {
> >> struct uinput_device *newdev;
> >> + int error;
> >>
> >> newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
> >> if (!newdev)
> >> @@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
> >>
> >> file->private_data = newdev;
> >>
> >> + error = nonseekable_open(inode, file);
> >> + if (error) {
> >> + kfree(newdev);
> >> + return error;
> >> + }
> >> +
> >> return 0;
> >> }
> >>
> >>
> >
> > Hmnn, if you look at nonseekable_open() it will always return 0. I
> > think you can just do the following.
> >
> > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> > index 18206e1..697c0a6 100644
> > --- a/drivers/input/misc/uinput.c
> > +++ b/drivers/input/misc/uinput.c
> > @@ -291,7 +291,7 @@ static int uinput_open(struct inode *inode, struct file *fil
> >
> > file->private_data = newdev;
> >
> > - return 0;
> > + return nonseekable_open(inode, file);
> > }
> >
> > Signed-off-by: John Kacur <[email protected]>
> >
>
> Btw, Thadeu Lima de Souza Cascardo should just combine that all into
> one patch, no point really in making two patches out of that.
That's fine to me. But since Dmitry has already applied it, I see no
problem at all that this is two commits. Or would there be any problem
removing the lock in open and not doing nonseekable_open?
As far as I get, nonseekable_open only resets the flags that will make
it do the right thing for lseek, pread and pwrite. This will get rid of
the BKL for these calls, but this is independent of getting rid of it
for the open call.
I don't disagree that doing both at the same time is OK. But I don't
agree that doing them separately is not OK. This way, you may get the
credits for what you (and not I) have done. :-)
But either way is fine for me.
Regards,
Cascardo.
On Mon, Feb 1, 2010 at 9:46 PM, Thadeu Lima de Souza Cascardo
<[email protected]> wrote:
> On Mon, Feb 01, 2010 at 09:27:22PM +0100, John Kacur wrote:
>> On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <[email protected]> wrote:
>> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov
>> > <[email protected]> wrote:
>> >> On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
>> >>> On Sunday 31 January 2010, John Kacur wrote:
>> >>> > > Sorry, I should have been clearer, but not implementing llseek
>> >>> > > is the problem I was referring to: When a driver has no explicit
>> >>> > > .llseek operation in its file operations and does not call
>> >>> > > nonseekable_open from its open operation, the VFS layer will
>> >>> > > implicitly use default_llseek, which takes the BKL. We're
>> >>> > > in the process of changing drivers not to do this, one by one
>> >>> > > so we can kill the BKL in the end.
>> >>> > >
>> >>> >
>> >>> > I know we've discussed this before, but why wouldn't the following
>> >>> > make more sense?
>> >>> > ?.llseek ? ? ? ? = no_llseek,
>> >>>
>> >>> That's one of the possible solutions. Assigning it to generic_file_llseek
>> >>> also gets rid of the BKL but keeps the current behaviour (calling seek
>> >>> returns success without having an effect, no_llseek returns -ESPIPE),
>> >>> while calling nonseekable_open has the other side-effect of making
>> >>> pread/pwrite fail with -ESPIPE, which is more consistent than
>> >>> only failing seek.
>> >>>
>> >>
>> >> OK, so how about the patch below (on top of Thadeu's patch)?
>> >>
>> >> --
>> >> Dmitry
>> >>
>> >> Input: uinput - use nonseekable_open
>> >>
>> >> Seeking does not make sense for uinput so let's use nonseekable_open
>> >> to mark the device non-seekable.
>> >>
>> >> Signed-off-by: Dmitry Torokhov <[email protected]>
>> >> ---
>> >>
>> >> ?drivers/input/misc/uinput.c | ? ?7 +++++++
>> >> ?1 files changed, 7 insertions(+), 0 deletions(-)
>> >>
>> >>
>> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> >> index 18206e1..7089151 100644
>> >> --- a/drivers/input/misc/uinput.c
>> >> +++ b/drivers/input/misc/uinput.c
>> >> @@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
>> >> ?static int uinput_open(struct inode *inode, struct file *file)
>> >> ?{
>> >> ? ? ? ?struct uinput_device *newdev;
>> >> + ? ? ? int error;
>> >>
>> >> ? ? ? ?newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
>> >> ? ? ? ?if (!newdev)
>> >> @@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
>> >>
>> >> ? ? ? ?file->private_data = newdev;
>> >>
>> >> + ? ? ? error = nonseekable_open(inode, file);
>> >> + ? ? ? if (error) {
>> >> + ? ? ? ? ? ? ? kfree(newdev);
>> >> + ? ? ? ? ? ? ? return error;
>> >> + ? ? ? }
>> >> +
>> >> ? ? ? ?return 0;
>> >> ?}
>> >>
>> >>
>> >
>> > Hmnn, if you look at nonseekable_open() it will always return 0. I
>> > think you can just do the following.
>> >
>> > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> > index 18206e1..697c0a6 100644
>> > --- a/drivers/input/misc/uinput.c
>> > +++ b/drivers/input/misc/uinput.c
>> > @@ -291,7 +291,7 @@ static int uinput_open(struct inode *inode, struct file *fil
>> >
>> > ? ? ? ?file->private_data = newdev;
>> >
>> > - ? ? ? return 0;
>> > + ? ? ? return nonseekable_open(inode, file);
>> > ?}
>> >
>> > Signed-off-by: John Kacur <[email protected]>
>> >
>>
>> Btw, Thadeu Lima de Souza Cascardo should just combine that all into
>> one patch, no point really in making two patches out of that.
>
> That's fine to me. But since Dmitry has already applied it, I see no
> problem at all that this is two commits. Or would there be any problem
> removing the lock in open and not doing nonseekable_open?
>
> As far as I get, nonseekable_open only resets the flags that will make
> it do the right thing for lseek, pread and pwrite. This will get rid of
> the BKL for these calls, but this is independent of getting rid of it
> for the open call.
>
> I don't disagree that doing both at the same time is OK. But I don't
> agree that doing them separately is not OK. This way, you may get the
> credits for what you (and not I) have done. ?:-)
>
> But either way is fine for me.
>
> Regards,
> Cascardo.
>
Ok, I didn't know that he already applied it. No need to make a big
deal about it, two commits are fine.
If he hadn't already applied it then it could logically go together as
one commit.
John
On Mon, Feb 01, 2010 at 09:27:22PM +0100, John Kacur wrote:
> On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <[email protected]> wrote:
> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov
> > <[email protected]> wrote:
> >> On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
> >>> On Sunday 31 January 2010, John Kacur wrote:
> >>> > > Sorry, I should have been clearer, but not implementing llseek
> >>> > > is the problem I was referring to: When a driver has no explicit
> >>> > > .llseek operation in its file operations and does not call
> >>> > > nonseekable_open from its open operation, the VFS layer will
> >>> > > implicitly use default_llseek, which takes the BKL. We're
> >>> > > in the process of changing drivers not to do this, one by one
> >>> > > so we can kill the BKL in the end.
> >>> > >
> >>> >
> >>> > I know we've discussed this before, but why wouldn't the following
> >>> > make more sense?
> >>> > ?.llseek ? ? ? ? = no_llseek,
> >>>
> >>> That's one of the possible solutions. Assigning it to generic_file_llseek
> >>> also gets rid of the BKL but keeps the current behaviour (calling seek
> >>> returns success without having an effect, no_llseek returns -ESPIPE),
> >>> while calling nonseekable_open has the other side-effect of making
> >>> pread/pwrite fail with -ESPIPE, which is more consistent than
> >>> only failing seek.
> >>>
> >>
> >> OK, so how about the patch below (on top of Thadeu's patch)?
> >>
> >> --
> >> Dmitry
> >>
> >> Input: uinput - use nonseekable_open
> >>
> >> Seeking does not make sense for uinput so let's use nonseekable_open
> >> to mark the device non-seekable.
> >>
> >> Signed-off-by: Dmitry Torokhov <[email protected]>
> >> ---
> >>
> >> ?drivers/input/misc/uinput.c | ? ?7 +++++++
> >> ?1 files changed, 7 insertions(+), 0 deletions(-)
> >>
> >>
> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> >> index 18206e1..7089151 100644
> >> --- a/drivers/input/misc/uinput.c
> >> +++ b/drivers/input/misc/uinput.c
> >> @@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
> >> ?static int uinput_open(struct inode *inode, struct file *file)
> >> ?{
> >> ? ? ? ?struct uinput_device *newdev;
> >> + ? ? ? int error;
> >>
> >> ? ? ? ?newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
> >> ? ? ? ?if (!newdev)
> >> @@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
> >>
> >> ? ? ? ?file->private_data = newdev;
> >>
> >> + ? ? ? error = nonseekable_open(inode, file);
> >> + ? ? ? if (error) {
> >> + ? ? ? ? ? ? ? kfree(newdev);
> >> + ? ? ? ? ? ? ? return error;
> >> + ? ? ? }
> >> +
> >> ? ? ? ?return 0;
> >> ?}
> >>
> >>
> >
> > Hmnn, if you look at nonseekable_open() it will always return 0. I
> > think you can just do the following.
It always returns 0 _now_ but I do not see any guarantees that it will
never ever return anything but 0. If somebody would provide such
garantee then we certainly would not need to handle errors.
> >
> > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> > index 18206e1..697c0a6 100644
> > --- a/drivers/input/misc/uinput.c
> > +++ b/drivers/input/misc/uinput.c
> > @@ -291,7 +291,7 @@ static int uinput_open(struct inode *inode, struct file *fil
> >
> > ? ? ? ?file->private_data = newdev;
> >
> > - ? ? ? return 0;
> > + ? ? ? return nonseekable_open(inode, file);
> > ?}
> >
> > Signed-off-by: John Kacur <[email protected]>
> >
>
> Btw, Thadeu Lima de Souza Cascardo should just combine that all into
> one patch, no point really in making two patches out of that.
I think these are 2 separate changes (the fact that nonseekable_open
also gets rid of BKL invocation is a side-effect), that is not
considering the fact that I already applied Thadeu's change and don't
want to rewind my public branch unless really necessary.
Thanks.
--
Dmitry
On Mon, Feb 1, 2010 at 10:21 PM, Dmitry Torokhov
<[email protected]> wrote:
> On Mon, Feb 01, 2010 at 09:27:22PM +0100, John Kacur wrote:
>> On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <[email protected]> wrote:
>> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov
>> > <[email protected]> wrote:
>> >> On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
>> >>> On Sunday 31 January 2010, John Kacur wrote:
>> >>> > > Sorry, I should have been clearer, but not implementing llseek
>> >>> > > is the problem I was referring to: When a driver has no explicit
>> >>> > > .llseek operation in its file operations and does not call
>> >>> > > nonseekable_open from its open operation, the VFS layer will
>> >>> > > implicitly use default_llseek, which takes the BKL. We're
>> >>> > > in the process of changing drivers not to do this, one by one
>> >>> > > so we can kill the BKL in the end.
>> >>> > >
>> >>> >
>> >>> > I know we've discussed this before, but why wouldn't the following
>> >>> > make more sense?
>> >>> > ?.llseek ? ? ? ? = no_llseek,
>> >>>
>> >>> That's one of the possible solutions. Assigning it to generic_file_llseek
>> >>> also gets rid of the BKL but keeps the current behaviour (calling seek
>> >>> returns success without having an effect, no_llseek returns -ESPIPE),
>> >>> while calling nonseekable_open has the other side-effect of making
>> >>> pread/pwrite fail with -ESPIPE, which is more consistent than
>> >>> only failing seek.
>> >>>
>> >>
>> >> OK, so how about the patch below (on top of Thadeu's patch)?
>> >>
>> >> --
>> >> Dmitry
>> >>
>> >> Input: uinput - use nonseekable_open
>> >>
>> >> Seeking does not make sense for uinput so let's use nonseekable_open
>> >> to mark the device non-seekable.
>> >>
>> >> Signed-off-by: Dmitry Torokhov <[email protected]>
>> >> ---
>> >>
>> >> ?drivers/input/misc/uinput.c | ? ?7 +++++++
>> >> ?1 files changed, 7 insertions(+), 0 deletions(-)
>> >>
>> >>
>> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> >> index 18206e1..7089151 100644
>> >> --- a/drivers/input/misc/uinput.c
>> >> +++ b/drivers/input/misc/uinput.c
>> >> @@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
>> >> ?static int uinput_open(struct inode *inode, struct file *file)
>> >> ?{
>> >> ? ? ? ?struct uinput_device *newdev;
>> >> + ? ? ? int error;
>> >>
>> >> ? ? ? ?newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
>> >> ? ? ? ?if (!newdev)
>> >> @@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
>> >>
>> >> ? ? ? ?file->private_data = newdev;
>> >>
>> >> + ? ? ? error = nonseekable_open(inode, file);
>> >> + ? ? ? if (error) {
>> >> + ? ? ? ? ? ? ? kfree(newdev);
>> >> + ? ? ? ? ? ? ? return error;
>> >> + ? ? ? }
>> >> +
>> >> ? ? ? ?return 0;
>> >> ?}
>> >>
>> >>
>> >
>> > Hmnn, if you look at nonseekable_open() it will always return 0. I
>> > think you can just do the following.
>
> It always returns 0 _now_ but I do not see any guarantees that it will
> never ever return anything but 0. If somebody would provide such
> garantee then we certainly would not need to handle errors.
Well, all it's doing is changing the f_mode. If anyone ever changes
that function
to return anything other than 0 it will be their responsibility to go
fix all the
uses of it. If you do a git grep of nonseekable_open, you'll see that this
is a very common paradigm. (to return 0). It makes your code shorter,
and more readable. Plus, you are writing speculative code based on
what might exist in the future? Also, then should uinput_release be called?
If it is called will kfree be called twice on the same memory. If it
isn't called, is
that a problem because you've already done most of the work that requires
a call to uinput_destroy_device ?
>
>> >
>> > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> > index 18206e1..697c0a6 100644
>> > --- a/drivers/input/misc/uinput.c
>> > +++ b/drivers/input/misc/uinput.c
>> > @@ -291,7 +291,7 @@ static int uinput_open(struct inode *inode, struct file *fil
>> >
>> > ? ? ? ?file->private_data = newdev;
>> >
>> > - ? ? ? return 0;
>> > + ? ? ? return nonseekable_open(inode, file);
>> > ?}
>> >
>> > Signed-off-by: John Kacur <[email protected]>
>> >
>>
>> Btw, Thadeu Lima de Souza Cascardo just combine that all into
>> one patch, no point really in making two patches out of that.
>
> I think these are 2 separate changes (the fact that nonseekable_open
> also gets rid of BKL invocation is a side-effect), that is not
> considering the fact that I already applied Thadeu's change and don't
> want to rewind my public branch unless really necessary.
Yeah, I agree, it's a PITA to rewind a public branch, in fact, you
should revert it
if it necessary. But, no worries, it's not necessary here.
However, generally, when you get rid of the BKL you do it in all
functions at once
as Arnd points out in another mail.
John
On Mon, Feb 01, 2010 at 10:50:25PM +0100, John Kacur wrote:
> On Mon, Feb 1, 2010 at 10:21 PM, Dmitry Torokhov
> <[email protected]> wrote:
> > On Mon, Feb 01, 2010 at 09:27:22PM +0100, John Kacur wrote:
> >> On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <[email protected]> wrote:
> >> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov
> >> > <[email protected]> wrote:
> >> >> On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
> >> >>> On Sunday 31 January 2010, John Kacur wrote:
> >> >>> > > Sorry, I should have been clearer, but not implementing llseek
> >> >>> > > is the problem I was referring to: When a driver has no explicit
> >> >>> > > .llseek operation in its file operations and does not call
> >> >>> > > nonseekable_open from its open operation, the VFS layer will
> >> >>> > > implicitly use default_llseek, which takes the BKL. We're
> >> >>> > > in the process of changing drivers not to do this, one by one
> >> >>> > > so we can kill the BKL in the end.
> >> >>> > >
> >> >>> >
> >> >>> > I know we've discussed this before, but why wouldn't the following
> >> >>> > make more sense?
> >> >>> > ?.llseek ? ? ? ? = no_llseek,
> >> >>>
> >> >>> That's one of the possible solutions. Assigning it to generic_file_llseek
> >> >>> also gets rid of the BKL but keeps the current behaviour (calling seek
> >> >>> returns success without having an effect, no_llseek returns -ESPIPE),
> >> >>> while calling nonseekable_open has the other side-effect of making
> >> >>> pread/pwrite fail with -ESPIPE, which is more consistent than
> >> >>> only failing seek.
> >> >>>
> >> >>
> >> >> OK, so how about the patch below (on top of Thadeu's patch)?
> >> >>
> >> >> --
> >> >> Dmitry
> >> >>
> >> >> Input: uinput - use nonseekable_open
> >> >>
> >> >> Seeking does not make sense for uinput so let's use nonseekable_open
> >> >> to mark the device non-seekable.
> >> >>
> >> >> Signed-off-by: Dmitry Torokhov <[email protected]>
> >> >> ---
> >> >>
> >> >> ?drivers/input/misc/uinput.c | ? ?7 +++++++
> >> >> ?1 files changed, 7 insertions(+), 0 deletions(-)
> >> >>
> >> >>
> >> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> >> >> index 18206e1..7089151 100644
> >> >> --- a/drivers/input/misc/uinput.c
> >> >> +++ b/drivers/input/misc/uinput.c
> >> >> @@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
> >> >> ?static int uinput_open(struct inode *inode, struct file *file)
> >> >> ?{
> >> >> ? ? ? ?struct uinput_device *newdev;
> >> >> + ? ? ? int error;
> >> >>
> >> >> ? ? ? ?newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
> >> >> ? ? ? ?if (!newdev)
> >> >> @@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
> >> >>
> >> >> ? ? ? ?file->private_data = newdev;
> >> >>
> >> >> + ? ? ? error = nonseekable_open(inode, file);
> >> >> + ? ? ? if (error) {
> >> >> + ? ? ? ? ? ? ? kfree(newdev);
> >> >> + ? ? ? ? ? ? ? return error;
> >> >> + ? ? ? }
> >> >> +
> >> >> ? ? ? ?return 0;
> >> >> ?}
> >> >>
> >> >>
> >> >
> >> > Hmnn, if you look at nonseekable_open() it will always return 0. I
> >> > think you can just do the following.
> >
> > It always returns 0 _now_ but I do not see any guarantees that it will
> > never ever return anything but 0. If somebody would provide such
> > garantee then we certainly would not need to handle errors.
>
> Well, all it's doing is changing the f_mode. If anyone ever changes
> that function
> to return anything other than 0 it will be their responsibility to go
> fix all the
> uses of it.
No, not really.
> If you do a git grep of nonseekable_open, you'll see that this
> is a very common paradigm. (to return 0).
The reason for nonseekable_open return 0 is so that you can plug it
directly into fsops. The fact that many users abuse that and do:
return nonseekable_open(indoe, file);
when doing:
nonseekable_open(indoe, file);
return 0;
would not introduce any complexity if they dont want to handle errors at
this time, and would be much safer (and one could mark
nonseekable_open() __must_check down the road if it is ever changed
to actually fail), does not validate such practice in any way.
> It makes your code shorter,
> and more readable. Plus, you are writing speculative code based on
> what might exist in the future?
No, I try to write the code that handles errors from functions that
could return errors even if current implementation does not do that.
> Also, then should uinput_release be called?
> If it is called will kfree be called twice on the same memory. If it
> isn't called, is
> that a problem because you've already done most of the work that requires
> a call to uinput_destroy_device ?
Why would release be called if open failed?
--
Dmitry
On Mon, Feb 1, 2010 at 11:08 PM, Dmitry Torokhov
<[email protected]> wrote:
> On Mon, Feb 01, 2010 at 10:50:25PM +0100, John Kacur wrote:
>> On Mon, Feb 1, 2010 at 10:21 PM, Dmitry Torokhov
>> <[email protected]> wrote:
>> > On Mon, Feb 01, 2010 at 09:27:22PM +0100, John Kacur wrote:
>> >> On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <[email protected]> wrote:
>> >> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov
>> >> > <[email protected]> wrote:
>> >> >> On Sun, Jan 31, 2010 at 05:20:55AM +0100, Arnd Bergmann wrote:
>> >> >>> On Sunday 31 January 2010, John Kacur wrote:
>> >> >>> > > Sorry, I should have been clearer, but not implementing llseek
>> >> >>> > > is the problem I was referring to: When a driver has no explicit
>> >> >>> > > .llseek operation in its file operations and does not call
>> >> >>> > > nonseekable_open from its open operation, the VFS layer will
>> >> >>> > > implicitly use default_llseek, which takes the BKL. We're
>> >> >>> > > in the process of changing drivers not to do this, one by one
>> >> >>> > > so we can kill the BKL in the end.
>> >> >>> > >
>> >> >>> >
>> >> >>> > I know we've discussed this before, but why wouldn't the following
>> >> >>> > make more sense?
>> >> >>> > ?.llseek ? ? ? ? = no_llseek,
>> >> >>>
>> >> >>> That's one of the possible solutions. Assigning it to generic_file_llseek
>> >> >>> also gets rid of the BKL but keeps the current behaviour (calling seek
>> >> >>> returns success without having an effect, no_llseek returns -ESPIPE),
>> >> >>> while calling nonseekable_open has the other side-effect of making
>> >> >>> pread/pwrite fail with -ESPIPE, which is more consistent than
>> >> >>> only failing seek.
>> >> >>>
>> >> >>
>> >> >> OK, so how about the patch below (on top of Thadeu's patch)?
>> >> >>
>> >> >> --
>> >> >> Dmitry
>> >> >>
>> >> >> Input: uinput - use nonseekable_open
>> >> >>
>> >> >> Seeking does not make sense for uinput so let's use nonseekable_open
>> >> >> to mark the device non-seekable.
>> >> >>
>> >> >> Signed-off-by: Dmitry Torokhov <[email protected]>
>> >> >> ---
>> >> >>
>> >> >> ?drivers/input/misc/uinput.c | ? ?7 +++++++
>> >> >> ?1 files changed, 7 insertions(+), 0 deletions(-)
>> >> >>
>> >> >>
>> >> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> >> >> index 18206e1..7089151 100644
>> >> >> --- a/drivers/input/misc/uinput.c
>> >> >> +++ b/drivers/input/misc/uinput.c
>> >> >> @@ -278,6 +278,7 @@ static int uinput_create_device(struct uinput_device *udev)
>> >> >> ?static int uinput_open(struct inode *inode, struct file *file)
>> >> >> ?{
>> >> >> ? ? ? ?struct uinput_device *newdev;
>> >> >> + ? ? ? int error;
>> >> >>
>> >> >> ? ? ? ?newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
>> >> >> ? ? ? ?if (!newdev)
>> >> >> @@ -291,6 +292,12 @@ static int uinput_open(struct inode *inode, struct file *file)
>> >> >>
>> >> >> ? ? ? ?file->private_data = newdev;
>> >> >>
>> >> >> + ? ? ? error = nonseekable_open(inode, file);
>> >> >> + ? ? ? if (error) {
>> >> >> + ? ? ? ? ? ? ? kfree(newdev);
>> >> >> + ? ? ? ? ? ? ? return error;
>> >> >> + ? ? ? }
>> >> >> +
>> >> >> ? ? ? ?return 0;
>> >> >> ?}
>> >> >>
>> >> >>
>> >> >
>> >> > Hmnn, if you look at nonseekable_open() it will always return 0. I
>> >> > think you can just do the following.
>> >
>> > It always returns 0 _now_ but I do not see any guarantees that it will
>> > never ever return anything but 0. If somebody would provide such
>> > garantee then we certainly would not need to handle errors.
>>
>> Well, all it's doing is changing the f_mode. If anyone ever changes
>> that function
>> to return anything other than 0 it will be their responsibility to go
>> fix all the
>> uses of it.
>
> No, not really.
>
>> If you do a git grep of nonseekable_open, you'll see that this
>> is a very common paradigm. (to return 0).
>
> The reason for nonseekable_open return 0 is so that you can plug it
> directly into fsops. The fact that many users abuse that and do:
>
> ? ? ? ?return nonseekable_open(indoe, file);
>
> when doing:
>
> ? ? ? ?nonseekable_open(indoe, file);
> ? ? ? ?return 0;
>
> would not introduce any complexity if they dont want to handle errors at
> this time, and would be much safer (and one could mark
> nonseekable_open() __must_check down the road if it is ever changed
> to actually fail), does not validate such practice in any way.
>
>> It makes your code shorter,
>> and more readable. Plus, you are writing speculative code based on
>> what might exist in the future?
>
> No, I try to write the code that handles errors from functions that
> could return errors even if current implementation does not do that.
>
>> Also, then should uinput_release be called?
>> If it is called will kfree be called twice on the same memory. If it
>> isn't called, is
>> that a problem because you've already done most of the work that requires
>> a call to uinput_destroy_device ?
>
> Why would release be called if open failed?
Sorry, maybe I am doing a poor job of explaining myself. My question
was whether your driver needs to call uinput_release() or not if it
went through your proposed error path, because that is where you have
the call to the uinput_destroy_device() function.
After taking a fresh look at your code I don't believe that it does.
However, you could still hoist your code that calls nonseekable_open()
above all that init stuff in uinput_open(), just under the return
-ENOMEM if you think that it could fail.
However, I still think that nonseekable_open() is designed from the
"get-go" to never fail, so I think your code is unnecessarily
complicated, by just a little bit. It will still work, so you decide
which to go with. I'm fine with either way.
John
On Tue, Feb 02, 2010 at 12:18:35AM +0100, John Kacur wrote:
>
> Sorry, maybe I am doing a poor job of explaining myself. My question
> was whether your driver needs to call uinput_release() or not if it
> went through your proposed error path, because that is where you have
> the call to the uinput_destroy_device() function.
> After taking a fresh look at your code I don't believe that it does.
> However, you could still hoist your code that calls nonseekable_open()
> above all that init stuff in uinput_open(), just under the return
> -ENOMEM if you think that it could fail.
> However, I still think that nonseekable_open() is designed from the
> "get-go" to never fail, so I think your code is unnecessarily
> complicated, by just a little bit. It will still work, so you decide
> which to go with. I'm fine with either way.
>
OK, so how about the patch below? If it is accepted I will just switch
to
nonseekable_open(inode, file);
return 0;
style. I gonna add Al and akpm to CC to see if the patch will stick...
--
Dmitry
VFS: clarify that nonseekable_open() will never fail
Signed-off-by: Dmitry Torokhov <[email protected]>
---
fs/open.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/fs/open.c b/fs/open.c
index 040cef7..02ceb73 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1200,7 +1200,9 @@ EXPORT_SYMBOL(generic_file_open);
/*
* This is used by subsystems that don't want seekable
- * file descriptors
+ * file descriptors. The function is not supposed to ever fail, the only
+ * reason it returns an 'int' and not 'void' is so that it can be plugged
+ * directly into file_operations structure.
*/
int nonseekable_open(struct inode *inode, struct file *filp)
{
On Wednesday 03 February 2010, Dmitry Torokhov wrote:
> OK, so how about the patch below? If it is accepted I will just switch
> to
>
> nonseekable_open(inode, file);
> return 0;
>
> style. I gonna add Al and akpm to CC to see if the patch will stick...
Sounds reasonable, if only to prevent this from becoming a FAQ.
> VFS: clarify that nonseekable_open() will never fail
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
On Thu, Feb 4, 2010 at 8:32 AM, Arnd Bergmann <[email protected]> wrote:
> On Wednesday 03 February 2010, Dmitry Torokhov wrote:
>> OK, so how about the patch below? If it is accepted I will just switch
>> to
>>
>> ? ? ? ? nonseekable_open(inode, file);
>> ? ? ? ? return 0;
>>
>> style. I gonna add Al and akpm to CC to see if the patch will stick...
>
> Sounds reasonable, if only to prevent this from becoming a FAQ.
>
>> VFS: clarify that nonseekable_open() will never fail
>>
>> Signed-off-by: Dmitry Torokhov <[email protected]>
>
> Acked-by: Arnd Bergmann <[email protected]>
> --
Acked-by: John Kacur <[email protected]>