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

[Michael Ellerman <michael@ellerman.id.au>]

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