* [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed
@ 2008-04-23 22:25 ` Roel Kluin
0 siblings, 0 replies; 15+ messages in thread
From: Roel Kluin @ 2008-04-23 22:25 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: paulus, linuxppc-dev, 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] 15+ messages in thread* Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed
2008-04-23 22:25 ` Roel Kluin
@ 2008-04-23 23:03 ` Roel Kluin
-1 siblings, 0 replies; 15+ 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] 15+ messages in thread* Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed
@ 2008-04-23 23:03 ` Roel Kluin
0 siblings, 0 replies; 15+ messages in thread
From: Roel Kluin @ 2008-04-23 23:03 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: paulus, linuxppc-dev, 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] 15+ messages in thread
* Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed
2008-04-23 22:25 ` Roel Kluin
@ 2008-04-23 23:36 ` Michael Ellerman
-1 siblings, 0 replies; 15+ 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] 15+ messages in thread* Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed
@ 2008-04-23 23:36 ` Michael Ellerman
0 siblings, 0 replies; 15+ messages in thread
From: Michael Ellerman @ 2008-04-23 23:36 UTC (permalink / raw)
To: Roel Kluin; +Cc: Segher Boessenkool, 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] 15+ 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
-1 siblings, 0 replies; 15+ 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] 15+ messages in thread
* Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed
@ 2008-04-24 0:42 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Herrenschmidt @ 2008-04-24 0:42 UTC (permalink / raw)
To: michael; +Cc: Roel Kluin, linuxppc-dev, paulus, lkml
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] 15+ messages in thread