[PATCH] sym8xx_2: Fixes call to wait_for_completion_timeout() with NULL argument

[PATCH] sym8xx_2: Fixes call to wait_for_completion_timeout() with NULL argument

[Krzysztof Helt]

From: Krzysztof Helt <krzysztof.h1@wp.pl>

This patch fixes call to wait_for_completion_timeout()
with NULL argument.

Signed-off-by: Krzysztof Helt <krzysztof.h1@w.pl>

---

diff -urp linux-ref/drivers/scsi/sym53c8xx_2/sym_glue.c linux-new/drivers/scsi/sym53c8xx_2/sym_glue.c
--- linux-ref/drivers/scsi/sym53c8xx_2/sym_glue.c	2007-10-28 11:11:02.000000000 +0100
+++ linux-new/drivers/scsi/sym53c8xx_2/sym_glue.c	2007-10-28 14:25:08.000000000 +0100
@@ -609,8 +609,7 @@ static int sym_eh_handler(int op, char *
 	 */
 #define WAIT_FOR_PCI_RECOVERY	35
 	if (pci_channel_offline(pdev)) {
-		struct completion *io_reset;
-		int finished_reset = 0;
+		struct completion *io_reset = NULL;
 		init_completion(&eh_done);
 		spin_lock_irq(shost->host_lock);
 		/* Make sure we didn't race */
@@ -618,15 +617,12 @@ static int sym_eh_handler(int op, char *
 			if (!sym_data->io_reset)
 				sym_data->io_reset = &eh_done;
 			io_reset = sym_data->io_reset;
-		} else {
-			finished_reset = 1;
 		}
 		spin_unlock_irq(shost->host_lock);
-		if (!finished_reset)
-			finished_reset = wait_for_completion_timeout(io_reset,
-						WAIT_FOR_PCI_RECOVERY*HZ);
-		if (!finished_reset)
-			return SCSI_FAILED;
+		if (io_reset &&
+		    !wait_for_completion_timeout(io_reset,
+						 WAIT_FOR_PCI_RECOVERY * HZ))
+				return SCSI_FAILED;
 	}
 
 	spin_lock_irq(shost->host_lock);


----------------------------------------------------------------------
Walcz z zombie jako kosmiczny marine!
Kliknij >>> http://link.interia.pl/f1cc3

Re: [PATCH] sym8xx_2: Fixes call to wait_for_completion_timeout() with NULL argument

[James Bottomley]

On Mon, 2008-01-07 at 22:56 +0100, Krzysztof Helt wrote:
> From: Krzysztof Helt <krzysztof.h1@wp.pl>
> 
> This patch fixes call to wait_for_completion_timeout()
> with NULL argument.

That doesn't seem to be at all what your patch is doing.  I can't see
any case in the old code where wait_for_completion_timeout() could be
called with a NULL that you fix.  What it seems you are doing is
altering the code to eliminate the finished_reset variable.

James

Re: [PATCH] sym8xx_2: kill compilation warning

[Krzysztof Helt]

On Mon, 07 Jan 2008 16:39:35 -0600
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> 
> On Mon, 2008-01-07 at 22:56 +0100, Krzysztof Helt wrote:
> > From: Krzysztof Helt <krzysztof.h1@wp.pl>
> > 
> > This patch fixes call to wait_for_completion_timeout()
> > with NULL argument.
> 
> That doesn't seem to be at all what your patch is doing.  I can't see
> any case in the old code where wait_for_completion_timeout() could be
> called with a NULL that you fix.  What it seems you are doing is
> altering the code to eliminate the finished_reset variable.
> 

I am sorry for the mess. I put the wrong description of the patch.
Here is the correct one:
---

The purpose of the patch is to kill the compilation warning with gcc-4.1.1 on sparc64:

sym_glue.c: In function sym_eh_handler:
sym_glue.c:612: warning: io_reset may be used uninitialized in this function

This patch also eliminates the finished_reset variable.

Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>

---

diff -urp linux-ref/drivers/scsi/sym53c8xx_2/sym_glue.c linux-new/drivers/scsi/sym53c8xx_2/sym_glue.c
--- linux-ref/drivers/scsi/sym53c8xx_2/sym_glue.c	2007-10-28 11:11:02.000000000 +0100
+++ linux-new/drivers/scsi/sym53c8xx_2/sym_glue.c	2007-10-28 14:25:08.000000000 +0100
@@ -609,8 +609,7 @@ static int sym_eh_handler(int op, char *
 	 */
 #define WAIT_FOR_PCI_RECOVERY	35
 	if (pci_channel_offline(pdev)) {
-		struct completion *io_reset;
-		int finished_reset = 0;
+		struct completion *io_reset = NULL;
 		init_completion(&eh_done);
 		spin_lock_irq(shost->host_lock);
 		/* Make sure we didn't race */
@@ -618,15 +617,12 @@ static int sym_eh_handler(int op, char *
 			if (!sym_data->io_reset)
 				sym_data->io_reset = &eh_done;
 			io_reset = sym_data->io_reset;
-		} else {
-			finished_reset = 1;
 		}
 		spin_unlock_irq(shost->host_lock);
-		if (!finished_reset)
-			finished_reset = wait_for_completion_timeout(io_reset,
-						WAIT_FOR_PCI_RECOVERY*HZ);
-		if (!finished_reset)
-			return SCSI_FAILED;
+		if (io_reset &&
+		    !wait_for_completion_timeout(io_reset,
+						 WAIT_FOR_PCI_RECOVERY * HZ))
+				return SCSI_FAILED;
 	}
 
 	spin_lock_irq(shost->host_lock);




----------------------------------------------------------------------
Nadchodzi wojna miedzygalaktyczna! 
Sprawdz! >>> http://link.interia.pl/f1cc2

Re: [PATCH] sym8xx_2: kill compilation warning

[James Bottomley]

On Tue, 2008-01-08 at 07:40 +0100, Krzysztof Helt wrote:
> On Mon, 07 Jan 2008 16:39:35 -0600
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > 
> > On Mon, 2008-01-07 at 22:56 +0100, Krzysztof Helt wrote:
> > > From: Krzysztof Helt <krzysztof.h1@wp.pl>
> > > 
> > > This patch fixes call to wait_for_completion_timeout()
> > > with NULL argument.
> > 
> > That doesn't seem to be at all what your patch is doing.  I can't see
> > any case in the old code where wait_for_completion_timeout() could be
> > called with a NULL that you fix.  What it seems you are doing is
> > altering the code to eliminate the finished_reset variable.
> > 
> 
> I am sorry for the mess. I put the wrong description of the patch.
> Here is the correct one:
> ---
> 
> The purpose of the patch is to kill the compilation warning with gcc-4.1.1 on sparc64:
> 
> sym_glue.c: In function sym_eh_handler:
> sym_glue.c:612: warning: io_reset may be used uninitialized in this function
> 
> This patch also eliminates the finished_reset variable.

OK, ordinarily we don't change kernel code for what is clearly a
compiler bug (as in other versions of gcc 4.1 don't produce this warning
because they see the tie between the two variables).

However, now that I actually look at this code, it has two clear bugs in
it:

     1. the if (!sym_data->io_reset).  That variable is only ever filled
        by a stack based completion.  If we find it non empty it means
        this code has been entered twice and we have a severe problem,
        so that should just become a BUG_ON(!sym_data->io_reset)
     2. sym_data->io_reset should be set to NULL before the routine is
        exited otherwise the PCI recovery code could end up completing
        what will be a bogus pointer into the stack.

The vaule of sym_data->io_reset looks like it should remain current
across the lock, so there's no need to save it in a variable (the
sym2_io_resume() routine also shouldn't be setting it to NULL, and the
complete_all should be complete---just because we can only have a single
thread waiting for the completion otherwise all hell would break loose).

So, fix these issues (and quietly change the code not to produce the
compile warning on sparc64) and it can go in.

James