Re: [patch V2 06/40] PCI/MSI: Provide static key for parent mask/unmask

[Marc Zyngier]

On Mon, 21 Nov 2022 14:39:36 +0000,
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 
> Most ARM(64) PCI/MSI domains mask and unmask in the parent domain after or
> before the PCI mask/unmask operation takes place. So there are more than a
> dozen of the same wrapper implementation all over the place.
> 
> Don't make the same mistake with the new per device PCI/MSI domains and
> provide a static key which lets the domain implementation enable this
> sequence in the PCI/MSI code.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> ---
>  drivers/pci/msi/irqdomain.c |   30 ++++++++++++++++++++++++++++++
>  include/linux/msi.h         |    2 ++
>  2 files changed, 32 insertions(+)
> 
> --- a/drivers/pci/msi/irqdomain.c
> +++ b/drivers/pci/msi/irqdomain.c
> @@ -148,17 +148,45 @@ static void pci_device_domain_set_desc(m
>  	arg->hwirq = desc->msi_index;
>  }
>  
> +static DEFINE_STATIC_KEY_FALSE(pci_msi_mask_unmask_parent);
> +
> +/**
> + * pci_device_msi_mask_unmask_parent_enable - Enable propagation of mask/unmask
> + *					      to the parent interrupt chip
> + *
> + * For MSI parent interrupt domains which want to mask at the parent interrupt
> + * chip too.
> + */
> +void pci_device_msi_mask_unmask_parent_enable(void)
> +{
> +	static_branch_enable(&pci_msi_mask_unmask_parent);
> +}
> +
> +static __always_inline void cond_mask_parent(struct irq_data *data)
> +{
> +	if (static_branch_unlikely(&pci_msi_mask_unmask_parent))
> +		irq_chip_mask_parent(data);
> +}
> +
> +static __always_inline void cond_unmask_parent(struct irq_data *data)
> +{
> +	if (static_branch_unlikely(&pci_msi_mask_unmask_parent))
> +		irq_chip_unmask_parent(data);
> +}
> +
>  static void pci_mask_msi(struct irq_data *data)
>  {
>  	struct msi_desc *desc = irq_data_get_msi_desc(data);
>  
>  	pci_msi_mask(desc, BIT(data->irq - desc->irq));
> +	cond_mask_parent(data);

I find this a bit odd. If anything, I'd rather drop the masking at the
PCI level and keep it local to the interrupt controller, because this
is likely to be more universal than the equivalent PCI operation
(think multi-MSI, for example, which cannot masks individual MSIs).

Another thing is that the static key is a global state. Nothing says
that masking one way or the other is a universal thing, specially when
you have multiple interrupt controllers dealing with MSIs in different
ways. For example, GICv3 can use both the ITS and the GICv3-MBI frame
at the same time for different PCI RC. OK, they happen to deal with
MSIs in the same way, but you hopefully get my point.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.