Hi Marc,

On Nov 18, 2020, at 5:27 PM, Marc Zyngier [off-list ref] wrote:

Hi Chen,

On top of Bjorn's comments:

On 2020-11-17 13:42, Chen Baozi wrote:

quoted

---
drivers/acpi/irq.c          | 22 +++++++++++++++++++++-
drivers/acpi/pci_irq.c      |  6 ++++--
drivers/acpi/pci_link.c     | 17 +++++++++++++++--
include/acpi/acpi_drivers.h |  2 +-
include/linux/acpi.h        |  4 ++++
5 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
index e209081d644b..e78a44815c44 100644
--- a/drivers/acpi/irq.c
+++ b/drivers/acpi/irq.c

@@ -81,6 +81,25 @@ void acpi_unregister_gsi(u32 gsi)

}
EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
+int acpi_register_irq(struct device *dev, u32 irq, int trigger,
+		      int polarity, struct fwnode_handle *domain_id)
+{
+	struct irq_fwspec fwspec;
+
+	if (WARN_ON(!domain_id)) {
+		pr_warn("GSI: No registered irqchip, giving up\n");

A fwnode_handle is not an irqchip. It's just an opaque identifier
for a HW block. Furthermore, there is no need to have both a WARN_ON()
and a pr_warn(). Please pick one.

I'd also suggest you rename domain_id to fwnode, which is the commonly
used idiom (yes, I know about the unfortunate precedent in acpi_register_gsi()).

quoted

+		return -EINVAL;
+	}
+
+	fwspec.fwnode = domain_id;
+	fwspec.param[0] = irq;
+	fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
+	fwspec.param_count = 2;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+EXPORT_SYMBOL_GPL(acpi_register_irq);

By the way, this is almost an exact duplicate of acpi_register_gsi().
You definitely want to make this code common.

quoted

@@ -115,6 +134,7 @@ acpi_get_irq_source_fwhandle(const struct

acpi_resource_source *source)
	acpi_bus_put_acpi_device(device);
	return result;
}
+EXPORT_SYMBOL_GPL(acpi_get_irq_source_fwhandle);
/*
 * Context for the resource walk used to lookup IRQ resources.

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 14ee631cb7cf..19296d70c95c 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c

@@ -410,6 +410,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)

	char *link = NULL;
	char link_desc[16];
	int rc;
+	struct fwnode_handle *irq_domain;

fwnode_handle is most definitely not an IRQ domain.

quoted

@@ -140,6 +143,12 @@ static acpi_status

acpi_pci_link_check_possible(struct acpi_resource *resource,
			link->irq.triggering = p->triggering;
			link->irq.polarity = p->polarity;
			link->irq.resource_type = ACPI_RESOURCE_TYPE_EXTENDED_IRQ;
+			if (p->resource_source.string_length) {
+				rs->index = p->resource_source.index;
+				rs->string_length = p->resource_source.string_length;
+				rs->string_ptr = kmalloc(rs->string_length, GFP_KERNEL);
+				strcpy(rs->string_ptr, p->resource_source.string_ptr);

We have kstrdup() for this kind of things, as using rs->string_length to allocate
the buffer and strcpy() to copy it feels... dangerous.

quoted

+			}
			break;
		}
	default:

@@ -612,7 +622,7 @@ static int acpi_pci_link_allocate(struct

acpi_pci_link *link)
 * failure: return -1
 */
int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
-			       int *polarity, char **name)
+			       int *polarity, char **name, struct fwnode_handle **irq_domain)

Same remark about the naming.

Thanks. It is very helpful. I’ll fix it in next version.

Baozi.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel