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
[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 -
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.
> 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
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,
* There were no RTC clocks before 1900.
*/
if (arg < 1900) {
+ unlock_kernel();
ret = -EINVAL;
break;
}
if (!capable(CAP_SYS_TIME)) {
+ unlock_kernel();
ret = -EACCES;
break;
}
@@ -257,6 +265,7 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
case RTC_WKALM_SET:
ret = copy_from_user(&alrm, uarg, sizeof(alrm));
if (ret) {
+ unlock_kernel();
ret = -EFAULT;
break;
}
@@ -268,8 +277,10 @@ static int rtc_ioctl(struct inode *inode, struct
file *file, unsigned int cmd,
if (ret)
break;
ret = copy_to_user(uarg, &alrm, sizeof(alrm));
- 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();
.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();
};
static struct miscdevice rtc_miscdev = {
Unforunately, after I wrote the patch I noticed that on a vanilla
kernel that file doesn't compile
so I cannot verify the work I did.
Am I'm working in the right direction or should I completely redo the patch?
Thanks.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
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."
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/
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."
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."
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.
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/
> 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
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/
> 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
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:
paolo@paolo-desktop:~/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/
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
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&bug_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,
>
>
> paolo@paolo-desktop:~/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
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
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
> 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
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 <><
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
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 <><
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
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
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.
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.
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;
+ if (val >= MULDIV(INT_MAX, USER_HZ, HZ))
+ val = MULDIV(INT_MAX, USER_HZ, HZ);
sfp->timeout_user = val;
sfp->timeout = MULDIV (val, HZ, USER_HZ);
- return 0;
+ result = 0;
+ goto out;
case SG_GET_TIMEOUT: /* N.B. User receives timeout as return value */
/* strange ..., for backward compatibility */
- return sfp->timeout_user;
+ result = sfp->timeout_user;
+ goto out;
case SG_SET_FORCE_LOW_DMA:
result = get_user(val, ip);
if (result)
- return result;
+ goto out;
if (val) {
sfp->low_dma = 1;
if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
@@ -846,21 +858,26 @@ sg_ioctl(struct inode *inode, struct file *filp,
sg_build_reserve(sfp, val);
}
} else {
+ result = -ENODEV;
if (sdp->detached)
- return -ENODEV;
+ goto out;
sfp->low_dma = sdp->device->host->unchecked_isa_dma;
}
- return 0;
+ result = 0;
+ goto out;
case SG_GET_LOW_DMA:
- return put_user((int) sfp->low_dma, ip);
+ result = put_user((int) sfp->low_dma, ip);
+ goto out;
case SG_GET_SCSI_ID:
+ result = -EFAULT;
if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
- return -EFAULT;
- else {
+ goto out;
+ {
sg_scsi_id_t __user *sg_idp = p;
+ result = -ENODEV;
if (sdp->detached)
- return -ENODEV;
+ goto out;
__put_user((int) sdp->device->host->host_no,
&sg_idp->host_no);
__put_user((int) sdp->device->channel,
@@ -874,29 +891,33 @@ sg_ioctl(struct inode *inode, struct file *filp,
&sg_idp->d_queue_depth);
__put_user(0, &sg_idp->unused[0]);
__put_user(0, &sg_idp->unused[1]);
- return 0;
+ result = 0;
+ goto out;
}
case SG_SET_FORCE_PACK_ID:
result = get_user(val, ip);
if (result)
- return result;
+ goto out;
sfp->force_packid = val ? 1 : 0;
- return 0;
+ result = 0;
+ goto out;
case SG_GET_PACK_ID:
+ result = -EFAULT;
if (!access_ok(VERIFY_WRITE, ip, sizeof (int)))
- return -EFAULT;
+ goto out;
read_lock_irqsave(&sfp->rq_list_lock, iflags);
+ result = 0;
for (srp = sfp->headrp; srp; srp = srp->nextrp) {
if ((1 == srp->done) && (!srp->sg_io_owned)) {
read_unlock_irqrestore(&sfp->rq_list_lock,
iflags);
__put_user(srp->header.pack_id, ip);
- return 0;
+ goto out;
}
}
read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
__put_user(-1, ip);
- return 0;
+ goto out;
case SG_GET_NUM_WAITING:
read_lock_irqsave(&sfp->rq_list_lock, iflags);
for (val = 0, srp = sfp->headrp; srp; srp = srp->nextrp) {
@@ -904,67 +925,76 @@ sg_ioctl(struct inode *inode, struct file *filp,
++val;
}
read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
- return put_user(val, ip);
+ result = put_user(val, ip);
+ goto out;
case SG_GET_SG_TABLESIZE:
- return put_user(sdp->sg_tablesize, ip);
+ result = put_user(sdp->sg_tablesize, ip);
+ goto out;
case SG_SET_RESERVED_SIZE:
result = get_user(val, ip);
if (result)
- return result;
- if (val < 0)
- return -EINVAL;
+ goto out;
+ result = -EINVAL;
+ if (val < 0)
+ goto out;
val = min_t(int, val,
sdp->device->request_queue->max_sectors * 512);
if (val != sfp->reserve.bufflen) {
+ result = -EBUSY;
if (sg_res_in_use(sfp) || sfp->mmap_called)
- return -EBUSY;
+ goto out;
sg_remove_scat(&sfp->reserve);
sg_build_reserve(sfp, val);
}
- return 0;
+ result = 0;
+ goto out;
case SG_GET_RESERVED_SIZE:
val = min_t(int, sfp->reserve.bufflen,
sdp->device->request_queue->max_sectors * 512);
- return put_user(val, ip);
+ result = put_user(val, ip);
+ goto out;
case SG_SET_COMMAND_Q:
result = get_user(val, ip);
- if (result)
- return result;
- sfp->cmd_q = val ? 1 : 0;
- return 0;
+ if (!result)
+ sfp->cmd_q = val ? 1 : 0;
+ goto out;
case SG_GET_COMMAND_Q:
- return put_user((int) sfp->cmd_q, ip);
+ result = put_user((int) sfp->cmd_q, ip);
+ goto out;
case SG_SET_KEEP_ORPHAN:
result = get_user(val, ip);
- if (result)
- return result;
- sfp->keep_orphan = val;
- return 0;
+ if (!result)
+ sfp->keep_orphan = val;
+ goto out;
case SG_GET_KEEP_ORPHAN:
- return put_user((int) sfp->keep_orphan, ip);
+ result = put_user((int) sfp->keep_orphan, ip);
+ goto out;
case SG_NEXT_CMD_LEN:
result = get_user(val, ip);
- if (result)
- return result;
- sfp->next_cmd_len = (val > 0) ? val : 0;
- return 0;
+ if (!result)
+ sfp->next_cmd_len = (val > 0) ? val : 0;
+ goto out;
case SG_GET_VERSION_NUM:
- return put_user(sg_version_num, ip);
+ result = put_user(sg_version_num, ip);
+ goto out;
case SG_GET_ACCESS_COUNT:
/* faked - we don't have a real access count anymore */
val = (sdp->device ? 1 : 0);
- return put_user(val, ip);
+ result = put_user(val, ip);
+ goto out;
case SG_GET_REQUEST_TABLE:
+ result = -EFAULT;
if (!access_ok(VERIFY_WRITE, p, SZ_SG_REQ_INFO * SG_MAX_QUEUE))
- return -EFAULT;
- else {
+ goto out;
+ {
sg_req_info_t *rinfo;
unsigned int ms;
rinfo = kmalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
GFP_KERNEL);
+ result = -ENOMEM;
if (!rinfo)
- return -ENOMEM;
+ goto out;
read_lock_irqsave(&sfp->rq_list_lock, iflags);
for (srp = sfp->headrp, val = 0; val < SG_MAX_QUEUE;
++val, srp = srp ? srp->nextrp : srp) {
@@ -998,25 +1028,29 @@ sg_ioctl(struct inode *inode, struct file *filp,
SZ_SG_REQ_INFO * SG_MAX_QUEUE);
result = result ? -EFAULT : 0;
kfree(rinfo);
- return result;
}
+ goto out;
case SG_EMULATED_HOST:
if (sdp->detached)
- return -ENODEV;
- return put_user(sdp->device->host->hostt->emulated, ip);
+ result = -ENODEV;
+ else
+ result = put_user(sdp->device->host->hostt->emulated, ip);
+ goto out;
case SG_SCSI_RESET:
+ result = -ENODEV;
if (sdp->detached)
- return -ENODEV;
+ goto out;
+ result = -EBUSY;
if (filp->f_flags & O_NONBLOCK) {
if (scsi_host_in_recovery(sdp->device->host))
- return -EBUSY;
+ goto out;
} else if (!scsi_block_when_processing_errors(sdp->device))
- return -EBUSY;
+ goto out;
result = get_user(val, ip);
if (result)
- return result;
+ goto out;
if (SG_SCSI_RESET_NOTHING == val)
- return 0;
+ goto out;
switch (val) {
case SG_SCSI_RESET_DEVICE:
val = SCSI_TRY_RESET_DEVICE;
@@ -1028,46 +1062,60 @@ sg_ioctl(struct inode *inode, struct file *filp,
val = SCSI_TRY_RESET_HOST;
break;
default:
- return -EINVAL;
+ result = -EINVAL;
+ goto out;
}
+ result = -EACCES;
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
- return -EACCES;
- return (scsi_reset_provider(sdp->device, val) ==
- SUCCESS) ? 0 : -EIO;
+ goto out;
+ result = scsi_reset_provider(sdp->device, val) == SUCCESS?
+ 0 : -EIO;
+ goto out;
case SCSI_IOCTL_SEND_COMMAND:
+ result = -ENODEV;
if (sdp->detached)
- return -ENODEV;
+ goto out;
if (read_only) {
unsigned char opcode = WRITE_6;
Scsi_Ioctl_Command __user *siocp = p;
+ result = -EFAULT;
if (copy_from_user(&opcode, siocp->data, 1))
- return -EFAULT;
+ goto out;
+ result = -EPERM;
if (!sg_allow_access(opcode, sdp->device->type))
- return -EPERM;
+ goto out;
}
- return sg_scsi_ioctl(filp, sdp->device->request_queue, NULL, p);
+ result = sg_scsi_ioctl(filp, sdp->device->request_queue, NULL, p);
+ goto out;
case SG_SET_DEBUG:
result = get_user(val, ip);
- if (result)
- return result;
- sdp->sgdebug = (char) val;
- return 0;
+ if (!result)
+ sdp->sgdebug = (char) val;
+ goto out;
case SCSI_IOCTL_GET_IDLUN:
case SCSI_IOCTL_GET_BUS_NUMBER:
case SCSI_IOCTL_PROBE_HOST:
case SG_GET_TRANSFORM:
+ result = -ENODEV;
if (sdp->detached)
- return -ENODEV;
- return scsi_ioctl(sdp->device, cmd_in, p);
+ goto out;
+ result = scsi_ioctl(sdp->device, cmd_in, p);
+ goto out;
case BLKSECTGET:
- return put_user(sdp->device->request_queue->max_sectors * 512,
+ result = put_user(sdp->device->request_queue->max_sectors * 512,
ip);
+ goto out;
default:
+ result = -EPERM; /* don't know so take safe approach */
if (read_only)
- return -EPERM; /* don't know so take safe approach */
- return scsi_ioctl(sdp->device, cmd_in, p);
+ goto out;
+ result = scsi_ioctl(sdp->device, cmd_in, p);
+ goto out;
}
+out:
+ unlock_kernel();
+ return result;
}
#ifdef CONFIG_COMPAT
@@ -1314,7 +1362,7 @@ static struct file_operations sg_fops = {
.read = sg_read,
.write = sg_write,
.poll = sg_poll,
- .ioctl = sg_ioctl,
+ .unlocked_ioctl = sg_ioctl,
#ifdef CONFIG_COMPAT
.compat_ioctl = sg_compat_ioctl,
#endif
--
The only person who always got his work done by Friday was Robinson Crusoe
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 = -Exxxx;
goto out;
}
then:
result = -Exxxx;
if (x)
goto out;
but it looks correct.
cu,
Richard
On 14:17, Richard Knutsson wrote:
> Would had preferred:
>
> if (x) {
> result = -Exxxx;
> goto out;
> }
>
> then:
>
> result = -Exxxx;
> 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
> 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
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.
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]
On Wed, Jan 09, 2008 at 10:40:26PM +0000, 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
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]
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
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]
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
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]
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
On Wed, Jan 09, 2008 at 10:45:13PM +0000, 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.
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
> 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
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
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
> 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
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]
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
> 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
Nice start, Paolo, really!
I'd recommend to start supervising all this effort, if you wish to
participate. This doesn't require C knowledge at all, not every sw
manager shall have lang:c [+]. But you will save other's valuable time
and effort spent. Also you will eliminate rude and sucking comments like
<[email protected]>.
If you know how to make simple "unlocked_ioctl progress" web page for that
on wiki, and to look after it, please do so on kernelnewbies.org site.
For now i see on the list following:
Mathieu Segaud, Sergio Luis:
'PCI: change procfs'
Andre Noll:
'drivers/scsi/sg.c to unlocked_ioctl'
Kevin Winchester:
'drivers/char/drm to use .unlocked_ioctl'
Nikanth Karthikesan:
'drm_ioctl as an unlocked_ioctl'
'x86 Machine check handler to use unlocked_iocl'
'paride driver to use unlocked_ioctl'
'Random number driver: make random_ioctl as an unlocked_ioctl'
Also you can make a "Tools" section there, where experience and tools
may be presented. This knowledge may improve future source tree
processing efforts, and save one's time and bandwidth (searching
archives, wrong patches, etc.) as well.
I refer to the _text_ processing tools and techniques, i'm going to
present below (skip it guys, if you don't like it).
== Tools ==
As discussions show, knowledge of C doesn't mean solving this task at
all. Even worse, one may re-factor/change functions in the way they like,
but maintainers don't. Compile-time "testing" is a distraction from the
cause. Note, however, that my vision of the root also may have some flaws.
Documentation and tutorials. While i'm willing to help in any technical
things, i know, please look at this first:
http://unix.org.ua/orelly/unix/
"UNIX Power Tools" is a must (without perl part).
Keywords: kernel, shell, BRE, sed.
So, expressions. Let's see on the error you've made. This is not a C
error at first, this is an expression error:
Paolo Ciarrocchi @ Tue, 8 Jan 2008 21:03:13 +0100:
>> > -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.
what you've did is introducing more special symbols, to say least.
This often leads to definition and meaning changes, rather than
execution (locks, flow) ones.
While C is not easy to understand generally, basic things, like functions
and structs with good coding style, as Linux has, are quite
distinguishable.
So what key thing(s) that can make expression of a function? This are
braces '(', ')', but see [0] for more.
[0] http://www.mers.byu.edu/docs/standardC/declare.html#Reading%20Declarations
Based on that one can `grep` for functions easily[1] (kind of).
[1] http://article.gmane.org/gmane.linux.kernel/120209
Globally defined structs OTOH are storage things, and usually they are
initialized with '='. What make expression for them easy is that block
('{', '}') is usually started inline with declaration, while it is
accepted coding style for Linux, that functions have blocks started
from the new line.
Thus, it's obvious that special symbols are key things in building of
expressions and recognizing constructs.
Let's consider [1].
1) multiline declarations/definitions are not easy to `grep`
2) proposed method have problems with all things, that can appear from
the start of the string to the '(', i.e. returing of pointers '*',
__attiributes__ '((', '"', etc. (you never now).
3) similar patterns can be inside
3.1) multiline comment blocks
3.2) multiline macro definitions
*) add yours
= c_func.sh =
#!/bin/sh
# find linux-style function definitions (lang:C)
# Time-stamp: "Thu Mar 6 12:59:10 CET 2008 olecom@flower"
sed -n '
# 3.1: /* skip one line comment (for multiline to work) */
/^\/\*.*\*\/$/b
# 3.1: /* skip multiline comment
# inside can be anything, that match a function */
/^\/\*/,/\*\/$/b
# 3.2: skip multiline macros (they generate noise)
# TODO: find function-generating macros
/^[[:blank:]]*#/{
:_macro
/\\$/{
N
b_macro
}
b
}
# function definition; ends with ")", "," or is like
# "int foo(int pabar) { return 0; }"
/^[^[:blank:]{].*[),}]$/{ # 2: look at the end of the line
/,$/{ # 1: multiline param. block
:_start
N
/;$/b; # skip declarations (ends with ";")
s=[)}]$=&= ; t_end ; b_start
:_end
# post-processing
## make inline
/\n/s-\n[[:blank:]]*- -g
}
## remove inline body
/}$/s- *{.*--
p}' "$@"
This script addresses all cases, but not a returning type, if it is on
prev. line.
== semantic buzzwords ==
I saw a [C]"semantic patch" in the janitor list. Don't know if `sed` can
beat it, but at least, it's not a x86 3M binary blob and without any
configuration files. But i'm sure, that sooner or later, scripting or
semantic source code precessing on the git's side will be inevitable
(automatic audit, yet another check-style, *switch imagination*).
With some kind of special web site or effort to have reviewed regular
expressions, IMHO it will be much more easier to move forward, than to
invent `perl` stuff over and over again (cleanpatch, cleanfile,
checkpatch.pl, unifdef.c, etc.).
Syntax is hard, yes, especially without syntax highlighting even in
cool 20 years old GNU Emacs. But it is like assembly, and assembly is
language of The Kernel Hackers[3].
[3] http://kerneltrap.org/Linux/Decoding_Oops
> Switching to a different file now...
Hope you like this start. Maybe i will do all additional stuff for
"unlocked_ioctl" case:
* identifying need of the change,
* lock / unlock in all return variants,
* changing in-kernel users of the changed functions,
* testing with existing manual patches.
But this is very unlikely in case of lack of conversations and curiosity
(at least).
p.s.
> --
> 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/
We still (after 13 years) don't know how to use mailng lists, do we?
--
-o--=O`C
#oo'L O
<___=E M