All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap
@ 2011-05-31 14:16 ` Julia Lawall
  0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-05-31 14:16 UTC (permalink / raw)
  To: Brian King
  Cc: kernel-janitors, James E.J. Bottomley, linux-scsi, linux-kernel

From: Julia Lawall <julia@diku.dk>

The out_msi_disable label should be before cleanup_nomem to additionally
benefit from the call to iounmap.  Subsequent gotos are adjusted to go to
out_msi_disable instead of cleanup_nomem, which now follows it.  This is
safe because pci_disable_msi does nothing if pci_enable_msi was not called.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r@
expression e1,e2;
statement S;
@@

e1 = pci_ioremap_bar(...);
... when != e1 = e2
    when != iounmap(e1)
    when any
(
 if (<+...e1...+>) S
|
 if(...) { ... return 0; }
|
 if (...) { ... when != iounmap(e1)
                when != if (...) { ... iounmap(e1) ... }
* return ...;
 } else S
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/scsi/ipr.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 888086c..8d63630 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -8778,14 +8778,14 @@ static int __devinit ipr_probe_ioa(struct pci_dev *pdev,
 	if (rc != PCIBIOS_SUCCESSFUL) {
 		dev_err(&pdev->dev, "Failed to save PCI config space\n");
 		rc = -EIO;
-		goto cleanup_nomem;
+		goto out_msi_disable;
 	}
 
 	if ((rc = ipr_save_pcix_cmd_reg(ioa_cfg)))
-		goto cleanup_nomem;
+		goto out_msi_disable;
 
 	if ((rc = ipr_set_pcix_cmd_reg(ioa_cfg)))
-		goto cleanup_nomem;
+		goto out_msi_disable;
 
 	if (ioa_cfg->sis64)
 		ioa_cfg->cfg_table_size = (sizeof(struct ipr_config_table_hdr64)
@@ -8800,7 +8800,7 @@ static int __devinit ipr_probe_ioa(struct pci_dev *pdev,
 	if (rc < 0) {
 		dev_err(&pdev->dev,
 			"Couldn't allocate enough memory for device driver!\n");
-		goto cleanup_nomem;
+		goto out_msi_disable;
 	}
 
 	/*
@@ -8845,10 +8845,10 @@ out:
 
 cleanup_nolog:
 	ipr_free_mem(ioa_cfg);
-cleanup_nomem:
-	iounmap(ipr_regs);
 out_msi_disable:
 	pci_disable_msi(pdev);
+cleanup_nomem:
+	iounmap(ipr_regs);
 out_release_regions:
 	pci_release_regions(pdev);
 out_scsi_host_put:


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap
@ 2011-05-31 14:16 ` Julia Lawall
  0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-05-31 14:16 UTC (permalink / raw)
  To: Brian King
  Cc: kernel-janitors, James E.J. Bottomley, linux-scsi, linux-kernel

From: Julia Lawall <julia@diku.dk>

The out_msi_disable label should be before cleanup_nomem to additionally
benefit from the call to iounmap.  Subsequent gotos are adjusted to go to
out_msi_disable instead of cleanup_nomem, which now follows it.  This is
safe because pci_disable_msi does nothing if pci_enable_msi was not called.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r@
expression e1,e2;
statement S;
@@

e1 = pci_ioremap_bar(...);
... when != e1 = e2
    when != iounmap(e1)
    when any
(
 if (<+...e1...+>) S
|
 if(...) { ... return 0; }
|
 if (...) { ... when != iounmap(e1)
                when != if (...) { ... iounmap(e1) ... }
* return ...;
 } else S
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/scsi/ipr.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 888086c..8d63630 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -8778,14 +8778,14 @@ static int __devinit ipr_probe_ioa(struct pci_dev *pdev,
 	if (rc != PCIBIOS_SUCCESSFUL) {
 		dev_err(&pdev->dev, "Failed to save PCI config space\n");
 		rc = -EIO;
-		goto cleanup_nomem;
+		goto out_msi_disable;
 	}
 
 	if ((rc = ipr_save_pcix_cmd_reg(ioa_cfg)))
-		goto cleanup_nomem;
+		goto out_msi_disable;
 
 	if ((rc = ipr_set_pcix_cmd_reg(ioa_cfg)))
-		goto cleanup_nomem;
+		goto out_msi_disable;
 
 	if (ioa_cfg->sis64)
 		ioa_cfg->cfg_table_size = (sizeof(struct ipr_config_table_hdr64)
@@ -8800,7 +8800,7 @@ static int __devinit ipr_probe_ioa(struct pci_dev *pdev,
 	if (rc < 0) {
 		dev_err(&pdev->dev,
 			"Couldn't allocate enough memory for device driver!\n");
-		goto cleanup_nomem;
+		goto out_msi_disable;
 	}
 
 	/*
@@ -8845,10 +8845,10 @@ out:
 
 cleanup_nolog:
 	ipr_free_mem(ioa_cfg);
-cleanup_nomem:
-	iounmap(ipr_regs);
 out_msi_disable:
 	pci_disable_msi(pdev);
+cleanup_nomem:
+	iounmap(ipr_regs);
 out_release_regions:
 	pci_release_regions(pdev);
 out_scsi_host_put:


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap
  2011-05-31 14:16 ` Julia Lawall
@ 2011-06-01 15:02   ` Brian King
  -1 siblings, 0 replies; 12+ messages in thread
From: Brian King @ 2011-06-01 15:02 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Brian King, kernel-janitors, James E.J. Bottomley, linux-scsi,
	linux-kernel

Acked-by: Brian King <brking@linux.vnet.ibm.com>

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include
@ 2011-06-01 15:02   ` Brian King
  0 siblings, 0 replies; 12+ messages in thread
From: Brian King @ 2011-06-01 15:02 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Brian King, kernel-janitors, James E.J. Bottomley, linux-scsi,
	linux-kernel

Acked-by: Brian King <brking@linux.vnet.ibm.com>

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap
  2011-05-31 14:16 ` Julia Lawall
@ 2011-06-09 15:51   ` Wayne Boyer
  -1 siblings, 0 replies; 12+ messages in thread
From: Wayne Boyer @ 2011-06-09 15:51 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Brian King, kernel-janitors, James E.J. Bottomley, linux-scsi,
	linux-kernel

On 05/31/2011 07:16 AM, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> The out_msi_disable label should be before cleanup_nomem to additionally
> benefit from the call to iounmap.

Yes, this is a problem.  I propose the following patch instead.

---

In the case where ipr_test_msi returns an error that is not -EOPNOTSUPP, the
execution jumps to the out_msi_disable label.  This misses the call to iounmap
for ipr_regs which was initialized earlier.

The fix is to do the call to pci_disable_msi when the error case is detected
and then goto the cleanup_nomem label if the error is not -EOPNOTSUPP.

Signed-off-by: Wayne Boyer <wayneb@linux.vnet.ibm.com>
---

 drivers/scsi/ipr.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Index: b/drivers/scsi/ipr.c
===================================================================
--- a/drivers/scsi/ipr.c        2011-06-09 08:14:44.927740117 -0700
+++ b/drivers/scsi/ipr.c        2011-06-09 08:50:10.105630466 -0700
@@ -8763,11 +8763,11 @@ static int __devinit ipr_probe_ioa(struc
        /* Enable MSI style interrupts if they are supported. */
        if (ioa_cfg->ipr_chip->intr_type == IPR_USE_MSI && !pci_enable_msi(pdev)) {
                rc = ipr_test_msi(ioa_cfg, pdev);
-               if (rc == -EOPNOTSUPP)
+               if (rc) {
                        pci_disable_msi(pdev);
-               else if (rc)
-                       goto out_msi_disable;
-               else
+                       if (rc != -EOPNOTSUPP)
+                               goto cleanup_nomem;
+               } else
                        dev_info(&pdev->dev, "MSI enabled with IRQ: %d\n", pdev->irq);
        } else if (ipr_debug)
                dev_info(&pdev->dev, "Cannot enable MSI.\n");
@@ -8847,8 +8847,6 @@ cleanup_nolog:
        ipr_free_mem(ioa_cfg);
 cleanup_nomem:
        iounmap(ipr_regs);
-out_msi_disable:
-       pci_disable_msi(pdev);
 out_release_regions:
        pci_release_regions(pdev);
 out_scsi_host_put:


-- 
Wayne Boyer
IBM - Beaverton, Oregon
LTC S/W Development
(503) 578-5236, T/L 775-5236

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include
@ 2011-06-09 15:51   ` Wayne Boyer
  0 siblings, 0 replies; 12+ messages in thread
From: Wayne Boyer @ 2011-06-09 15:51 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Brian King, kernel-janitors, James E.J. Bottomley, linux-scsi,
	linux-kernel

On 05/31/2011 07:16 AM, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> The out_msi_disable label should be before cleanup_nomem to additionally
> benefit from the call to iounmap.

Yes, this is a problem.  I propose the following patch instead.

---

In the case where ipr_test_msi returns an error that is not -EOPNOTSUPP, the
execution jumps to the out_msi_disable label.  This misses the call to iounmap
for ipr_regs which was initialized earlier.

The fix is to do the call to pci_disable_msi when the error case is detected
and then goto the cleanup_nomem label if the error is not -EOPNOTSUPP.

Signed-off-by: Wayne Boyer <wayneb@linux.vnet.ibm.com>
---

 drivers/scsi/ipr.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Index: b/drivers/scsi/ipr.c
=================================--- a/drivers/scsi/ipr.c        2011-06-09 08:14:44.927740117 -0700
+++ b/drivers/scsi/ipr.c        2011-06-09 08:50:10.105630466 -0700
@@ -8763,11 +8763,11 @@ static int __devinit ipr_probe_ioa(struc
        /* Enable MSI style interrupts if they are supported. */
        if (ioa_cfg->ipr_chip->intr_type = IPR_USE_MSI && !pci_enable_msi(pdev)) {
                rc = ipr_test_msi(ioa_cfg, pdev);
-               if (rc = -EOPNOTSUPP)
+               if (rc) {
                        pci_disable_msi(pdev);
-               else if (rc)
-                       goto out_msi_disable;
-               else
+                       if (rc != -EOPNOTSUPP)
+                               goto cleanup_nomem;
+               } else
                        dev_info(&pdev->dev, "MSI enabled with IRQ: %d\n", pdev->irq);
        } else if (ipr_debug)
                dev_info(&pdev->dev, "Cannot enable MSI.\n");
@@ -8847,8 +8847,6 @@ cleanup_nolog:
        ipr_free_mem(ioa_cfg);
 cleanup_nomem:
        iounmap(ipr_regs);
-out_msi_disable:
-       pci_disable_msi(pdev);
 out_release_regions:
        pci_release_regions(pdev);
 out_scsi_host_put:


-- 
Wayne Boyer
IBM - Beaverton, Oregon
LTC S/W Development
(503) 578-5236, T/L 775-5236

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap
  2011-06-09 15:51   ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include Wayne Boyer
@ 2011-06-14 16:09     ` Brian King
  -1 siblings, 0 replies; 12+ messages in thread
From: Brian King @ 2011-06-14 16:09 UTC (permalink / raw)
  To: Wayne Boyer
  Cc: Julia Lawall, Brian King, kernel-janitors, James E.J. Bottomley,
	linux-scsi, linux-kernel

On 06/09/2011 10:51 AM, Wayne Boyer wrote:
> On 05/31/2011 07:16 AM, Julia Lawall wrote:
>> From: Julia Lawall <julia@diku.dk>
>>
>> The out_msi_disable label should be before cleanup_nomem to additionally
>> benefit from the call to iounmap.
> 
> Yes, this is a problem.  I propose the following patch instead.

By removing the out_msi_disable label, if you fail initialization later
on and goto cleanup_nomem, you will end up leaving MSI enabled when you exit
with this patch.

-Brian



> 
> ---
> 
> In the case where ipr_test_msi returns an error that is not -EOPNOTSUPP, the
> execution jumps to the out_msi_disable label.  This misses the call to iounmap
> for ipr_regs which was initialized earlier.
> 
> The fix is to do the call to pci_disable_msi when the error case is detected
> and then goto the cleanup_nomem label if the error is not -EOPNOTSUPP.
> 
> Signed-off-by: Wayne Boyer <wayneb@linux.vnet.ibm.com>
> ---
> 
>  drivers/scsi/ipr.c |   10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> Index: b/drivers/scsi/ipr.c
> ===================================================================
> --- a/drivers/scsi/ipr.c        2011-06-09 08:14:44.927740117 -0700
> +++ b/drivers/scsi/ipr.c        2011-06-09 08:50:10.105630466 -0700
> @@ -8763,11 +8763,11 @@ static int __devinit ipr_probe_ioa(struc
>         /* Enable MSI style interrupts if they are supported. */
>         if (ioa_cfg->ipr_chip->intr_type == IPR_USE_MSI && !pci_enable_msi(pdev)) {
>                 rc = ipr_test_msi(ioa_cfg, pdev);
> -               if (rc == -EOPNOTSUPP)
> +               if (rc) {
>                         pci_disable_msi(pdev);
> -               else if (rc)
> -                       goto out_msi_disable;
> -               else
> +                       if (rc != -EOPNOTSUPP)
> +                               goto cleanup_nomem;
> +               } else
>                         dev_info(&pdev->dev, "MSI enabled with IRQ: %d\n", pdev->irq);
>         } else if (ipr_debug)
>                 dev_info(&pdev->dev, "Cannot enable MSI.\n");
> @@ -8847,8 +8847,6 @@ cleanup_nolog:
>         ipr_free_mem(ioa_cfg);
>  cleanup_nomem:
>         iounmap(ipr_regs);
> -out_msi_disable:
> -       pci_disable_msi(pdev);
>  out_release_regions:
>         pci_release_regions(pdev);
>  out_scsi_host_put:
> 
> 


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include
@ 2011-06-14 16:09     ` Brian King
  0 siblings, 0 replies; 12+ messages in thread
From: Brian King @ 2011-06-14 16:09 UTC (permalink / raw)
  To: Wayne Boyer
  Cc: Julia Lawall, Brian King, kernel-janitors, James E.J. Bottomley,
	linux-scsi, linux-kernel

On 06/09/2011 10:51 AM, Wayne Boyer wrote:
> On 05/31/2011 07:16 AM, Julia Lawall wrote:
>> From: Julia Lawall <julia@diku.dk>
>>
>> The out_msi_disable label should be before cleanup_nomem to additionally
>> benefit from the call to iounmap.
> 
> Yes, this is a problem.  I propose the following patch instead.

By removing the out_msi_disable label, if you fail initialization later
on and goto cleanup_nomem, you will end up leaving MSI enabled when you exit
with this patch.

-Brian



> 
> ---
> 
> In the case where ipr_test_msi returns an error that is not -EOPNOTSUPP, the
> execution jumps to the out_msi_disable label.  This misses the call to iounmap
> for ipr_regs which was initialized earlier.
> 
> The fix is to do the call to pci_disable_msi when the error case is detected
> and then goto the cleanup_nomem label if the error is not -EOPNOTSUPP.
> 
> Signed-off-by: Wayne Boyer <wayneb@linux.vnet.ibm.com>
> ---
> 
>  drivers/scsi/ipr.c |   10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> Index: b/drivers/scsi/ipr.c
> =================================> --- a/drivers/scsi/ipr.c        2011-06-09 08:14:44.927740117 -0700
> +++ b/drivers/scsi/ipr.c        2011-06-09 08:50:10.105630466 -0700
> @@ -8763,11 +8763,11 @@ static int __devinit ipr_probe_ioa(struc
>         /* Enable MSI style interrupts if they are supported. */
>         if (ioa_cfg->ipr_chip->intr_type = IPR_USE_MSI && !pci_enable_msi(pdev)) {
>                 rc = ipr_test_msi(ioa_cfg, pdev);
> -               if (rc = -EOPNOTSUPP)
> +               if (rc) {
>                         pci_disable_msi(pdev);
> -               else if (rc)
> -                       goto out_msi_disable;
> -               else
> +                       if (rc != -EOPNOTSUPP)
> +                               goto cleanup_nomem;
> +               } else
>                         dev_info(&pdev->dev, "MSI enabled with IRQ: %d\n", pdev->irq);
>         } else if (ipr_debug)
>                 dev_info(&pdev->dev, "Cannot enable MSI.\n");
> @@ -8847,8 +8847,6 @@ cleanup_nolog:
>         ipr_free_mem(ioa_cfg);
>  cleanup_nomem:
>         iounmap(ipr_regs);
> -out_msi_disable:
> -       pci_disable_msi(pdev);
>  out_release_regions:
>         pci_release_regions(pdev);
>  out_scsi_host_put:
> 
> 


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap
  2011-06-14 16:09     ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include Brian King
@ 2011-06-15  8:50       ` Julia Lawall
  -1 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-06-15  8:50 UTC (permalink / raw)
  To: Brian King
  Cc: Wayne Boyer, Brian King, kernel-janitors, James E.J. Bottomley,
	linux-scsi, linux-kernel

On Tue, 14 Jun 2011, Brian King wrote:

> On 06/09/2011 10:51 AM, Wayne Boyer wrote:
> > On 05/31/2011 07:16 AM, Julia Lawall wrote:
> >> From: Julia Lawall <julia@diku.dk>
> >>
> >> The out_msi_disable label should be before cleanup_nomem to additionally
> >> benefit from the call to iounmap.
> > 
> > Yes, this is a problem.  I propose the following patch instead.
> 
> By removing the out_msi_disable label, if you fail initialization later
> on and goto cleanup_nomem, you will end up leaving MSI enabled when you exit
> with this patch.

I agree.

julia

> -Brian
> 
> 
> 
> > 
> > ---
> > 
> > In the case where ipr_test_msi returns an error that is not -EOPNOTSUPP, the
> > execution jumps to the out_msi_disable label.  This misses the call to iounmap
> > for ipr_regs which was initialized earlier.
> > 
> > The fix is to do the call to pci_disable_msi when the error case is detected
> > and then goto the cleanup_nomem label if the error is not -EOPNOTSUPP.
> > 
> > Signed-off-by: Wayne Boyer <wayneb@linux.vnet.ibm.com>
> > ---
> > 
> >  drivers/scsi/ipr.c |   10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > Index: b/drivers/scsi/ipr.c
> > ===================================================================
> > --- a/drivers/scsi/ipr.c        2011-06-09 08:14:44.927740117 -0700
> > +++ b/drivers/scsi/ipr.c        2011-06-09 08:50:10.105630466 -0700
> > @@ -8763,11 +8763,11 @@ static int __devinit ipr_probe_ioa(struc
> >         /* Enable MSI style interrupts if they are supported. */
> >         if (ioa_cfg->ipr_chip->intr_type == IPR_USE_MSI && !pci_enable_msi(pdev)) {
> >                 rc = ipr_test_msi(ioa_cfg, pdev);
> > -               if (rc == -EOPNOTSUPP)
> > +               if (rc) {
> >                         pci_disable_msi(pdev);
> > -               else if (rc)
> > -                       goto out_msi_disable;
> > -               else
> > +                       if (rc != -EOPNOTSUPP)
> > +                               goto cleanup_nomem;
> > +               } else
> >                         dev_info(&pdev->dev, "MSI enabled with IRQ: %d\n", pdev->irq);
> >         } else if (ipr_debug)
> >                 dev_info(&pdev->dev, "Cannot enable MSI.\n");
> > @@ -8847,8 +8847,6 @@ cleanup_nolog:
> >         ipr_free_mem(ioa_cfg);
> >  cleanup_nomem:
> >         iounmap(ipr_regs);
> > -out_msi_disable:
> > -       pci_disable_msi(pdev);
> >  out_release_regions:
> >         pci_release_regions(pdev);
> >  out_scsi_host_put:
> > 
> > 
> 
> 
> -- 
> Brian King
> Linux on Power Virtualization
> IBM Linux Technology Center
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to
@ 2011-06-15  8:50       ` Julia Lawall
  0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-06-15  8:50 UTC (permalink / raw)
  To: Brian King
  Cc: Wayne Boyer, Brian King, kernel-janitors, James E.J. Bottomley,
	linux-scsi, linux-kernel

On Tue, 14 Jun 2011, Brian King wrote:

> On 06/09/2011 10:51 AM, Wayne Boyer wrote:
> > On 05/31/2011 07:16 AM, Julia Lawall wrote:
> >> From: Julia Lawall <julia@diku.dk>
> >>
> >> The out_msi_disable label should be before cleanup_nomem to additionally
> >> benefit from the call to iounmap.
> > 
> > Yes, this is a problem.  I propose the following patch instead.
> 
> By removing the out_msi_disable label, if you fail initialization later
> on and goto cleanup_nomem, you will end up leaving MSI enabled when you exit
> with this patch.

I agree.

julia

> -Brian
> 
> 
> 
> > 
> > ---
> > 
> > In the case where ipr_test_msi returns an error that is not -EOPNOTSUPP, the
> > execution jumps to the out_msi_disable label.  This misses the call to iounmap
> > for ipr_regs which was initialized earlier.
> > 
> > The fix is to do the call to pci_disable_msi when the error case is detected
> > and then goto the cleanup_nomem label if the error is not -EOPNOTSUPP.
> > 
> > Signed-off-by: Wayne Boyer <wayneb@linux.vnet.ibm.com>
> > ---
> > 
> >  drivers/scsi/ipr.c |   10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > Index: b/drivers/scsi/ipr.c
> > =================================> > --- a/drivers/scsi/ipr.c        2011-06-09 08:14:44.927740117 -0700
> > +++ b/drivers/scsi/ipr.c        2011-06-09 08:50:10.105630466 -0700
> > @@ -8763,11 +8763,11 @@ static int __devinit ipr_probe_ioa(struc
> >         /* Enable MSI style interrupts if they are supported. */
> >         if (ioa_cfg->ipr_chip->intr_type = IPR_USE_MSI && !pci_enable_msi(pdev)) {
> >                 rc = ipr_test_msi(ioa_cfg, pdev);
> > -               if (rc = -EOPNOTSUPP)
> > +               if (rc) {
> >                         pci_disable_msi(pdev);
> > -               else if (rc)
> > -                       goto out_msi_disable;
> > -               else
> > +                       if (rc != -EOPNOTSUPP)
> > +                               goto cleanup_nomem;
> > +               } else
> >                         dev_info(&pdev->dev, "MSI enabled with IRQ: %d\n", pdev->irq);
> >         } else if (ipr_debug)
> >                 dev_info(&pdev->dev, "Cannot enable MSI.\n");
> > @@ -8847,8 +8847,6 @@ cleanup_nolog:
> >         ipr_free_mem(ioa_cfg);
> >  cleanup_nomem:
> >         iounmap(ipr_regs);
> > -out_msi_disable:
> > -       pci_disable_msi(pdev);
> >  out_release_regions:
> >         pci_release_regions(pdev);
> >  out_scsi_host_put:
> > 
> > 
> 
> 
> -- 
> Brian King
> Linux on Power Virtualization
> IBM Linux Technology Center
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap
  2011-06-15  8:50       ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to Julia Lawall
@ 2011-06-20 16:47         ` Wayne Boyer
  -1 siblings, 0 replies; 12+ messages in thread
From: Wayne Boyer @ 2011-06-20 16:47 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Brian King, kernel-janitors, James E.J. Bottomley, linux-scsi,
	linux-kernel

On 06/15/2011 01:50 AM, Julia Lawall wrote:
> On Tue, 14 Jun 2011, Brian King wrote:
> 
>> On 06/09/2011 10:51 AM, Wayne Boyer wrote:
>>> On 05/31/2011 07:16 AM, Julia Lawall wrote:
>>>> From: Julia Lawall <julia@diku.dk>
>>>>
>>>> The out_msi_disable label should be before cleanup_nomem to additionally
>>>> benefit from the call to iounmap.
>>>
>>> Yes, this is a problem.  I propose the following patch instead.
>>
>> By removing the out_msi_disable label, if you fail initialization later
>> on and goto cleanup_nomem, you will end up leaving MSI enabled when you exit
>> with this patch.
> 
> I agree.
> 
> julia
> 

I also agree.  Please disregard my patch.

-- 
Wayne Boyer
IBM - Beaverton, Oregon
LTC S/W Development
(503) 578-5236, T/L 775-5236

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drivers/scsi/ipr.c: reorder error handling code to include
@ 2011-06-20 16:47         ` Wayne Boyer
  0 siblings, 0 replies; 12+ messages in thread
From: Wayne Boyer @ 2011-06-20 16:47 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Brian King, kernel-janitors, James E.J. Bottomley, linux-scsi,
	linux-kernel

On 06/15/2011 01:50 AM, Julia Lawall wrote:
> On Tue, 14 Jun 2011, Brian King wrote:
> 
>> On 06/09/2011 10:51 AM, Wayne Boyer wrote:
>>> On 05/31/2011 07:16 AM, Julia Lawall wrote:
>>>> From: Julia Lawall <julia@diku.dk>
>>>>
>>>> The out_msi_disable label should be before cleanup_nomem to additionally
>>>> benefit from the call to iounmap.
>>>
>>> Yes, this is a problem.  I propose the following patch instead.
>>
>> By removing the out_msi_disable label, if you fail initialization later
>> on and goto cleanup_nomem, you will end up leaving MSI enabled when you exit
>> with this patch.
> 
> I agree.
> 
> julia
> 

I also agree.  Please disregard my patch.

-- 
Wayne Boyer
IBM - Beaverton, Oregon
LTC S/W Development
(503) 578-5236, T/L 775-5236

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2011-06-20 16:47 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-31 14:16 [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap Julia Lawall
2011-05-31 14:16 ` Julia Lawall
2011-06-01 15:02 ` Brian King
2011-06-01 15:02   ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include Brian King
2011-06-09 15:51 ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap Wayne Boyer
2011-06-09 15:51   ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include Wayne Boyer
2011-06-14 16:09   ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap Brian King
2011-06-14 16:09     ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include Brian King
2011-06-15  8:50     ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap Julia Lawall
2011-06-15  8:50       ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to Julia Lawall
2011-06-20 16:47       ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include iounmap Wayne Boyer
2011-06-20 16:47         ` [PATCH] drivers/scsi/ipr.c: reorder error handling code to include Wayne Boyer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.