Re: [RFC] x86: Switch apm to unlocked_kernel

[Kevin Winchester <kjwinchester@gmail.com>]

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