On Thu, 2008-04-24 at 00:25 +0200, Roel Kluin wrote:
quoted text
> 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.
> >=20
> >>      list_for_each_entry(entry, &pdev->msi_list, list) {
> >>          hwirq =3D mpic_msi_alloc_hwirqs(msi_mpic, 1);
> >> -        if (hwirq < 0) {
> >> +        if (hwirq =3D=3D -ENOMEM) {
>=20
> >=20
> > Please test for _all_ error values, instead.
> >
> > Segher
>=20
> 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.
>=20
> diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_=
u3msi.c
quoted text
> 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 *pde=
v, int nvec, int type)
quoted text
>  	struct msi_desc *entry;
>  	struct msi_msg msg;
>  	u64 addr;
> +	int ret;
> =20
>  	addr =3D find_ht_magic_addr(pdev);
>  	msg.address_lo =3D addr & 0xFFFFFFFF;
>  	msg.address_hi =3D addr >> 32;
> =20
>  	list_for_each_entry(entry, &pdev->msi_list, list) {
> -		hwirq =3D mpic_msi_alloc_hwirqs(msi_mpic, 1);
> -		if (hwirq < 0) {
> +		ret =3D mpic_msi_alloc_hwirqs(msi_mpic, 1);
> +		hwirq =3D 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 =EF=BB=BFmpic_msi_alloc_hwirqs() should return a long, which all=
ows
it to return a full u32 plus the negative error values.

cheers

--=20
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