Re: modification of tp-smapi module for -rt

[Robin Gareus <robin@gareus.org>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Pedro Ribeiro wrote:
> Hi all,
> 
> I use an out of tree module called tp-smapi, which is basically an
> advanced battery control module for thinkpads.
> This module uses a couple of mutexes which I had to modify for it to
> build with the latest .31.12-rt20.
> 
> However, I'm clueless about this, so basically what I did was look at
> the rt patch for similar sections and change it in the tp-smapi
> source.
> 
> Can anyone please have a look at what I've done and tell me if its
> correct or not? The module appears to work correctly but you never
> know.
> 
> I replaced a
> static DECLARE_MUTEX(thinkpad_ec_mutex);
> with a
> static DEFINE_SEMAPHORE(thinkpad_ec_mutex);
> 
> This is called in
> -----------------------------------------------------------------------------
> /**
>  * thinkpad_ec_lock - get lock on the ThinkPad EC
>  *
>  * Get exclusive lock for accesing the ThinkPad embedded controller LPC3
>  * interface. Returns 0 iff lock acquired.
>  */
> int thinkpad_ec_lock(void)
> {
> 	int ret;
> 	ret = down_interruptible(&thinkpad_ec_mutex);
> 	return ret;
> }
> EXPORT_SYMBOL_GPL(thinkpad_ec_lock);
> 
> /**
>  * thinkpad_ec_try_lock - try getting lock on the ThinkPad EC
>  *
>  * Try getting an exclusive lock for accesing the ThinkPad embedded
>  * controller LPC3. Returns immediately if lock is not available; neither
>  * blocks nor sleeps. Returns 0 iff lock acquired .
>  */
> int thinkpad_ec_try_lock(void)
> {
> 	return down_trylock(&thinkpad_ec_mutex);
> }
> EXPORT_SYMBOL_GPL(thinkpad_ec_try_lock);
> 
> /**
>  * thinkpad_ec_unlock - release lock on ThinkPad EC
>  *
>  * Release a previously acquired exclusive lock on the ThinkPad ebmedded
>  * controller LPC3 interface.
>  */
> void thinkpad_ec_unlock(void)
> {
> 	up(&thinkpad_ec_mutex);
> }
> EXPORT_SYMBOL_GPL(thinkpad_ec_unlock);
> ------------------------------------------------------------------
> 
> Then I replaced a
> static DECLARE_MUTEX(smapi_mutex);
> with a
> static DEFINE_MUTEX(smapi_mutex);
> 
> which is called in various other code sections with
> down(&smapi_mutex);
> and
> up(&smapi_mutex);
> 
> 
> Please let me know if this is enough, or if you need to look into the code!

Looks good to me.

> Your help is much appreciated.

I basically did the same a few months ago:
http://rg42.org/blog/thinkpad_smapi_and_realtime_linux

I chose to change
- -static DECLARE_MUTEX(thinkpad_ec_mutex);
+static struct mutex thinkpad_ec_mutex;

and replaced down_interruptible(), down_trylock(), up() and down() with
mutex_lock_killable(), mutex_trylock(), mutex_unlock() and mutex_lock()
respectively; plus adding a mutex_init() call.

Here's a direct link to the diff:
http://rg42.org/_media/blog/tpsmapi/tp_smapi-rt.diff

Your solution is less intrusive but also less compatible. The
mutex_..() functions work with both -rt and vanilla kernels.

Cheers!
robin
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkt59hEACgkQeVUk8U+VK0IxkQCeM4TIhrypHz4yED5wWRnN5J/G
AHcAn0LqL/vnh82STtrPDSxLYll5MIia
=KJJY
-----END PGP SIGNATURE-----