On 2/12/2025 9:42 AM, Bjorn Helgaas wrote:
On Tue, Feb 11, 2025 at 05:43:21PM -0800, Roman Kisel wrote:
[...]
*/
- hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
- fn, &hv_pci_domain_ops,
- chip_data);
+#ifdef CONFIG_ACPI
+ if (!acpi_disabled)
+ hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
+ fn, &hv_pci_domain_ops,
+ chip_data);
+#endif
+#if defined(CONFIG_OF)
+ if (!hv_msi_gic_irq_domain)
+ hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
+ hv_pci_of_irq_domain_parent(), 0, HV_PCI_MSI_SPI_NR,
+ fn, &hv_pci_domain_ops,
+ chip_data);
+#endif
I don't know if acpi_irq_create_hierarchy() is helping or hurting
here. It obscures the fact that the only difference is the first
argument to irq_domain_create_hierarchy(). If we could open-code or
have a helper to figure out that irq_domain "parent" argument for the
ACPI case, then we'd only have one call of
irq_domain_create_hierarchy() here and it seems like it might be
simpler.
Hey Bjorn, folks,
I've added few ACPI maintainers and the ACPI list as we're discussing
making a small change to the ACPI subsystem to make one static variable
available to make the code above less messy.
Change [1] makes the GSI dispatcher function available to
the outside world. Would you suggest going in that direction or there
is a better approach to converge the code above that deals with IRQ
domains both in the ACPI and DT cases?
[1]
From c6fb8bda21d6c00a308b1febc201a3a7e704c5a9 Mon Sep 17 00:00:00 2001
From: Roman Kisel <romank@xxxxxxxxxxxxxxxxxxx>
Date: Wed, 19 Feb 2025 15:04:06 -0800
Subject: [PATCH] Refactor the ACPI GIC case
---
drivers/acpi/irq.c | 14 ++++++-
drivers/pci/controller/pci-hyperv.c | 62 +++++++++++++++++------------
include/linux/acpi.h | 5 ++-
3 files changed, 52 insertions(+), 29 deletions(-)
diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
index 1687483ff319..6243db610137 100644
--- a/drivers/acpi/irq.c
+++ b/drivers/acpi/irq.c
@@ -12,7 +12,7 @@
enum acpi_irq_model_id acpi_irq_model;
-static struct fwnode_handle *(*acpi_get_gsi_domain_id)(u32 gsi);
+static acpi_gsi_domain_disp_fn acpi_get_gsi_domain_id;
static u32 (*acpi_gsi_to_irq_fallback)(u32 gsi);
/**
@@ -307,12 +307,22 @@ EXPORT_SYMBOL_GPL(acpi_irq_get);
* for a given GSI
*/
void __init acpi_set_irq_model(enum acpi_irq_model_id model,
- struct fwnode_handle *(*fn)(u32))
+ acpi_gsi_domain_disp_fn fn)
{
acpi_irq_model = model;
acpi_get_gsi_domain_id = fn;
}
+/**
+ * acpi_get_gsi_dispatcher - Returns dispatcher function that
+ * computes the domain fwnode for a
+ * given GSI.
+ */
+acpi_gsi_domain_disp_fn acpi_get_gsi_dispatcher(void)
+{
+ return acpi_get_gsi_domain_id;
+}
+
/**
* acpi_set_gsi_to_irq_fallback - Register a GSI transfer
* callback to fallback to arch specified implementation.
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index 24725bea9ef1..59e670e1cb6e 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -910,16 +910,29 @@ static struct irq_domain
*hv_pci_of_irq_domain_parent(void)
of_node_put(parent);
}
- /*
- * `domain == NULL` shouldn't happen.
- *
- * If somehow the code does end up in that state, treat this as a
configuration
- * issue rather than a hard error, emit a warning, and let the code
proceed.
- * The NULL parent domain is an acceptable option for the
`irq_domain_create_hierarchy`
- * function called later.
- */
+ return domain;
+}
+
+#endif
+
+#ifdef CONFIG_ACPI
+
+static struct irq_domain *hv_pci_acpi_irq_domain_parent(void)
+{
+ struct irq_domain *domain;
+ acpi_gsi_domain_disp_fn gsi_domain_disp_fn;
+
+ if (acpi_irq_model != ACPI_IRQ_MODEL_GIC)
+ return NULL;
+ gsi_domain_disp_fn = acpi_get_gsi_dispatcher();
+ if (!gsi_domain_disp_fn)
+ return NULL;
+ domain = irq_find_matching_fwnode(gsi_domain_disp_fn(0),
+ DOMAIN_BUS_ANY);
+
if (!domain)
- WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n");
+ return NULL;
+
return domain;
}
@@ -929,6 +942,7 @@ static int hv_pci_irqchip_init(void)
{
static struct hv_pci_chip_data *chip_data;
struct fwnode_handle *fn = NULL;
+ struct irq_domain *irq_domain_parent = NULL;
int ret = -ENOMEM;
chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
@@ -944,29 +958,25 @@ static int hv_pci_irqchip_init(void)
* IRQ domain once enabled, should not be removed since there is no
* way to ensure that all the corresponding devices are also gone and
* no interrupts will be generated.
- *
- * In the ACPI case, the parent IRQ domain is supplied by the ACPI
- * subsystem, and it is the default GSI domain pointing to the GIC.
- * Neither is available outside of the ACPI subsystem, cannot avoid
- * the messy ifdef below.
- * There is apparently no such default in the OF subsystem, and
- * `hv_pci_of_irq_domain_parent` finds the parent IRQ domain that
- * points to the GIC as well.
- * None of these two cases reaches for the MSI parent domain.
*/
#ifdef CONFIG_ACPI
if (!acpi_disabled)
- hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
- fn, &hv_pci_domain_ops,
- chip_data);
+ irq_domain_parent = hv_pci_acpi_irq_domain_parent();
#endif
#if defined(CONFIG_OF)
- if (!hv_msi_gic_irq_domain)
- hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
- hv_pci_of_irq_domain_parent(), 0, HV_PCI_MSI_SPI_NR,
- fn, &hv_pci_domain_ops,
- chip_data);
+ if (!irq_domain_parent)
+ irq_domain_parent = hv_pci_of_irq_domain_parent();
#endif
+ if (!irq_domain_parent) {
+ WARN_ONCE(1, "Invalid firmware configuration for VMBus interrupts\n");
+ ret = -EINVAL;
+ goto free_chip;
+ }
+
+ hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
+ irq_domain_parent, 0, HV_PCI_MSI_SPI_NR,
+ fn, &hv_pci_domain_ops,
+ chip_data);
if (!hv_msi_gic_irq_domain) {
pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6adcd1b92b20..cd70a72c7073 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -336,8 +336,11 @@ int acpi_register_gsi (struct device *dev, u32 gsi,
int triggering, int polarity
int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
int acpi_isa_irq_to_gsi (unsigned isa_irq, u32 *gsi);
+typedef struct fwnode_handle *(*acpi_gsi_domain_disp_fn)(u32);
+
void acpi_set_irq_model(enum acpi_irq_model_id model,
- struct fwnode_handle *(*)(u32));
+ acpi_gsi_domain_disp_fn fn);
+acpi_gsi_domain_disp_fn acpi_get_gsi_dispatcher(void);
void acpi_set_gsi_to_irq_fallback(u32 (*)(u32));
struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
--
2.43.0
--
Thank you,
Roman
