Messages in this thread | | | Subject | Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed | From | Michael Ellerman <> | Date | Thu, 24 Apr 2008 09:36:23 +1000 |
| |
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 [unhandled content-type:application/pgp-signature] | |