Re: [PATCH ] scsi-misc-2.6: File System going into read-only mode

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

Penchala Narasimha Reddy Chilakala, TLS-Chennai wrote:
> Hi Stefan,
> 
> Thanks a lot for your suggestion and feedback.
> 
> Aacraid driver has been using down_interruptible () and up () for
> quite some time. We will take your suggestion into consideration
> and keep in our mind. But it is quite difficult to implement your
> suggestion at this point of time as we need to modify the code in
> lot places and we need to test a lot.

dpcsup.c:	2x up(&fib->event_wait)
commsup.c:	allocation and initialization of the semaphore
		1x down_trylock(&fibptr->event_wait) +
		1x down_interruptible(&fibptr->event_wait) + up()
		1x up(&fib->event_wait)

So there are just two places which conditionally wait for completion (or
just one place but with a differentiation between sync and async mode),
and three places which signal "done".

You are right that this would be a non-trivial change and will require
respective testing.  However, (1.) only the place which waits for
completion is the non-trivial part (because it's currently coded in a
rather obfuscated way), (2.) I expect that the result would be cleaner
code which is (3.) more obvious to be correct and (4.) can also be
checked for correctness by the lockdep facility --- this is not possible
with legacy semaphores.

Anyway; it's up to those who use or maintain this driver.  I for one
converted another subsystem away from two or three different misused
semaphores to more appropriate APIs and it wasn't too hard; but I can't
do this for aacraid since I don't have hardware to test.

(In my rather irrelevant opinion, whatever you decide for now, just do
_not_ do this here:

>>> we can replace the above code with either
>>>
>>>             } else if (down_interruptible (&fibptr->event_wait));
>>>
>>>                         Or
>>>
>>>             } else {
>>>                  if (down_interruptible (&fibptr->event_wait))
>>>                         ;
>>>             }

because it can really hurt in the long run if sensible warnings are hidden.)
-- 
Stefan Richter
-=====-==--= =-== ---==
http://arcgraph.de/sr/