Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754588Ab0BAVub (ORCPT ); Mon, 1 Feb 2010 16:50:31 -0500 Received: from mail-ew0-f228.google.com ([209.85.219.228]:63452 "EHLO mail-ew0-f228.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754264Ab0BAVu2 convert rfc822-to-8bit (ORCPT ); Mon, 1 Feb 2010 16:50:28 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=Ij0jYb7ZRFt748K6XlVgMH7gww09TlHsTi8ojzxKY3kb9C8Q8LP/MR9nv5S6H8oWdJ bRIit6orSrxMC3lKvmOSVEFbj5lcI9hSNxF3LJtV8ncYCvSPkeonXYkSf8W3GNCzlOGl hOOpSIyQIpb+BkZ5/96GDT4w6t4D+L/NOuwPI= MIME-Version: 1.0 In-Reply-To: <20100201212132.GA7380@core.coreip.homeip.net> References: <1264800197-29523-1-git-send-email-cascardo@holoscopio.com> <201001302257.09354.arnd@arndb.de> <520f0cf11001301507k20e3cf8dqa73026e12f3a1767@mail.gmail.com> <201001310520.55813.arnd@arndb.de> <20100131052942.GA12320@core.coreip.homeip.net> <520f0cf11002011222h134dbf06rf1db612da9a9728@mail.gmail.com> <520f0cf11002011227s74e57673j3922941f7ee87989@mail.gmail.com> <20100201212132.GA7380@core.coreip.homeip.net> Date: Mon, 1 Feb 2010 22:50:25 +0100 Message-ID: <520f0cf11002011350u3b541a0cxfb0ed882dca13afe@mail.gmail.com> Subject: Re: [PATCH] input: remove BKL from uinput open function From: John Kacur To: Dmitry Torokhov Cc: Arnd Bergmann , Thadeu Lima de Souza Cascardo , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5215 Lines: 131 On Mon, Feb 1, 2010 at 10:21 PM, Dmitry Torokhov 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 wrote: >> > On Sun, Jan 31, 2010 at 6:29 AM, Dmitry Torokhov >> > 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 >> >> --- >> >> >> >> ?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 >> > >> >> 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/