[patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful

[patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful

[Simon Horman]

Aesthetic issues aside is it safe to call up() if down_trylock() failed?

arch/ia64/kernel/salinfo.c: In function `salinfo_work_to_do':
arch/ia64/kernel/salinfo.c:195: warning: ignoring return value of `down_trylock'

Signed-off-by: Simon Horman <horms@verge.net.au>

Index: linux-2.6/arch/ia64/kernel/salinfo.c
===================================================================
--- linux-2.6.orig/arch/ia64/kernel/salinfo.c	2008-07-29 19:06:33.000000000 +1000
+++ linux-2.6/arch/ia64/kernel/salinfo.c	2008-07-29 19:40:02.000000000 +1000
@@ -192,8 +192,8 @@ struct salinfo_platform_oemdata_parms {
 static void
 salinfo_work_to_do(struct salinfo_data *data)
 {
-	down_trylock(&data->mutex);
-	up(&data->mutex);
+	if (down_trylock(&data->mutex) == 0)
+		up(&data->mutex);
 }
 
 static void

Re: [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful

[Keith Owens]

Simon Horman (on Tue, 29 Jul 2008 19:47:20 +1000) wrote:
>Aesthetic issues aside is it safe to call up() if down_trylock() failed?
>
>arch/ia64/kernel/salinfo.c: In function `salinfo_work_to_do':
>arch/ia64/kernel/salinfo.c:195: warning: ignoring return value of `down_trylock'
>
>Signed-off-by: Simon Horman <horms@verge.net.au>
>
>Index: linux-2.6/arch/ia64/kernel/salinfo.c
>===================================================================
>--- linux-2.6.orig/arch/ia64/kernel/salinfo.c	2008-07-29 19:06:33.000000000 +1000
>+++ linux-2.6/arch/ia64/kernel/salinfo.c	2008-07-29 19:40:02.000000000 +1000
>@@ -192,8 +192,8 @@ struct salinfo_platform_oemdata_parms {
> static void
> salinfo_work_to_do(struct salinfo_data *data)
> {
>-	down_trylock(&data->mutex);
>-	up(&data->mutex);
>+	if (down_trylock(&data->mutex) == 0)
>+		up(&data->mutex);
> }
> 
> static void

NAK.  The whole point of this function is to set the mutex to the up
state, irrespective of whether it was already down or not.  Tracking
the state of data->mutex in all the possible contexts is just too
fragile, especially since it can be modified from NMI context.
salinfo_work_to_do() ensures that the mtuex ends in the up state.

To remove the warning, just stick '(void)' in front of down_trylock().

Re: [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful

[Simon Horman]

On Tue, Jul 29, 2008 at 10:47:09PM +1000, Keith Owens wrote:
> Simon Horman (on Tue, 29 Jul 2008 19:47:20 +1000) wrote:
> >Aesthetic issues aside is it safe to call up() if down_trylock() failed?
> >
> >arch/ia64/kernel/salinfo.c: In function `salinfo_work_to_do':
> >arch/ia64/kernel/salinfo.c:195: warning: ignoring return value of `down_trylock'
> >
> >Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> >Index: linux-2.6/arch/ia64/kernel/salinfo.c
> >===================================================================
> >--- linux-2.6.orig/arch/ia64/kernel/salinfo.c	2008-07-29 19:06:33.000000000 +1000
> >+++ linux-2.6/arch/ia64/kernel/salinfo.c	2008-07-29 19:40:02.000000000 +1000
> >@@ -192,8 +192,8 @@ struct salinfo_platform_oemdata_parms {
> > static void
> > salinfo_work_to_do(struct salinfo_data *data)
> > {
> >-	down_trylock(&data->mutex);
> >-	up(&data->mutex);
> >+	if (down_trylock(&data->mutex) == 0)
> >+		up(&data->mutex);
> > }
> > 
> > static void
> 
> NAK.  The whole point of this function is to set the mutex to the up
> state, irrespective of whether it was already down or not.  Tracking
> the state of data->mutex in all the possible contexts is just too
> fragile, especially since it can be modified from NMI context.
> salinfo_work_to_do() ensures that the mtuex ends in the up state.
> 
> To remove the warning, just stick '(void)' in front of down_trylock().

Thanks, will do.

-- 
Horms

Re: [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful

[Andrew Morton]

On Tue, 29 Jul 2008 23:07:45 +1000 Simon Horman <horms@verge.net.au> wrote:

> On Tue, Jul 29, 2008 at 10:47:09PM +1000, Keith Owens wrote:
> > Simon Horman (on Tue, 29 Jul 2008 19:47:20 +1000) wrote:
> > >Aesthetic issues aside is it safe to call up() if down_trylock() failed?
> > >
> > >arch/ia64/kernel/salinfo.c: In function `salinfo_work_to_do':
> > >arch/ia64/kernel/salinfo.c:195: warning: ignoring return value of `down_trylock'
> > >
> > >Signed-off-by: Simon Horman <horms@verge.net.au>
> > >
> > >Index: linux-2.6/arch/ia64/kernel/salinfo.c
> > >===================================================================
> > >--- linux-2.6.orig/arch/ia64/kernel/salinfo.c	2008-07-29 19:06:33.000000000 +1000
> > >+++ linux-2.6/arch/ia64/kernel/salinfo.c	2008-07-29 19:40:02.000000000 +1000
> > >@@ -192,8 +192,8 @@ struct salinfo_platform_oemdata_parms {
> > > static void
> > > salinfo_work_to_do(struct salinfo_data *data)
> > > {
> > >-	down_trylock(&data->mutex);
> > >-	up(&data->mutex);
> > >+	if (down_trylock(&data->mutex) == 0)
> > >+		up(&data->mutex);
> > > }
> > > 
> > > static void
> > 
> > NAK.  The whole point of this function is to set the mutex to the up
> > state, irrespective of whether it was already down or not.  Tracking
> > the state of data->mutex in all the possible contexts is just too
> > fragile, especially since it can be modified from NMI context.
> > salinfo_work_to_do() ensures that the mtuex ends in the up state.

boggle.  I daren't look.

> > To remove the warning, just stick '(void)' in front of down_trylock().
> 
> Thanks, will do.
> 

For gawd's sake add a comment there too.

Re: [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful

[Keith Owens]

Andrew Morton (on Wed, 30 Jul 2008 02:43:29 -0700) wrote:
>On Tue, 29 Jul 2008 23:07:45 +1000 Simon Horman <horms@verge.net.au> wrote:
>
>> On Tue, Jul 29, 2008 at 10:47:09PM +1000, Keith Owens wrote:
>> > Simon Horman (on Tue, 29 Jul 2008 19:47:20 +1000) wrote:
>> > >Aesthetic issues aside is it safe to call up() if down_trylock() failed?
>> > >
>> > >arch/ia64/kernel/salinfo.c: In function `salinfo_work_to_do':
>> > >arch/ia64/kernel/salinfo.c:195: warning: ignoring return value of `down_trylock'
>> > >
>> > >Signed-off-by: Simon Horman <horms@verge.net.au>
>> > >
>> > >Index: linux-2.6/arch/ia64/kernel/salinfo.c
>> > >===================================================================
>> > >--- linux-2.6.orig/arch/ia64/kernel/salinfo.c	2008-07-29 19:06:33.000000000 +1000
>> > >+++ linux-2.6/arch/ia64/kernel/salinfo.c	2008-07-29 19:40:02.000000000 +1000
>> > >@@ -192,8 +192,8 @@ struct salinfo_platform_oemdata_parms {
>> > > static void
>> > > salinfo_work_to_do(struct salinfo_data *data)
>> > > {
>> > >-	down_trylock(&data->mutex);
>> > >-	up(&data->mutex);
>> > >+	if (down_trylock(&data->mutex) == 0)
>> > >+		up(&data->mutex);
>> > > }
>> > > 
>> > > static void
>> > 
>> > NAK.  The whole point of this function is to set the mutex to the up
>> > state, irrespective of whether it was already down or not.  Tracking
>> > the state of data->mutex in all the possible contexts is just too
>> > fragile, especially since it can be modified from NMI context.
>> > salinfo_work_to_do() ensures that the mtuex ends in the up state.
>
>boggle.  I daren't look.
>
>> > To remove the warning, just stick '(void)' in front of down_trylock().
>> 
>> Thanks, will do.
>> 
>
>For gawd's sake add a comment there too.

You mean like the comment that is already in there?

/* Kick the mutex that tells user space that there is work to do.  Instead of
 * trying to track the state of the mutex across multiple cpus, in user
 * context, interrupt context, non-maskable interrupt context and hotplug cpu,
 * it is far easier just to grab the mutex if it is free then release it.
 *
 * This routine must be called with data_saved_lock held, to make the down/up
 * operation atomic.
 */