Re: [RFC PATCH V2] acpi/irq: Add stacked IRQ domain support to PCI interrupt link

[Chen Baozi <chenbaozi@phytium.com.cn>]

Hi Marc,

> On Nov 18, 2020, at 5:27 PM, Marc Zyngier <maz@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.