Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[Suravee Suthikulanit]

Mark,

Thank you for all your comments. Please see my reply below. I have 
omitted the minor ones.

On 6/24/2014 5:11 AM, Mark Rutland wrote:
> On Tue, Jun 24, 2014 at 01:33:00AM +0100, suravee.suthikulpanit@xxxxxxx wrote:
> > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> > +static int alloc_msi_irq(struct gicv2m_msi_data *data, int nvec, int *irq)
> > +{
> > +       int size = sizeof(unsigned long) * data->bm_sz;
> > +       int next = size, ret, num;
> > +
> > +       spin_lock(&data->msi_cnt_lock);
> > +
> > +       for (num = nvec; num > 0; num--) {
> > +               next = bitmap_find_next_zero_area(data->bm,
> > +                                       size, 0, num, 0);
> > +               if (next < size)
> > +                       break;
> > +       }
>
> If max_spi_cnt is not a multiple of BITS_PER_LONG, this can allocate a
> number greater or equal to max_spi_cnt, but below size. We should never
> allocate max_spi_cnt or above.
Thanks for the catch. I also need to clean up this logic for V2.

> > +       spin_unlock(&data->msi_cnt_lock);
> > +
> > +       return ret;
> > +}
> > +
> > +static void free_msi_irq(struct gicv2m_msi_data *data, unsigned int irq)
> > +{
> > +       int pos;
> > +
> > +       spin_lock(&data->msi_cnt_lock);
> > +
> > +       pos = irq - data->spi_start;
> > +       if (pos >= 0 && pos < data->max_spi_cnt)
>
> Should either of these cases ever happen?

This is to check if the irq provided is within the MSI range.

> > +static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
> > +                     struct msi_desc *desc)
> > +{
> > +       int avail, irq = 0;
> > +       struct msi_msg msg;
> > +       phys_addr_t addr;
> > +       struct gicv2m_msi_data *data = to_gicv2m_msi_data(chip);
> > +
> > +       if (data == NULL) {
>
> If container_of returns NULL, you have bigger problems.

It was just sanity check. But, if you think this is obvious, I'll remove 
this check then.

> > +int __init gicv2m_msi_init(struct device_node *node,
> > +                          struct gicv2m_msi_data *msi)
> > +{
> > +       unsigned int val;
> > +       const __be32 *msi_be;
>
> Every time I see a __be32* in a DT probe function I weep. There is no
> need to deal with the internal details of the DTB.
>
> > +
> > +       msi_be = of_get_address(node, GIC_OF_MSIV2M_RANGE_INDEX, NULL, NULL);
> > +       if (!msi_be) {
> > +               pr_err("GICv2m failed. Fail to locate MSI base.\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       msi->paddr64 = of_translate_address(node, msi_be);
> > +       msi->base = of_iomap(node, GIC_OF_MSIV2M_RANGE_INDEX);
>
> You can instead use of_address_to_resource to query the address from the
> DTB once, without having to muck about with endianness conversion
> manually. Take a look at what of_iomap does.

Thanks for this suggestion. I was not quite familiar with the "of_" 
interface. I am cleaning up this whole section now.

> I'm surprised we don't have an ioremap_resource given we have a devm_
> variant.

> > +
> > +       /*
> > +       * MSI_TYPER:
> > +       *     [31:26] Reserved
> > +       *     [25:16] lowest SPI assigned to MSI
> > +       *     [15:10] Reserved
> > +       *     [9:0]   Numer of SPIs assigned to MSI
> > +       */
> > +       val = __raw_readl(msi->base + MSI_TYPER);
>
> Are you sure you want to use __raw_readl?
Probably not.  I am replacing this with ioread32(msi->base + MSI_TYPER).

> > +       if (!val) {
> > +               pr_warn("GICv2m: Failed to read MSI_TYPER register\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       msi->spi_start = (val >> 16) & 0x3ff;
> > +       msi->max_spi_cnt = val & 0x3ff;
> > +
> > +       pr_debug("GICv2m: SPI = %u, cnt = %u\n",
> > +               msi->spi_start, msi->max_spi_cnt);
> > +
> > +       if (msi->spi_start < GIC_V2M_MIN_SPI ||
> > +               msi->max_spi_cnt >= GIC_V2M_MAX_SPI) {
> > +                       pr_err("GICv2m: Init failed\n");
> > +                       return -EINVAL;
> > +       }
> > +
> > +       msi->bm_sz = BITS_TO_LONGS(msi->max_spi_cnt);
>
> So msi->bm_sz is msi->max_spi_cnt in _longs_ (rounded up)...
>
> > +       msi->bm = kzalloc(msi->bm_sz, GFP_KERNEL);
>
> ...yet we allocate that many _bytes_?
Sorry, I got a bit confused here. I'll fix this.

> > +
> > +       gic_arch_extn.irq_mask = gicv2m_mask_msi;
> > +       gic_arch_extn.irq_unmask = gicv2m_unmask_msi;
>
> I'll leave others to comment on the validity of this...
Marc has commented this part in the other patch.

> > +       void __iomem *base;       /* GICv2m virt address */
> > +       unsigned int spi_start;   /* The SPI number that MSIs start */
> > +       unsigned int max_spi_cnt; /* The number of SPIs for MSIs */
> > +       unsigned long *bm;        /* MSI vector bitmap */
> > +       unsigned long bm_sz;      /* MSI bitmap size */
>
> It's a bit odd in my mind that this is in longs. Why not just use
> max_spi_cnt, which you can trivially use to determine bytes or longs?

That's true. I'm cleaning this up.

> > @@ -246,7 +254,8 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> >                              bool force)
> >   {
> >          void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
> > -       unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
> > +       unsigned int shift = (gic_irq(d) % 4) * 8;
> > +       unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
>
> Unrelated change?

Sorry, this was not supposed to be part of this patch.

Thanks,

Suravee

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/