lkml.org 
[lkml]   [2008]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed
From
Date
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]
\
 
 \ /
  Last update: 2008-04-24 01:39    [W:0.047 / U:0.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site