* [PATCH] powerpc: mpic_pasemi_msi: failed allocation unnoticed
@ 2008-04-23 16:51 Roel Kluin
2008-04-23 16:54 ` [PATCH 2/2] " Roel Kluin
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Roel Kluin @ 2008-04-23 16:51 UTC (permalink / raw)
To: paulus, linuxppc-dev; +Cc: lkml
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;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/2] powerpc: mpic_pasemi_msi: failed allocation unnoticed 2008-04-23 16:51 [PATCH] powerpc: mpic_pasemi_msi: failed allocation unnoticed Roel Kluin @ 2008-04-23 16:54 ` Roel Kluin 2008-04-23 19:00 ` [PATCH 2/2] mpic_u3msi: mpic_u3msi: " Roel Kluin 2008-04-23 22:32 ` [PATCH 1/2 v2] powerpc: mpic_pasemi_msi: " Roel Kluin 2 siblings, 0 replies; 9+ messages in thread From: Roel Kluin @ 2008-04-23 16:54 UTC (permalink / raw) To: paulus, linuxppc-dev; +Cc: lkml 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; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed 2008-04-23 16:51 [PATCH] powerpc: mpic_pasemi_msi: failed allocation unnoticed Roel Kluin 2008-04-23 16:54 ` [PATCH 2/2] " Roel Kluin @ 2008-04-23 19:00 ` Roel Kluin 2008-04-23 22:09 ` Segher Boessenkool 2008-04-23 22:32 ` [PATCH 1/2 v2] powerpc: mpic_pasemi_msi: " Roel Kluin 2 siblings, 1 reply; 9+ messages in thread From: Roel Kluin @ 2008-04-23 19:00 UTC (permalink / raw) To: paulus, linuxppc-dev; +Cc: lkml 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; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed 2008-04-23 19:00 ` [PATCH 2/2] mpic_u3msi: mpic_u3msi: " Roel Kluin @ 2008-04-23 22:09 ` Segher Boessenkool 2008-04-23 22:25 ` [PATCH 2/2 v2] " Roel Kluin 0 siblings, 1 reply; 9+ messages in thread From: Segher Boessenkool @ 2008-04-23 22:09 UTC (permalink / raw) To: Roel Kluin; +Cc: linuxppc-dev, paulus, lkml > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed 2008-04-23 22:09 ` Segher Boessenkool @ 2008-04-23 22:25 ` Roel Kluin 2008-04-23 23:03 ` Roel Kluin 2008-04-23 23:36 ` Michael Ellerman 0 siblings, 2 replies; 9+ messages in thread From: Roel Kluin @ 2008-04-23 22:25 UTC (permalink / raw) To: Segher Boessenkool; +Cc: linuxppc-dev, paulus, lkml 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; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed 2008-04-23 22:25 ` [PATCH 2/2 v2] " Roel Kluin @ 2008-04-23 23:03 ` Roel Kluin 2008-04-23 23:36 ` Michael Ellerman 1 sibling, 0 replies; 9+ messages in thread From: Roel Kluin @ 2008-04-23 23:03 UTC (permalink / raw) To: Segher Boessenkool; +Cc: linuxppc-dev, paulus, lkml 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) { ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed 2008-04-23 22:25 ` [PATCH 2/2 v2] " Roel Kluin 2008-04-23 23:03 ` Roel Kluin @ 2008-04-23 23:36 ` Michael Ellerman 2008-04-24 0:42 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 9+ messages in thread From: Michael Ellerman @ 2008-04-23 23:36 UTC (permalink / raw) To: Roel Kluin; +Cc: linuxppc-dev, paulus, lkml [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed 2008-04-23 23:36 ` Michael Ellerman @ 2008-04-24 0:42 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 9+ messages in thread From: Benjamin Herrenschmidt @ 2008-04-24 0:42 UTC (permalink / raw) To: michael; +Cc: linuxppc-dev, Roel Kluin, lkml, paulus 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2 v2] powerpc: mpic_pasemi_msi: failed allocation unnoticed 2008-04-23 16:51 [PATCH] powerpc: mpic_pasemi_msi: failed allocation unnoticed Roel Kluin 2008-04-23 16:54 ` [PATCH 2/2] " Roel Kluin 2008-04-23 19:00 ` [PATCH 2/2] mpic_u3msi: mpic_u3msi: " Roel Kluin @ 2008-04-23 22:32 ` Roel Kluin 2 siblings, 0 replies; 9+ messages in thread From: Roel Kluin @ 2008-04-23 22:32 UTC (permalink / raw) To: paulus, linuxppc-dev; +Cc: lkml 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; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-04-24 0:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-04-23 16:51 [PATCH] powerpc: mpic_pasemi_msi: failed allocation unnoticed Roel Kluin 2008-04-23 16:54 ` [PATCH 2/2] " Roel Kluin 2008-04-23 19:00 ` [PATCH 2/2] mpic_u3msi: mpic_u3msi: " Roel Kluin 2008-04-23 22:09 ` Segher Boessenkool 2008-04-23 22:25 ` [PATCH 2/2 v2] " Roel Kluin 2008-04-23 23:03 ` Roel Kluin 2008-04-23 23:36 ` Michael Ellerman 2008-04-24 0:42 ` Benjamin Herrenschmidt 2008-04-23 22:32 ` [PATCH 1/2 v2] powerpc: mpic_pasemi_msi: " Roel Kluin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).