x86: Switch apm to unlocked_kernel

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.

index bf9290e..00e6d13 100644
@@ -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,
 };

Hi Alan,

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

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

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

Hi Alan,

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                    [email protected]
http://www.canb.auug.org.au/~sfr/

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.

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

I think those changes went above and beyond the call.

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.

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

(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