Re: [PATCH 1/2] [v3] powerpc/44x: Fix PCI MSI support for Maui APM821xx SoC and Bluestone board

[Josh Boyer <jwboyer@gmail.com>]

On Mon, Mar 12, 2012 at 4:07 AM, Mai La <mla@apm.com> wrote:
> diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4x=
x_msi.c
> index 1c2d7af..63989d0 100644
> --- a/arch/powerpc/sysdev/ppc4xx_msi.c
> +++ b/arch/powerpc/sysdev/ppc4xx_msi.c
> @@ -28,10 +28,11 @@
> =A0#include <linux/of_platform.h>
> =A0#include <linux/interrupt.h>
> =A0#include <linux/export.h>
> +#include <linux/kernel.h>
> =A0#include <asm/prom.h>
> =A0#include <asm/hw_irq.h>
> =A0#include <asm/ppc-pci.h>
> -#include <boot/dcr.h>
> +#include <asm/dcr.h>
> =A0#include <asm/dcr-regs.h>
> =A0#include <asm/msi_bitmap.h>
>
> @@ -43,13 +44,14 @@
> =A0#define PEIH_FLUSH0 =A0 =A00x30
> =A0#define PEIH_FLUSH1 =A0 =A00x38
> =A0#define PEIH_CNTRST =A0 =A00x48
> -#define NR_MSI_IRQS =A0 =A04
> +
> +int msi_irqs;

This should be static.

> =A0struct ppc4xx_msi {
> =A0 =A0 =A0 =A0u32 msi_addr_lo;
> =A0 =A0 =A0 =A0u32 msi_addr_hi;
> =A0 =A0 =A0 =A0void __iomem *msi_regs;
> - =A0 =A0 =A0 int msi_virqs[NR_MSI_IRQS];
> + =A0 =A0 =A0 int *msi_virqs;
> =A0 =A0 =A0 =A0struct msi_bitmap bitmap;
> =A0 =A0 =A0 =A0struct device_node *msi_dev;
> =A0};
> @@ -61,7 +63,7 @@ static int ppc4xx_msi_init_allocator(struct platform_de=
vice *dev,
> =A0{
> =A0 =A0 =A0 =A0int err;
>
> - =A0 =A0 =A0 err =3D msi_bitmap_alloc(&msi_data->bitmap, NR_MSI_IRQS,
> + =A0 =A0 =A0 err =3D msi_bitmap_alloc(&msi_data->bitmap, msi_irqs,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev->dev.of_no=
de);
> =A0 =A0 =A0 =A0if (err)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return err;
> @@ -83,6 +85,9 @@ static int ppc4xx_setup_msi_irqs(struct pci_dev *dev, i=
nt nvec, int type)
> =A0 =A0 =A0 =A0struct msi_desc *entry;
> =A0 =A0 =A0 =A0struct ppc4xx_msi *msi_data =3D &ppc4xx_msi;
>
> + =A0 =A0 =A0 msi_data->msi_virqs =3D kmalloc((msi_irqs) * sizeof(int),
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 GFP_KERNEL);
> +

What happens if this allocation fails?  Unlikely perhaps, but we should sti=
ll
probably try and handle the error.

> =A0 =A0 =A0 =A0list_for_each_entry(entry, &dev->msi_list, list) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int_no =3D msi_bitmap_alloc_hwirqs(&msi_da=
ta->bitmap, 1);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (int_no >=3D 0)

<snip>

> @@ -234,6 +241,10 @@ static int __devinit ppc4xx_msi_probe(struct platfor=
m_device *dev)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto error_out;
> =A0 =A0 =A0 =A0}
>
> + =A0 =A0 =A0 msi_irqs =3D of_irq_count(dev->dev.of_node);
> + =A0 =A0 =A0 if (!msi_irqs)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -1;
> +

Maybe return -ENODEV here instead of -1?  It probably doesn't matter at the
moment, but if anything actually checked why the probe function failed, tha=
t
would be more descriptive.

josh