[JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andi Kleen]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Cyrill Gorcunov]

[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 -

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Alexey Dobriyan]

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.

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Paolo Ciarrocchi]

On Jan 8, 2008 5:40 PM, Andi Kleen <andi@firstfloor.org> 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/

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Matthew Wilcox]

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."

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Paolo Ciarrocchi]

On Jan 8, 2008 9:00 PM, Matthew Wilcox <matthew@wil.cx> 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/

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Matthew Wilcox]

On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
> On Jan 8, 2008 9:00 PM, Matthew Wilcox <matthew@wil.cx> 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."

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.

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."

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Paolo Ciarrocchi]

On Jan 8, 2008 9:21 PM, Matthew Wilcox <matthew@wil.cx> 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/

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Dmitri Vorobiev]

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,
> 
> 

supervising, text processing, semantic "patching" (Re: [JANITOR PROPOSAL] Switch ioctl functions to

[Oleg Verych]

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
<20080111223651.GA5942@kroah.com>.

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

We still (after 13 years) don't know how to use mailng lists, do we?
--
-o--=O`C
 #oo'L O
<___=E M

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Rik van Riel]

On Tue, 8 Jan 2008 20:58:04 +0100
"Paolo Ciarrocchi" <paolo.ciarrocchi@gmail.com> 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.

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andi Kleen]

> 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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Paolo Ciarrocchi]

On Jan 8, 2008 9:42 PM, Andi Kleen <andi@firstfloor.org> 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/

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl II

[Andi Kleen]

> 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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl II

[Paolo Ciarrocchi]

On Jan 9, 2008 12:06 AM, Andi Kleen <andi@firstfloor.org> 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/

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl II

[Andi Kleen]

> 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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Matt Mackall]

On Tue, 2008-01-08 at 20:58 +0100, Paolo Ciarrocchi wrote:
> On Jan 8, 2008 5:40 PM, Andi Kleen <andi@firstfloor.org> 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.

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Alasdair G Kergon]

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
-- 
agk@redhat.com

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andi Kleen]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Alasdair G Kergon]

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
-- 
agk@redhat.com

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Chris Friesen]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Alasdair G Kergon]

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
-- 
agk@redhat.com

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Vadim Lobanov]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Alasdair G Kergon]

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
-- 
agk@redhat.com

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Vadim Lobanov]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Christoph Hellwig]

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.

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Daniel Phillips]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Alasdair G Kergon]

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
-- 
agk@redhat.com

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Daniel Phillips]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Pavel Machek]

> 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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Kevin Winchester]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andi Kleen]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Kevin Winchester]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andi Kleen]

> 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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 12532 bytes --]

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 <maan@systemlinux.org>

---
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Richard Knutsson]

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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andi Kleen]

> 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

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andi Kleen]

> 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