* [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful
@ 2008-07-29 9:47 Simon Horman
2008-07-29 12:47 ` Keith Owens
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2008-07-29 9:47 UTC (permalink / raw)
To: linux-ia64, linux-kernel; +Cc: Tony Luck
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful
2008-07-29 9:47 [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful Simon Horman
@ 2008-07-29 12:47 ` Keith Owens
2008-07-29 13:07 ` Simon Horman
0 siblings, 1 reply; 5+ messages in thread
From: Keith Owens @ 2008-07-29 12:47 UTC (permalink / raw)
To: Simon Horman; +Cc: linux-ia64, linux-kernel, Tony Luck
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().
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful
2008-07-29 12:47 ` Keith Owens
@ 2008-07-29 13:07 ` Simon Horman
2008-07-30 9:43 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2008-07-29 13:07 UTC (permalink / raw)
To: Keith Owens; +Cc: linux-ia64, linux-kernel, Tony Luck
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful
2008-07-29 13:07 ` Simon Horman
@ 2008-07-30 9:43 ` Andrew Morton
2008-07-30 16:12 ` Keith Owens
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2008-07-30 9:43 UTC (permalink / raw)
To: Simon Horman; +Cc: Keith Owens, linux-ia64, linux-kernel, Tony Luck
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful
2008-07-30 9:43 ` Andrew Morton
@ 2008-07-30 16:12 ` Keith Owens
0 siblings, 0 replies; 5+ messages in thread
From: Keith Owens @ 2008-07-30 16:12 UTC (permalink / raw)
To: Andrew Morton; +Cc: Simon Horman, linux-ia64, linux-kernel, Tony Luck
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.
*/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-07-30 16:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-07-29 9:47 [patch] IA64: only call up() in salinfo_work_to_do() if down_trylock() was successful Simon Horman
2008-07-29 12:47 ` Keith Owens
2008-07-29 13:07 ` Simon Horman
2008-07-30 9:43 ` Andrew Morton
2008-07-30 16:12 ` Keith Owens
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).