[v3,11/14] ACPI: irq: introduce interrupt producer

Commit Message

Hanjun Guo Oct. 25, 2016, 3:09 p.m. UTC

From: Hanjun Guo <[e-mail]>

In ACPI 6.1 spec, section 19.6.62, Interrupt Resource Descriptor Macro,

Interrupt (ResourceUsage, EdgeLevel, ActiveLevel, Shared,
ResourceSourceIndex, ResourceSource, DescriptorName)
{ InterruptList } => Buffer

For the arguement ResourceUsage and DescriptorName, which means:

ResourceUsage describes whether the device consumes the specified
interrupt ( ResourceConsumer ) or produces it for use by a child
device ( ResourceProducer ).
If nothing is specified, then ResourceConsumer is assumed.

DescriptorName evaluates to a name string which refers to the
entire resource descriptor.

So it can be used for devices connecting to a specific interrupt
prodcucer instead of the main interrupt controller in MADT. In the
real world, we have irqchip such as mbi-gen which connecting to
a group of wired interrupts and then issue msi to the ITS, devices
connecting to such interrupt controller fit this scope.

For now the irq for ACPI only pointer to the main interrupt
controller's irqdomain, for devices not connecting to those
irqdomains, which need to present its irq parent, we can use
following ASL code to represent it:

Interrupt(ResourceConsumer,..., "\_SB.IRQP") {12,14,....}

then we can parse the interrupt producer with the full
path name "\_SB.IRQP".

In order to do that, we introduce a pointer interrupt_producer
in struct acpi_device, and fill it when scanning irq resources
for acpi device if it specifies the interrupt producer.

But for now when parsing the resources for acpi devices, we don't
pass the acpi device for acpi_walk_resoures() in drivers/acpi/resource.c,
so introduce a adev in struct res_proc_context to pass it as a context
to scan the interrupt resources, then finally pass to acpi_register_gsi()
to find its interrupt producer to get the virq from diffrent domains.

With steps above ready, rework acpi_register_gsi() to get other
interrupt producer if devices not connecting to main interrupt
controller.

Since we often pass NULL to acpi_register_gsi() and there is no interrupt
producer for devices connect to gicd on ARM or io-apic on X86, so it will
use the default irqdomain for those deivces and no functional changes to
those devices.

Signed-off-by: Hanjun Guo <[e-mail]>
Cc: Rafael J. Wysocki <[e-mail]>
Cc: Marc Zyngier <[e-mail]>
Cc: Agustin Vega-Frias <[e-mail]>
Cc: Lorenzo Pieralisi <[e-mail]>
---
 drivers/acpi/gsi.c      | 10 ++++--
 drivers/acpi/resource.c | 85 ++++++++++++++++++++++++++++++++++---------------
 include/acpi/acpi_bus.h |  1 +
 3 files changed, 68 insertions(+), 28 deletions(-)

Comments

Agustin Vega-Frias Oct. 28, 2016, 7:32 p.m. UTC | #1

Hey Hanjun,

On 2016-10-25 11:09, Hanjun Guo wrote:
> From: Hanjun Guo <[e-mail]>
> 
> In ACPI 6.1 spec, section 19.6.62, Interrupt Resource Descriptor Macro,
> 
> Interrupt (ResourceUsage, EdgeLevel, ActiveLevel, Shared,
> ResourceSourceIndex, ResourceSource, DescriptorName)
> { InterruptList } => Buffer
> 
> For the arguement ResourceUsage and DescriptorName, which means:
> 
> ResourceUsage describes whether the device consumes the specified
> interrupt ( ResourceConsumer ) or produces it for use by a child
> device ( ResourceProducer ).
> If nothing is specified, then ResourceConsumer is assumed.
> 
> DescriptorName evaluates to a name string which refers to the
> entire resource descriptor.
> 
> So it can be used for devices connecting to a specific interrupt
> prodcucer instead of the main interrupt controller in MADT. In the
> real world, we have irqchip such as mbi-gen which connecting to
> a group of wired interrupts and then issue msi to the ITS, devices
> connecting to such interrupt controller fit this scope.
> 
> For now the irq for ACPI only pointer to the main interrupt
> controller's irqdomain, for devices not connecting to those
> irqdomains, which need to present its irq parent, we can use
> following ASL code to represent it:
> 
> Interrupt(ResourceConsumer,..., "\_SB.IRQP") {12,14,....}
> 
> then we can parse the interrupt producer with the full
> path name "\_SB.IRQP".
> 
> In order to do that, we introduce a pointer interrupt_producer
> in struct acpi_device, and fill it when scanning irq resources
> for acpi device if it specifies the interrupt producer.
> 
> But for now when parsing the resources for acpi devices, we don't
> pass the acpi device for acpi_walk_resoures() in 
> drivers/acpi/resource.c,
> so introduce a adev in struct res_proc_context to pass it as a context
> to scan the interrupt resources, then finally pass to 
> acpi_register_gsi()
> to find its interrupt producer to get the virq from diffrent domains.
> 
> With steps above ready, rework acpi_register_gsi() to get other
> interrupt producer if devices not connecting to main interrupt
> controller.
> 
> Since we often pass NULL to acpi_register_gsi() and there is no 
> interrupt
> producer for devices connect to gicd on ARM or io-apic on X86, so it 
> will
> use the default irqdomain for those deivces and no functional changes 
> to
> those devices.
> 
> Signed-off-by: Hanjun Guo <[e-mail]>
> Cc: Rafael J. Wysocki <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>
> Cc: Agustin Vega-Frias <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>
> ---
>  drivers/acpi/gsi.c      | 10 ++++--
>  drivers/acpi/resource.c | 85 
> ++++++++++++++++++++++++++++++++++---------------
>  include/acpi/acpi_bus.h |  1 +
>  3 files changed, 68 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
> index ee9e0f2..29ee547 100644
> --- a/drivers/acpi/gsi.c
> +++ b/drivers/acpi/gsi.c
> @@ -55,13 +55,19 @@ int acpi_register_gsi(struct device *dev, u32 gsi,
> int trigger,
>  		      int polarity)
>  {
>  	struct irq_fwspec fwspec;
> +	struct acpi_device *adev = dev ? to_acpi_device(dev) : NULL;
> 
> -	if (WARN_ON(!acpi_gsi_domain_id)) {
> +	if (adev && &adev->fwnode && adev->interrupt_producer)
> +		/* devices in DSDT connecting to spefic interrupt producer */
> +		fwspec.fwnode = adev->interrupt_producer;
> +	else if (acpi_gsi_domain_id)
> +		/* devices connecting to gicd in default */
> +		fwspec.fwnode = acpi_gsi_domain_id;
> +	else {
>  		pr_warn("GSI: No registered irqchip, giving up\n");
>  		return -EINVAL;
>  	}
> 
> -	fwspec.fwnode = acpi_gsi_domain_id;
>  	fwspec.param[0] = gsi;
>  	fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
>  	fwspec.param_count = 2;
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 56241eb..f1371cf 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -381,7 +381,7 @@ static void acpi_dev_irqresource_disabled(struct
> resource *res, u32 gsi)
>  	res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
>  }
> 
> -static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> +static void acpi_dev_get_irqresource(struct acpi_device *adev, struct
> resource *res, u32 gsi,
>  				     u8 triggering, u8 polarity, u8 shareable,
>  				     bool legacy)
>  {
> @@ -415,7 +415,7 @@ static void acpi_dev_get_irqresource(struct
> resource *res, u32 gsi,
>  	}
> 
>  	res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
> -	irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
> +	irq = acpi_register_gsi(&adev->dev, gsi, triggering, polarity);
>  	if (irq >= 0) {
>  		res->start = irq;
>  		res->end = irq;
> @@ -424,27 +424,9 @@ static void acpi_dev_get_irqresource(struct
> resource *res, u32 gsi,
>  	}
>  }
> 
> -/**
> - * acpi_dev_resource_interrupt - Extract ACPI interrupt resource 
> information.
> - * @ares: Input ACPI resource object.
> - * @index: Index into the array of GSIs represented by the resource.
> - * @res: Output generic resource object.
> - *
> - * Check if the given ACPI resource object represents an interrupt 
> resource
> - * and @index does not exceed the resource's interrupt count (true is 
> returned
> - * in that case regardless of the results of the other checks)).  If 
> that's the
> - * case, register the GSI corresponding to @index from the array of 
> interrupts
> - * represented by the resource and populate the generic resource 
> object pointed
> - * to by @res accordingly.  If the registration of the GSI is not 
> successful,
> - * IORESOURCE_DISABLED will be set it that object's flags.
> - *
> - * Return:
> - * 1) false with res->flags setting to zero: not the expected resource 
> type
> - * 2) false with IORESOURCE_DISABLED in res->flags: valid unassigned 
> resource
> - * 3) true: valid assigned resource
> - */
> -bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int 
> index,
> -				 struct resource *res)
> +static bool __acpi_dev_resource_interrupt(struct acpi_device *adev,
> +					  struct acpi_resource *ares, int index,
> +					  struct resource *res)
>  {
>  	struct acpi_resource_irq *irq;
>  	struct acpi_resource_extended_irq *ext_irq;
> @@ -460,7 +442,7 @@ bool acpi_dev_resource_interrupt(struct
> acpi_resource *ares, int index,
>  			acpi_dev_irqresource_disabled(res, 0);
>  			return false;
>  		}
> -		acpi_dev_get_irqresource(res, irq->interrupts[index],
> +		acpi_dev_get_irqresource(adev, res, irq->interrupts[index],
>  					 irq->triggering, irq->polarity,
>  					 irq->sharable, true);
>  		break;
> @@ -470,7 +452,31 @@ bool acpi_dev_resource_interrupt(struct
> acpi_resource *ares, int index,
>  			acpi_dev_irqresource_disabled(res, 0);
>  			return false;
>  		}
> -		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
> +
> +		/*
> +		 * It's a interrupt consumer device and connecting to specfic
> +		 * interrupt controller. For now, we only support connecting
> +		 * interrupts to one irq controller for a single device
> +		 */
> +		if (ext_irq->producer_consumer == ACPI_CONSUMER
> +		    && ext_irq->resource_source.string_length != 0
> +		    && !adev->interrupt_producer) {
> +			acpi_status status;
> +			acpi_handle handle;
> +			struct acpi_device *device;
> +
> +			status = acpi_get_handle(NULL,
> ext_irq->resource_source.string_ptr, &handle);
> +			if (ACPI_FAILURE(status))
> +				return false;
> +
> +			device = acpi_bus_get_acpi_device(handle);
> +			if (!device)
> +				return false;
> +
> +			adev->interrupt_producer = &device->fwnode;

You are missing an 'acpi_bus_put_acpi_device(device)' call here.

Besides that, this approach will not work in the case where a device
wants to consume interrupts from multiple controllers since you are
forcing adev->interrupt_producer to be the first resource_source
found.

That's the reason I was advocating dynamic lookup (see [1]).

I am about to submit V6 of my series where I also address the probe
ordering issues by enabling re-initialization of platform_device
resources when the resource was marked disabled due to the domain
nor being there during ACPI bus scan.

Thanks,
Agustin

[1] https://lkml.org/lkml/2016/10/18/592

> +		}
> +
> +		acpi_dev_get_irqresource(adev, res, ext_irq->interrupts[index],
>  					 ext_irq->triggering, ext_irq->polarity,
>  					 ext_irq->sharable, false);
>  		break;
> @@ -481,6 +487,31 @@ bool acpi_dev_resource_interrupt(struct
> acpi_resource *ares, int index,
> 
>  	return true;
>  }
> +
> +/**
> + * acpi_dev_resource_interrupt - Extract ACPI interrupt resource 
> information.
> + * @ares: Input ACPI resource object.
> + * @index: Index into the array of GSIs represented by the resource.
> + * @res: Output generic resource object.
> + *
> + * Check if the given ACPI resource object represents an interrupt 
> resource
> + * and @index does not exceed the resource's interrupt count (true is 
> returned
> + * in that case regardless of the results of the other checks)).  If 
> that's the
> + * case, register the GSI corresponding to @index from the array of 
> interrupts
> + * represented by the resource and populate the generic resource 
> object pointed
> + * to by @res accordingly.  If the registration of the GSI is not 
> successful,
> + * IORESOURCE_DISABLED will be set it that object's flags.
> + *
> + * Return:
> + * 1) false with res->flags setting to zero: not the expected resource 
> type
> + * 2) false with IORESOURCE_DISABLED in res->flags: valid unassigned 
> resource
> + * 3) true: valid assigned resource
> + */
> +bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int 
> index,
> +				 struct resource *res)
> +{
> +	return __acpi_dev_resource_interrupt(NULL, ares, index, res);
> +}
>  EXPORT_SYMBOL_GPL(acpi_dev_resource_interrupt);
> 
>  /**
> @@ -499,6 +530,7 @@ struct res_proc_context {
>  	void *preproc_data;
>  	int count;
>  	int error;
> +	struct acpi_device *adev;
>  };
> 
>  static acpi_status acpi_dev_new_resource_entry(struct resource_win 
> *win,
> @@ -546,7 +578,7 @@ static acpi_status
> acpi_dev_process_resource(struct acpi_resource *ares,
>  	    || acpi_dev_resource_ext_address_space(ares, &win))
>  		return acpi_dev_new_resource_entry(&win, c);
> 
> -	for (i = 0; acpi_dev_resource_interrupt(ares, i, res); i++) {
> +	for (i = 0; __acpi_dev_resource_interrupt(c->adev, ares, i, res); 
> i++) {
>  		acpi_status status;
> 
>  		status = acpi_dev_new_resource_entry(&win, c);
> @@ -599,6 +631,7 @@ int acpi_dev_get_resources(struct acpi_device
> *adev, struct list_head *list,
>  	c.preproc_data = preproc_data;
>  	c.count = 0;
>  	c.error = 0;
> +	c.adev = adev;
>  	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
>  				     acpi_dev_process_resource, &c);
>  	if (ACPI_FAILURE(status)) {
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 4242c31..5410d3b 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -355,6 +355,7 @@ struct acpi_device {
>  	int device_type;
>  	acpi_handle handle;		/* no handle for fixed hardware */
>  	struct fwnode_handle fwnode;
> +	struct fwnode_handle *interrupt_producer;
>  	struct acpi_device *parent;
>  	struct list_head children;
>  	struct list_head node;
> --
> 1.7.12.4

Hanjun Guo Nov. 2, 2016, 9:09 p.m. UTC | #2

Hi Agustin,

Sorry for the late reply, on travailing for now.

On 10/29/2016 03:32 AM, [e-mail] wrote:
> Hey Hanjun,
>
> On 2016-10-25 11:09, Hanjun Guo wrote:
>> From: Hanjun Guo <[e-mail]>
>>
>> In ACPI 6.1 spec, section 19.6.62, Interrupt Resource Descriptor Macro,
>>
>> Interrupt (ResourceUsage, EdgeLevel, ActiveLevel, Shared,
>> ResourceSourceIndex, ResourceSource, DescriptorName)
>> { InterruptList } => Buffer
>>
>> For the arguement ResourceUsage and DescriptorName, which means:
>>
>> ResourceUsage describes whether the device consumes the specified
>> interrupt ( ResourceConsumer ) or produces it for use by a child
>> device ( ResourceProducer ).
>> If nothing is specified, then ResourceConsumer is assumed.
>>
>> DescriptorName evaluates to a name string which refers to the
>> entire resource descriptor.
>>
>> So it can be used for devices connecting to a specific interrupt
>> prodcucer instead of the main interrupt controller in MADT. In the
>> real world, we have irqchip such as mbi-gen which connecting to
>> a group of wired interrupts and then issue msi to the ITS, devices
>> connecting to such interrupt controller fit this scope.
>>
>> For now the irq for ACPI only pointer to the main interrupt
>> controller's irqdomain, for devices not connecting to those
>> irqdomains, which need to present its irq parent, we can use
>> following ASL code to represent it:
>>
>> Interrupt(ResourceConsumer,..., "\_SB.IRQP") {12,14,....}
>>
>> then we can parse the interrupt producer with the full
>> path name "\_SB.IRQP".
>>
>> In order to do that, we introduce a pointer interrupt_producer
>> in struct acpi_device, and fill it when scanning irq resources
>> for acpi device if it specifies the interrupt producer.
>>
>> But for now when parsing the resources for acpi devices, we don't
>> pass the acpi device for acpi_walk_resoures() in drivers/acpi/resource.c,
>> so introduce a adev in struct res_proc_context to pass it as a context
>> to scan the interrupt resources, then finally pass to acpi_register_gsi()
>> to find its interrupt producer to get the virq from diffrent domains.
>>
>> With steps above ready, rework acpi_register_gsi() to get other
>> interrupt producer if devices not connecting to main interrupt
>> controller.
>>
>> Since we often pass NULL to acpi_register_gsi() and there is no interrupt
>> producer for devices connect to gicd on ARM or io-apic on X86, so it will
>> use the default irqdomain for those deivces and no functional changes to
>> those devices.
>>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> Cc: Rafael J. Wysocki <[e-mail]>
>> Cc: Marc Zyngier <[e-mail]>
>> Cc: Agustin Vega-Frias <[e-mail]>
>> Cc: Lorenzo Pieralisi <[e-mail]>
>> ---
>>  drivers/acpi/gsi.c      | 10 ++++--
>>  drivers/acpi/resource.c | 85
>> ++++++++++++++++++++++++++++++++++---------------
>>  include/acpi/acpi_bus.h |  1 +
>>  3 files changed, 68 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
>> index ee9e0f2..29ee547 100644
>> --- a/drivers/acpi/gsi.c
>> +++ b/drivers/acpi/gsi.c
>> @@ -55,13 +55,19 @@ int acpi_register_gsi(struct device *dev, u32 gsi,
>> int trigger,
>>                int polarity)
>>  {
>>      struct irq_fwspec fwspec;
>> +    struct acpi_device *adev = dev ? to_acpi_device(dev) : NULL;
>>
>> -    if (WARN_ON(!acpi_gsi_domain_id)) {
>> +    if (adev && &adev->fwnode && adev->interrupt_producer)
>> +        /* devices in DSDT connecting to spefic interrupt producer */
>> +        fwspec.fwnode = adev->interrupt_producer;
>> +    else if (acpi_gsi_domain_id)
>> +        /* devices connecting to gicd in default */
>> +        fwspec.fwnode = acpi_gsi_domain_id;
>> +    else {
>>          pr_warn("GSI: No registered irqchip, giving up\n");
>>          return -EINVAL;
>>      }
>>
>> -    fwspec.fwnode = acpi_gsi_domain_id;
>>      fwspec.param[0] = gsi;
>>      fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
>>      fwspec.param_count = 2;
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index 56241eb..f1371cf 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -381,7 +381,7 @@ static void acpi_dev_irqresource_disabled(struct
>> resource *res, u32 gsi)
>>      res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED |
>> IORESOURCE_UNSET;
>>  }
>>
>> -static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
>> +static void acpi_dev_get_irqresource(struct acpi_device *adev, struct
>> resource *res, u32 gsi,
>>                       u8 triggering, u8 polarity, u8 shareable,
>>                       bool legacy)
>>  {
>> @@ -415,7 +415,7 @@ static void acpi_dev_get_irqresource(struct
>> resource *res, u32 gsi,
>>      }
>>
>>      res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
>> -    irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
>> +    irq = acpi_register_gsi(&adev->dev, gsi, triggering, polarity);
>>      if (irq >= 0) {
>>          res->start = irq;
>>          res->end = irq;
>> @@ -424,27 +424,9 @@ static void acpi_dev_get_irqresource(struct
>> resource *res, u32 gsi,
>>      }
>>  }
>>
>> -/**
>> - * acpi_dev_resource_interrupt - Extract ACPI interrupt resource
>> information.
>> - * @ares: Input ACPI resource object.
>> - * @index: Index into the array of GSIs represented by the resource.
>> - * @res: Output generic resource object.
>> - *
>> - * Check if the given ACPI resource object represents an interrupt
>> resource
>> - * and @index does not exceed the resource's interrupt count (true is
>> returned
>> - * in that case regardless of the results of the other checks)).  If
>> that's the
>> - * case, register the GSI corresponding to @index from the array of
>> interrupts
>> - * represented by the resource and populate the generic resource
>> object pointed
>> - * to by @res accordingly.  If the registration of the GSI is not
>> successful,
>> - * IORESOURCE_DISABLED will be set it that object's flags.
>> - *
>> - * Return:
>> - * 1) false with res->flags setting to zero: not the expected
>> resource type
>> - * 2) false with IORESOURCE_DISABLED in res->flags: valid unassigned
>> resource
>> - * 3) true: valid assigned resource
>> - */
>> -bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>> -                 struct resource *res)
>> +static bool __acpi_dev_resource_interrupt(struct acpi_device *adev,
>> +                      struct acpi_resource *ares, int index,
>> +                      struct resource *res)
>>  {
>>      struct acpi_resource_irq *irq;
>>      struct acpi_resource_extended_irq *ext_irq;
>> @@ -460,7 +442,7 @@ bool acpi_dev_resource_interrupt(struct
>> acpi_resource *ares, int index,
>>              acpi_dev_irqresource_disabled(res, 0);
>>              return false;
>>          }
>> -        acpi_dev_get_irqresource(res, irq->interrupts[index],
>> +        acpi_dev_get_irqresource(adev, res, irq->interrupts[index],
>>                       irq->triggering, irq->polarity,
>>                       irq->sharable, true);
>>          break;
>> @@ -470,7 +452,31 @@ bool acpi_dev_resource_interrupt(struct
>> acpi_resource *ares, int index,
>>              acpi_dev_irqresource_disabled(res, 0);
>>              return false;
>>          }
>> -        acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
>> +
>> +        /*
>> +         * It's a interrupt consumer device and connecting to specfic
>> +         * interrupt controller. For now, we only support connecting
>> +         * interrupts to one irq controller for a single device
>> +         */
>> +        if (ext_irq->producer_consumer == ACPI_CONSUMER
>> +            && ext_irq->resource_source.string_length != 0
>> +            && !adev->interrupt_producer) {
>> +            acpi_status status;
>> +            acpi_handle handle;
>> +            struct acpi_device *device;
>> +
>> +            status = acpi_get_handle(NULL,
>> ext_irq->resource_source.string_ptr, &handle);
>> +            if (ACPI_FAILURE(status))
>> +                return false;
>> +
>> +            device = acpi_bus_get_acpi_device(handle);
>> +            if (!device)
>> +                return false;
>> +
>> +            adev->interrupt_producer = &device->fwnode;
>
> You are missing an 'acpi_bus_put_acpi_device(device)' call here.

good catch!

>
> Besides that, this approach will not work in the case where a device
> wants to consume interrupts from multiple controllers since you are
> forcing adev->interrupt_producer to be the first resource_source
> found.

Yes, you are right, and it's in the comment of the code, we
can fix that to add some extra code.

>
> That's the reason I was advocating dynamic lookup (see [1]).
>
> I am about to submit V6 of my series where I also address the probe
> ordering issues by enabling re-initialization of platform_device
> resources when the resource was marked disabled due to the domain
> nor being there during ACPI bus scan.

I will take a deep look for your v6 patch set, probably we can
work together to get things down.

Thanks
Hanjun

Aleksey Makarov Dec. 1, 2016, 11:12 a.m. UTC | #3

Hi Hanjun,

On 10/25/2016 09:09 PM, Hanjun Guo wrote:
> From: Hanjun Guo <[e-mail]>
>
> In ACPI 6.1 spec, section 19.6.62, Interrupt Resource Descriptor Macro,

[ ... ]

> ---
>  drivers/acpi/gsi.c      | 10 ++++--
>  drivers/acpi/resource.c | 85 ++++++++++++++++++++++++++++++++++---------------
>  include/acpi/acpi_bus.h |  1 +
>  3 files changed, 68 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
> index ee9e0f2..29ee547 100644
> --- a/drivers/acpi/gsi.c
> +++ b/drivers/acpi/gsi.c
> @@ -55,13 +55,19 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
>  		      int polarity)
>  {
>  	struct irq_fwspec fwspec;
> +	struct acpi_device *adev = dev ? to_acpi_device(dev) : NULL;

Why are you sure dev is always an acpi device?
Look for example at drivers/acpi/pci_irq.c:377 where this function
is called for a PCI device

>
> -	if (WARN_ON(!acpi_gsi_domain_id)) {
> +	if (adev && &adev->fwnode && adev->interrupt_producer)

&adev->fwnode is always true

> +		/* devices in DSDT connecting to spefic interrupt producer */
> +		fwspec.fwnode = adev->interrupt_producer;
> +	else if (acpi_gsi_domain_id)
> +		/* devices connecting to gicd in default */
> +		fwspec.fwnode = acpi_gsi_domain_id;
> +	else {
>  		pr_warn("GSI: No registered irqchip, giving up\n");
>  		return -EINVAL;
>  	}

[ ... ]

All the best
Aleksey Makarov

Hanjun Guo Dec. 2, 2016, 10:07 a.m. UTC | #4

Hi Aleksey,

On 2016/12/1 19:12, Aleksey Makarov wrote:
>
> Hi Hanjun,
>
> On 10/25/2016 09:09 PM, Hanjun Guo wrote:
>> From: Hanjun Guo <[e-mail]>
>>
>> In ACPI 6.1 spec, section 19.6.62, Interrupt Resource Descriptor Macro,
>
> [ ... ]
>
>> ---
>>  drivers/acpi/gsi.c      | 10 ++++--
>>  drivers/acpi/resource.c | 85
>> ++++++++++++++++++++++++++++++++++---------------
>>  include/acpi/acpi_bus.h |  1 +
>>  3 files changed, 68 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
>> index ee9e0f2..29ee547 100644
>> --- a/drivers/acpi/gsi.c
>> +++ b/drivers/acpi/gsi.c
>> @@ -55,13 +55,19 @@ int acpi_register_gsi(struct device *dev, u32 gsi,
>> int trigger,
>>                int polarity)
>>  {
>>      struct irq_fwspec fwspec;
>> +    struct acpi_device *adev = dev ? to_acpi_device(dev) : NULL;
>
> Why are you sure dev is always an acpi device?
> Look for example at drivers/acpi/pci_irq.c:377 where this function
> is called for a PCI device

Good catch, but I will drop this patch and use Agustin's one [1].

[1]: https://mail-archive.com/[e-mail]/msg1283116.html

Thanks
Hanjun

Graeme Gregory Dec. 3, 2016, 12:18 p.m. UTC | #5

On Tue, Oct 25, 2016 at 11:09:26PM +0800, Hanjun Guo wrote:
> From: Hanjun Guo <[e-mail]>
> 
> In ACPI 6.1 spec, section 19.6.62, Interrupt Resource Descriptor Macro,
> 
> Interrupt (ResourceUsage, EdgeLevel, ActiveLevel, Shared,
> ResourceSourceIndex, ResourceSource, DescriptorName)
> { InterruptList } => Buffer
> 
> For the arguement ResourceUsage and DescriptorName, which means:
> 
> ResourceUsage describes whether the device consumes the specified
> interrupt ( ResourceConsumer ) or produces it for use by a child
> device ( ResourceProducer ).
> If nothing is specified, then ResourceConsumer is assumed.
> 
> DescriptorName evaluates to a name string which refers to the
> entire resource descriptor.
> 
> So it can be used for devices connecting to a specific interrupt
> prodcucer instead of the main interrupt controller in MADT. In the
> real world, we have irqchip such as mbi-gen which connecting to
> a group of wired interrupts and then issue msi to the ITS, devices
> connecting to such interrupt controller fit this scope.
> 
> For now the irq for ACPI only pointer to the main interrupt
> controller's irqdomain, for devices not connecting to those
> irqdomains, which need to present its irq parent, we can use
> following ASL code to represent it:
> 
> Interrupt(ResourceConsumer,..., "\_SB.IRQP") {12,14,....}
> 
> then we can parse the interrupt producer with the full
> path name "\_SB.IRQP".
> 
> In order to do that, we introduce a pointer interrupt_producer
> in struct acpi_device, and fill it when scanning irq resources
> for acpi device if it specifies the interrupt producer.
> 
> But for now when parsing the resources for acpi devices, we don't
> pass the acpi device for acpi_walk_resoures() in drivers/acpi/resource.c,
> so introduce a adev in struct res_proc_context to pass it as a context
> to scan the interrupt resources, then finally pass to acpi_register_gsi()
> to find its interrupt producer to get the virq from diffrent domains.
> 
> With steps above ready, rework acpi_register_gsi() to get other
> interrupt producer if devices not connecting to main interrupt
> controller.
> 
> Since we often pass NULL to acpi_register_gsi() and there is no interrupt
> producer for devices connect to gicd on ARM or io-apic on X86, so it will
> use the default irqdomain for those deivces and no functional changes to
> those devices.
> 
> Signed-off-by: Hanjun Guo <[e-mail]>
> Cc: Rafael J. Wysocki <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>
> Cc: Agustin Vega-Frias <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>
> ---
>  drivers/acpi/gsi.c      | 10 ++++--
>  drivers/acpi/resource.c | 85 ++++++++++++++++++++++++++++++++++---------------
>  include/acpi/acpi_bus.h |  1 +
>  3 files changed, 68 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
> index ee9e0f2..29ee547 100644
> --- a/drivers/acpi/gsi.c
> +++ b/drivers/acpi/gsi.c
> @@ -55,13 +55,19 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
>  		      int polarity)
>  {
>  	struct irq_fwspec fwspec;
> +	struct acpi_device *adev = dev ? to_acpi_device(dev) : NULL;
>  
> -	if (WARN_ON(!acpi_gsi_domain_id)) {
> +	if (adev && &adev->fwnode && adev->interrupt_producer)
> +		/* devices in DSDT connecting to spefic interrupt producer */
> +		fwspec.fwnode = adev->interrupt_producer;
> +	else if (acpi_gsi_domain_id)
> +		/* devices connecting to gicd in default */
> +		fwspec.fwnode = acpi_gsi_domain_id;
> +	else {
>  		pr_warn("GSI: No registered irqchip, giving up\n");
>  		return -EINVAL;
>  	}
>  
> -	fwspec.fwnode = acpi_gsi_domain_id;
>  	fwspec.param[0] = gsi;
>  	fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
>  	fwspec.param_count = 2;
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 56241eb..f1371cf 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -381,7 +381,7 @@ static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
>  	res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
>  }
>  
> -static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> +static void acpi_dev_get_irqresource(struct acpi_device *adev, struct resource *res, u32 gsi,
>  				     u8 triggering, u8 polarity, u8 shareable,
>  				     bool legacy)
>  {
> @@ -415,7 +415,7 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
>  	}
>  
>  	res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
> -	irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
> +	irq = acpi_register_gsi(&adev->dev, gsi, triggering, polarity);
>  	if (irq >= 0) {
>  		res->start = irq;
>  		res->end = irq;
> @@ -424,27 +424,9 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
>  	}
>  }
>  
> -/**
> - * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information.
> - * @ares: Input ACPI resource object.
> - * @index: Index into the array of GSIs represented by the resource.
> - * @res: Output generic resource object.
> - *
> - * Check if the given ACPI resource object represents an interrupt resource
> - * and @index does not exceed the resource's interrupt count (true is returned
> - * in that case regardless of the results of the other checks)).  If that's the
> - * case, register the GSI corresponding to @index from the array of interrupts
> - * represented by the resource and populate the generic resource object pointed
> - * to by @res accordingly.  If the registration of the GSI is not successful,
> - * IORESOURCE_DISABLED will be set it that object's flags.
> - *
> - * Return:
> - * 1) false with res->flags setting to zero: not the expected resource type
> - * 2) false with IORESOURCE_DISABLED in res->flags: valid unassigned resource
> - * 3) true: valid assigned resource
> - */
> -bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> -				 struct resource *res)
> +static bool __acpi_dev_resource_interrupt(struct acpi_device *adev,
> +					  struct acpi_resource *ares, int index,
> +					  struct resource *res)
>  {
>  	struct acpi_resource_irq *irq;
>  	struct acpi_resource_extended_irq *ext_irq;
> @@ -460,7 +442,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>  			acpi_dev_irqresource_disabled(res, 0);
>  			return false;
>  		}
> -		acpi_dev_get_irqresource(res, irq->interrupts[index],
> +		acpi_dev_get_irqresource(adev, res, irq->interrupts[index],
>  					 irq->triggering, irq->polarity,
>  					 irq->sharable, true);
>  		break;
> @@ -470,7 +452,31 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>  			acpi_dev_irqresource_disabled(res, 0);
>  			return false;
>  		}
> -		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
> +
> +		/*
> +		 * It's a interrupt consumer device and connecting to specfic
> +		 * interrupt controller. For now, we only support connecting
> +		 * interrupts to one irq controller for a single device
> +		 */
> +		if (ext_irq->producer_consumer == ACPI_CONSUMER
> +		    && ext_irq->resource_source.string_length != 0
> +		    && !adev->interrupt_producer) {
> +			acpi_status status;
> +			acpi_handle handle;
> +			struct acpi_device *device;
> +
> +			status = acpi_get_handle(NULL, ext_irq->resource_source.string_ptr, &handle);
> +			if (ACPI_FAILURE(status))
> +				return false;
> +
> +			device = acpi_bus_get_acpi_device(handle);
> +			if (!device)
> +				return false;
> +
> +			adev->interrupt_producer = &device->fwnode;
> +		}
> +
> +		acpi_dev_get_irqresource(adev, res, ext_irq->interrupts[index],
>  					 ext_irq->triggering, ext_irq->polarity,
>  					 ext_irq->sharable, false);
>  		break;
> @@ -481,6 +487,31 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>  
>  	return true;
>  }
> +
> +/**
> + * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information.
> + * @ares: Input ACPI resource object.
> + * @index: Index into the array of GSIs represented by the resource.
> + * @res: Output generic resource object.
> + *
> + * Check if the given ACPI resource object represents an interrupt resource
> + * and @index does not exceed the resource's interrupt count (true is returned
> + * in that case regardless of the results of the other checks)).  If that's the
> + * case, register the GSI corresponding to @index from the array of interrupts
> + * represented by the resource and populate the generic resource object pointed
> + * to by @res accordingly.  If the registration of the GSI is not successful,
> + * IORESOURCE_DISABLED will be set it that object's flags.
> + *
> + * Return:
> + * 1) false with res->flags setting to zero: not the expected resource type
> + * 2) false with IORESOURCE_DISABLED in res->flags: valid unassigned resource
> + * 3) true: valid assigned resource
> + */
> +bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> +				 struct resource *res)
> +{
> +	return __acpi_dev_resource_interrupt(NULL, ares, index, res);
> +}
>  EXPORT_SYMBOL_GPL(acpi_dev_resource_interrupt);

You cannot do this as &adev->dev is deferenced in acpi_dev_get_irqresource
without a check that adev is NULL, and so passes an offset from NULL to
acpi_register_gsi() which breaks everything.

Graeme

>  
>  /**
> @@ -499,6 +530,7 @@ struct res_proc_context {
>  	void *preproc_data;
>  	int count;
>  	int error;
> +	struct acpi_device *adev;
>  };
>  
>  static acpi_status acpi_dev_new_resource_entry(struct resource_win *win,
> @@ -546,7 +578,7 @@ static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
>  	    || acpi_dev_resource_ext_address_space(ares, &win))
>  		return acpi_dev_new_resource_entry(&win, c);
>  
> -	for (i = 0; acpi_dev_resource_interrupt(ares, i, res); i++) {
> +	for (i = 0; __acpi_dev_resource_interrupt(c->adev, ares, i, res); i++) {
>  		acpi_status status;
>  
>  		status = acpi_dev_new_resource_entry(&win, c);
> @@ -599,6 +631,7 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
>  	c.preproc_data = preproc_data;
>  	c.count = 0;
>  	c.error = 0;
> +	c.adev = adev;
>  	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
>  				     acpi_dev_process_resource, &c);
>  	if (ACPI_FAILURE(status)) {
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 4242c31..5410d3b 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -355,6 +355,7 @@ struct acpi_device {
>  	int device_type;
>  	acpi_handle handle;		/* no handle for fixed hardware */
>  	struct fwnode_handle fwnode;
> +	struct fwnode_handle *interrupt_producer;
>  	struct acpi_device *parent;
>  	struct list_head children;
>  	struct list_head node;
> -- 
> 1.7.12.4
>

Patch

diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
index ee9e0f2..29ee547 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/gsi.c
@@ -55,13 +55,19 @@  int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
 		      int polarity)
 {
 	struct irq_fwspec fwspec;
+	struct acpi_device *adev = dev ? to_acpi_device(dev) : NULL;
 
-	if (WARN_ON(!acpi_gsi_domain_id)) {
+	if (adev && &adev->fwnode && adev->interrupt_producer)
+		/* devices in DSDT connecting to spefic interrupt producer */
+		fwspec.fwnode = adev->interrupt_producer;
+	else if (acpi_gsi_domain_id)
+		/* devices connecting to gicd in default */
+		fwspec.fwnode = acpi_gsi_domain_id;
+	else {
 		pr_warn("GSI: No registered irqchip, giving up\n");
 		return -EINVAL;
 	}
 
-	fwspec.fwnode = acpi_gsi_domain_id;
 	fwspec.param[0] = gsi;
 	fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
 	fwspec.param_count = 2;
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 56241eb..f1371cf 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -381,7 +381,7 @@  static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
 	res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
 }
 
-static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
+static void acpi_dev_get_irqresource(struct acpi_device *adev, struct resource *res, u32 gsi,
 				     u8 triggering, u8 polarity, u8 shareable,
 				     bool legacy)
 {
@@ -415,7 +415,7 @@  static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
 	}
 
 	res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
-	irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
+	irq = acpi_register_gsi(&adev->dev, gsi, triggering, polarity);
 	if (irq >= 0) {
 		res->start = irq;
 		res->end = irq;
@@ -424,27 +424,9 @@  static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
 	}
 }
 
-/**
- * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information.
- * @ares: Input ACPI resource object.
- * @index: Index into the array of GSIs represented by the resource.
- * @res: Output generic resource object.
- *
- * Check if the given ACPI resource object represents an interrupt resource
- * and @index does not exceed the resource's interrupt count (true is returned
- * in that case regardless of the results of the other checks)).  If that's the
- * case, register the GSI corresponding to @index from the array of interrupts
- * represented by the resource and populate the generic resource object pointed
- * to by @res accordingly.  If the registration of the GSI is not successful,
- * IORESOURCE_DISABLED will be set it that object's flags.
- *
- * Return:
- * 1) false with res->flags setting to zero: not the expected resource type
- * 2) false with IORESOURCE_DISABLED in res->flags: valid unassigned resource
- * 3) true: valid assigned resource
- */
-bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
-				 struct resource *res)
+static bool __acpi_dev_resource_interrupt(struct acpi_device *adev,
+					  struct acpi_resource *ares, int index,
+					  struct resource *res)
 {
 	struct acpi_resource_irq *irq;
 	struct acpi_resource_extended_irq *ext_irq;
@@ -460,7 +442,7 @@  bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 			acpi_dev_irqresource_disabled(res, 0);
 			return false;
 		}
-		acpi_dev_get_irqresource(res, irq->interrupts[index],
+		acpi_dev_get_irqresource(adev, res, irq->interrupts[index],
 					 irq->triggering, irq->polarity,
 					 irq->sharable, true);
 		break;
@@ -470,7 +452,31 @@  bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 			acpi_dev_irqresource_disabled(res, 0);
 			return false;
 		}
-		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
+
+		/*
+		 * It's a interrupt consumer device and connecting to specfic
+		 * interrupt controller. For now, we only support connecting
+		 * interrupts to one irq controller for a single device
+		 */
+		if (ext_irq->producer_consumer == ACPI_CONSUMER
+		    && ext_irq->resource_source.string_length != 0
+		    && !adev->interrupt_producer) {
+			acpi_status status;
+			acpi_handle handle;
+			struct acpi_device *device;
+
+			status = acpi_get_handle(NULL, ext_irq->resource_source.string_ptr, &handle);
+			if (ACPI_FAILURE(status))
+				return false;
+
+			device = acpi_bus_get_acpi_device(handle);
+			if (!device)
+				return false;
+
+			adev->interrupt_producer = &device->fwnode;
+		}
+
+		acpi_dev_get_irqresource(adev, res, ext_irq->interrupts[index],
 					 ext_irq->triggering, ext_irq->polarity,
 					 ext_irq->sharable, false);
 		break;
@@ -481,6 +487,31 @@  bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 
 	return true;
 }
+
+/**
+ * acpi_dev_resource_interrupt - Extract ACPI interrupt resource information.
+ * @ares: Input ACPI resource object.
+ * @index: Index into the array of GSIs represented by the resource.
+ * @res: Output generic resource object.
+ *
+ * Check if the given ACPI resource object represents an interrupt resource
+ * and @index does not exceed the resource's interrupt count (true is returned
+ * in that case regardless of the results of the other checks)).  If that's the
+ * case, register the GSI corresponding to @index from the array of interrupts
+ * represented by the resource and populate the generic resource object pointed
+ * to by @res accordingly.  If the registration of the GSI is not successful,
+ * IORESOURCE_DISABLED will be set it that object's flags.
+ *
+ * Return:
+ * 1) false with res->flags setting to zero: not the expected resource type
+ * 2) false with IORESOURCE_DISABLED in res->flags: valid unassigned resource
+ * 3) true: valid assigned resource
+ */
+bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
+				 struct resource *res)
+{
+	return __acpi_dev_resource_interrupt(NULL, ares, index, res);
+}
 EXPORT_SYMBOL_GPL(acpi_dev_resource_interrupt);
 
 /**
@@ -499,6 +530,7 @@  struct res_proc_context {
 	void *preproc_data;
 	int count;
 	int error;
+	struct acpi_device *adev;
 };
 
 static acpi_status acpi_dev_new_resource_entry(struct resource_win *win,
@@ -546,7 +578,7 @@  static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
 	    || acpi_dev_resource_ext_address_space(ares, &win))
 		return acpi_dev_new_resource_entry(&win, c);
 
-	for (i = 0; acpi_dev_resource_interrupt(ares, i, res); i++) {
+	for (i = 0; __acpi_dev_resource_interrupt(c->adev, ares, i, res); i++) {
 		acpi_status status;
 
 		status = acpi_dev_new_resource_entry(&win, c);
@@ -599,6 +631,7 @@  int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
 	c.preproc_data = preproc_data;
 	c.count = 0;
 	c.error = 0;
+	c.adev = adev;
 	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
 				     acpi_dev_process_resource, &c);
 	if (ACPI_FAILURE(status)) {
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 4242c31..5410d3b 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -355,6 +355,7 @@  struct acpi_device {
 	int device_type;
 	acpi_handle handle;		/* no handle for fixed hardware */
 	struct fwnode_handle fwnode;
+	struct fwnode_handle *interrupt_producer;
 	struct acpi_device *parent;
 	struct list_head children;
 	struct list_head node;