Hi Marc,
> On Nov 18, 2020, at 5:27 PM, Marc Zyngier <m...@kernel.org> wrote:
>
> Hi Chen,
>
> On top of Bjorn's comments:
>
> On 2020-11-17 13:42, Chen Baozi wrote:
>>
>> ---
>> 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()).
>
>> + 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.
>
>> @@ -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.
>
>> @@ -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.
>
>> + }
>> 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.
