Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
> Hi Andi,
> I grepped and tried to do what you suggested.
> The first file that git grep reported was:
> arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
>
> So I cooked up the following patch (probably mangled, just to give you
> a rough idea of what I did):
> diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
> index bf1075e..19dedb5 100644
> --- a/arch/arm/common/rtctime.c
> +++ b/arch/arm/common/rtctime.c
> @@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
> file *file, unsigned int cmd,
> if (ret)
> break;
> ret = copy_to_user(uarg, , sizeof(tm));
> - if (ret)
> + if (ret) {
> + unlock_kernel();
> ret = -EFAULT;
> + }
> break;
Just put unlock kernel near return ret;...
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Hi Andi,
I grepped and tried to do what you suggested.
The first file that git grep reported was:
arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
So I cooked up the following patch (probably mangled, just to give you
a rough idea of what I did):
diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
index bf1075e..19dedb5 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
if (ret)
break;
ret = copy_to_user(uarg, alrm.time, sizeof(tm));
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
+ }
break;
Just put unlock kernel near return ret;...
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Thursday 10 January 2008 03:39, Alasdair G Kergon wrote: > On Thu, Jan 10, 2008 at 01:49:15AM -0800, Daniel Phillips wrote: > > So what stops you from changing to unlocked_ioctl for the main > > device mapper ctl_ioctl? > > Nothing - patches to do this are queued for 2.6.25: Nice. This removes a deadlock we hit, where if creating a device mapper target blocks indefinitely (say on network IO) then nobody else can complete a device mapper operation because BKL is held. If completing the device create depends on some other device mapper operation, then it is game over. Our current workaround is to test for and drop BKL, ugh. Thanks for the cleanup. Regards, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Thu, Jan 10, 2008 at 01:49:15AM -0800, Daniel Phillips wrote: > So what stops you from changing to unlocked_ioctl for the main device > mapper ctl_ioctl? Nothing - patches to do this are queued for 2.6.25: http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-ioctl-remove-lock_kernel.patch http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-ioctl-move-compat-code.patch Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
> So if I write my own driver and have never heard of ioctls running on BKL > before I can rather be sure that I just can change the interface of the ioctl > function, put it in unlocked_ioctl and are fine? Cool. If you know the BKL is not needed in your code you should use unlocked_ioctl correct. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Andi Kleen wrote: > > Can you explain the rationale behind that running on the BKL? What type > > of > > It used to always run with the BKL because everything used to > and originally nobody wanted to review all ioctl handlers in tree to see if > they can run with more fine grained locking. A lot probably can though. > > > things needs to be protected so that this huge hammer is needed? What > > would be an earlier point to release the BKL? > > That depends on the driver. A lot don't need BKL at all and > in others it can be easily eliminated. But it needs case-by-case > review of the locking situation. > > The goal of the proposal here is just to make it more visible. So if I write my own driver and have never heard of ioctls running on BKL before I can rather be sure that I just can change the interface of the ioctl function, put it in unlocked_ioctl and are fine? Cool. Eike signature.asc Description: This is a digitally signed message part.
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Hi Alasdair, On Wednesday 09 January 2008 14:40, Alasdair G Kergon wrote: > Device-mapper for example in dm_blk_ioctl(), has no need for BKL so > drops it immediately, but it does need the inode parameter, so it is > unable to switch as things stand. So what stops you from changing to unlocked_ioctl for the main device mapper ctl_ioctl? You aren't using the inode parameter, or the file parameter: http://lxr.linux.no/linux+v2.6.23.12/drivers/md/dm-ioctl.c#L1402 Regards, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
> Can you explain the rationale behind that running on the BKL? What type of It used to always run with the BKL because everything used to and originally nobody wanted to review all ioctl handlers in tree to see if they can run with more fine grained locking. A lot probably can though. > things needs to be protected so that this huge hammer is needed? What would > be an earlier point to release the BKL? That depends on the driver. A lot don't need BKL at all and in others it can be easily eliminated. But it needs case-by-case review of the locking situation. The goal of the proposal here is just to make it more visible. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Andi Kleen wrote: > Here's a proposal for some useful code transformations the kernel janitors > could do as opposed to running checkpatch.pl. > > Most ioctl handlers still running implicitely under the big kernel > lock (BKL). But long term Linux is trying to get away from that. There is a > new ->unlocked_ioctl entry point that allows ioctls without BKL, but the > code needs to be explicitely converted to use this. > > The first step of getting rid of the BKL is typically to make it visible > in the source. Once it is visible people will have incentive to eliminate > it. That is how the BKL conversion project for Linux long ago started too. > On 2.0 all system calls were still implicitely BKL and in 2.1 the > lock/unlock_kernel()s were moved into the various syscall functions and > then step by step eliminated. Can you explain the rationale behind that running on the BKL? What type of things needs to be protected so that this huge hammer is needed? What would be an earlier point to release the BKL? Greetings, Eike signature.asc Description: This is a digitally signed message part.
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wed, Jan 09, 2008 at 10:45:13PM +, Alasdair G Kergon wrote: > On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote: > > struct inode *inode = file->f_dentry->d_inode; > > And oops if that's not defined? For file_operations which we talk about here it always is defined. Block_device is a different story, but it'll get a completely different prototype soon with neither file nor inode passed to it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wed, Jan 09, 2008 at 10:45:13PM +, Alasdair G Kergon wrote: On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote: struct inode *inode = file-f_dentry-d_inode; And oops if that's not defined? For file_operations which we talk about here it always is defined. Block_device is a different story, but it'll get a completely different prototype soon with neither file nor inode passed to it. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Andi Kleen wrote: Here's a proposal for some useful code transformations the kernel janitors could do as opposed to running checkpatch.pl. Most ioctl handlers still running implicitely under the big kernel lock (BKL). But long term Linux is trying to get away from that. There is a new -unlocked_ioctl entry point that allows ioctls without BKL, but the code needs to be explicitely converted to use this. The first step of getting rid of the BKL is typically to make it visible in the source. Once it is visible people will have incentive to eliminate it. That is how the BKL conversion project for Linux long ago started too. On 2.0 all system calls were still implicitely BKL and in 2.1 the lock/unlock_kernel()s were moved into the various syscall functions and then step by step eliminated. Can you explain the rationale behind that running on the BKL? What type of things needs to be protected so that this huge hammer is needed? What would be an earlier point to release the BKL? Greetings, Eike signature.asc Description: This is a digitally signed message part.
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Can you explain the rationale behind that running on the BKL? What type of It used to always run with the BKL because everything used to and originally nobody wanted to review all ioctl handlers in tree to see if they can run with more fine grained locking. A lot probably can though. things needs to be protected so that this huge hammer is needed? What would be an earlier point to release the BKL? That depends on the driver. A lot don't need BKL at all and in others it can be easily eliminated. But it needs case-by-case review of the locking situation. The goal of the proposal here is just to make it more visible. -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Hi Alasdair, On Wednesday 09 January 2008 14:40, Alasdair G Kergon wrote: Device-mapper for example in dm_blk_ioctl(), has no need for BKL so drops it immediately, but it does need the inode parameter, so it is unable to switch as things stand. So what stops you from changing to unlocked_ioctl for the main device mapper ctl_ioctl? You aren't using the inode parameter, or the file parameter: http://lxr.linux.no/linux+v2.6.23.12/drivers/md/dm-ioctl.c#L1402 Regards, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Andi Kleen wrote: Can you explain the rationale behind that running on the BKL? What type of It used to always run with the BKL because everything used to and originally nobody wanted to review all ioctl handlers in tree to see if they can run with more fine grained locking. A lot probably can though. things needs to be protected so that this huge hammer is needed? What would be an earlier point to release the BKL? That depends on the driver. A lot don't need BKL at all and in others it can be easily eliminated. But it needs case-by-case review of the locking situation. The goal of the proposal here is just to make it more visible. So if I write my own driver and have never heard of ioctls running on BKL before I can rather be sure that I just can change the interface of the ioctl function, put it in unlocked_ioctl and are fine? Cool. Eike signature.asc Description: This is a digitally signed message part.
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
So if I write my own driver and have never heard of ioctls running on BKL before I can rather be sure that I just can change the interface of the ioctl function, put it in unlocked_ioctl and are fine? Cool. If you know the BKL is not needed in your code you should use unlocked_ioctl correct. -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Thu, Jan 10, 2008 at 01:49:15AM -0800, Daniel Phillips wrote: So what stops you from changing to unlocked_ioctl for the main device mapper ctl_ioctl? Nothing - patches to do this are queued for 2.6.25: http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-ioctl-remove-lock_kernel.patch http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-ioctl-move-compat-code.patch Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Thursday 10 January 2008 03:39, Alasdair G Kergon wrote: On Thu, Jan 10, 2008 at 01:49:15AM -0800, Daniel Phillips wrote: So what stops you from changing to unlocked_ioctl for the main device mapper ctl_ioctl? Nothing - patches to do this are queued for 2.6.25: Nice. This removes a deadlock we hit, where if creating a device mapper target blocks indefinitely (say on network IO) then nobody else can complete a device mapper operation because BKL is held. If completing the device create depends on some other device mapper operation, then it is game over. Our current workaround is to test for and drop BKL, ugh. Thanks for the cleanup. Regards, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wednesday 09 January 2008 04:00:43 pm Alasdair G Kergon wrote: > On Wed, Jan 09, 2008 at 03:31:00PM -0800, Vadim Lobanov wrote: > > From 2.6.23's fs/ioctl.c - do_ioctl(): > > Ah - you're talking about struct file_operations of course; > I was talking about struct block_device_operations. > > Alasdair Good point. :-) -- Vadim Lobanov -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wed, Jan 09, 2008 at 03:31:00PM -0800, Vadim Lobanov wrote: > From 2.6.23's fs/ioctl.c - do_ioctl(): Ah - you're talking about struct file_operations of course; I was talking about struct block_device_operations. Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wednesday 09 January 2008 03:05:45 pm Alasdair G Kergon wrote:
> On Wed, Jan 09, 2008 at 04:58:46PM -0600, Chris Friesen wrote:
> > Alasdair G Kergon wrote:
> > >On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote:
> > >>struct inode *inode = file->f_dentry->d_inode;
> > >
> > >And oops if that's not defined?
> >
> > Isn't this basically identical to what was being passed in to .ioctl()?
>
> Not in every case, unless someone's been through and created fake
> structures in all the remaining places that pass in a NULL 'file' because
> there isn't one available.
>From 2.6.23's fs/ioctl.c - do_ioctl():
if (filp->f_op->unlocked_ioctl) {
error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
if (error == -ENOIOCTLCMD)
error = -EINVAL;
goto out;
} else if (filp->f_op->ioctl) {
lock_kernel();
error = filp->f_op->ioctl(filp->f_path.dentry->d_inode,
filp, cmd, arg);
unlock_kernel();
}
As a sidenote, please don't use f_dentry and f_vfsmnt, since they are just
#defines for the correct fields. They were meant to be temporary transition
helpers, but (alas) have refused to die thus far. If noone beats me to it,
I'll take a look-see at deprecating them all the way.
-- Vadim Lobanov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wed, Jan 09, 2008 at 04:58:46PM -0600, Chris Friesen wrote: > Alasdair G Kergon wrote: > >On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote: > >>struct inode *inode = file->f_dentry->d_inode; > >And oops if that's not defined? > Isn't this basically identical to what was being passed in to .ioctl()? Not in every case, unless someone's been through and created fake structures in all the remaining places that pass in a NULL 'file' because there isn't one available. Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Alasdair G Kergon wrote: On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote: struct inode *inode = file->f_dentry->d_inode; And oops if that's not defined? Isn't this basically identical to what was being passed in to .ioctl()? Chris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote: > struct inode *inode = file->f_dentry->d_inode; And oops if that's not defined? Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wed, Jan 09, 2008 at 10:40:26PM +, Alasdair G Kergon wrote: > On Wed, Jan 09, 2008 at 02:12:40PM -0600, Matt Mackall wrote: > > You'll need to change the prototype, the unlocked version doesn't take > > an inode. And you'll need to make sure that nothing in the function uses > > the inode, which I think Andi forgot to mention. > > This old chestnut again. Perhaps we could have inode passed to > unlocked_ioctl? > I never understood why it wasn't there in the first place if the plan was for > .unlocked_ioctl to supercede .ioctl whenever possible. If you still need inode use struct inode *inode = file->f_dentry->d_inode; -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wed, Jan 09, 2008 at 02:12:40PM -0600, Matt Mackall wrote: > You'll need to change the prototype, the unlocked version doesn't take > an inode. And you'll need to make sure that nothing in the function uses > the inode, which I think Andi forgot to mention. This old chestnut again. Perhaps we could have inode passed to unlocked_ioctl? I never understood why it wasn't there in the first place if the plan was for .unlocked_ioctl to supercede .ioctl whenever possible. Device-mapper for example in dm_blk_ioctl(), has no need for BKL so drops it immediately, but it does need the inode parameter, so it is unable to switch as things stand. Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tue, 2008-01-08 at 20:58 +0100, Paolo Ciarrocchi wrote:
> On Jan 8, 2008 5:40 PM, Andi Kleen <[EMAIL PROTECTED]> wrote:
> So I cooked up the following patch (probably mangled, just to give you
> a rough idea of what I did):
> diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
> index bf1075e..19dedb5 100644
> --- a/arch/arm/common/rtctime.c
> +++ b/arch/arm/common/rtctime.c
> @@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
> file *file, unsigned int cmd,
You'll need to change the prototype, the unlocked version doesn't take
an inode. And you'll need to make sure that nothing in the function uses
the inode, which I think Andi forgot to mention.
> + if (ret) {
> + unlock_kernel();
> ret = -EFAULT;
This is not a return statement. You only need to unlock before the
actual return.
> -static const struct file_operations rtc_fops = {
> +static long rtc_fioctl(struct file_operations rtc_fops)
> +{
> + lock_kernel();
Heh.
--
Mathematics is the supreme nostalgia of our time.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
> Yes, very good point. Does checkpatch.pl enforce diff -p already? > If not, should it? I don't think it should. That would reject a lot of perfectly good patches. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On 14:17, Richard Knutsson wrote:
> Would had preferred:
>
> if (x) {
>result = -E;
>goto out;
> }
>
> then:
>
> result = -E;
> if (x)
>goto out;
>
AFAIK, the second form is preferred in Linux because it is better
readable and it generates slightly better code.
Thanks for the review.
Andre
--
The only person who always got his work done by Friday was Robinson Crusoe
signature.asc
Description: Digital signature
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Andre Noll wrote:
On 17:40, Andi Kleen wrote:
Here's a proposal for some useful code transformations the kernel janitors
could do as opposed to running checkpatch.pl.
Here's my take on drivers/scsi/sg.c. It's only compile-tested on x86-64.
... and x86 with all(yes|mod)config. :)
Would had preferred:
if (x) {
result = -E;
goto out;
}
then:
result = -E;
if (x)
goto out;
but it looks correct.
cu,
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On 17:40, Andi Kleen wrote:
> Here's a proposal for some useful code transformations the kernel janitors
> could do as opposed to running checkpatch.pl.
Here's my take on drivers/scsi/sg.c. It's only compile-tested on x86-64.
Please review.
Andre
---
Convert sg.c to the new unlocked_ioctl entry point.
Signed-off-by: Andre Noll <[EMAIL PROTECTED]>
---
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f1871ea..3063307 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -48,6 +48,7 @@ static int sg_version_num = 30534;/* 2 digits for each
component */
#include
#include
#include
+#include
#include "scsi.h"
#include
@@ -764,20 +765,22 @@ sg_srp_done(Sg_request *srp, Sg_fd *sfp)
return done;
}
-static int
-sg_ioctl(struct inode *inode, struct file *filp,
-unsigned int cmd_in, unsigned long arg)
+static long
+sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
{
void __user *p = (void __user *)arg;
int __user *ip = p;
- int result, val, read_only;
+ int val, read_only;
Sg_device *sdp;
Sg_fd *sfp;
Sg_request *srp;
unsigned long iflags;
+ long result;
+ lock_kernel();
+ result = -ENXIO;
if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
- return -ENXIO;
+ goto out;
SCSI_LOG_TIMEOUT(3, printk("sg_ioctl: %s, cmd=0x%x\n",
sdp->disk->disk_name, (int) cmd_in));
read_only = (O_RDWR != (filp->f_flags & O_ACCMODE));
@@ -787,57 +790,66 @@ sg_ioctl(struct inode *inode, struct file *filp,
{
int blocking = 1; /* ignore O_NONBLOCK flag */
+ result = -ENODEV;
if (sdp->detached)
- return -ENODEV;
+ goto out;
+ result = -ENXIO;
if (!scsi_block_when_processing_errors(sdp->device))
- return -ENXIO;
+ goto out;
+ result = -EFAULT;
if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
- return -EFAULT;
- result =
- sg_new_write(sfp, p, SZ_SG_IO_HDR,
-blocking, read_only, );
+ goto out;
+ result = sg_new_write(sfp, p, SZ_SG_IO_HDR, blocking,
+ read_only, );
if (result < 0)
- return result;
+ goto out;
srp->sg_io_owned = 1;
while (1) {
result = 0; /* following macro to beat race
condition */
__wait_event_interruptible(sfp->read_wait,
(sdp->detached || sfp->closed ||
sg_srp_done(srp, sfp)),
result);
- if (sdp->detached)
- return -ENODEV;
+ if (sdp->detached) {
+ result = -ENODEV;
+ goto out;
+ }
if (sfp->closed)
- return 0; /* request packet
dropped already */
+ goto out; /* request packet
dropped already */
if (0 == result)
break;
srp->orphan = 1;
- return result; /* -ERESTARTSYS because signal
hit process */
+ goto out; /* -ERESTARTSYS because signal
hit process */
}
write_lock_irqsave(>rq_list_lock, iflags);
srp->done = 2;
write_unlock_irqrestore(>rq_list_lock, iflags);
result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
- return (result < 0) ? result : 0;
+ if (result > 0)
+ result = 0;
+ goto out;
}
case SG_SET_TIMEOUT:
result = get_user(val, ip);
if (result)
- return result;
+ goto out;
+ result = -EIO;
if (val < 0)
- return -EIO;
- if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
- val = MULDIV (INT_MAX, USER_HZ, HZ);
+ goto out;
+ if (val >= MULDIV(INT_MAX, USER_HZ, HZ))
+
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Andi Kleen <[EMAIL PROTECTED]> writes:
> I imagined it would check for
>
> +struct file_operations ... = {
> + ...
> + .ioctl = ...
>
> That wouldn't catch the case of someone adding only .ioctl to an
> already existing file_operations which is not visible in the patch context,
> but that should be hopefully rare. The more common case is adding
> completely new operations.
Because "diff -p" format used by the kernel mailing list takes
the most recent line that begins with an identifier letter and
put that on the hunk header line, even in such a case, you will
see:
@@ -l,k +m,n @@ struct file_operations ... = {
...
...
+ ...
+ .ioctl = ...
+ ...
...
which would be a good enough cue that the new .ioctl member is
added to file_operations.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wed, Jan 09, 2008 at 02:41:52AM +0100, Andi Kleen wrote: > There are a few like scsi_host_template that don't have a unlocked_ioctl yet, > but that is just something that needs to be fixed. There's few enough scsi LLDDs with an ioctl method that ->ioctl should be switched over in a single patch. No need to add another method. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wed, Jan 09, 2008 at 02:41:52AM +0100, Andi Kleen wrote: There are a few like scsi_host_template that don't have a unlocked_ioctl yet, but that is just something that needs to be fixed. There's few enough scsi LLDDs with an ioctl method that -ioctl should be switched over in a single patch. No need to add another method. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Andi Kleen [EMAIL PROTECTED] writes:
I imagined it would check for
+struct file_operations ... = {
+ ...
+ .ioctl = ...
That wouldn't catch the case of someone adding only .ioctl to an
already existing file_operations which is not visible in the patch context,
but that should be hopefully rare. The more common case is adding
completely new operations.
Because diff -p format used by the kernel mailing list takes
the most recent line that begins with an identifier letter and
put that on the hunk header line, even in such a case, you will
see:
@@ -l,k +m,n @@ struct file_operations ... = {
...
...
+ ...
+ .ioctl = ...
+ ...
...
which would be a good enough cue that the new .ioctl member is
added to file_operations.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On 17:40, Andi Kleen wrote:
Here's a proposal for some useful code transformations the kernel janitors
could do as opposed to running checkpatch.pl.
Here's my take on drivers/scsi/sg.c. It's only compile-tested on x86-64.
Please review.
Andre
---
Convert sg.c to the new unlocked_ioctl entry point.
Signed-off-by: Andre Noll [EMAIL PROTECTED]
---
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f1871ea..3063307 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -48,6 +48,7 @@ static int sg_version_num = 30534;/* 2 digits for each
component */
#include linux/blkdev.h
#include linux/delay.h
#include linux/scatterlist.h
+#include linux/smp_lock.h
#include scsi.h
#include scsi/scsi_dbg.h
@@ -764,20 +765,22 @@ sg_srp_done(Sg_request *srp, Sg_fd *sfp)
return done;
}
-static int
-sg_ioctl(struct inode *inode, struct file *filp,
-unsigned int cmd_in, unsigned long arg)
+static long
+sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
{
void __user *p = (void __user *)arg;
int __user *ip = p;
- int result, val, read_only;
+ int val, read_only;
Sg_device *sdp;
Sg_fd *sfp;
Sg_request *srp;
unsigned long iflags;
+ long result;
+ lock_kernel();
+ result = -ENXIO;
if ((!(sfp = (Sg_fd *) filp-private_data)) || (!(sdp = sfp-parentdp)))
- return -ENXIO;
+ goto out;
SCSI_LOG_TIMEOUT(3, printk(sg_ioctl: %s, cmd=0x%x\n,
sdp-disk-disk_name, (int) cmd_in));
read_only = (O_RDWR != (filp-f_flags O_ACCMODE));
@@ -787,57 +790,66 @@ sg_ioctl(struct inode *inode, struct file *filp,
{
int blocking = 1; /* ignore O_NONBLOCK flag */
+ result = -ENODEV;
if (sdp-detached)
- return -ENODEV;
+ goto out;
+ result = -ENXIO;
if (!scsi_block_when_processing_errors(sdp-device))
- return -ENXIO;
+ goto out;
+ result = -EFAULT;
if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
- return -EFAULT;
- result =
- sg_new_write(sfp, p, SZ_SG_IO_HDR,
-blocking, read_only, srp);
+ goto out;
+ result = sg_new_write(sfp, p, SZ_SG_IO_HDR, blocking,
+ read_only, srp);
if (result 0)
- return result;
+ goto out;
srp-sg_io_owned = 1;
while (1) {
result = 0; /* following macro to beat race
condition */
__wait_event_interruptible(sfp-read_wait,
(sdp-detached || sfp-closed ||
sg_srp_done(srp, sfp)),
result);
- if (sdp-detached)
- return -ENODEV;
+ if (sdp-detached) {
+ result = -ENODEV;
+ goto out;
+ }
if (sfp-closed)
- return 0; /* request packet
dropped already */
+ goto out; /* request packet
dropped already */
if (0 == result)
break;
srp-orphan = 1;
- return result; /* -ERESTARTSYS because signal
hit process */
+ goto out; /* -ERESTARTSYS because signal
hit process */
}
write_lock_irqsave(sfp-rq_list_lock, iflags);
srp-done = 2;
write_unlock_irqrestore(sfp-rq_list_lock, iflags);
result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
- return (result 0) ? result : 0;
+ if (result 0)
+ result = 0;
+ goto out;
}
case SG_SET_TIMEOUT:
result = get_user(val, ip);
if (result)
- return result;
+ goto out;
+ result = -EIO;
if (val 0)
- return -EIO;
- if (val = MULDIV (INT_MAX, USER_HZ, HZ))
- val = MULDIV (INT_MAX, USER_HZ, HZ);
+ goto out;
+
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Andre Noll wrote:
On 17:40, Andi Kleen wrote:
Here's a proposal for some useful code transformations the kernel janitors
could do as opposed to running checkpatch.pl.
Here's my take on drivers/scsi/sg.c. It's only compile-tested on x86-64.
... and x86 with all(yes|mod)config. :)
Would had preferred:
if (x) {
result = -E;
goto out;
}
then:
result = -E;
if (x)
goto out;
but it looks correct.
cu,
Richard
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On 14:17, Richard Knutsson wrote:
Would had preferred:
if (x) {
result = -E;
goto out;
}
then:
result = -E;
if (x)
goto out;
AFAIK, the second form is preferred in Linux because it is better
readable and it generates slightly better code.
Thanks for the review.
Andre
--
The only person who always got his work done by Friday was Robinson Crusoe
signature.asc
Description: Digital signature
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Yes, very good point. Does checkpatch.pl enforce diff -p already? If not, should it? I don't think it should. That would reject a lot of perfectly good patches. -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tue, 2008-01-08 at 20:58 +0100, Paolo Ciarrocchi wrote:
On Jan 8, 2008 5:40 PM, Andi Kleen [EMAIL PROTECTED] wrote:
So I cooked up the following patch (probably mangled, just to give you
a rough idea of what I did):
diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
index bf1075e..19dedb5 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
You'll need to change the prototype, the unlocked version doesn't take
an inode. And you'll need to make sure that nothing in the function uses
the inode, which I think Andi forgot to mention.
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
This is not a return statement. You only need to unlock before the
actual return.
-static const struct file_operations rtc_fops = {
+static long rtc_fioctl(struct file_operations rtc_fops)
+{
+ lock_kernel();
Heh.
--
Mathematics is the supreme nostalgia of our time.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wed, Jan 09, 2008 at 02:12:40PM -0600, Matt Mackall wrote: You'll need to change the prototype, the unlocked version doesn't take an inode. And you'll need to make sure that nothing in the function uses the inode, which I think Andi forgot to mention. This old chestnut again. Perhaps we could have inode passed to unlocked_ioctl? I never understood why it wasn't there in the first place if the plan was for .unlocked_ioctl to supercede .ioctl whenever possible. Device-mapper for example in dm_blk_ioctl(), has no need for BKL so drops it immediately, but it does need the inode parameter, so it is unable to switch as things stand. Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wed, Jan 09, 2008 at 10:40:26PM +, Alasdair G Kergon wrote: On Wed, Jan 09, 2008 at 02:12:40PM -0600, Matt Mackall wrote: You'll need to change the prototype, the unlocked version doesn't take an inode. And you'll need to make sure that nothing in the function uses the inode, which I think Andi forgot to mention. This old chestnut again. Perhaps we could have inode passed to unlocked_ioctl? I never understood why it wasn't there in the first place if the plan was for .unlocked_ioctl to supercede .ioctl whenever possible. If you still need inode use struct inode *inode = file-f_dentry-d_inode; -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote: struct inode *inode = file-f_dentry-d_inode; And oops if that's not defined? Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Alasdair G Kergon wrote: On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote: struct inode *inode = file-f_dentry-d_inode; And oops if that's not defined? Isn't this basically identical to what was being passed in to .ioctl()? Chris -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wed, Jan 09, 2008 at 04:58:46PM -0600, Chris Friesen wrote: Alasdair G Kergon wrote: On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote: struct inode *inode = file-f_dentry-d_inode; And oops if that's not defined? Isn't this basically identical to what was being passed in to .ioctl()? Not in every case, unless someone's been through and created fake structures in all the remaining places that pass in a NULL 'file' because there isn't one available. Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wednesday 09 January 2008 03:05:45 pm Alasdair G Kergon wrote:
On Wed, Jan 09, 2008 at 04:58:46PM -0600, Chris Friesen wrote:
Alasdair G Kergon wrote:
On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote:
struct inode *inode = file-f_dentry-d_inode;
And oops if that's not defined?
Isn't this basically identical to what was being passed in to .ioctl()?
Not in every case, unless someone's been through and created fake
structures in all the remaining places that pass in a NULL 'file' because
there isn't one available.
From 2.6.23's fs/ioctl.c - do_ioctl():
if (filp-f_op-unlocked_ioctl) {
error = filp-f_op-unlocked_ioctl(filp, cmd, arg);
if (error == -ENOIOCTLCMD)
error = -EINVAL;
goto out;
} else if (filp-f_op-ioctl) {
lock_kernel();
error = filp-f_op-ioctl(filp-f_path.dentry-d_inode,
filp, cmd, arg);
unlock_kernel();
}
As a sidenote, please don't use f_dentry and f_vfsmnt, since they are just
#defines for the correct fields. They were meant to be temporary transition
helpers, but (alas) have refused to die thus far. If noone beats me to it,
I'll take a look-see at deprecating them all the way.
-- Vadim Lobanov
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wed, Jan 09, 2008 at 03:31:00PM -0800, Vadim Lobanov wrote: From 2.6.23's fs/ioctl.c - do_ioctl(): Ah - you're talking about struct file_operations of course; I was talking about struct block_device_operations. Alasdair -- [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wednesday 09 January 2008 04:00:43 pm Alasdair G Kergon wrote: On Wed, Jan 09, 2008 at 03:31:00PM -0800, Vadim Lobanov wrote: From 2.6.23's fs/ioctl.c - do_ioctl(): Ah - you're talking about struct file_operations of course; I was talking about struct block_device_operations. Alasdair Good point. :-) -- Vadim Lobanov -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tue, Jan 08, 2008 at 09:31:24PM -0400, Kevin Winchester wrote:
> Arnd Bergmann wrote:
> > On Wednesday 09 January 2008, Andi Kleen wrote:
> >> I imagined it would check for
> >>
> >> +struct file_operations ... = {
> >> + ...
> >> + .ioctl = ...
> >>
> >> That wouldn't catch the case of someone adding only .ioctl to an
> >> already existing file_operations which is not visible in the patch
> >> context,
> >> but that should be hopefully rare. The more common case is adding
> >> completely new operations
> >
> > Right, this would work fine. We can probably even have a list of
> > data structures that work like file_operations in this regard.
> >
>
> file_operations & block_device_operations are the only two that I can find.
There are a few like scsi_host_template that don't have a unlocked_ioctl yet,
but that is just something that needs to be fixed.
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Arnd Bergmann wrote:
> On Wednesday 09 January 2008, Andi Kleen wrote:
>> I imagined it would check for
>>
>> +struct file_operations ... = {
>> + ...
>> + .ioctl = ...
>>
>> That wouldn't catch the case of someone adding only .ioctl to an
>> already existing file_operations which is not visible in the patch context,
>> but that should be hopefully rare. The more common case is adding
>> completely new operations
>
> Right, this would work fine. We can probably even have a list of
> data structures that work like file_operations in this regard.
>
file_operations & block_device_operations are the only two that I can find.
--
Kevin Winchester
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wednesday 09 January 2008, Andi Kleen wrote:
> I imagined it would check for
>
> +struct file_operations ... = {
> + ...
> + .ioctl = ...
>
> That wouldn't catch the case of someone adding only .ioctl to an
> already existing file_operations which is not visible in the patch context,
> but that should be hopefully rare. The more common case is adding
> completely new operations
Right, this would work fine. We can probably even have a list of
data structures that work like file_operations in this regard.
Arnd <><
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Wed, Jan 09, 2008 at 01:40:58AM +0100, Arnd Bergmann wrote:
> On Tuesday 08 January 2008, Andi Kleen wrote:
> > > Thanks, Andi! I think it'd very useful change.
> >
> > Reminds me this is something that should be actually flagged
> > in checkpatch.pl too
> >
> > Andy, it would be good if checkpatch.pl complained about .ioctl =
> > as opposed to .unlocked_ioctl = ...
>
> This is rather hard, as there are different data structures that
> all contain ->ioctl and/or ->unlocked_ioctl function pointers.
> Some of them already use ->ioctl in an unlocked fashion only,
> so blindly warning about this would give lots of false positives.
I imagined it would check for
+struct file_operations ... = {
+ ...
+ .ioctl = ...
That wouldn't catch the case of someone adding only .ioctl to an
already existing file_operations which is not visible in the patch context,
but that should be hopefully rare. The more common case is adding
completely new operations
>
> > Also perhaps if a whole new file_operations with a ioctl is added
> > complain about missing compat_ioctl as a low prioritity warning?
> > (might be ok if it's architecture specific on architectures without
> > compat layer)
>
> Also, not every data structure that provides a ->ioctl callback
> also has a ->compat_ioctl, although there should be fewer exceptions
That's probably a bug in general. e.g. those likely won't work
at all on the "compat by default" architectures like sparc or ppc64.
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tuesday 08 January 2008, Andi Kleen wrote: > > Thanks, Andi! I think it'd very useful change. > > Reminds me this is something that should be actually flagged > in checkpatch.pl too > > Andy, it would be good if checkpatch.pl complained about .ioctl = > as opposed to .unlocked_ioctl = ... This is rather hard, as there are different data structures that all contain ->ioctl and/or ->unlocked_ioctl function pointers. Some of them already use ->ioctl in an unlocked fashion only, so blindly warning about this would give lots of false positives. > Also perhaps if a whole new file_operations with a ioctl is added > complain about missing compat_ioctl as a low prioritity warning? > (might be ok if it's architecture specific on architectures without > compat layer) Also, not every data structure that provides a ->ioctl callback also has a ->compat_ioctl, although there should be fewer exceptions here. Arnd <>< -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
> Sorry about the noise here - I now notice that not all .ioctl function > pointers have the option of changing to .unlocked_ioctl. In this case, > the ioctl is in the struct scsi_host_template, rather than struct > file_operations. > > I'll try to be a little more careful about the git grepping in the future. Well it just points to another area that needs to be improved. Clearly scsi_host_template should have a unlocked_ioctl too. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Andi Kleen wrote: > On Tue, Jan 08, 2008 at 07:50:47PM -0400, Kevin Winchester wrote: >> Andi Kleen wrote: >>> Here's a proposal for some useful code transformations the kernel janitors >>> could do as opposed to running checkpatch.pl. >>> >> >> >> I notice that every driver in drivers/ata uses a .ioctl that points to >> ata_scsi_ioctl(). I could add the BKL to that function, and then change > > This might be a little more complicated. These > are funnelled through the block/SCSI layers which might not have separate > unlocked ioctl callbacks yet. Would be probably not very difficult > to add though. > >> all of the drivers to .unlocked_ioctl, but I assume this would be a >> candidate to actually clean up by determining why the lock is needed and >> removing it if necessary. Does anyone know off-hand the reason for >> needing the lock (I assume someone does or it wouldn't have survived >> this long)? If the lock is absolutely required, then I can write the >> patch to add lock_kernel() and unlock_kernel(). > > Just sending the patch to add lock/unlock_kernel() is probably a good idea > anyways -- > Jeff will then feel bad over it and eventually remove it when he figures out > it is safe ;-) > Sorry about the noise here - I now notice that not all .ioctl function pointers have the option of changing to .unlocked_ioctl. In this case, the ioctl is in the struct scsi_host_template, rather than struct file_operations. I'll try to be a little more careful about the git grepping in the future. -- Kevin Winchester -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tue, Jan 08, 2008 at 07:50:47PM -0400, Kevin Winchester wrote: > Andi Kleen wrote: > > Here's a proposal for some useful code transformations the kernel janitors > > could do as opposed to running checkpatch.pl. > > > > > I notice that every driver in drivers/ata uses a .ioctl that points to > ata_scsi_ioctl(). I could add the BKL to that function, and then change This might be a little more complicated. These are funnelled through the block/SCSI layers which might not have separate unlocked ioctl callbacks yet. Would be probably not very difficult to add though. > all of the drivers to .unlocked_ioctl, but I assume this would be a > candidate to actually clean up by determining why the lock is needed and > removing it if necessary. Does anyone know off-hand the reason for > needing the lock (I assume someone does or it wouldn't have survived > this long)? If the lock is absolutely required, then I can write the > patch to add lock_kernel() and unlock_kernel(). Just sending the patch to add lock/unlock_kernel() is probably a good idea anyways -- Jeff will then feel bad over it and eventually remove it when he figures out it is safe ;-) -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl II
> [EMAIL PROTECTED]:~/linux-2.6/mm$ grep "struct file_operations" *
> shmem.c:static const struct file_operations shmem_file_operations;
> shmem.c:static const struct file_operations shmem_file_operations = {
> swapfile.c:static const struct file_operations proc_swaps_operations = {
>
> Am I right in saying that both the files don't need to be modified?
If they don't have an ioctl handler they don't need to be modified, correct.
>
> There is nothing like:
> struct file_operations xyz_ops = {
>...
>.ioctl = xyz_ioctl
> };
>
> in there.
>
> So I guess I need a smarter trick to find out which files need to be modified
> as you previously suggested.
grep -P '\.ioctl.*=' $(grep -rl 'struct file_operations' * )
should work. There are also special multiline greps iirc that might also be able
to do this better (like sgrep)
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Matthew Wilcox пишет:
> On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
>> On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
>>> Yes of course, I've been silly in didn't verify whether the file compile
>>> but I would appreciate to know whether I'm on the right track or not.
>> Well ... you're not.
>
> Here's what a correct conversion might look like. I haven't tried to
> compile it, so I'm copying and pasting it in order to damage whitespace
> and make sure nobody tries to compile it.
It seems that the kernel janitors project needs to explicitly describe the
prerequisites a person should meet before tackling the enterprise's toughest
technical challenges such as operating system kernel development.
On the face of it, it seems quite ridiculous to me that a team of extremely
qualified kernel engineers spend their valuable time teaching the basics of
the C language using this mailing list. Time is precious, and even more so
when we are having that many unresolved problems, e.g. when the Kernel Bug
Tracker lists more than 1300 open bugs:
http://bugzilla.kernel.org/buglist.cgi?order=relevance_status=__open__
Paolo, I have nothing against you personally as you seem to have adequately
reacted to what is going on and went on strengthening your C skills; my point
is that an operating system kernel development facility such as LKML is most
probably not the right place to set up a correspondence course in programming
basics.
>
> index bf1075e..0c543a8 100644
> --- a/arch/arm/common/rtctime.c
> +++ b/arch/arm/common/rtctime.c
> @@ -174,8 +174,7 @@ static unsigned int rtc_poll(struct file *file,
> poll_table *
> return data != 0 ? POLLIN | POLLRDNORM : 0;
> }
>
> -static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int
> cmd,
> -unsigned long arg)
> +static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> struct rtc_ops *ops = file->private_data;
> struct rtc_time tm;
> @@ -183,6 +182,8 @@ static int rtc_ioctl(struct inode *inode, struct file
> *file,
> void __user *uarg = (void __user *)arg;
> int ret = -EINVAL;
>
> + lock_kernel();
> +
> switch (cmd) {
> case RTC_ALM_READ:
> ret = rtc_arm_read_alarm(ops, );
> @@ -277,6 +278,9 @@ static int rtc_ioctl(struct inode *inode, struct file
> *file,
> ret = ops->ioctl(cmd, arg);
> break;
> }
> +
> + unlock_kernel();
> +
> return ret;
> }
>
> @@ -334,7 +338,7 @@ static const struct file_operations rtc_fops = {
> .llseek = no_llseek,
> .read = rtc_read,
> .poll = rtc_poll,
> - .ioctl = rtc_ioctl,
> + .unlocked_ioctl = rtc_ioctl,
> .open = rtc_open,
> .release= rtc_release,
> .fasync = rtc_fasync,
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Andi Kleen wrote: > Here's a proposal for some useful code transformations the kernel janitors > could do as opposed to running checkpatch.pl. > I notice that every driver in drivers/ata uses a .ioctl that points to ata_scsi_ioctl(). I could add the BKL to that function, and then change all of the drivers to .unlocked_ioctl, but I assume this would be a candidate to actually clean up by determining why the lock is needed and removing it if necessary. Does anyone know off-hand the reason for needing the lock (I assume someone does or it wouldn't have survived this long)? If the lock is absolutely required, then I can write the patch to add lock_kernel() and unlock_kernel(). -- Kevin Winchester -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl II
On Jan 9, 2008 12:06 AM, Andi Kleen <[EMAIL PROTECTED]> wrote:
> > I would suggest to only work on files that compile. e.g. do a
> >
> > make allyesconfig
> > make -j$[$(grep -c processor /proc/cpuinfo)*2] &1 |tee LOG
> > (will probably take a long time)
> >
> > first and then only modify files when are mentioned in "LOG"
>
> Actually since this will probably take very long on a slower machine you can
> refer to
>
> http://halobates.de/allyes/
Thank you Andi.
> for some allyes buildlogs of recent kernels for i386 and x86-64. A trick to
> quickly check
> if something compiles is also to do
>
> make allyesconfig
> make path/to/file.o
>
> That won't catch linker errors, but if you don't have warnings there are
> normally no
> linker errors either.
I did grep for "struct file_operations" in mm:
[EMAIL PROTECTED]:~/linux-2.6/mm$ grep "struct file_operations" *
shmem.c:static const struct file_operations shmem_file_operations;
shmem.c:static const struct file_operations shmem_file_operations = {
swapfile.c:static const struct file_operations proc_swaps_operations = {
Am I right in saying that both the files don't need to be modified?
There is nothing like:
struct file_operations xyz_ops = {
...
.ioctl = xyz_ioctl
};
in there.
So I guess I need a smarter trick to find out which files need to be modified
as you previously suggested.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl II
> I would suggest to only work on files that compile. e.g. do a > > make allyesconfig > make -j$[$(grep -c processor /proc/cpuinfo)*2] &1 |tee LOG (will > probably take a long time) > > first and then only modify files when are mentioned in "LOG" Actually since this will probably take very long on a slower machine you can refer to http://halobates.de/allyes/ for some allyes buildlogs of recent kernels for i386 and x86-64. A trick to quickly check if something compiles is also to do make allyesconfig make path/to/file.o That won't catch linker errors, but if you don't have warnings there are normally no linker errors either. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Jan 8, 2008 9:42 PM, Andi Kleen <[EMAIL PROTECTED]> wrote:
> > I grepped and tried to do what you suggested.
>
> First a quick question: how would you rate your C knowledge? Did you
> ever write a program yourself?
Yes I did but I probably beeing inactive for too much time,
I need to back studing a bit before submitting another patch.
> My proposal assumes that you have at least basic C knowledge.
>
> > The first file that git grep reported was:
> > arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
>
> It's probably better to only do that on files which you can easily compile.
> For ARM you would need a cross compiler.
>
> >
> > So I cooked up the following patch (probably mangled, just to give you
> > a rough idea of what I did):
> > diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
> > index bf1075e..19dedb5 100644
> > --- a/arch/arm/common/rtctime.c
> > +++ b/arch/arm/common/rtctime.c
> > @@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
> > file *file, unsigned int cmd,
> > if (ret)
> > break;
> > ret = copy_to_user(uarg, , sizeof(tm));
> > - if (ret)
> > + if (ret) {
> > + unlock_kernel();
> > ret = -EFAULT;
>
> In this case it would be better to just put the unlock_kernel() directly
> before the single return. You only need to sprinkle them all over when
> the function has multiple returns.
Understood. As Matthew did in his patch.
> Or then change the flow to only
> have a single return, but that would be slightly advanced.
>
>
> > - if (ret)
> > + if (ret) {
> > + unlock_kernel();
> > ret = -EFAULT;
> > + }
> > break;
> >
> > default:
> > @@ -329,15 +340,18 @@ static int rtc_fasync(int fd, struct file *file, int
> > on)
> > return fasync_helper(fd, file, on, _async_queue);
> > }
> >
> > -static const struct file_operations rtc_fops = {
> > +static long rtc_fioctl(struct file_operations rtc_fops)
> > +{
> > + lock_kernel();
>
> No the lock_kernel() of course has to be in the function, not in the structure
> definition which does not contain any code.
Yes, understood now. Silly me :-/
> Also the _operations structure stays the same (except for .ioctl ->
> .unlocked_ioctl);
> only the the ioctl function prototype changes.
>
>
> > Am I'm working in the right direction or should I completely redo the patch?
>
> I would suggest to only work on files that compile. e.g. do a
>
> make allyesconfig
> make -j$[$(grep -c processor /proc/cpuinfo)*2] &1 |tee LOG (will
> probably take a long time)
>
> first and then only modify files when are mentioned in "LOG"
Thanks you.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
> I grepped and tried to do what you suggested.
First a quick question: how would you rate your C knowledge? Did you
ever write a program yourself?
My proposal assumes that you have at least basic C knowledge.
> The first file that git grep reported was:
> arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
It's probably better to only do that on files which you can easily compile.
For ARM you would need a cross compiler.
>
> So I cooked up the following patch (probably mangled, just to give you
> a rough idea of what I did):
> diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
> index bf1075e..19dedb5 100644
> --- a/arch/arm/common/rtctime.c
> +++ b/arch/arm/common/rtctime.c
> @@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
> file *file, unsigned int cmd,
> if (ret)
> break;
> ret = copy_to_user(uarg, , sizeof(tm));
> - if (ret)
> + if (ret) {
> + unlock_kernel();
> ret = -EFAULT;
In this case it would be better to just put the unlock_kernel() directly
before the single return. You only need to sprinkle them all over when
the function has multiple returns. Or then change the flow to only
have a single return, but that would be slightly advanced.
> - if (ret)
> + if (ret) {
> + unlock_kernel();
> ret = -EFAULT;
> + }
> break;
>
> default:
> @@ -329,15 +340,18 @@ static int rtc_fasync(int fd, struct file *file, int on)
> return fasync_helper(fd, file, on, _async_queue);
> }
>
> -static const struct file_operations rtc_fops = {
> +static long rtc_fioctl(struct file_operations rtc_fops)
> +{
> + lock_kernel();
No the lock_kernel() of course has to be in the function, not in the structure
definition which does not contain any code.
Also the _operations structure stays the same (except for .ioctl ->
.unlocked_ioctl);
only the the ioctl function prototype changes.
> Am I'm working in the right direction or should I completely redo the patch?
I would suggest to only work on files that compile. e.g. do a
make allyesconfig
make -j$[$(grep -c processor /proc/cpuinfo)*2] &1 |tee LOG (will
probably take a long time)
first and then only modify files when are mentioned in "LOG"
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Jan 8, 2008 9:21 PM, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
> > On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
> > > Yes of course, I've been silly in didn't verify whether the file compile
> > > but I would appreciate to know whether I'm on the right track or not.
> >
> > Well ... you're not.
>
> Here's what a correct conversion might look like. I haven't tried to
> compile it, so I'm copying and pasting it in order to damage whitespace
> and make sure nobody tries to compile it.
>
> index bf1075e..0c543a8 100644
> --- a/arch/arm/common/rtctime.c
> +++ b/arch/arm/common/rtctime.c
> @@ -174,8 +174,7 @@ static unsigned int rtc_poll(struct file *file,
> poll_table *
> return data != 0 ? POLLIN | POLLRDNORM : 0;
> }
>
> -static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int
> cmd,
> -unsigned long arg)
> +static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> struct rtc_ops *ops = file->private_data;
> struct rtc_time tm;
> @@ -183,6 +182,8 @@ static int rtc_ioctl(struct inode *inode, struct file
> *file,
> void __user *uarg = (void __user *)arg;
> int ret = -EINVAL;
>
> + lock_kernel();
> +
> switch (cmd) {
> case RTC_ALM_READ:
> ret = rtc_arm_read_alarm(ops, );
> @@ -277,6 +278,9 @@ static int rtc_ioctl(struct inode *inode, struct file
> *file,
> ret = ops->ioctl(cmd, arg);
> break;
> }
> +
> + unlock_kernel();
> +
> return ret;
> }
>
> @@ -334,7 +338,7 @@ static const struct file_operations rtc_fops = {
> .llseek = no_llseek,
> .read = rtc_read,
> .poll = rtc_poll,
> - .ioctl = rtc_ioctl,
> + .unlocked_ioctl = rtc_ioctl,
> .open = rtc_open,
> .release= rtc_release,
> .fasync = rtc_fasync,
>
Thank you Matthew,
I definitely need to back studying before submitting another patch.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tue, 8 Jan 2008 20:58:04 +0100
"Paolo Ciarrocchi" <[EMAIL PROTECTED]> wrote:
> -static const struct file_operations rtc_fops = {
> +static long rtc_fioctl(struct file_operations rtc_fops)
> +{
> + lock_kernel();
> .owner = THIS_MODULE,
> .llseek = no_llseek,
> .read = rtc_read,
> .poll = rtc_poll,
> - .ioctl = rtc_ioctl,
> + .unlocked_ioctl = rtc_ioctl,
> .open = rtc_open,
> .release= rtc_release,
> .fasync = rtc_fasync,
> + unlock_kernel();
> };
This is a struct with function pointers, not a function.
No wonder it doesn't compile after you tried turning it into one :)
--
All rights reversed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
> On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
> > Yes of course, I've been silly in didn't verify whether the file compile
> > but I would appreciate to know whether I'm on the right track or not.
>
> Well ... you're not.
Here's what a correct conversion might look like. I haven't tried to
compile it, so I'm copying and pasting it in order to damage whitespace
and make sure nobody tries to compile it.
index bf1075e..0c543a8 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -174,8 +174,7 @@ static unsigned int rtc_poll(struct file *file, poll_table *
return data != 0 ? POLLIN | POLLRDNORM : 0;
}
-static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
-unsigned long arg)
+static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
struct rtc_ops *ops = file->private_data;
struct rtc_time tm;
@@ -183,6 +182,8 @@ static int rtc_ioctl(struct inode *inode, struct file *file,
void __user *uarg = (void __user *)arg;
int ret = -EINVAL;
+ lock_kernel();
+
switch (cmd) {
case RTC_ALM_READ:
ret = rtc_arm_read_alarm(ops, );
@@ -277,6 +278,9 @@ static int rtc_ioctl(struct inode *inode, struct file *file,
ret = ops->ioctl(cmd, arg);
break;
}
+
+ unlock_kernel();
+
return ret;
}
@@ -334,7 +338,7 @@ static const struct file_operations rtc_fops = {
.llseek = no_llseek,
.read = rtc_read,
.poll = rtc_poll,
- .ioctl = rtc_ioctl,
+ .unlocked_ioctl = rtc_ioctl,
.open = rtc_open,
.release= rtc_release,
.fasync = rtc_fasync,
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
> On Jan 8, 2008 9:00 PM, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> > On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
> > > -static const struct file_operations rtc_fops = {
> > > +static long rtc_fioctl(struct file_operations rtc_fops)
> > > +{
> > > + lock_kernel();
> > > .owner = THIS_MODULE,
> >
> > You should probably restrict yourself to files that you can at least
> > compile ...
>
> Yes of course, I've been silly in didn't verify whether the file compile
> but I would appreciate to know whether I'm on the right track or not.
Well ... you're not.
You've deleted the definition of rtc_fops for no reason I can tell.
You put an unlock_kernel() on every place that invokes break;. You
should instead have put an unlock_kernel() before every return;.
You needed to put a lock_kernel() at the beginning of rtc_ioctl().
You needed to adjust the prototype of rtc_ioctl().
Honestly, if your C skills are as weak as they seem to be, you may be
better off trying to gain experience with some other project -- the
patch you sent appeared to be simple cargo-cult programming, and we
don't need that kind of contribution.
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Jan 8, 2008 9:00 PM, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
> > -static const struct file_operations rtc_fops = {
> > +static long rtc_fioctl(struct file_operations rtc_fops)
> > +{
> > + lock_kernel();
> > .owner = THIS_MODULE,
>
> You should probably restrict yourself to files that you can at least
> compile ...
Yes of course, I've been silly in didn't verify whether the file compile
but I would appreciate to know whether I'm on the right track or not.
Switching to a different file now...
Thanks.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
> -static const struct file_operations rtc_fops = {
> +static long rtc_fioctl(struct file_operations rtc_fops)
> +{
> + lock_kernel();
> .owner = THIS_MODULE,
You should probably restrict yourself to files that you can at least
compile ...
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Jan 8, 2008 5:40 PM, Andi Kleen <[EMAIL PROTECTED]> wrote:
>
> Here's a proposal for some useful code transformations the kernel janitors
> could do as opposed to running checkpatch.pl.
>
> Most ioctl handlers still running implicitely under the big kernel
> lock (BKL). But long term Linux is trying to get away from that. There is a
> new
> ->unlocked_ioctl entry point that allows ioctls without BKL, but the code
> needs to be explicitely converted to use this.
>
> The first step of getting rid of the BKL is typically to make it visible
> in the source. Once it is visible people will have incentive to eliminate it.
> That is how the BKL conversion project for Linux long ago started too.
> On 2.0 all system calls were still implicitely BKL and in 2.1 the
> lock/unlock_kernel()s were moved into the various syscall functions and then
> step by step eliminated.
>
> And now it's time to do the same for all the ioctls too.
>
> So my proposal is to convert the ->ioctl handlers all over the tree
> to ->unlocked_ioctl with explicit lock_kernel()/unlock_kernel.
>
> It is not a completely trivial conversion. You will have to
> at least read the source, although not completely understand it.
> But it is not very difficult.
>
> Rough recipe:
>
> Grep the source tree for "struct file_operations". You should
> fine something like
>
> static int xyz_ioctl(struct inode *i, struct file *f, unsigned cmd, unsigned
> long arg)
> {
> switch (cmd) {
> case IOCTL1:
> err = ...;
> ...
> break;
> case IOCTL2:
> ...
> err = ...;
> break;
> default:
> err = -ENOTTY;
> }
> return err;
> }
> ...
>
> struct file_operations xyz_ops = {
> ...
> .ioctl = xyz_ioctl
> };
>
> The conversion is now to change the prototype of xyz_ioctl to
>
> static long xyz_ioctl(struct file *f, unsigned cmd, unsigned long arg)
> {
> }
>
> This means return type from int to long and drop the struct inode * argument
>
> Then add lock_kernel() to the entry point and unlock_kernel() to all exit
> points.
> So you should get
>
> static long xyz_ioctl(struct file *f, unsigned cmd, unsigned long arg)
> {
> lock_kernel();
> ...
> unlock_kernel();
> return err;
> }
>
> The only thing you need to watch out for is that all returns get an
> unlock_kernel.
> so e.g. if the ioctl handler has early error exits they all need an own
> unlock_kernel()
> (if you prefer it you can also use a goto to handle this, but just adding own
> unlock_kernels is easier)
>
> so e.g. if you have
>
> case IOCTL1:
>
> ...
> if (something failed)
> return -EIO;
>
> you would convert it to
>
> if (something failed) {
> unlock_kernel();
> return -EIO;
> }
>
> It is very important that all returns have an unlock_kernel(), please always
> double and triple check that!
>
> Then change
>
> .ioctl = xyz_ioctl
>
> to
>
> .unlocked_ioctl = xyz_ioctl
>
> Then compile ideally test the result and submit it.
Hi Andi,
I grepped and tried to do what you suggested.
The first file that git grep reported was:
arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
So I cooked up the following patch (probably mangled, just to give you
a rough idea of what I did):
diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
index bf1075e..19dedb5 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
if (ret)
break;
ret = copy_to_user(uarg, , sizeof(tm));
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
+ }
break;
case RTC_ALM_SET:
ret = copy_from_user(, uarg, sizeof(tm));
if (ret) {
+ unlock_kernel();
ret = -EFAULT;
break;
}
@@ -215,8 +218,10 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
if (ret)
break;
ret = copy_to_user(uarg, , sizeof(tm));
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
+ }
break;
case RTC_SET_TIME:
@@ -226,6 +231,7 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
}
ret = copy_from_user(, uarg, sizeof(tm));
if (ret) {
+ unlock_kernel();
ret = -EFAULT;
break;
}
@@ -238,10 +244,12
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
> Thanks, Andi! I think it'd very useful change. Reminds me this is something that should be actually flagged in checkpatch.pl too Andy, it would be good if checkpatch.pl complained about .ioctl = as opposed to .unlocked_ioctl = ... Also perhaps if a whole new file_operations with a ioctl is added complain about missing compat_ioctl as a low prioritity warning? (might be ok if it's architecture specific on architectures without compat layer) -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
On Tue, Jan 08, 2008 at 05:40:15PM +0100, Andi Kleen wrote: > Here's a proposal for some useful code transformations the kernel janitors > could do as opposed to running checkpatch.pl. > > Most ioctl handlers still running implicitely under the big kernel > lock (BKL). But long term Linux is trying to get away from that. There is a > new > ->unlocked_ioctl entry point that allows ioctls without BKL, but the code > needs to be explicitely converted to use this. > > The first step of getting rid of the BKL is typically to make it visible > in the source. Once it is visible people will have incentive to eliminate it. > That is how the BKL conversion project for Linux long ago started too. > On 2.0 all system calls were still implicitely BKL and in 2.1 the > lock/unlock_kernel()s were moved into the various syscall functions and then > step by step eliminated. > > And now it's time to do the same for all the ioctls too. > > So my proposal is to convert the ->ioctl handlers all over the tree > to ->unlocked_ioctl with explicit lock_kernel()/unlock_kernel. Thanks, Andi! I think it'd very useful change. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
[Andi Kleen - Tue, Jan 08, 2008 at 05:40:15PM +0100] | | Here's a proposal for some useful code transformations the kernel janitors | could do as opposed to running checkpatch.pl. | [...snip...] | -Andi | | got it, thanks - Cyrill - -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl
Here's a proposal for some useful code transformations the kernel janitors
could do as opposed to running checkpatch.pl.
Most ioctl handlers still running implicitely under the big kernel
lock (BKL). But long term Linux is trying to get away from that. There is a new
->unlocked_ioctl entry point that allows ioctls without BKL, but the code
needs to be explicitely converted to use this.
The first step of getting rid of the BKL is typically to make it visible
in the source. Once it is visible people will have incentive to eliminate it.
That is how the BKL conversion project for Linux long ago started too.
On 2.0 all system calls were still implicitely BKL and in 2.1 the
lock/unlock_kernel()s were moved into the various syscall functions and then
step by step eliminated.
And now it's time to do the same for all the ioctls too.
So my proposal is to convert the ->ioctl handlers all over the tree
to ->unlocked_ioctl with explicit lock_kernel()/unlock_kernel.
It is not a completely trivial conversion. You will have to
at least read the source, although not completely understand it.
But it is not very difficult.
Rough recipe:
Grep the source tree for "struct file_operations". You should
fine something like
static int xyz_ioctl(struct inode *i, struct file *f, unsigned cmd, unsigned
long arg)
{
switch (cmd) {
case IOCTL1:
err = ...;
...
break;
case IOCTL2:
...
err = ...;
break;
default:
err = -ENOTTY;
}
return err;
}
...
struct file_operations xyz_ops = {
...
.ioctl = xyz_ioctl
};
The conversion is now to change the prototype of xyz_ioctl to
static long xyz_ioctl(struct file *f, unsigned cmd, unsigned long arg)
{
}
This means return type from int to long and drop the struct inode * argument
Then add lock_kernel() to the entry point and unlock_kernel() to all exit
points.
So you should get
static long xyz_ioctl(struct file *f, unsigned cmd, unsigned long arg)
{
lock_kernel();
...
unlock_kernel();
return err;
}
The only thing you need to watch out for is that all returns get an
unlock_kernel.
so e.g. if the ioctl handler has early error exits they all need an own
unlock_kernel()
(if you prefer it you can also use a goto to handle this, but just adding own
unlock_kernels is easier)
so e.g. if you have
case IOCTL1:
...
if (something failed)
return -EIO;
you would convert it to
if (something failed) {
unlock_kernel();
return -EIO;
}
It is very important that all returns have an unlock_kernel(), please always
double and triple check that!
Then change
.ioctl = xyz_ioctl
to
.unlocked_ioctl = xyz_ioctl
Then compile ideally test the result and submit it.
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Here's a proposal for some useful code transformations the kernel janitors
could do as opposed to running checkpatch.pl.
Most ioctl handlers still running implicitely under the big kernel
lock (BKL). But long term Linux is trying to get away from that. There is a new
-unlocked_ioctl entry point that allows ioctls without BKL, but the code
needs to be explicitely converted to use this.
The first step of getting rid of the BKL is typically to make it visible
in the source. Once it is visible people will have incentive to eliminate it.
That is how the BKL conversion project for Linux long ago started too.
On 2.0 all system calls were still implicitely BKL and in 2.1 the
lock/unlock_kernel()s were moved into the various syscall functions and then
step by step eliminated.
And now it's time to do the same for all the ioctls too.
So my proposal is to convert the -ioctl handlers all over the tree
to -unlocked_ioctl with explicit lock_kernel()/unlock_kernel.
It is not a completely trivial conversion. You will have to
at least read the source, although not completely understand it.
But it is not very difficult.
Rough recipe:
Grep the source tree for struct file_operations. You should
fine something like
static int xyz_ioctl(struct inode *i, struct file *f, unsigned cmd, unsigned
long arg)
{
switch (cmd) {
case IOCTL1:
err = ...;
...
break;
case IOCTL2:
...
err = ...;
break;
default:
err = -ENOTTY;
}
return err;
}
...
struct file_operations xyz_ops = {
...
.ioctl = xyz_ioctl
};
The conversion is now to change the prototype of xyz_ioctl to
static long xyz_ioctl(struct file *f, unsigned cmd, unsigned long arg)
{
}
This means return type from int to long and drop the struct inode * argument
Then add lock_kernel() to the entry point and unlock_kernel() to all exit
points.
So you should get
static long xyz_ioctl(struct file *f, unsigned cmd, unsigned long arg)
{
lock_kernel();
...
unlock_kernel();
return err;
}
The only thing you need to watch out for is that all returns get an
unlock_kernel.
so e.g. if the ioctl handler has early error exits they all need an own
unlock_kernel()
(if you prefer it you can also use a goto to handle this, but just adding own
unlock_kernels is easier)
so e.g. if you have
case IOCTL1:
...
if (something failed)
return -EIO;
you would convert it to
if (something failed) {
unlock_kernel();
return -EIO;
}
It is very important that all returns have an unlock_kernel(), please always
double and triple check that!
Then change
.ioctl = xyz_ioctl
to
.unlocked_ioctl = xyz_ioctl
Then compile ideally test the result and submit it.
-Andi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
[Andi Kleen - Tue, Jan 08, 2008 at 05:40:15PM +0100] | | Here's a proposal for some useful code transformations the kernel janitors | could do as opposed to running checkpatch.pl. | [...snip...] | -Andi | | got it, thanks - Cyrill - -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tue, Jan 08, 2008 at 05:40:15PM +0100, Andi Kleen wrote: Here's a proposal for some useful code transformations the kernel janitors could do as opposed to running checkpatch.pl. Most ioctl handlers still running implicitely under the big kernel lock (BKL). But long term Linux is trying to get away from that. There is a new -unlocked_ioctl entry point that allows ioctls without BKL, but the code needs to be explicitely converted to use this. The first step of getting rid of the BKL is typically to make it visible in the source. Once it is visible people will have incentive to eliminate it. That is how the BKL conversion project for Linux long ago started too. On 2.0 all system calls were still implicitely BKL and in 2.1 the lock/unlock_kernel()s were moved into the various syscall functions and then step by step eliminated. And now it's time to do the same for all the ioctls too. So my proposal is to convert the -ioctl handlers all over the tree to -unlocked_ioctl with explicit lock_kernel()/unlock_kernel. Thanks, Andi! I think it'd very useful change. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Thanks, Andi! I think it'd very useful change. Reminds me this is something that should be actually flagged in checkpatch.pl too Andy, it would be good if checkpatch.pl complained about .ioctl = as opposed to .unlocked_ioctl = ... Also perhaps if a whole new file_operations with a ioctl is added complain about missing compat_ioctl as a low prioritity warning? (might be ok if it's architecture specific on architectures without compat layer) -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tue, 8 Jan 2008 20:58:04 +0100
Paolo Ciarrocchi [EMAIL PROTECTED] wrote:
-static const struct file_operations rtc_fops = {
+static long rtc_fioctl(struct file_operations rtc_fops)
+{
+ lock_kernel();
.owner = THIS_MODULE,
.llseek = no_llseek,
.read = rtc_read,
.poll = rtc_poll,
- .ioctl = rtc_ioctl,
+ .unlocked_ioctl = rtc_ioctl,
.open = rtc_open,
.release= rtc_release,
.fasync = rtc_fasync,
+ unlock_kernel();
};
This is a struct with function pointers, not a function.
No wonder it doesn't compile after you tried turning it into one :)
--
All rights reversed.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
Yes of course, I've been silly in didn't verify whether the file compile
but I would appreciate to know whether I'm on the right track or not.
Well ... you're not.
Here's what a correct conversion might look like. I haven't tried to
compile it, so I'm copying and pasting it in order to damage whitespace
and make sure nobody tries to compile it.
index bf1075e..0c543a8 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -174,8 +174,7 @@ static unsigned int rtc_poll(struct file *file, poll_table *
return data != 0 ? POLLIN | POLLRDNORM : 0;
}
-static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
-unsigned long arg)
+static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
struct rtc_ops *ops = file-private_data;
struct rtc_time tm;
@@ -183,6 +182,8 @@ static int rtc_ioctl(struct inode *inode, struct file *file,
void __user *uarg = (void __user *)arg;
int ret = -EINVAL;
+ lock_kernel();
+
switch (cmd) {
case RTC_ALM_READ:
ret = rtc_arm_read_alarm(ops, alrm);
@@ -277,6 +278,9 @@ static int rtc_ioctl(struct inode *inode, struct file *file,
ret = ops-ioctl(cmd, arg);
break;
}
+
+ unlock_kernel();
+
return ret;
}
@@ -334,7 +338,7 @@ static const struct file_operations rtc_fops = {
.llseek = no_llseek,
.read = rtc_read,
.poll = rtc_poll,
- .ioctl = rtc_ioctl,
+ .unlocked_ioctl = rtc_ioctl,
.open = rtc_open,
.release= rtc_release,
.fasync = rtc_fasync,
--
Intel are signing my paycheques ... these opinions are still mine
Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Jan 8, 2008 9:00 PM, Matthew Wilcox [EMAIL PROTECTED] wrote:
On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
-static const struct file_operations rtc_fops = {
+static long rtc_fioctl(struct file_operations rtc_fops)
+{
+ lock_kernel();
.owner = THIS_MODULE,
You should probably restrict yourself to files that you can at least
compile ...
Yes of course, I've been silly in didn't verify whether the file compile
but I would appreciate to know whether I'm on the right track or not.
Switching to a different file now...
Thanks.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Jan 8, 2008 5:40 PM, Andi Kleen [EMAIL PROTECTED] wrote:
Here's a proposal for some useful code transformations the kernel janitors
could do as opposed to running checkpatch.pl.
Most ioctl handlers still running implicitely under the big kernel
lock (BKL). But long term Linux is trying to get away from that. There is a
new
-unlocked_ioctl entry point that allows ioctls without BKL, but the code
needs to be explicitely converted to use this.
The first step of getting rid of the BKL is typically to make it visible
in the source. Once it is visible people will have incentive to eliminate it.
That is how the BKL conversion project for Linux long ago started too.
On 2.0 all system calls were still implicitely BKL and in 2.1 the
lock/unlock_kernel()s were moved into the various syscall functions and then
step by step eliminated.
And now it's time to do the same for all the ioctls too.
So my proposal is to convert the -ioctl handlers all over the tree
to -unlocked_ioctl with explicit lock_kernel()/unlock_kernel.
It is not a completely trivial conversion. You will have to
at least read the source, although not completely understand it.
But it is not very difficult.
Rough recipe:
Grep the source tree for struct file_operations. You should
fine something like
static int xyz_ioctl(struct inode *i, struct file *f, unsigned cmd, unsigned
long arg)
{
switch (cmd) {
case IOCTL1:
err = ...;
...
break;
case IOCTL2:
...
err = ...;
break;
default:
err = -ENOTTY;
}
return err;
}
...
struct file_operations xyz_ops = {
...
.ioctl = xyz_ioctl
};
The conversion is now to change the prototype of xyz_ioctl to
static long xyz_ioctl(struct file *f, unsigned cmd, unsigned long arg)
{
}
This means return type from int to long and drop the struct inode * argument
Then add lock_kernel() to the entry point and unlock_kernel() to all exit
points.
So you should get
static long xyz_ioctl(struct file *f, unsigned cmd, unsigned long arg)
{
lock_kernel();
...
unlock_kernel();
return err;
}
The only thing you need to watch out for is that all returns get an
unlock_kernel.
so e.g. if the ioctl handler has early error exits they all need an own
unlock_kernel()
(if you prefer it you can also use a goto to handle this, but just adding own
unlock_kernels is easier)
so e.g. if you have
case IOCTL1:
...
if (something failed)
return -EIO;
you would convert it to
if (something failed) {
unlock_kernel();
return -EIO;
}
It is very important that all returns have an unlock_kernel(), please always
double and triple check that!
Then change
.ioctl = xyz_ioctl
to
.unlocked_ioctl = xyz_ioctl
Then compile ideally test the result and submit it.
Hi Andi,
I grepped and tried to do what you suggested.
The first file that git grep reported was:
arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
So I cooked up the following patch (probably mangled, just to give you
a rough idea of what I did):
diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
index bf1075e..19dedb5 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
if (ret)
break;
ret = copy_to_user(uarg, alrm.time, sizeof(tm));
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
+ }
break;
case RTC_ALM_SET:
ret = copy_from_user(alrm.time, uarg, sizeof(tm));
if (ret) {
+ unlock_kernel();
ret = -EFAULT;
break;
}
@@ -215,8 +218,10 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
if (ret)
break;
ret = copy_to_user(uarg, tm, sizeof(tm));
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
+ }
break;
case RTC_SET_TIME:
@@ -226,6 +231,7 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
}
ret = copy_from_user(tm, uarg, sizeof(tm));
if (ret) {
+ unlock_kernel();
ret = -EFAULT;
break;
}
@@ -238,10 +244,12 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
-static const struct file_operations rtc_fops = {
+static long rtc_fioctl(struct file_operations rtc_fops)
+{
+ lock_kernel();
.owner = THIS_MODULE,
You should probably restrict yourself to files that you can at least
compile ...
--
Intel are signing my paycheques ... these opinions are still mine
Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
On Jan 8, 2008 9:00 PM, Matthew Wilcox [EMAIL PROTECTED] wrote:
On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
-static const struct file_operations rtc_fops = {
+static long rtc_fioctl(struct file_operations rtc_fops)
+{
+ lock_kernel();
.owner = THIS_MODULE,
You should probably restrict yourself to files that you can at least
compile ...
Yes of course, I've been silly in didn't verify whether the file compile
but I would appreciate to know whether I'm on the right track or not.
Well ... you're not.
You've deleted the definition of rtc_fops for no reason I can tell.
You put an unlock_kernel() on every place that invokes break;. You
should instead have put an unlock_kernel() before every return;.
You needed to put a lock_kernel() at the beginning of rtc_ioctl().
You needed to adjust the prototype of rtc_ioctl().
Honestly, if your C skills are as weak as they seem to be, you may be
better off trying to gain experience with some other project -- the
patch you sent appeared to be simple cargo-cult programming, and we
don't need that kind of contribution.
--
Intel are signing my paycheques ... these opinions are still mine
Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Jan 8, 2008 9:42 PM, Andi Kleen [EMAIL PROTECTED] wrote:
I grepped and tried to do what you suggested.
First a quick question: how would you rate your C knowledge? Did you
ever write a program yourself?
Yes I did but I probably beeing inactive for too much time,
I need to back studing a bit before submitting another patch.
My proposal assumes that you have at least basic C knowledge.
The first file that git grep reported was:
arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
It's probably better to only do that on files which you can easily compile.
For ARM you would need a cross compiler.
So I cooked up the following patch (probably mangled, just to give you
a rough idea of what I did):
diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
index bf1075e..19dedb5 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
if (ret)
break;
ret = copy_to_user(uarg, alrm.time, sizeof(tm));
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
In this case it would be better to just put the unlock_kernel() directly
before the single return. You only need to sprinkle them all over when
the function has multiple returns.
Understood. As Matthew did in his patch.
Or then change the flow to only
have a single return, but that would be slightly advanced.
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
+ }
break;
default:
@@ -329,15 +340,18 @@ static int rtc_fasync(int fd, struct file *file, int
on)
return fasync_helper(fd, file, on, rtc_async_queue);
}
-static const struct file_operations rtc_fops = {
+static long rtc_fioctl(struct file_operations rtc_fops)
+{
+ lock_kernel();
No the lock_kernel() of course has to be in the function, not in the structure
definition which does not contain any code.
Yes, understood now. Silly me :-/
Also the _operations structure stays the same (except for .ioctl -
.unlocked_ioctl);
only the the ioctl function prototype changes.
Am I'm working in the right direction or should I completely redo the patch?
I would suggest to only work on files that compile. e.g. do a
make allyesconfig
make -j$[$(grep -c processor /proc/cpuinfo)*2] 1 |tee LOG (will
probably take a long time)
first and then only modify files when are mentioned in LOG
Thanks you.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Jan 8, 2008 9:21 PM, Matthew Wilcox [EMAIL PROTECTED] wrote:
On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
Yes of course, I've been silly in didn't verify whether the file compile
but I would appreciate to know whether I'm on the right track or not.
Well ... you're not.
Here's what a correct conversion might look like. I haven't tried to
compile it, so I'm copying and pasting it in order to damage whitespace
and make sure nobody tries to compile it.
index bf1075e..0c543a8 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -174,8 +174,7 @@ static unsigned int rtc_poll(struct file *file,
poll_table *
return data != 0 ? POLLIN | POLLRDNORM : 0;
}
-static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int
cmd,
-unsigned long arg)
+static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
struct rtc_ops *ops = file-private_data;
struct rtc_time tm;
@@ -183,6 +182,8 @@ static int rtc_ioctl(struct inode *inode, struct file
*file,
void __user *uarg = (void __user *)arg;
int ret = -EINVAL;
+ lock_kernel();
+
switch (cmd) {
case RTC_ALM_READ:
ret = rtc_arm_read_alarm(ops, alrm);
@@ -277,6 +278,9 @@ static int rtc_ioctl(struct inode *inode, struct file
*file,
ret = ops-ioctl(cmd, arg);
break;
}
+
+ unlock_kernel();
+
return ret;
}
@@ -334,7 +338,7 @@ static const struct file_operations rtc_fops = {
.llseek = no_llseek,
.read = rtc_read,
.poll = rtc_poll,
- .ioctl = rtc_ioctl,
+ .unlocked_ioctl = rtc_ioctl,
.open = rtc_open,
.release= rtc_release,
.fasync = rtc_fasync,
Thank you Matthew,
I definitely need to back studying before submitting another patch.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
I grepped and tried to do what you suggested.
First a quick question: how would you rate your C knowledge? Did you
ever write a program yourself?
My proposal assumes that you have at least basic C knowledge.
The first file that git grep reported was:
arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
It's probably better to only do that on files which you can easily compile.
For ARM you would need a cross compiler.
So I cooked up the following patch (probably mangled, just to give you
a rough idea of what I did):
diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
index bf1075e..19dedb5 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
if (ret)
break;
ret = copy_to_user(uarg, alrm.time, sizeof(tm));
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
In this case it would be better to just put the unlock_kernel() directly
before the single return. You only need to sprinkle them all over when
the function has multiple returns. Or then change the flow to only
have a single return, but that would be slightly advanced.
- if (ret)
+ if (ret) {
+ unlock_kernel();
ret = -EFAULT;
+ }
break;
default:
@@ -329,15 +340,18 @@ static int rtc_fasync(int fd, struct file *file, int on)
return fasync_helper(fd, file, on, rtc_async_queue);
}
-static const struct file_operations rtc_fops = {
+static long rtc_fioctl(struct file_operations rtc_fops)
+{
+ lock_kernel();
No the lock_kernel() of course has to be in the function, not in the structure
definition which does not contain any code.
Also the _operations structure stays the same (except for .ioctl -
.unlocked_ioctl);
only the the ioctl function prototype changes.
Am I'm working in the right direction or should I completely redo the patch?
I would suggest to only work on files that compile. e.g. do a
make allyesconfig
make -j$[$(grep -c processor /proc/cpuinfo)*2] 1 |tee LOG (will
probably take a long time)
first and then only modify files when are mentioned in LOG
-Andi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl II
On Jan 9, 2008 12:06 AM, Andi Kleen [EMAIL PROTECTED] wrote:
I would suggest to only work on files that compile. e.g. do a
make allyesconfig
make -j$[$(grep -c processor /proc/cpuinfo)*2] 1 |tee LOG
(will probably take a long time)
first and then only modify files when are mentioned in LOG
Actually since this will probably take very long on a slower machine you can
refer to
http://halobates.de/allyes/
Thank you Andi.
for some allyes buildlogs of recent kernels for i386 and x86-64. A trick to
quickly check
if something compiles is also to do
make allyesconfig
make path/to/file.o
That won't catch linker errors, but if you don't have warnings there are
normally no
linker errors either.
I did grep for struct file_operations in mm:
[EMAIL PROTECTED]:~/linux-2.6/mm$ grep struct file_operations *
shmem.c:static const struct file_operations shmem_file_operations;
shmem.c:static const struct file_operations shmem_file_operations = {
swapfile.c:static const struct file_operations proc_swaps_operations = {
Am I right in saying that both the files don't need to be modified?
There is nothing like:
struct file_operations xyz_ops = {
...
.ioctl = xyz_ioctl
};
in there.
So I guess I need a smarter trick to find out which files need to be modified
as you previously suggested.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl II
I would suggest to only work on files that compile. e.g. do a make allyesconfig make -j$[$(grep -c processor /proc/cpuinfo)*2] 1 |tee LOG (will probably take a long time) first and then only modify files when are mentioned in LOG Actually since this will probably take very long on a slower machine you can refer to http://halobates.de/allyes/ for some allyes buildlogs of recent kernels for i386 and x86-64. A trick to quickly check if something compiles is also to do make allyesconfig make path/to/file.o That won't catch linker errors, but if you don't have warnings there are normally no linker errors either. -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Matthew Wilcox пишет:
On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
Yes of course, I've been silly in didn't verify whether the file compile
but I would appreciate to know whether I'm on the right track or not.
Well ... you're not.
Here's what a correct conversion might look like. I haven't tried to
compile it, so I'm copying and pasting it in order to damage whitespace
and make sure nobody tries to compile it.
It seems that the kernel janitors project needs to explicitly describe the
prerequisites a person should meet before tackling the enterprise's toughest
technical challenges such as operating system kernel development.
On the face of it, it seems quite ridiculous to me that a team of extremely
qualified kernel engineers spend their valuable time teaching the basics of
the C language using this mailing list. Time is precious, and even more so
when we are having that many unresolved problems, e.g. when the Kernel Bug
Tracker lists more than 1300 open bugs:
http://bugzilla.kernel.org/buglist.cgi?order=relevancebug_status=__open__
Paolo, I have nothing against you personally as you seem to have adequately
reacted to what is going on and went on strengthening your C skills; my point
is that an operating system kernel development facility such as LKML is most
probably not the right place to set up a correspondence course in programming
basics.
index bf1075e..0c543a8 100644
--- a/arch/arm/common/rtctime.c
+++ b/arch/arm/common/rtctime.c
@@ -174,8 +174,7 @@ static unsigned int rtc_poll(struct file *file,
poll_table *
return data != 0 ? POLLIN | POLLRDNORM : 0;
}
-static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int
cmd,
-unsigned long arg)
+static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
struct rtc_ops *ops = file-private_data;
struct rtc_time tm;
@@ -183,6 +182,8 @@ static int rtc_ioctl(struct inode *inode, struct file
*file,
void __user *uarg = (void __user *)arg;
int ret = -EINVAL;
+ lock_kernel();
+
switch (cmd) {
case RTC_ALM_READ:
ret = rtc_arm_read_alarm(ops, alrm);
@@ -277,6 +278,9 @@ static int rtc_ioctl(struct inode *inode, struct file
*file,
ret = ops-ioctl(cmd, arg);
break;
}
+
+ unlock_kernel();
+
return ret;
}
@@ -334,7 +338,7 @@ static const struct file_operations rtc_fops = {
.llseek = no_llseek,
.read = rtc_read,
.poll = rtc_poll,
- .ioctl = rtc_ioctl,
+ .unlocked_ioctl = rtc_ioctl,
.open = rtc_open,
.release= rtc_release,
.fasync = rtc_fasync,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl II
[EMAIL PROTECTED]:~/linux-2.6/mm$ grep struct file_operations *
shmem.c:static const struct file_operations shmem_file_operations;
shmem.c:static const struct file_operations shmem_file_operations = {
swapfile.c:static const struct file_operations proc_swaps_operations = {
Am I right in saying that both the files don't need to be modified?
If they don't have an ioctl handler they don't need to be modified, correct.
There is nothing like:
struct file_operations xyz_ops = {
...
.ioctl = xyz_ioctl
};
in there.
So I guess I need a smarter trick to find out which files need to be modified
as you previously suggested.
grep -P '\.ioctl.*=' $(grep -rl 'struct file_operations' * )
should work. There are also special multiline greps iirc that might also be able
to do this better (like sgrep)
-Andi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Andi Kleen wrote: Here's a proposal for some useful code transformations the kernel janitors could do as opposed to running checkpatch.pl. snip I notice that every driver in drivers/ata uses a .ioctl that points to ata_scsi_ioctl(). I could add the BKL to that function, and then change all of the drivers to .unlocked_ioctl, but I assume this would be a candidate to actually clean up by determining why the lock is needed and removing it if necessary. Does anyone know off-hand the reason for needing the lock (I assume someone does or it wouldn't have survived this long)? If the lock is absolutely required, then I can write the patch to add lock_kernel() and unlock_kernel(). -- Kevin Winchester -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tue, Jan 08, 2008 at 07:50:47PM -0400, Kevin Winchester wrote: Andi Kleen wrote: Here's a proposal for some useful code transformations the kernel janitors could do as opposed to running checkpatch.pl. snip I notice that every driver in drivers/ata uses a .ioctl that points to ata_scsi_ioctl(). I could add the BKL to that function, and then change This might be a little more complicated. These are funnelled through the block/SCSI layers which might not have separate unlocked ioctl callbacks yet. Would be probably not very difficult to add though. all of the drivers to .unlocked_ioctl, but I assume this would be a candidate to actually clean up by determining why the lock is needed and removing it if necessary. Does anyone know off-hand the reason for needing the lock (I assume someone does or it wouldn't have survived this long)? If the lock is absolutely required, then I can write the patch to add lock_kernel() and unlock_kernel(). Just sending the patch to add lock/unlock_kernel() is probably a good idea anyways -- Jeff will then feel bad over it and eventually remove it when he figures out it is safe ;-) -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Andi Kleen wrote: On Tue, Jan 08, 2008 at 07:50:47PM -0400, Kevin Winchester wrote: Andi Kleen wrote: Here's a proposal for some useful code transformations the kernel janitors could do as opposed to running checkpatch.pl. snip I notice that every driver in drivers/ata uses a .ioctl that points to ata_scsi_ioctl(). I could add the BKL to that function, and then change This might be a little more complicated. These are funnelled through the block/SCSI layers which might not have separate unlocked ioctl callbacks yet. Would be probably not very difficult to add though. all of the drivers to .unlocked_ioctl, but I assume this would be a candidate to actually clean up by determining why the lock is needed and removing it if necessary. Does anyone know off-hand the reason for needing the lock (I assume someone does or it wouldn't have survived this long)? If the lock is absolutely required, then I can write the patch to add lock_kernel() and unlock_kernel(). Just sending the patch to add lock/unlock_kernel() is probably a good idea anyways -- Jeff will then feel bad over it and eventually remove it when he figures out it is safe ;-) Sorry about the noise here - I now notice that not all .ioctl function pointers have the option of changing to .unlocked_ioctl. In this case, the ioctl is in the struct scsi_host_template, rather than struct file_operations. I'll try to be a little more careful about the git grepping in the future. -- Kevin Winchester -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Sorry about the noise here - I now notice that not all .ioctl function pointers have the option of changing to .unlocked_ioctl. In this case, the ioctl is in the struct scsi_host_template, rather than struct file_operations. I'll try to be a little more careful about the git grepping in the future. Well it just points to another area that needs to be improved. Clearly scsi_host_template should have a unlocked_ioctl too. -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tuesday 08 January 2008, Andi Kleen wrote: Thanks, Andi! I think it'd very useful change. Reminds me this is something that should be actually flagged in checkpatch.pl too Andy, it would be good if checkpatch.pl complained about .ioctl = as opposed to .unlocked_ioctl = ... This is rather hard, as there are different data structures that all contain -ioctl and/or -unlocked_ioctl function pointers. Some of them already use -ioctl in an unlocked fashion only, so blindly warning about this would give lots of false positives. Also perhaps if a whole new file_operations with a ioctl is added complain about missing compat_ioctl as a low prioritity warning? (might be ok if it's architecture specific on architectures without compat layer) Also, not every data structure that provides a -ioctl callback also has a -compat_ioctl, although there should be fewer exceptions here. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wed, Jan 09, 2008 at 01:40:58AM +0100, Arnd Bergmann wrote:
On Tuesday 08 January 2008, Andi Kleen wrote:
Thanks, Andi! I think it'd very useful change.
Reminds me this is something that should be actually flagged
in checkpatch.pl too
Andy, it would be good if checkpatch.pl complained about .ioctl =
as opposed to .unlocked_ioctl = ...
This is rather hard, as there are different data structures that
all contain -ioctl and/or -unlocked_ioctl function pointers.
Some of them already use -ioctl in an unlocked fashion only,
so blindly warning about this would give lots of false positives.
I imagined it would check for
+struct file_operations ... = {
+ ...
+ .ioctl = ...
That wouldn't catch the case of someone adding only .ioctl to an
already existing file_operations which is not visible in the patch context,
but that should be hopefully rare. The more common case is adding
completely new operations
Also perhaps if a whole new file_operations with a ioctl is added
complain about missing compat_ioctl as a low prioritity warning?
(might be ok if it's architecture specific on architectures without
compat layer)
Also, not every data structure that provides a -ioctl callback
also has a -compat_ioctl, although there should be fewer exceptions
That's probably a bug in general. e.g. those likely won't work
at all on the compat by default architectures like sparc or ppc64.
-Andi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Wednesday 09 January 2008, Andi Kleen wrote:
I imagined it would check for
+struct file_operations ... = {
+ ...
+ .ioctl = ...
That wouldn't catch the case of someone adding only .ioctl to an
already existing file_operations which is not visible in the patch context,
but that should be hopefully rare. The more common case is adding
completely new operations
Right, this would work fine. We can probably even have a list of
data structures that work like file_operations in this regard.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
Arnd Bergmann wrote:
On Wednesday 09 January 2008, Andi Kleen wrote:
I imagined it would check for
+struct file_operations ... = {
+ ...
+ .ioctl = ...
That wouldn't catch the case of someone adding only .ioctl to an
already existing file_operations which is not visible in the patch context,
but that should be hopefully rare. The more common case is adding
completely new operations
Right, this would work fine. We can probably even have a list of
data structures that work like file_operations in this regard.
file_operations block_device_operations are the only two that I can find.
--
Kevin Winchester
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [JANITOR PROPOSAL] Switch ioctl functions to -unlocked_ioctl
On Tue, Jan 08, 2008 at 09:31:24PM -0400, Kevin Winchester wrote:
Arnd Bergmann wrote:
On Wednesday 09 January 2008, Andi Kleen wrote:
I imagined it would check for
+struct file_operations ... = {
+ ...
+ .ioctl = ...
That wouldn't catch the case of someone adding only .ioctl to an
already existing file_operations which is not visible in the patch
context,
but that should be hopefully rare. The more common case is adding
completely new operations
Right, this would work fine. We can probably even have a list of
data structures that work like file_operations in this regard.
file_operations block_device_operations are the only two that I can find.
There are a few like scsi_host_template that don't have a unlocked_ioctl yet,
but that is just something that needs to be fixed.
-Andi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
