Hi Ben,
On Wed, 12 Jun 2019 06:16:05 +0100,
Benjamin Herrenschmidt <b...@kernel.crashing.org> wrote:
>
> pci_msi_create_irq_domain -> pci_msi_domain_update_chip_ops will
> set those two already since the driver sets MSI_FLAG_USE_DEF_CHIP_OPS
>
> Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> ---
>
> [UNTESTED]
>
> Just something I noticed while browsing through those drivers in
> search of ways to factor some of the code.
>
> That leads to a question here:
>
> Some MSI drivers such as this one (or any using the defaults mask/unmask
> provided by drivers/pci/msi.c) only call the PCI MSI mask/unmask functions.
>
> Some other drivers call those PCI function but *also* call the parent
> mask/unmask (giv-v2m for example) which generally is the inner domain
> which just itself forwards to its own parent.
>
> Is there any preference for doing it one way or the other ? I can see
> that in cases where the device doesn't support MSI masking, calling the
> parent could be useful but we don't know that at the moment in the
> corresponding code.
>
> It feels like something we should consolidate (and remove code from
> drivers). For example, the defaults in drivers/pci/msi.c could always
> call the parent if it exists and has a mask/unmask callback.
>
> Opinions ? I'm happy to produce patches once we agree...
>
> diff --git a/drivers/irqchip/irq-armada-370-xp.c
> b/drivers/irqchip/irq-armada-370-xp.c
> index c9bdc5221b82..911230f28e2d 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -197,8 +197,6 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
>
> static struct irq_chip armada_370_xp_msi_irq_chip = {
> .name = "MPIC MSI",
> - .irq_mask = pci_msi_mask_irq,
> - .irq_unmask = pci_msi_unmask_irq,
> };
>
> static struct msi_domain_info armada_370_xp_msi_domain_info = {
>
It looks to me that masking at the PCI level is rather superfluous as
long as the MSI controller HW has the capability to mask the interrupt
on a per MSI basis. After all, most non MSI-X endpoint lack support
for masking of individual vectors, so I think that we should just mask
things at the irqchip level. This is also consistent with what you'd
have to do for non-PCI MSI, where nothing standardises the MSI
masking.
I think this is in effect a split in responsibilities:
- the end-point driver should (directly or indirectly) control the
interrupt generation at the end-point level,
- the MSI controller driver should control the signalling of the MSI
to the CPU.
The only case where we should rely on masking interrupts at the
end-point level is when the MSI controller doesn't provide a method to
do so (hopefully a rare exception).
To take the example of the gicv2m driver that you mention above, I'd
suggest the following:
diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index 3c77ab676e54..2ce801207acd 100644
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -72,22 +72,10 @@ struct v2m_data {
u32 flags; /* v2m flags for specific implementation */
};
-static void gicv2m_mask_msi_irq(struct irq_data *d)
-{
- pci_msi_mask_irq(d);
- irq_chip_mask_parent(d);
-}
-
-static void gicv2m_unmask_msi_irq(struct irq_data *d)
-{
- pci_msi_unmask_irq(d);
- irq_chip_unmask_parent(d);
-}
-
static struct irq_chip gicv2m_msi_irq_chip = {
.name = "MSI",
- .irq_mask = gicv2m_mask_msi_irq,
- .irq_unmask = gicv2m_unmask_msi_irq,
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
.irq_eoi = irq_chip_eoi_parent,
.irq_write_msi_msg = pci_msi_domain_write_msg,
};
The same should be applied to a number of drivers in the tree, which
seem to have cargo-culted the wrong idiom (and I take responsibility
for that).
Thanks,
M.
--
Jazz is not dead, it just smells funny.
