Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment

[Marc Zyngier]

Hi Ben,

On Wed, 12 Jun 2019 06:16:05 +0100,
Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> 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 <benh@xxxxxxxxxxxxxxxxxxx>
> ---
> 
> [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.