[patch] IA64: suppress return value of down_trylock() in salinfo_work_to_do()

[patch] IA64: suppress return value of down_trylock() in salinfo_work_to_do()

[Simon Horman]

salinfo_work_to_do() intentionally ignores the return value of
down_trylock() and calls up() regardless of if the lock
was taken or not.

This patch suppresses the warning generated by ignoring
this return value - down_trylock() is annotated with __must_check.

(void)down_trylock() was not sufficient.

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

# ia64-unknown-linux-gnu-gcc --version
ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

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-08-03 09:52:08.000000000 +1000
+++ linux-2.6/arch/ia64/kernel/salinfo.c	2008-08-03 10:03:36.000000000 +1000
@@ -192,7 +192,7 @@ struct salinfo_platform_oemdata_parms {
 static void
 salinfo_work_to_do(struct salinfo_data *data)
 {
-	down_trylock(&data->mutex);
+	if (down_trylock(&data->mutex)) { ; }; /* Ignore return value */
 	up(&data->mutex);
 }
 

Re: [patch] IA64: suppress return value of down_trylock() in salinfo_work_to_do()

[Matthew Wilcox]

On Sun, Aug 03, 2008 at 10:06:58AM +1000, Simon Horman wrote:
> salinfo_work_to_do() intentionally ignores the return value of
> down_trylock() and calls up() regardless of if the lock
> was taken or not.
> 
> This patch suppresses the warning generated by ignoring
> this return value - down_trylock() is annotated with __must_check.

I can't say that I think this is a good idea.  Has anyone looked at what
it would take to actually track this?  For example, could we ever have
the situation where:

task A acquires sem

task B tries to acquire the sem, fails
task B releases the sem that it didn't acquire

task A releases the sem, falls down, goes boom?

(of course, this is a semaphores, not a mutex, so it'll now be a
counting semaphore with n=2, not protecting a damn thing).

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [patch] IA64: suppress return value of down_trylock() in salinfo_work_to_do()

[Matthew Wilcox]

On Sat, Aug 02, 2008 at 07:32:00PM -0600, Matthew Wilcox wrote:
> I can't say that I think this is a good idea.  Has anyone looked at what
> it would take to actually track this?  For example, could we ever have
> the situation where:

Looking at this a bit more deeply, it seems to be a simple abuse of a
semaphore.

It's used both to exclude other tasks in salinfo_event_read() and as a
wake queue from interrupt context.  I think what we want is something
more like this:

	spin_lock(&data_saved_lock);
	if (!worktodo) {
		if (O_NONBLOCK)
			spin_unlock()
			return;
		else
			add to wait queue
			spin_unlock()
			schedule()
			spin_lock()
	}

Then we can just use wake_up() from interrupt context.

Thoughts?  I don't have any idea how to generate salinfo events, so I
wouldn't be able to test.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [patch] IA64: suppress return value of down_trylock() in salinfo_work_to_do()

[Keith Owens]

Matthew Wilcox (on Sat, 2 Aug 2008 19:32:00 -0600) wrote:
>On Sun, Aug 03, 2008 at 10:06:58AM +1000, Simon Horman wrote:
>> salinfo_work_to_do() intentionally ignores the return value of
>> down_trylock() and calls up() regardless of if the lock
>> was taken or not.
>> 
>> This patch suppresses the warning generated by ignoring
>> this return value - down_trylock() is annotated with __must_check.
>
>I can't say that I think this is a good idea.  Has anyone looked at what
>it would take to actually track this?  For example, could we ever have
>the situation where:
>
>task A acquires sem
>
>task B tries to acquire the sem, fails
>task B releases the sem that it didn't acquire
>
>task A releases the sem, falls down, goes boom?

Cannot happen.  See the comment above the function:

  This routine must be called with data_saved_lock held, to make the down/up
  operation atomic

Re: [patch] IA64: suppress return value of down_trylock() in salinfo_work_to_do()

[Matthew Wilcox]

On Sun, Aug 03, 2008 at 12:08:08PM +1000, Keith Owens wrote:
> >task A acquires sem
> >
> >task B tries to acquire the sem, fails
> >task B releases the sem that it didn't acquire
> >
> >task A releases the sem, falls down, goes boom?
> 
> Cannot happen.  See the comment above the function:
> 
>   This routine must be called with data_saved_lock held, to make the down/up
>   operation atomic

... that requres one to know that salinfo_work_to_do() is the only place
which calls up() (and it always calls down_trylock first).  This is a
really creative misuse of semaphores.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."