[PATCH] powerpc/fsl_msi: Handle msi-available-ranges better

[Tabi Timur-B04825]

On Mon, Jan 17, 2011 at 2:25 PM, Scott Wood <scottwood at freescale.com> wrote:

> +       for (irq_index = 0, i = 0; i < len / (2 * sizeof(u32)); i++) {
> +               if (p[i * 2] % IRQS_PER_MSI_REG ||
> +                   p[i * 2 + 1] % IRQS_PER_MSI_REG) {
> +                       printk(KERN_WARNING "%s: %s: msi available range of %u at %u is not IRQ-aligned\n",
> +                              __func__, dev->dev.of_node->full_name,
> +                              p[i * 2 + 1], p[i * 2]);
> +                       err = -EINVAL;
> +                       goto error_out;
> +               }
> +
> +               offset = p[i * 2] / IRQS_PER_MSI_REG;
> +               count = p[i * 2 + 1] / IRQS_PER_MSI_REG;
> +
> +               for (j = 0; j < count; j++, irq_index++) {
> +                       err = fsl_msi_setup_hwirq(msi, dev, offset, irq_index);

Scott,

I know it's been eight months since you posted this patch, but I'm not
sure this part is correct.   fsl_msi_setup_hwirq() does this:

cascade_data->index = offset + irq_index;

"cascade_data->index" is supposed to be the index of the MSIIR
register, so it's a number between 0 and 7.  irq_index is incremented
on every pass of loop 'j'.  My understanding is that the following two
definitions of msi-available-ranges are equivalent:

static const u32 all_avail[] = { 0, NR_MSI_IRQS };

static const u32 all_avail[] = { 0, 64, 64, 32, 96, 32, 128, 32, 160, 96};

When I use the first definition, I get this on each call to
fsl_msi_setup_hwirq():

fsl_msi_setup_hwirq:299 offset=0 irq_index=0 cascade_data->index=0
fsl_msi_setup_hwirq:299 offset=0 irq_index=1 cascade_data->index=1
fsl_msi_setup_hwirq:299 offset=0 irq_index=2 cascade_data->index=2
fsl_msi_setup_hwirq:299 offset=0 irq_index=3 cascade_data->index=3
fsl_msi_setup_hwirq:299 offset=0 irq_index=4 cascade_data->index=4
fsl_msi_setup_hwirq:299 offset=0 irq_index=5 cascade_data->index=5
fsl_msi_setup_hwirq:299 offset=0 irq_index=6 cascade_data->index=6
fsl_msi_setup_hwirq:299 offset=0 irq_index=7 cascade_data->index=7

But when I use the second definition of all_avail[], I get this instead:

fsl_msi_setup_hwirq:299 offset=0 irq_index=0 cascade_data->index=0
fsl_msi_setup_hwirq:299 offset=0 irq_index=1 cascade_data->index=1
fsl_msi_setup_hwirq:299 offset=2 irq_index=2 cascade_data->index=4
fsl_msi_setup_hwirq:299 offset=3 irq_index=3 cascade_data->index=6
fsl_msi_setup_hwirq:299 offset=4 irq_index=4 cascade_data->index=8
fsl_msi_setup_hwirq:299 offset=5 irq_index=5 cascade_data->index=10
fsl_msi_setup_hwirq:299 offset=5 irq_index=6 cascade_data->index=11
fsl_msi_setup_hwirq:299 offset=5 irq_index=7 cascade_data->index=12

The problem is that both offset and irq_index are being incremented in
the loop, and cascade_data->index is set to the sum of the two.

Perhaps you meant this:

		err = fsl_msi_setup_hwirq(msi, dev, offset, j);

-- 
Timur Tabi
Linux kernel developer at Freescale