On Thu, 2008-04-10 at 13:26 -0700, akpm@xxxxxxxxxxxxxxxxxxxx wrote: > The patch titled > aacraid: fix unchecked down_interruptible() > has been added to the -mm tree. Its filename is > aacraid-fix-unchecked-down_interruptible.patch > > Before you just go and hit "reply", please: > a) Consider who else should be cc'ed > b) Prefer to cc a suitable mailing list as well > c) Ideally: find the original patch on the mailing list and do a > reply-to-all to that, adding suitable additional cc's > > *** Remember to use Documentation/SubmitChecklist when testing your code *** > > See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find > out what to do about this > > The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/ > > ------------------------------------------------------ > Subject: aacraid: fix unchecked down_interruptible() > From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > > drivers/scsi/aacraid/commsup.c: In function 'aac_fib_send': > drivers/scsi/aacraid/commsup.c:519: warning: ignoring return value of 'down_interruptible', declared with attribute warn_unused_result > > This is a bug. Code *must* check whether or not down_interruptible() > succeeded. This driver has lost track of whether or not the lock was taken. > > (I thought I already fixed this once?) You added the patch once before, it seems to have dropped out of the -mm tree, but it was never sent upstream. > Cc: Alan Cox <alan@xxxxxxxxxxxxxxxxxxx> > Cc: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> I researched this with Adaptec. This is what they say: > We had to use down_interruptible because if the Firmware locked up, > unresponsive or retiscent, and the system was being shut down, it > would never complete the shutdown. The user task is 'in driver' and > can not be shut down. > > The loss of this one fib because a user is interrupting the > management > applications is a small price to pay. This FIB remains in the open > queue and needs to since the Adapter has not completed it... So the point about this is that if you ever hit the interrupt, you lose a FIB, but on the other hand, there's nothing the driver can actually do to recover it, hence it wouldn't behave differently regardless of the return value of down_interruptible(). On the other hand, if you change the down_interruptible() to down() we lock up the system when we hit the wait forever condition. I'm sympathetic, even though I agree we don't want this bad programming example persisting for someone to cut and paste, so I think we have to keep the interruptible, but I'm open to other ways of making the warning go away. > --- > > drivers/scsi/aacraid/commsup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff -puN drivers/scsi/aacraid/commsup.c~aacraid-fix-unchecked-down_interruptible drivers/scsi/aacraid/commsup.c > --- a/drivers/scsi/aacraid/commsup.c~aacraid-fix-unchecked-down_interruptible > +++ a/drivers/scsi/aacraid/commsup.c > @@ -516,7 +516,7 @@ int aac_fib_send(u16 command, struct fib > udelay(5); > } > } else > - (void)down_interruptible(&fibptr->event_wait); > + down(&fibptr->event_wait); > spin_lock_irqsave(&fibptr->event_lock, flags); > if (fibptr->done == 0) { > fibptr->done = 2; /* Tell interrupt we aborted */ James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
