Re: [PATCH RFC 1/1] irqchip/GICv2m: Add support for multiple v2m frames

[Duc Dang]

On Fri, Oct 9, 2015 at 2:10 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> Hi Duc,
>
> On 08/10/15 08:48, Duc Dang wrote:
>> GICv2m driver currently only supports single v2m frame. This
>> patch extend this driver to support multiple v2m frames. All of
>> the v2m frames will be own by a single MSI domain. Each PCIe node
>> can specify msi-parent as the first frame of the v2m frame list
>> to be able to use all available v2m frames for MSI interrupts.
>>
>> This patch should be applied on top of patch
>> (irqchip/GICv2m: Add workaround for APM X-Gene GICv2m erratum):
>> https://lkml.org/lkml/2015/10/6/922
>>
>> Signed-off-by: Duc Dang <dhdang@xxxxxxx>
>> ---
>>  drivers/irqchip/irq-gic-v2m.c | 221 ++++++++++++++++++++++++++++++------------
>>  1 file changed, 159 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
>> index 4c17c18..8ecaf9e 100644
>> --- a/drivers/irqchip/irq-gic-v2m.c
>> +++ b/drivers/irqchip/irq-gic-v2m.c
>> @@ -51,13 +51,19 @@
>>  #define GICV2M_NEEDS_SPI_OFFSET              0x00000001
>>
>>  struct v2m_data {
>> -     spinlock_t msi_cnt_lock;
>>       struct resource res;    /* GICv2m resource */
>>       void __iomem *base;     /* GICv2m virt address */
>>       u32 spi_start;          /* The SPI number that MSIs start */
>>       u32 nr_spis;            /* The number of SPIs for MSIs */
>> -     unsigned long *bm;      /* MSI vector bitmap */
>>       u32 flags;              /* v2m flags for specific implementation */
>> +     struct list_head list;  /* Link to other v2m frames */
>> +};
>> +
>> +struct gicv2m_ctrlr {
>> +     spinlock_t v2m_ctrlr_lock;      /* Lock for all v2m frames */
>> +     u32 nr_vecs;                    /* Total MSI vectors */
>> +     unsigned long *bm;              /* MSI vector bitmap */
>> +     struct list_head v2m_frms;      /* List of v2m frames */
>>  };
>>
>>  static void gicv2m_mask_msi_irq(struct irq_data *d)
>> @@ -98,11 +104,29 @@ static int gicv2m_set_affinity(struct irq_data *irq_data,
>>       return ret;
>>  }
>>
>> +static struct v2m_data *irq_data_to_v2m_frm(struct irq_data *data,
>> +                                         struct gicv2m_ctrlr *v2m_ctrlr)
>> +{
>> +     struct v2m_data *v2m;
>> +
>> +     list_for_each_entry(v2m, &v2m_ctrlr->v2m_frms, list) {
>> +             if ((data->hwirq >= v2m->spi_start) &&
>> +                 (data->hwirq < (v2m->spi_start + v2m->nr_spis)))
>> +                     return v2m;
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>>  static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>>  {
>> -     struct v2m_data *v2m = irq_data_get_irq_chip_data(data);
>> -     phys_addr_t addr = v2m->res.start + V2M_MSI_SETSPI_NS;
>> +     struct gicv2m_ctrlr *v2m_ctrlr = irq_data_get_irq_chip_data(data);
>> +     struct v2m_data *v2m;
>> +     phys_addr_t addr;
>>
>> +     v2m = irq_data_to_v2m_frm(data, v2m_ctrlr);
>> +     WARN_ON(!v2m);
>> +     addr = v2m->res.start + V2M_MSI_SETSPI_NS;
>
> At that particular point, I've stopped reading.
>
> Pointlessly iterating over the frames when you can store the right one
> in irq_data was bad enough, but having a WARN_ON() followed by a panic
> is just not acceptable. Either you warn and avoid the crash, or you just
> assume that the whole thing is sane and will never be inconsistent.

Yes, my bad.
>
> You are also changing the way this driver works, and not for the best.
> I've just written something fairly similar, with the following diffstat:
>
>  1 file changed, 76 insertions(+), 45 deletions(-)
>
> The current infrastructure is sound, you just have to make use of it.
>
> I've pushed the following (untested) patch:
>
> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/multi-v2m&id=45e35adb60087792626570ff21bb23ab0f67a6ac
>
> Let me know if that works for you.

Yes! I try and your patch definitely works on my X-Gene 2 board.

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

Thanks and regards,
Duc Dang.
--
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/