[PATCH] powerpc: mpic_pasemi_msi: failed allocation unnoticed

[PATCH] powerpc: mpic_pasemi_msi: failed allocation unnoticed

[Roel Kluin]

bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may
return -ENOMEM, but hwirq of type irq_hw_number_t which is unsigned.

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
The functions can be found respectively in:
//---[ vi lib/bitmap.c +807 ]---
//---[ vi arch/powerpc/sysdev/mpic_msi.c +39 ]---

diff --git a/arch/powerpc/sysdev/mpic_pasemi_msi.c b/arch/powerpc/sysdev/mpic_pasemi_msi.c
index 33cbfb2..a15ac5c 100644
--- a/arch/powerpc/sysdev/mpic_pasemi_msi.c
+++ b/arch/powerpc/sysdev/mpic_pasemi_msi.c
@@ -109,7 +109,7 @@ static int pasemi_msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 		 * sources can be changed independently.
 		 */
 		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, ALLOC_CHUNK);
-		if (hwirq < 0) {
+		if (hwirq == -ENOMEM) {
 			pr_debug("pasemi_msi: failed allocating hwirq\n");
 			return hwirq;
 		}

[PATCH 2/2] powerpc: mpic_pasemi_msi: failed allocation unnoticed

[Roel Kluin]

A similar problem in arch/powerpc/sysdev/mpic_u3msi.c:
---
bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may
return -ENOMEM, but hwirq of type irq_hw_number_t which is unsigned.

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
The functions can be found respectively in:
//---[ vi lib/bitmap.c +807 ]---
//---[ vi arch/powerpc/sysdev/mpic_msi.c +39 ]---

diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_u3msi.c
index 1d5a408..30a4e27 100644
--- a/arch/powerpc/sysdev/mpic_u3msi.c
+++ b/arch/powerpc/sysdev/mpic_u3msi.c
@@ -122,7 +122,7 @@ static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 
 	list_for_each_entry(entry, &pdev->msi_list, list) {
 		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
-		if (hwirq < 0) {
+		if (hwirq == -ENOMEM) {
 			pr_debug("u3msi: failed allocating hwirq\n");
 			return hwirq;
 		}

[PATCH 2/2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed

[Roel Kluin]

A similar problem in arch/powerpc/sysdev/mpic_u3msi.c,
sorry for the dup, fixed header.
---
bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may
return -ENOMEM, but hwirq of type irq_hw_number_t which is unsigned.

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
The functions can be found respectively in:
//---[ vi lib/bitmap.c +807 ]---
//---[ vi arch/powerpc/sysdev/mpic_msi.c +39 ]---

diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_u3msi.c
index 1d5a408..30a4e27 100644
--- a/arch/powerpc/sysdev/mpic_u3msi.c
+++ b/arch/powerpc/sysdev/mpic_u3msi.c
@@ -122,7 +122,7 @@ static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 
 	list_for_each_entry(entry, &pdev->msi_list, list) {
 		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
-		if (hwirq < 0) {
+		if (hwirq == -ENOMEM) {
 			pr_debug("u3msi: failed allocating hwirq\n");
 			return hwirq;
 		}

Re: [PATCH 2/2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed

[Segher Boessenkool]

> bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may
> return -ENOMEM, but hwirq of type irq_hw_number_t which is unsigned.

>  	list_for_each_entry(entry, &pdev->msi_list, list) {
>  		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
> -		if (hwirq < 0) {
> +		if (hwirq == -ENOMEM) {
>  			pr_debug("u3msi: failed allocating hwirq\n");
>  			return hwirq;
>  		}

Please test for _all_ error values, instead.


Segher

[PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed

[Roel Kluin]

Segher Boessenkool wrote:
>> bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may
>> return -ENOMEM, but hwirq of type irq_hw_number_t which is unsigned.
> 
>>      list_for_each_entry(entry, &pdev->msi_list, list) {
>>          hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
>> -        if (hwirq < 0) {
>> +        if (hwirq == -ENOMEM) {

> 
> Please test for _all_ error values, instead.
>
> Segher

In this case -ENOMEM was _all_ error values, but I get your point.
---
bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may return
signed, but hwirq is unsigned. A failed allocation remains unnoticed.

diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_u3msi.c
index 1d5a408..e790f39 100644
--- a/arch/powerpc/sysdev/mpic_u3msi.c
+++ b/arch/powerpc/sysdev/mpic_u3msi.c
@@ -115,14 +115,16 @@ static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	struct msi_desc *entry;
 	struct msi_msg msg;
 	u64 addr;
+	int ret;
 
 	addr = find_ht_magic_addr(pdev);
 	msg.address_lo = addr & 0xFFFFFFFF;
 	msg.address_hi = addr >> 32;
 
 	list_for_each_entry(entry, &pdev->msi_list, list) {
-		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
-		if (hwirq < 0) {
+		ret = mpic_msi_alloc_hwirqs(msi_mpic, 1);
+		hwirq = ret;
+		if (ret < 0) {
 			pr_debug("u3msi: failed allocating hwirq\n");
 			return hwirq;
 		}

[PATCH 1/2 v2] powerpc: mpic_pasemi_msi: failed allocation unnoticed

[Roel Kluin]

also please add my signoff to 
[PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed
---
bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may return
signed, but hwirq is unsigned. A failed allocation remains unnoticed.

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/arch/powerpc/sysdev/mpic_pasemi_msi.c b/arch/powerpc/sysdev/mpic_pasemi_msi.c
index 33cbfb2..68aff60 100644
--- a/arch/powerpc/sysdev/mpic_pasemi_msi.c
+++ b/arch/powerpc/sysdev/mpic_pasemi_msi.c
@@ -95,6 +95,7 @@ static int pasemi_msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	unsigned int virq;
 	struct msi_desc *entry;
 	struct msi_msg msg;
+	int ret;
 
 	pr_debug("pasemi_msi_setup_msi_irqs, pdev %p nvec %d type %d\n",
 		 pdev, nvec, type);
@@ -108,8 +109,9 @@ static int pasemi_msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 		 * few MSIs for someone, but restrictions will apply to how the
 		 * sources can be changed independently.
 		 */
-		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, ALLOC_CHUNK);
-		if (hwirq < 0) {
+		ret = mpic_msi_alloc_hwirqs(msi_mpic, ALLOC_CHUNK);
+		hwirq = ret;
+		if (ret < 0) {
 			pr_debug("pasemi_msi: failed allocating hwirq\n");
 			return hwirq;
 		}

Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed

[Roel Kluin]

Again thanks to Segher and Joe Perches
---
bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may return
signed, but hwirq is unsigned. A failed allocation remains unnoticed.

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_u3msi.c
index 1d5a408..6e2f868 100644
--- a/arch/powerpc/sysdev/mpic_u3msi.c
+++ b/arch/powerpc/sysdev/mpic_u3msi.c
@@ -115,17 +115,19 @@ static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	struct msi_desc *entry;
 	struct msi_msg msg;
 	u64 addr;
+	int ret;
 
 	addr = find_ht_magic_addr(pdev);
 	msg.address_lo = addr & 0xFFFFFFFF;
 	msg.address_hi = addr >> 32;
 
 	list_for_each_entry(entry, &pdev->msi_list, list) {
-		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
-		if (hwirq < 0) {
+		ret = mpic_msi_alloc_hwirqs(msi_mpic, 1);
+		if (ret < 0) {
 			pr_debug("u3msi: failed allocating hwirq\n");
-			return hwirq;
+			return ret;
 		}
+		hwirq = ret;
 
 		virq = irq_create_mapping(msi_mpic->irqhost, hwirq);
 		if (virq == NO_IRQ) {

Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]

On Thu, 2008-04-24 at 00:25 +0200, Roel Kluin wrote:
> Segher Boessenkool wrote:
> >> bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may
> >> return -ENOMEM, but hwirq of type irq_hw_number_t which is unsigned.
> > 
> >>      list_for_each_entry(entry, &pdev->msi_list, list) {
> >>          hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
> >> -        if (hwirq < 0) {
> >> +        if (hwirq == -ENOMEM) {
> 
> > 
> > Please test for _all_ error values, instead.
> >
> > Segher
> 
> In this case -ENOMEM was _all_ error values, but I get your point.
> ---
> bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may return
> signed, but hwirq is unsigned. A failed allocation remains unnoticed.
> 
> diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_u3msi.c
> index 1d5a408..e790f39 100644
> --- a/arch/powerpc/sysdev/mpic_u3msi.c
> +++ b/arch/powerpc/sysdev/mpic_u3msi.c
> @@ -115,14 +115,16 @@ static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
>  	struct msi_desc *entry;
>  	struct msi_msg msg;
>  	u64 addr;
> +	int ret;
>  
>  	addr = find_ht_magic_addr(pdev);
>  	msg.address_lo = addr & 0xFFFFFFFF;
>  	msg.address_hi = addr >> 32;
>  
>  	list_for_each_entry(entry, &pdev->msi_list, list) {
> -		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
> -		if (hwirq < 0) {
> +		ret = mpic_msi_alloc_hwirqs(msi_mpic, 1);
> +		hwirq = ret;
> +		if (ret < 0) {
>  			pr_debug("u3msi: failed allocating hwirq\n");
>  			return hwirq;
>  		}

I'm not sure I like this.

I think the real bug is that we're using irq_hw_number_t to represent
something which isn't. At the end of the day we have to stash the hwirq
into the MSI message data, which is a u32.

I guess we could imagine a driver that does something magic to allow it
to put something bigger than a u32 in the MSI message, but I doubt it.

So I think mpic_msi_alloc_hwirqs() should return a long, which allows
it to return a full u32 plus the negative error values.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed

[Benjamin Herrenschmidt]

On Thu, 2008-04-24 at 09:36 +1000, Michael Ellerman wrote:
> 
> I think the real bug is that we're using irq_hw_number_t to represent
> something which isn't. At the end of the day we have to stash the
> hwirq
> into the MSI message data, which is a u32.
> 
> I guess we could imagine a driver that does something magic to allow
> it
> to put something bigger than a u32 in the MSI message, but I doubt it.
> 
> So I think mpic_msi_alloc_hwirqs() should return a long, which allows
> it to return a full u32 plus the negative error values.

Until it's used on 32 bits...

Make it return an int error code and pass the hwirq elsewhere or use the
"illegal" hwirq number (each PIC defines one) as the error return.

Cheers,
Ben.