* [PATCH] input: remove BKL from uinput open function @ 2010-01-29 21:23 Thadeu Lima de Souza Cascardo 2010-01-30 6:41 ` Arnd Bergmann 2010-01-30 7:57 ` Dmitry Torokhov 0 siblings, 2 replies; 19+ messages in thread From: Thadeu Lima de Souza Cascardo @ 2010-01-29 21:23 UTC (permalink / raw) To: Arnd Bergmann Cc: Thadeu Lima de Souza Cascardo, Dmitry Torokhov, linux-input, linux-kernel 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 <cascardo@holoscopio.com> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-01-29 21:23 [PATCH] input: remove BKL from uinput open function Thadeu Lima de Souza Cascardo @ 2010-01-30 6:41 ` Arnd Bergmann 2010-01-30 7:22 ` Dmitry Torokhov 2010-01-30 7:57 ` Dmitry Torokhov 1 sibling, 1 reply; 19+ messages in thread From: Arnd Bergmann @ 2010-01-30 6:41 UTC (permalink / raw) To: Thadeu Lima de Souza Cascardo; +Cc: Dmitry Torokhov, linux-input, linux-kernel 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 <cascardo@holoscopio.com> 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-01-30 6:41 ` Arnd Bergmann @ 2010-01-30 7:22 ` Dmitry Torokhov 2010-01-30 21:57 ` Arnd Bergmann 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Torokhov @ 2010-01-30 7:22 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Thadeu Lima de Souza Cascardo, linux-input, linux-kernel 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 <cascardo@holoscopio.com> > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-01-30 7:22 ` Dmitry Torokhov @ 2010-01-30 21:57 ` Arnd Bergmann 2010-01-30 23:07 ` John Kacur 0 siblings, 1 reply; 19+ messages in thread From: Arnd Bergmann @ 2010-01-30 21:57 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Thadeu Lima de Souza Cascardo, linux-input, linux-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-01-30 21:57 ` Arnd Bergmann @ 2010-01-30 23:07 ` John Kacur 2010-01-31 4:20 ` Arnd Bergmann 0 siblings, 1 reply; 19+ messages in thread From: John Kacur @ 2010-01-30 23:07 UTC (permalink / raw) To: Arnd Bergmann Cc: Dmitry Torokhov, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel On Sat, Jan 30, 2010 at 10:57 PM, Arnd Bergmann <arnd@arndb.de> 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, ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-01-30 23:07 ` John Kacur @ 2010-01-31 4:20 ` Arnd Bergmann 2010-01-31 5:29 ` Dmitry Torokhov 0 siblings, 1 reply; 19+ messages in thread From: Arnd Bergmann @ 2010-01-31 4:20 UTC (permalink / raw) To: John Kacur Cc: Dmitry Torokhov, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-01-31 4:20 ` Arnd Bergmann @ 2010-01-31 5:29 ` Dmitry Torokhov 2010-02-01 20:22 ` John Kacur 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Torokhov @ 2010-01-31 5:29 UTC (permalink / raw) To: Arnd Bergmann Cc: John Kacur, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel 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 <dtor@mail.ru> --- 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; } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-01-31 5:29 ` Dmitry Torokhov @ 2010-02-01 20:22 ` John Kacur 2010-02-01 20:27 ` John Kacur 0 siblings, 1 reply; 19+ messages in thread From: John Kacur @ 2010-02-01 20:22 UTC (permalink / raw) To: Dmitry Torokhov Cc: Arnd Bergmann, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 <dtor@mail.ru> > --- > > 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 <jkacur@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-01 20:22 ` John Kacur @ 2010-02-01 20:27 ` John Kacur 2010-02-01 20:46 ` Thadeu Lima de Souza Cascardo 2010-02-01 21:21 ` Dmitry Torokhov 0 siblings, 2 replies; 19+ messages in thread From: John Kacur @ 2010-02-01 20:27 UTC (permalink / raw) To: Dmitry Torokhov Cc: Arnd Bergmann, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <jkacur@redhat.com> wrote: > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> 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 <dtor@mail.ru> >> --- >> >> 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 <jkacur@redhat.com> > Btw, Thadeu Lima de Souza Cascardo should just combine that all into one patch, no point really in making two patches out of that. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-01 20:27 ` John Kacur @ 2010-02-01 20:46 ` Thadeu Lima de Souza Cascardo 2010-02-01 21:04 ` John Kacur 2010-02-01 21:21 ` Dmitry Torokhov 1 sibling, 1 reply; 19+ messages in thread From: Thadeu Lima de Souza Cascardo @ 2010-02-01 20:46 UTC (permalink / raw) To: John Kacur; +Cc: Dmitry Torokhov, Arnd Bergmann, linux-input, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4261 bytes --] On Mon, Feb 01, 2010 at 09:27:22PM +0100, John Kacur wrote: > On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <jkacur@redhat.com> wrote: > > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> 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 <dtor@mail.ru> > >> --- > >> > >> 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 <jkacur@redhat.com> > > > > 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. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-01 20:46 ` Thadeu Lima de Souza Cascardo @ 2010-02-01 21:04 ` John Kacur 0 siblings, 0 replies; 19+ messages in thread From: John Kacur @ 2010-02-01 21:04 UTC (permalink / raw) To: Thadeu Lima de Souza Cascardo Cc: Dmitry Torokhov, Arnd Bergmann, linux-input, linux-kernel On Mon, Feb 1, 2010 at 9:46 PM, Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com> 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 <jkacur@redhat.com> wrote: >> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov >> > <dmitry.torokhov@gmail.com> 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 <dtor@mail.ru> >> >> --- >> >> >> >> 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 <jkacur@redhat.com> >> > >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-01 20:27 ` John Kacur 2010-02-01 20:46 ` Thadeu Lima de Souza Cascardo @ 2010-02-01 21:21 ` Dmitry Torokhov 2010-02-01 21:50 ` John Kacur 1 sibling, 1 reply; 19+ messages in thread From: Dmitry Torokhov @ 2010-02-01 21:21 UTC (permalink / raw) To: John Kacur Cc: Arnd Bergmann, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel On Mon, Feb 01, 2010 at 09:27:22PM +0100, John Kacur wrote: > On Mon, Feb 1, 2010 at 9:22 PM, John Kacur <jkacur@redhat.com> wrote: > > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> 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 <dtor@mail.ru> > >> --- > >> > >> 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 <jkacur@redhat.com> > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-01 21:21 ` Dmitry Torokhov @ 2010-02-01 21:50 ` John Kacur 2010-02-01 22:08 ` Dmitry Torokhov 0 siblings, 1 reply; 19+ messages in thread From: John Kacur @ 2010-02-01 21:50 UTC (permalink / raw) To: Dmitry Torokhov Cc: Arnd Bergmann, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel On Mon, Feb 1, 2010 at 10:21 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 <jkacur@redhat.com> wrote: >> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov >> > <dmitry.torokhov@gmail.com> 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 <dtor@mail.ru> >> >> --- >> >> >> >> 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 <jkacur@redhat.com> >> > >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-01 21:50 ` John Kacur @ 2010-02-01 22:08 ` Dmitry Torokhov 2010-02-01 23:18 ` John Kacur 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Torokhov @ 2010-02-01 22:08 UTC (permalink / raw) To: John Kacur Cc: Arnd Bergmann, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel On Mon, Feb 01, 2010 at 10:50:25PM +0100, John Kacur wrote: > On Mon, Feb 1, 2010 at 10:21 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> 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 <jkacur@redhat.com> wrote: > >> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov > >> > <dmitry.torokhov@gmail.com> 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 <dtor@mail.ru> > >> >> --- > >> >> > >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-01 22:08 ` Dmitry Torokhov @ 2010-02-01 23:18 ` John Kacur 2010-02-03 5:07 ` Dmitry Torokhov 0 siblings, 1 reply; 19+ messages in thread From: John Kacur @ 2010-02-01 23:18 UTC (permalink / raw) To: Dmitry Torokhov Cc: Arnd Bergmann, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel On Mon, Feb 1, 2010 at 11:08 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 >> <dmitry.torokhov@gmail.com> 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 <jkacur@redhat.com> wrote: >> >> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov >> >> > <dmitry.torokhov@gmail.com> 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 <dtor@mail.ru> >> >> >> --- >> >> >> >> >> >> á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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-01 23:18 ` John Kacur @ 2010-02-03 5:07 ` Dmitry Torokhov 2010-02-04 7:32 ` Arnd Bergmann 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Torokhov @ 2010-02-03 5:07 UTC (permalink / raw) To: John Kacur Cc: Arnd Bergmann, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel, Al Viro, Andrew Morton 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 <dtor@mail.ru> --- 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) { ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-03 5:07 ` Dmitry Torokhov @ 2010-02-04 7:32 ` Arnd Bergmann 2010-02-05 16:04 ` John Kacur 0 siblings, 1 reply; 19+ messages in thread From: Arnd Bergmann @ 2010-02-04 7:32 UTC (permalink / raw) To: Dmitry Torokhov Cc: John Kacur, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel, Al Viro, Andrew Morton 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 <dtor@mail.ru> Acked-by: Arnd Bergmann <arnd@arndb.de> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-02-04 7:32 ` Arnd Bergmann @ 2010-02-05 16:04 ` John Kacur 0 siblings, 0 replies; 19+ messages in thread From: John Kacur @ 2010-02-05 16:04 UTC (permalink / raw) To: Arnd Bergmann Cc: Dmitry Torokhov, Thadeu Lima de Souza Cascardo, linux-input, linux-kernel, Al Viro, Andrew Morton On Thu, Feb 4, 2010 at 8:32 AM, Arnd Bergmann <arnd@arndb.de> 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 <dtor@mail.ru> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > -- Acked-by: John Kacur <jkacur@redhat.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] input: remove BKL from uinput open function 2010-01-29 21:23 [PATCH] input: remove BKL from uinput open function Thadeu Lima de Souza Cascardo 2010-01-30 6:41 ` Arnd Bergmann @ 2010-01-30 7:57 ` Dmitry Torokhov 1 sibling, 0 replies; 19+ messages in thread From: Dmitry Torokhov @ 2010-01-30 7:57 UTC (permalink / raw) To: Thadeu Lima de Souza Cascardo; +Cc: Arnd Bergmann, linux-input, linux-kernel 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 <cascardo@holoscopio.com> Applied to next, thank you. -- Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2010-02-05 16:04 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-01-29 21:23 [PATCH] input: remove BKL from uinput open function Thadeu Lima de Souza Cascardo 2010-01-30 6:41 ` Arnd Bergmann 2010-01-30 7:22 ` Dmitry Torokhov 2010-01-30 21:57 ` Arnd Bergmann 2010-01-30 23:07 ` John Kacur 2010-01-31 4:20 ` Arnd Bergmann 2010-01-31 5:29 ` Dmitry Torokhov 2010-02-01 20:22 ` John Kacur 2010-02-01 20:27 ` John Kacur 2010-02-01 20:46 ` Thadeu Lima de Souza Cascardo 2010-02-01 21:04 ` John Kacur 2010-02-01 21:21 ` Dmitry Torokhov 2010-02-01 21:50 ` John Kacur 2010-02-01 22:08 ` Dmitry Torokhov 2010-02-01 23:18 ` John Kacur 2010-02-03 5:07 ` Dmitry Torokhov 2010-02-04 7:32 ` Arnd Bergmann 2010-02-05 16:04 ` John Kacur 2010-01-30 7:57 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).