[PATCH] x86: Switch apm to unlocked_kernel

[PATCH] x86: Switch apm to unlocked_kernel

[Alan Cox]

This pushes the lock a fair way down and the final kill looks like it
should be an easy project for someone who wants to have a shot at it.

Signed-off-by: Alan Cox <alan@redhat.com>

diff --git a/arch/x86/kernel/apm_32.c b/arch/x86/kernel/apm_32.c
index bf9290e..00e6d13 100644
--- a/arch/x86/kernel/apm_32.c
+++ b/arch/x86/kernel/apm_32.c
@@ -228,6 +228,7 @@
 #include <linux/suspend.h>
 #include <linux/kthread.h>
 #include <linux/jiffies.h>
+#include <linux/smp_lock.h>
 
 #include <asm/system.h>
 #include <asm/uaccess.h>
@@ -1149,7 +1150,7 @@ static void queue_event(apm_event_t event, struct apm_user *sender)
 				as->event_tail = 0;
 		}
 		as->events[as->event_head] = event;
-		if ((!as->suser) || (!as->writer))
+		if (!as->suser || !as->writer)
 			continue;
 		switch (event) {
 		case APM_SYS_SUSPEND:
@@ -1396,7 +1397,7 @@ static void apm_mainloop(void)
 
 static int check_apm_user(struct apm_user *as, const char *func)
 {
-	if ((as == NULL) || (as->magic != APM_BIOS_MAGIC)) {
+	if (as == NULL || as->magic != APM_BIOS_MAGIC) {
 		printk(KERN_ERR "apm: %s passed bad filp\n", func);
 		return 1;
 	}
@@ -1459,18 +1460,19 @@ static unsigned int do_poll(struct file *fp, poll_table *wait)
 	return 0;
 }
 
-static int do_ioctl(struct inode *inode, struct file *filp,
-		    u_int cmd, u_long arg)
+static long do_ioctl(struct file *filp, u_int cmd, u_long arg)
 {
 	struct apm_user *as;
+	int ret;
 
 	as = filp->private_data;
 	if (check_apm_user(as, "ioctl"))
 		return -EIO;
-	if ((!as->suser) || (!as->writer))
+	if (!as->suser || !as->writer)
 		return -EPERM;
 	switch (cmd) {
 	case APM_IOC_STANDBY:
+		lock_kernel();
 		if (as->standbys_read > 0) {
 			as->standbys_read--;
 			as->standbys_pending--;
@@ -1479,8 +1481,10 @@ static int do_ioctl(struct inode *inode, struct file *filp,
 			queue_event(APM_USER_STANDBY, as);
 		if (standbys_pending <= 0)
 			standby();
+		unlock_kernel();
 		break;
 	case APM_IOC_SUSPEND:
+		lock_kernel();
 		if (as->suspends_read > 0) {
 			as->suspends_read--;
 			as->suspends_pending--;
@@ -1488,16 +1492,17 @@ static int do_ioctl(struct inode *inode, struct file *filp,
 		} else
 			queue_event(APM_USER_SUSPEND, as);
 		if (suspends_pending <= 0) {
-			return suspend(1);
+			ret = suspend(1);
 		} else {
 			as->suspend_wait = 1;
 			wait_event_interruptible(apm_suspend_waitqueue,
 					as->suspend_wait == 0);
-			return as->suspend_result;
+			ret = as->suspend_result;
 		}
-		break;
+		unlock_kernel();
+		return ret;
 	default:
-		return -EINVAL;
+		return -ENOTTY;
 	}
 	return 0;
 }
@@ -1860,7 +1865,7 @@ static const struct file_operations apm_bios_fops = {
 	.owner		= THIS_MODULE,
 	.read		= do_read,
 	.poll		= do_poll,
-	.ioctl		= do_ioctl,
+	.unlocked_ioctl	= do_ioctl,
 	.open		= do_open,
 	.release	= do_release,
 };

Re: [PATCH] x86: Switch apm to unlocked_kernel

[Stephen Rothwell]

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

Hi Alan,

On Thu, 22 May 2008 21:22:04 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> This pushes the lock a fair way down and the final kill looks like it
> should be an easy project for someone who wants to have a shot at it.

and makes some gratuitous stylistic changes as well.  Please don't do
that!

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

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

Re: [PATCH] x86: Switch apm to unlocked_kernel

[Alan Cox]

On Fri, 23 May 2008 11:03:53 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Alan,
> 
> On Thu, 22 May 2008 21:22:04 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >
> > This pushes the lock a fair way down and the final kill looks like it
> > should be an easy project for someone who wants to have a shot at it.
> 
> and makes some gratuitous stylistic changes as well.  Please don't do
> that!

Andrew shouts at me if I send him patches that don't fix the style of
the lines around so you get some style changes even when I cut bits out
of diffs. I think that approach is *stupid* too and style should be done
after for the entire file.

Alan

Re: [PATCH] x86: Switch apm to unlocked_kernel

[Kevin Winchester]

On Thu, May 22, 2008 at 5:22 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> This pushes the lock a fair way down and the final kill looks like it
> should be an easy project for someone who wants to have a shot at it.
>

I would like to have a shot at this if no one else if currently
working on it.  This would be my first attempt to graduate from
testing (and a few simple patches) to an actual functional change.  I
would expect that the steps involved are:

- See what data is affected under the BKL
- See where else that data may be affected (including possible
parallel calls to the same path)
- Decide what locking is necessary
- Implement it (the easy part)

If this is about right (and I'm not missing anything major) - I will
give it a try and see where I get.

If, however, I have just proved that I do not have a full grasp of the
problem, or if someone else is already working on this particular path
- please let me know.

-- 
Kevin Winchester

Re: [PATCH] x86: Switch apm to unlocked_kernel

[Stephen Rothwell]

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

Hi Alan,

On Fri, 23 May 2008 12:06:17 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> Andrew shouts at me if I send him patches that don't fix the style of
> the lines around so you get some style changes even when I cut bits out
> of diffs. I think that approach is *stupid* too and style should be done
> after for the entire file.

Then let the maintainer (nominally me) shout at Andrew.  I don't agree
with those style changes and we usually leave such purely stylistic
things to the maintainer of the file in question.  If Andrew requires
these changes, then Andrew is wrong about this.  It just confuses the
real changes and adds to the overheads of those trying to do reviews (of
which we have too few).  And in this case "fix" is in the eye of the
beholder.

If you (or Andrew) thinks that those style changes are necessary, then
send them separately and I will NAK them :-)  In this case it is pure
churn for absolutely no gain whatsoever.  Two of the hunks weren't even
anywhere near the real changes.

/me is letting the linux-next work go to his head :-)
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

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

Re: [PATCH] x86: Switch apm to unlocked_kernel

[Andrew Morton]

On Fri, 23 May 2008 23:23:39 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Alan,
> 
> On Fri, 23 May 2008 12:06:17 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >
> > Andrew shouts at me if I send him patches that don't fix the style of
> > the lines around so you get some style changes even when I cut bits out
> > of diffs.

Don't think so.  Unrelated changes are well-known poor-form.

I do think that if one is already changing a line which is incorrectly
laid out then there's no point in _leaving_ it incorrect.  There's no
downside to fixing it.

That being said, it's often sorely tempting to go hunting down nearby
sillinesses.  I succumb to that temptation and usually won't complain
when others do also, up to a point.

> I think that approach is *stupid* too and style should be done
> > after for the entire file.
> 
> Then let the maintainer (nominally me) shout at Andrew.  I don't agree
> with those style changes and we usually leave such purely stylistic
> things to the maintainer of the file in question.

mm, not really.  Wrong is wrong and if nominal maintainer insists on
retaining wrong we have cheery bunfights about it.

> If Andrew requires
> these changes, then Andrew is wrong about this.  It just confuses the
> real changes and adds to the overheads of those trying to do reviews (of
> which we have too few).

I think those changes went above and beyond the call.

> And in this case "fix" is in the eye of the
> beholder.

And that is why we have a standard - so that different parts of the
kernel do not end up having different appearance due to local
preferences.

Re: [RFC] x86: Switch apm to unlocked_kernel

[Kevin Winchester]

Kevin Winchester wrote:
> On Thu, May 22, 2008 at 5:22 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> This pushes the lock a fair way down and the final kill looks like it
>> should be an easy project for someone who wants to have a shot at it.
>>
> 
> I would like to have a shot at this if no one else if currently
> working on it.  This would be my first attempt to graduate from
> testing (and a few simple patches) to an actual functional change.  I
> would expect that the steps involved are:
> 
> - See what data is affected under the BKL
> - See where else that data may be affected (including possible
> parallel calls to the same path)
> - Decide what locking is necessary
> - Implement it (the easy part)
> 
> If this is about right (and I'm not missing anything major) - I will
> give it a try and see where I get.
> 
> If, however, I have just proved that I do not have a full grasp of the
> problem, or if someone else is already working on this particular path
> - please let me know.
> 

(Added Stephen to cc as he is the maintainer here)

After looking at this for a while to figure out the necessary locking, I 
discovered drivers/char/apm_emulation.c, which has very similar 
structures to arch/x86/kernel/apm_32.c, and has much more locking (e.g. 
A state_lock mutex, a user_list_lock rwsem) and it uses a list_head for 
the apm_user_list instead of an open coded doubly linked list.

This brought up the following question:

- Should I just copy the locking from apm_emulation to apm?  Another 
patch from Alan moved the BKL down into the apm_emulation ioctl method, 
but it seems pretty well locked as it is - so that BKL can probably just 
be removed there.

Also - as far as I can tell, the two files both do the exact same thing 
from the point of view of the kapmd thread and /dev/apm_bios file, but 
the real apm makes actual bios calls to perform the suspend/standby 
operations, and apm_emulation just uses pm methods like pm_suspend().

- Thus, shouldn't the two files share code from a common third file?

If so, I could take on the task of that refactoring, if I were given an 
idea of where I should put the new file (where does an x86 and 
drivers/char common file go?).

Possible reasons this may not be worth it include:

- The APM code is possibly fragile, and not often used anymore.  If this 
is the case, then perhaps just adding the same locking from the 
emulation file to the true apm file would be a good idea.
- There are subtle differences I am not seeing that requires the code to 
be quite separate.  If so, please let me know.


Thank you for any comments,

-- 
Kevin Winchester