powerpc: mpic_pasemi_msi: failed allocation unnoticed

PATCH New Roel Kluin 4 participants 9 messages Apr 23, 2008 – Apr 24, 2008 PowerPC

Share & tools

Roel Kluin

author

2008-04-23 16:51 UTC #0 lore raw

PATCH 1/1 powerpc: mpic_pasemi_msi: failed allocation unnoticed

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 <[email protected]>

---
The functions can be found respectively in:
//---[ vi lib/bitmap.c +807 ]---
//---[ vi arch/powerpc/sysdev/mpic_msi.c +39 ]---

arch/powerpc/sysdev/mpic_pasemi_msi.c +1 -1

index 33cbfb2..a15ac5c 100644
@@ -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;
 		}

Roel Kluin

author

2008-04-23 16:54 UTC #1 lore raw

PATCH 2/2 powerpc: mpic_pasemi_msi: failed allocation unnoticed

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 <[email protected]>

---
The functions can be found respectively in:
//---[ vi lib/bitmap.c +807 ]---
//---[ vi arch/powerpc/sysdev/mpic_msi.c +39 ]---

arch/powerpc/sysdev/mpic_u3msi.c +1 -1

index 1d5a408..30a4e27 100644
@@ -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;
 		}

Roel Kluin

author

2008-04-23 19:00 UTC #2 lore raw

PATCH 2/2 mpic_u3msi: mpic_u3msi: failed allocation unnoticed

In reply to 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 <[email protected]>

---
The functions can be found respectively in:
//---[ vi lib/bitmap.c +807 ]---
//---[ vi arch/powerpc/sysdev/mpic_msi.c +39 ]---

arch/powerpc/sysdev/mpic_u3msi.c +1 -1

index 1d5a408..30a4e27 100644
@@ -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;
 		}

Segher Boessenkool

2008-04-23 22:09 UTC #3 lore raw

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

arch/powerpc/sysdev/mpic_u3msi.c static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type) · line 128

Quoted 10 lines

bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may return -ENOMEM, but hwirq of type irq_hw_number_t which is unsigned.
arch/powerpc/sysdev/mpic_u3msi.c +1 -1
 	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

Roel Kluin

author

2008-04-23 22:25 UTC #4 lore raw

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

Quoted 9 lines · Segher Boessenkool

Review
Quoted 7 lines
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);
(quoted diff) +1 -1
-        if (hwirq < 0) {
+        if (hwirq == -ENOMEM) {

Quoted 3 lines

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.

arch/powerpc/sysdev/mpic_u3msi.c +4 -2

index 1d5a408..e790f39 100644
@@ -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;
 		}

Roel Kluin

author

2008-04-23 22:32 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---

arch/powerpc/sysdev/mpic_pasemi_msi.c +4 -2

index 33cbfb2..68aff60 100644
@@ -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;
 		}

Roel Kluin

author

2008-04-23 23:03 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---

arch/powerpc/sysdev/mpic_u3msi.c +5 -3

index 1d5a408..6e2f868 100644
@@ -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) {

Michael Ellerman

2008-04-23 23:36 UTC #6 lore raw

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

In reply to Roel Kluin

Quoted 32 lines · Roel Kluin

Quoted 9 lines · Segher Boessenkool
Review
Quoted 7 lines
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);
(quoted diff) +1 -1
-        if (hwirq < 0) {
+        if (hwirq == -ENOMEM) {
Quoted 3 lines
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.
arch/powerpc/sysdev/mpic_u3msi.c +1 -0
index 1d5a408..e790f39 100644
@@ -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;

arch/powerpc/sysdev/mpic_u3msi.c static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type) · line 130

Quoted 10 lines

arch/powerpc/sysdev/mpic_u3msi.c +3 -2

 
 	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

Signature · 8 lines

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

Benjamin Herrenschmidt

2008-04-24 00:42 UTC #7 lore raw

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

Quoted 6 lines · Michael Ellerman

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.

Quoted 6 lines

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.