[permalink] [raw]

2024-06-14 16:43:49

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH v3 03/24] PCI/MSI: Provide MSI_FLAG_PCI_MSI_MASK_PARENT

On Fri, Jun 14, 2024 at 12:23:42PM +0200, Shivamurthy Shastri 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.

Is this an opportunity to clean up all these wrappers? If you could
mention an example or two here, maybe somebody would be motivated to
come back and simplify the existing wrappers to take advantage of this
new flag?

> Don't make the same mistake with the new per device PCI/MSI domains and
> provide a new MSI feature flag, which lets the domain implementation
> enable this sequence in the PCI/MSI code.
>
> Signed-off-by: Shivamurthy Shastri <[email protected]>

I assume you'll merge this series via some other tree, so:

Acked-by: Bjorn Helgaas <[email protected]>

> ---
> v3: new patch to replace the global static key - Marc Zyngier
> ---
> drivers/pci/msi/irqdomain.c | 21 +++++++++++++++++++++
> include/linux/msi.h | 2 ++
> 2 files changed, 23 insertions(+)
>
> diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
> index 03d2dd25790d..112c2ff3035c 100644
> --- a/drivers/pci/msi/irqdomain.c
> +++ b/drivers/pci/msi/irqdomain.c
> @@ -148,17 +148,35 @@ static void pci_device_domain_set_desc(msi_alloc_info_t *arg, struct msi_desc *d
> arg->hwirq = desc->msi_index;
> }
>
> +static __always_inline void cond_mask_parent(struct irq_data *data)
> +{
> + struct msi_domain_info *info = data->domain->host_data;
> +
> + if (unlikely(info->flags & MSI_FLAG_PCI_MSI_MASK_PARENT))
> + irq_chip_mask_parent(data);
> +}
> +
> +static __always_inline void cond_unmask_parent(struct irq_data *data)
> +{
> + struct msi_domain_info *info = data->domain->host_data;
> +
> + if (unlikely(info->flags & MSI_FLAG_PCI_MSI_MASK_PARENT))
> + irq_chip_unmask_parent(data);
> +}
> +
> static void pci_irq_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);
> }
>
> static void pci_irq_unmask_msi(struct irq_data *data)
> {
> struct msi_desc *desc = irq_data_get_msi_desc(data);
>
> + cond_unmask_parent(data);
> pci_msi_unmask(desc, BIT(data->irq - desc->irq));
> }
>
> @@ -170,6 +188,7 @@ static void pci_irq_unmask_msi(struct irq_data *data)
>
> #define MSI_COMMON_FLAGS (MSI_FLAG_FREE_MSI_DESCS | \
> MSI_FLAG_ACTIVATE_EARLY | \
> + MSI_FLAG_PCI_MSI_MASK_PARENT | \
> MSI_FLAG_DEV_SYSFS | \
> MSI_REACTIVATE)
>
> @@ -195,10 +214,12 @@ static const struct msi_domain_template pci_msi_template = {
> static void pci_irq_mask_msix(struct irq_data *data)
> {
> pci_msix_mask(irq_data_get_msi_desc(data));
> + cond_mask_parent(data);
> }
>
> static void pci_irq_unmask_msix(struct irq_data *data)
> {
> + cond_unmask_parent(data);
> pci_msix_unmask(irq_data_get_msi_desc(data));
> }
>
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index dc27cf3903d5..04f33e7f6f8b 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -556,6 +556,8 @@ enum {
> MSI_FLAG_USE_DEV_FWNODE = (1 << 7),
> /* Set parent->dev into domain->pm_dev on device domain creation */
> MSI_FLAG_PARENT_PM_DEV = (1 << 8),
> + /* Support for parent mask/unmask */
> + MSI_FLAG_PCI_MSI_MASK_PARENT = (1 << 9),
>
> /* Mask for the generic functionality */
> MSI_GENERIC_FLAGS_MASK = GENMASK(15, 0),
> --
> 2.34.1
>

2024-06-15 17:24:17

by Marc Zyngier

[permalink] [raw]

Subject: Re: [PATCH v3 14/24] genirq/gic-v3-mbi: Remove unused wired MSI mechanics

On Fri, 14 Jun 2024 11:23:53 +0100,
Shivamurthy Shastri <[email protected]> wrote:
>
> From: Thomas Gleixner <[email protected]>
>
> Nothing builds a platform_device MSI domain for wire to MSI on top of
> this. The "regular" users of the platform MSI domain just provide their own
> irq_write_msi_msg() callback.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Anna-Maria Behnsen <[email protected]>
> Signed-off-by: Shivamurthy Shastri <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-mbi.c | 17 +----------------
> 1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-mbi.c b/drivers/irqchip/irq-gic-v3-mbi.c
> index dbb8b1efda44..19298cc6c2ee 100644
> --- a/drivers/irqchip/irq-gic-v3-mbi.c
> +++ b/drivers/irqchip/irq-gic-v3-mbi.c
> @@ -199,31 +199,16 @@ static int mbi_allocate_pci_domain(struct irq_domain *nexus_domain,
> }
> #endif
>
> -static void mbi_compose_mbi_msg(struct irq_data *data, struct msi_msg *msg)
> -{
> - mbi_compose_msi_msg(data, msg);
> -
> - msg[1].address_hi = upper_32_bits(mbi_phys_base + GICD_CLRSPI_NSR);
> - msg[1].address_lo = lower_32_bits(mbi_phys_base + GICD_CLRSPI_NSR);
> - msg[1].data = data->parent_data->hwirq;
> -
> - iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), &msg[1]);
> -}
> -
> /* Platform-MSI specific irqchip */
> static struct irq_chip mbi_pmsi_irq_chip = {
> .name = "pMSI",
> - .irq_set_type = irq_chip_set_type_parent,
> - .irq_compose_msi_msg = mbi_compose_mbi_msg,
> - .flags = IRQCHIP_SUPPORTS_LEVEL_MSI,
> };
>
> static struct msi_domain_ops mbi_pmsi_ops = {
> };
>
> static struct msi_domain_info mbi_pmsi_domain_info = {
> - .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> - MSI_FLAG_LEVEL_CAPABLE),
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> .ops = &mbi_pmsi_ops,
> .chip = &mbi_pmsi_irq_chip,
> };

This patch doesn't do what it says. It simply kills any form of level
MSI support for *endpoints*, and has nothing to do with any sort of
"wire to MSI".

What replaces it?

M.

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