by Marc Zyngier

[permalink] [raw]

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

Hi Suravee,

On 27/06/14 16:43, Suravee Suthikulpanit wrote:
> Hi Marc,
>
> After looking at the GICv3 implementation and trying to understand how
> you architect the driver, I have a couple questions below.
>
> > On 06/24/2014 04:52 AM, Marc Zyngier wrote:
>> Hi Suravee,
>>
>> On 24/06/14 01:33, [email protected] wrote:
>>> + pr_info("GICv2m: SPI range [%d:%d]\n",
>>> + msi->spi_start, (msi->spi_start + msi->max_spi_cnt));
>>> +
>>> + gic_arch_extn.irq_mask = gicv2m_mask_msi;
>>> + gic_arch_extn.irq_unmask = gicv2m_unmask_msi;
>>
>> Right, I now see why you need to test on the MSI descriptor. Don't do
>> that. The gic_arch_extn crap should *never* *be* *used*.
>
> Hm, sounds like this should be removed all together then? In that case,
> this would require changes in the irq-gic.c to call these functions
> directly.

No. The gic_arch_extn's sole purpose in life is to allow a parallel
interrupt controller that mimics what the GIC does, and that can be used
as a wake up source. Clearly, MSI support is completely out of the scope
of this ... thing.

>>
>> What you want to do is do assign a different irq_chip to your MSI
>> interrupts. This will require a different integration with the main GIC
>> code though. For the GICv3 ITS, I do it when the interrupt gets mapped.
>
> Ah, that's what I was trying to avoid. Why should we need a whole
> different irq_chip just to add the MSI register frame support on top of
> the GICv2 which is already supported by the current irq-gic.c?

Because, as you've noticed, it has a different set of requirements
(accessing the MSI specific data, for a start). That's the very purpose
of the irq_chip structure. Don't work around it. Nothing prevents you
from reusing the existing code, by the way.

>>
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(gicv2m_msi_init);
>>> +
>>> +
>>> +
>>> +/**
>>> + * Override arch_setup_msi_irq in drivers/pci/msi.c
>>> + * since we don't want to change the chip_data
>>> + */
>>> +int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
>>> +{
>>> + struct msi_chip *chip = pdev->bus->msi;
>>> +
>>> + if (!chip || !chip->setup_irq)
>>> + return -EINVAL;
>>> +
>>> + return chip->setup_irq(chip, pdev, desc);
>>> +}
>>> +
>>> +/**
>>> + * Override arch_teardown_msi_irq in drivers/pci/msi.c
>>> + */
>>> +void arch_teardown_msi_irq(unsigned int irq)
>>> +{
>>> + struct msi_desc *desc;
>>> + struct msi_chip *chip;
>>> +
>>> + desc = irq_get_msi_desc(irq);
>>> + if (!desc)
>>> + return;a
>>> +
>>> + chip = desc->dev->bus->msi;
>>> + if (!chip)
>>> + return;
>>> +
>>> + chip->teardown_irq(chip, irq);
>>> +}
>>
>> Please don't overtide those. There shouldn't be any reason for a
>> *driver* to do so. Only architecture code should do it. And nothing in
>> your code requires it (at least once you've stopped playing with the
>> silly gic extension...).
>
> The reason I need these because the __weak version of arch_setup_msi_irq
> function in driver/pci/msi.c calls irq_set_chip_data and replace the
> chip_data with msi_chip (originally was pointing to the gic_chip_data
> structure). This ended up breaking the GIC.

This is exactly why I urged you to have a different irq_chip, pointing
to different methods. MSI and IRQs are treated a bit differently in the
kernel. Hijacking global symbols to work around this difference is
hardly acceptable. Here, you're mandating that all the MSI controllers,
past, present and future are going to fit the requirements you've now
dictated. Hmmm... ;-)

> Looking at the GICv3 ITS implementation, this seems to also have similar
> implementation. So, this was not an issue for you?

No. This is actually a very useful feature (and I should definitely make
use of it). You can embed the msi_chip into the gic_chip_data, and use
"container_of()" to go from one to the other. But again, this mandates
you provide your own methods (not that it is a lot of code, really).

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2014-06-30 18:27:33

by Jason Cooper

[permalink] [raw]

Subject: Re: [PATCH 0/2] Introduce ARM GICv2m MSI(-X) support

On Tue, Jun 24, 2014 at 07:19:26PM -0500, Suravee Suthikulanit wrote:
> On 6/24/2014 7:26 AM, Jason Cooper wrote:
> >On Mon, Jun 23, 2014 at 07:32:58PM -0500, [email protected] wrote:
> >>This patch set introduces support for MSI(-X) in GICv2m specification,
> >>which is implemented in some variation of GIC400.
> >>
> >>This depends on and has been tested with the V7 of "Add support for PCI in AArch64"
> >>(https://lkml.org/lkml/2014/3/14/320).
> >
> >Is this a build, boot, or runtime dependency?
> >
>
> For all of the above.

Ok, the gic change in #1 should be a trivial conflict to resolve, so
it'll be easiest for you guys to merge this with the other code you have
going into arm-soc.

Just keep me in the Cc and I'll Ack it when it's ready.

thx,

Jason.