[PATCH V2] acpi, pci, irq: account for early penalty assignment

[PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

A crash has been observed when assigning penalty on x86 systems.

It looks like this problem happens on x86 platforms with IOAPIC and an SCI
interrupt override in the ACPI table with interrupt number greater than
16. (22 in this example)

The bug has been introduced by "ACPI, PCI, irq: remove interrupt count
restriction" commit. The code was using kmalloc to resize the interrupt
list. In this use case, the set penalty call is coming from early phase
and the heap is not initialized yet.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
IP: [<ffffffff811e8b9d>] kmem_cache_alloc_trace+0xad/0x1c0
PGD 0
Oops: 0000 [#1] SMP
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc2Feb-3_RK #1
Hardware name: HP Superdome2 16s, BIOS Bundle: 007.006.000 SFW: 033.162.000
10/30/2015
[<ffffffff813bc190>] acpi_irq_set_penalty+0x60/0x8e
[<ffffffff813bc1df>] acpi_irq_add_penalty+0x21/0x26
[<ffffffff813bc76d>] acpi_penalize_sci_irq+0x25/0x28
[<ffffffff81b8260d>] acpi_sci_ioapic_setup+0x68/0x78
[<ffffffff81b830fc>] acpi_boot_init+0x2cc/0x533
[<ffffffff810677c8>] ? set_pte_vaddr_pud+0x48/0x50
[<ffffffff81b828cf>] ? acpi_parse_x2apic+0x77/0x77
[<ffffffff81b82858>] ? dmi_ignore_irq0_timer_override+0x30/0x30
[<ffffffff81b77c1e>] setup_arch+0xc24/0xce9
[<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
[<ffffffff81b6ed94>] start_kernel+0xfc/0x506
[<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
[<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
[<ffffffff81b6e5ee>] x86_64_start_reservations+0x2a/0x2c
[<ffffffff81b6e73c>] x86_64_start_kernel+0x14c/0x16f

Besides from the use case above, there is one more situation where
set_penalty is being called from the init context like. There is support
for setting the penalty through kernel command line.

Adding support to be called from early context for limited number of
interrupts.

Reported-by: Nalla, Ravikanth <ravikanth.nalla@hpe.com>
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/acpi/pci_link.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index fa28635..14fe3ca 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -47,6 +47,7 @@ ACPI_MODULE_NAME("pci_link");
 #define ACPI_PCI_LINK_FILE_INFO		"info"
 #define ACPI_PCI_LINK_FILE_STATUS	"state"
 #define ACPI_PCI_LINK_MAX_POSSIBLE	16
+#define ACPI_PCI_LINK_MAX_EARLY_IRQINFO 1024
 
 static int acpi_pci_link_add(struct acpi_device *device,
 			     const struct acpi_device_id *not_used);
@@ -473,6 +474,8 @@ struct irq_penalty_info {
 };
 
 static LIST_HEAD(acpi_irq_penalty_list);
+static struct irq_penalty_info early_irq_infos[ACPI_PCI_LINK_MAX_EARLY_IRQINFO];
+static int early_irq_info_counter;
 
 static int acpi_irq_get_penalty(int irq)
 {
@@ -507,10 +510,17 @@ static int acpi_irq_set_penalty(int irq, int new_penalty)
 		}
 	}
 
-	/* nope, let's allocate a slot for this IRQ */
-	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
-	if (!irq_info)
-		return -ENOMEM;
+	if (!acpi_gbl_permanent_mmap) {
+		if (early_irq_info_counter < ARRAY_SIZE(early_irq_infos))
+			irq_info = &early_irq_infos[early_irq_info_counter++];
+		else
+			return -ENOMEM;
+	} else {
+		/* nope, let's allocate a slot for this IRQ */
+		irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
+		if (!irq_info)
+			return -ENOMEM;
+	}
 
 	irq_info->irq = irq;
 	irq_info->penalty = new_penalty;
@@ -968,3 +978,4 @@ void __init acpi_pci_link_init(void)
 	register_syscore_ops(&irqrouter_syscore_ops);
 	acpi_scan_add_handler(&pci_link_handler);
 }
+
-- 
1.8.2.1

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Timur Tabi]

Sinan Kaya wrote:
> @@ -968,3 +978,4 @@ void __init acpi_pci_link_init(void)
>   	register_syscore_ops(&irqrouter_syscore_ops);
>   	acpi_scan_add_handler(&pci_link_handler);
>   }
> +

Unrelated whitespace change.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Rafael J. Wysocki]

On Thu, Feb 18, 2016 at 2:19 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> A crash has been observed when assigning penalty on x86 systems.
>
> It looks like this problem happens on x86 platforms with IOAPIC and an SCI
> interrupt override in the ACPI table with interrupt number greater than
> 16. (22 in this example)
>
> The bug has been introduced by "ACPI, PCI, irq: remove interrupt count
> restriction" commit. The code was using kmalloc to resize the interrupt
> list. In this use case, the set penalty call is coming from early phase
> and the heap is not initialized yet.
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
> IP: [<ffffffff811e8b9d>] kmem_cache_alloc_trace+0xad/0x1c0
> PGD 0
> Oops: 0000 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc2Feb-3_RK #1
> Hardware name: HP Superdome2 16s, BIOS Bundle: 007.006.000 SFW: 033.162.000
> 10/30/2015
> [<ffffffff813bc190>] acpi_irq_set_penalty+0x60/0x8e
> [<ffffffff813bc1df>] acpi_irq_add_penalty+0x21/0x26
> [<ffffffff813bc76d>] acpi_penalize_sci_irq+0x25/0x28
> [<ffffffff81b8260d>] acpi_sci_ioapic_setup+0x68/0x78
> [<ffffffff81b830fc>] acpi_boot_init+0x2cc/0x533
> [<ffffffff810677c8>] ? set_pte_vaddr_pud+0x48/0x50
> [<ffffffff81b828cf>] ? acpi_parse_x2apic+0x77/0x77
> [<ffffffff81b82858>] ? dmi_ignore_irq0_timer_override+0x30/0x30
> [<ffffffff81b77c1e>] setup_arch+0xc24/0xce9
> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
> [<ffffffff81b6ed94>] start_kernel+0xfc/0x506
> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
> [<ffffffff81b6e5ee>] x86_64_start_reservations+0x2a/0x2c
> [<ffffffff81b6e73c>] x86_64_start_kernel+0x14c/0x16f
>
> Besides from the use case above, there is one more situation where
> set_penalty is being called from the init context like. There is support
> for setting the penalty through kernel command line.
>
> Adding support to be called from early context for limited number of
> interrupts.
>
> Reported-by: Nalla, Ravikanth <ravikanth.nalla@hpe.com>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/acpi/pci_link.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index fa28635..14fe3ca 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -47,6 +47,7 @@ ACPI_MODULE_NAME("pci_link");
>  #define ACPI_PCI_LINK_FILE_INFO                "info"
>  #define ACPI_PCI_LINK_FILE_STATUS      "state"
>  #define ACPI_PCI_LINK_MAX_POSSIBLE     16
> +#define ACPI_PCI_LINK_MAX_EARLY_IRQINFO 1024

Why do we need so many of them?

>
>  static int acpi_pci_link_add(struct acpi_device *device,
>                              const struct acpi_device_id *not_used);
> @@ -473,6 +474,8 @@ struct irq_penalty_info {
>  };
>
>  static LIST_HEAD(acpi_irq_penalty_list);
> +static struct irq_penalty_info early_irq_infos[ACPI_PCI_LINK_MAX_EARLY_IRQINFO];
> +static int early_irq_info_counter;
>
>  static int acpi_irq_get_penalty(int irq)
>  {
> @@ -507,10 +510,17 @@ static int acpi_irq_set_penalty(int irq, int new_penalty)
>                 }
>         }
>
> -       /* nope, let's allocate a slot for this IRQ */
> -       irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
> -       if (!irq_info)
> -               return -ENOMEM;
> +       if (!acpi_gbl_permanent_mmap) {
> +               if (early_irq_info_counter < ARRAY_SIZE(early_irq_infos))
> +                       irq_info = &early_irq_infos[early_irq_info_counter++];
> +               else
> +                       return -ENOMEM;
> +       } else {
> +               /* nope, let's allocate a slot for this IRQ */
> +               irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
> +               if (!irq_info)
> +                       return -ENOMEM;
> +       }
>
>         irq_info->irq = irq;
>         irq_info->penalty = new_penalty;
> @@ -968,3 +978,4 @@ void __init acpi_pci_link_init(void)
>         register_syscore_ops(&irqrouter_syscore_ops);
>         acpi_scan_add_handler(&pci_link_handler);
>  }
> +
> --

Bjorn, what do you think about this one?

Thanks,
Rafael

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

On 2/18/2016 11:39 AM, Rafael J. Wysocki wrote:
>> +#define ACPI_PCI_LINK_MAX_EARLY_IRQINFO 1024
> Why do we need so many of them?
> 

The previous code supported 1024 max interrupts before 
"ACPI, PCI, irq: remove interrupt count restriction" change. 

I added back 1024 number but the limit is only for early IRQs now.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Bjorn Helgaas]

On Thu, Feb 18, 2016 at 08:19:41AM -0500, Sinan Kaya wrote:
> A crash has been observed when assigning penalty on x86 systems.
> 
> It looks like this problem happens on x86 platforms with IOAPIC and an SCI
> interrupt override in the ACPI table with interrupt number greater than
> 16. (22 in this example)
> 
> The bug has been introduced by "ACPI, PCI, irq: remove interrupt count
> restriction" commit. The code was using kmalloc to resize the interrupt

When referring to a previous commit, please include the SHA1, e.g.,

  b5bd02695471 ("ACPI, PCI, irq: remove interrupt count restriction")

> list. In this use case, the set penalty call is coming from early phase
> and the heap is not initialized yet.
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
> IP: [<ffffffff811e8b9d>] kmem_cache_alloc_trace+0xad/0x1c0
> PGD 0
> Oops: 0000 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc2Feb-3_RK #1
> Hardware name: HP Superdome2 16s, BIOS Bundle: 007.006.000 SFW: 033.162.000
> 10/30/2015
> [<ffffffff813bc190>] acpi_irq_set_penalty+0x60/0x8e
> [<ffffffff813bc1df>] acpi_irq_add_penalty+0x21/0x26
> [<ffffffff813bc76d>] acpi_penalize_sci_irq+0x25/0x28
> [<ffffffff81b8260d>] acpi_sci_ioapic_setup+0x68/0x78
> [<ffffffff81b830fc>] acpi_boot_init+0x2cc/0x533
> [<ffffffff810677c8>] ? set_pte_vaddr_pud+0x48/0x50
> [<ffffffff81b828cf>] ? acpi_parse_x2apic+0x77/0x77
> [<ffffffff81b82858>] ? dmi_ignore_irq0_timer_override+0x30/0x30
> [<ffffffff81b77c1e>] setup_arch+0xc24/0xce9
> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
> [<ffffffff81b6ed94>] start_kernel+0xfc/0x506
> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
> [<ffffffff81b6e5ee>] x86_64_start_reservations+0x2a/0x2c
> [<ffffffff81b6e73c>] x86_64_start_kernel+0x14c/0x16f
> 
> Besides from the use case above, there is one more situation where
> set_penalty is being called from the init context like. There is support
> for setting the penalty through kernel command line.
> 
> Adding support to be called from early context for limited number of
> interrupts.

I can't believe this whole IRQ penalty thing needs to be so
complicated.

The only time we actually use the penalty information is when we're
attaching a driver to a PCI device, i.e., in this path:

  pci_device_probe
    pcibios_alloc_irq
      pcibios_enable_irq

That happens pretty late, so there's no "can't allocate memory during
early boot" problem.

I bet the only thing that might happen early enough to be an issue is
the acpi_penalize_sci_irq() thing, which is a special case that
doesn't need to be handled generically.

> Reported-by: Nalla, Ravikanth <ravikanth.nalla@hpe.com>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/acpi/pci_link.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index fa28635..14fe3ca 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -47,6 +47,7 @@ ACPI_MODULE_NAME("pci_link");
>  #define ACPI_PCI_LINK_FILE_INFO		"info"
>  #define ACPI_PCI_LINK_FILE_STATUS	"state"
>  #define ACPI_PCI_LINK_MAX_POSSIBLE	16
> +#define ACPI_PCI_LINK_MAX_EARLY_IRQINFO 1024
>  
>  static int acpi_pci_link_add(struct acpi_device *device,
>  			     const struct acpi_device_id *not_used);
> @@ -473,6 +474,8 @@ struct irq_penalty_info {
>  };
>  
>  static LIST_HEAD(acpi_irq_penalty_list);
> +static struct irq_penalty_info early_irq_infos[ACPI_PCI_LINK_MAX_EARLY_IRQINFO];
> +static int early_irq_info_counter;
>  
>  static int acpi_irq_get_penalty(int irq)
>  {
> @@ -507,10 +510,17 @@ static int acpi_irq_set_penalty(int irq, int new_penalty)
>  		}
>  	}
>  
> -	/* nope, let's allocate a slot for this IRQ */
> -	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
> -	if (!irq_info)
> -		return -ENOMEM;
> +	if (!acpi_gbl_permanent_mmap) {
> +		if (early_irq_info_counter < ARRAY_SIZE(early_irq_infos))
> +			irq_info = &early_irq_infos[early_irq_info_counter++];
> +		else
> +			return -ENOMEM;
> +	} else {
> +		/* nope, let's allocate a slot for this IRQ */
> +		irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
> +		if (!irq_info)
> +			return -ENOMEM;
> +	}
>  
>  	irq_info->irq = irq;
>  	irq_info->penalty = new_penalty;
> @@ -968,3 +978,4 @@ void __init acpi_pci_link_init(void)
>  	register_syscore_ops(&irqrouter_syscore_ops);
>  	acpi_scan_add_handler(&pci_link_handler);
>  }
> +
> -- 
> 1.8.2.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

Hi Bjorn,

On 2/29/2016 2:24 PM, Bjorn Helgaas wrote:
> On Thu, Feb 18, 2016 at 08:19:41AM -0500, Sinan Kaya wrote:
>> A crash has been observed when assigning penalty on x86 systems.
>>
>> It looks like this problem happens on x86 platforms with IOAPIC and an SCI
>> interrupt override in the ACPI table with interrupt number greater than
>> 16. (22 in this example)
>>
>> The bug has been introduced by "ACPI, PCI, irq: remove interrupt count
>> restriction" commit. The code was using kmalloc to resize the interrupt
> 
> When referring to a previous commit, please include the SHA1, e.g.,
> 
>   b5bd02695471 ("ACPI, PCI, irq: remove interrupt count restriction")

OK

> 
>> list. In this use case, the set penalty call is coming from early phase
>> and the heap is not initialized yet.
>>
>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
>> IP: [<ffffffff811e8b9d>] kmem_cache_alloc_trace+0xad/0x1c0
>> PGD 0
>> Oops: 0000 [#1] SMP
>> Modules linked in:
>> CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc2Feb-3_RK #1
>> Hardware name: HP Superdome2 16s, BIOS Bundle: 007.006.000 SFW: 033.162.000
>> 10/30/2015
>> [<ffffffff813bc190>] acpi_irq_set_penalty+0x60/0x8e
>> [<ffffffff813bc1df>] acpi_irq_add_penalty+0x21/0x26
>> [<ffffffff813bc76d>] acpi_penalize_sci_irq+0x25/0x28
>> [<ffffffff81b8260d>] acpi_sci_ioapic_setup+0x68/0x78
>> [<ffffffff81b830fc>] acpi_boot_init+0x2cc/0x533
>> [<ffffffff810677c8>] ? set_pte_vaddr_pud+0x48/0x50
>> [<ffffffff81b828cf>] ? acpi_parse_x2apic+0x77/0x77
>> [<ffffffff81b82858>] ? dmi_ignore_irq0_timer_override+0x30/0x30
>> [<ffffffff81b77c1e>] setup_arch+0xc24/0xce9
>> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
>> [<ffffffff81b6ed94>] start_kernel+0xfc/0x506
>> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
>> [<ffffffff81b6e120>] ? early_idt_handler_array+0x120/0x120
>> [<ffffffff81b6e5ee>] x86_64_start_reservations+0x2a/0x2c
>> [<ffffffff81b6e73c>] x86_64_start_kernel+0x14c/0x16f
>>
>> Besides from the use case above, there is one more situation where
>> set_penalty is being called from the init context like. There is support
>> for setting the penalty through kernel command line.
>>
>> Adding support to be called from early context for limited number of
>> interrupts.
> 
> I can't believe this whole IRQ penalty thing needs to be so
> complicated.
> 
> The only time we actually use the penalty information is when we're
> attaching a driver to a PCI device, i.e., in this path:
> 
>   pci_device_probe
>     pcibios_alloc_irq
>       pcibios_enable_irq
> 
> That happens pretty late, so there's no "can't allocate memory during
> early boot" problem.

Correct, this is the path that code is intended for.

> 
> I bet the only thing that might happen early enough to be an issue is
> the acpi_penalize_sci_irq() thing, which is a special case that
> doesn't need to be handled generically.

The second use case is the kernel command line. See the bottom of the code, 
there are routines there to go get the penalty information from command line.

How would you like to proceed ?

- merge this to the original patch
- remove the acpi_penalize_sci_irq code to somewhere else.
- what about the kernel command line?


> 
>> Reported-by: Nalla, Ravikanth <ravikanth.nalla@hpe.com>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/acpi/pci_link.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index fa28635..14fe3ca 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -47,6 +47,7 @@ ACPI_MODULE_NAME("pci_link");
>>  #define ACPI_PCI_LINK_FILE_INFO		"info"
>>  #define ACPI_PCI_LINK_FILE_STATUS	"state"
>>  #define ACPI_PCI_LINK_MAX_POSSIBLE	16
>> +#define ACPI_PCI_LINK_MAX_EARLY_IRQINFO 1024
>>  
>>  static int acpi_pci_link_add(struct acpi_device *device,
>>  			     const struct acpi_device_id *not_used);
>> @@ -473,6 +474,8 @@ struct irq_penalty_info {
>>  };
>>  
>>  static LIST_HEAD(acpi_irq_penalty_list);
>> +static struct irq_penalty_info early_irq_infos[ACPI_PCI_LINK_MAX_EARLY_IRQINFO];
>> +static int early_irq_info_counter;
>>  
>>  static int acpi_irq_get_penalty(int irq)
>>  {
>> @@ -507,10 +510,17 @@ static int acpi_irq_set_penalty(int irq, int new_penalty)
>>  		}
>>  	}
>>  
>> -	/* nope, let's allocate a slot for this IRQ */
>> -	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
>> -	if (!irq_info)
>> -		return -ENOMEM;
>> +	if (!acpi_gbl_permanent_mmap) {
>> +		if (early_irq_info_counter < ARRAY_SIZE(early_irq_infos))
>> +			irq_info = &early_irq_infos[early_irq_info_counter++];
>> +		else
>> +			return -ENOMEM;
>> +	} else {
>> +		/* nope, let's allocate a slot for this IRQ */
>> +		irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
>> +		if (!irq_info)
>> +			return -ENOMEM;
>> +	}
>>  
>>  	irq_info->irq = irq;
>>  	irq_info->penalty = new_penalty;
>> @@ -968,3 +978,4 @@ void __init acpi_pci_link_init(void)
>>  	register_syscore_ops(&irqrouter_syscore_ops);
>>  	acpi_scan_add_handler(&pci_link_handler);
>>  }
>> +
>> -- 
>> 1.8.2.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Bjorn Helgaas]

On Mon, Feb 29, 2016 at 03:08:26PM -0500, Sinan Kaya wrote:
> Hi Bjorn,
> 
> On 2/29/2016 2:24 PM, Bjorn Helgaas wrote:
> > On Thu, Feb 18, 2016 at 08:19:41AM -0500, Sinan Kaya wrote:
> >> A crash has been observed when assigning penalty on x86 systems.
> >>
> >> It looks like this problem happens on x86 platforms with IOAPIC and an SCI
> >> interrupt override in the ACPI table with interrupt number greater than
> >> 16. (22 in this example)
> >>
> >> The bug has been introduced by "ACPI, PCI, irq: remove interrupt count
> >> restriction" commit. The code was using kmalloc to resize the interrupt
> >> list. In this use case, the set penalty call is coming from early phase
> >> and the heap is not initialized yet.
> >> ...

> >> Besides from the use case above, there is one more situation where
> >> set_penalty is being called from the init context like. There is support
> >> for setting the penalty through kernel command line.
> >>
> >> Adding support to be called from early context for limited number of
> >> interrupts.
> > 
> > I can't believe this whole IRQ penalty thing needs to be so
> > complicated.
> > 
> > The only time we actually use the penalty information is when we're
> > attaching a driver to a PCI device, i.e., in this path:
> > 
> >   pci_device_probe
> >     pcibios_alloc_irq
> >       pcibios_enable_irq
> > 
> > That happens pretty late, so there's no "can't allocate memory during
> > early boot" problem.
> 
> Correct, this is the path that code is intended for.
> 
> > I bet the only thing that might happen early enough to be an issue is
> > the acpi_penalize_sci_irq() thing, which is a special case that
> > doesn't need to be handled generically.
> 
> The second use case is the kernel command line. See the bottom of the code, 
> there are routines there to go get the penalty information from command line.

Right.  But if we don't *use* the information until later, there's
probably no need to parse the command line and set it up so early.

> How would you like to proceed ?
> 
> - merge this to the original patch
> - remove the acpi_penalize_sci_irq code to somewhere else.
> - what about the kernel command line?

There's so much code there, that I think all the code obscures the
fact that there's almost nothing really happening.  In broad outline,
I think we care about:

  - the legacy ISA IRQs, i.e., the contents of acpi_irq_isa_penalty[]
  - acpi_irq_isa= from command line
  - acpi_irq_pci= from command line
  - which IRQ is used for SCI
  - number of PCI Interrupt Link devices sharing an IRQ

I doubt we need any dynamic allocation at all to manage this.  We
already have the acpi_irq_isa_penalty[] table statically allocated.
The SCI IRQ is one word.  I bet the command-line stuff is only
useful for the 16 ISA IRQs and could be merged into
acpi_irq_isa_penalty[].  Same for acpi_penalize_isa_irq() and
acpi_isa_irq_available().  We could easily compute the
number of links sharing an IRQ by traversing acpi_link_list.

I think only x86 cares about the first three items (legacy ISA IRQs
and command-line args).  This should be reflected in the code.  Only
x86 calls acpi_irq_penalty_init(), but that's pretty non-obvious.

I think it would be better to completely rewrite this penalty stuff
than to keep making it more complicated by fixing things in the
existing design.

> >> Reported-by: Nalla, Ravikanth <ravikanth.nalla@hpe.com>
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> ---
> >>  drivers/acpi/pci_link.c | 19 +++++++++++++++----
> >>  1 file changed, 15 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> >> index fa28635..14fe3ca 100644
> >> --- a/drivers/acpi/pci_link.c
> >> +++ b/drivers/acpi/pci_link.c
> >> @@ -47,6 +47,7 @@ ACPI_MODULE_NAME("pci_link");
> >>  #define ACPI_PCI_LINK_FILE_INFO		"info"
> >>  #define ACPI_PCI_LINK_FILE_STATUS	"state"
> >>  #define ACPI_PCI_LINK_MAX_POSSIBLE	16
> >> +#define ACPI_PCI_LINK_MAX_EARLY_IRQINFO 1024
> >>  
> >>  static int acpi_pci_link_add(struct acpi_device *device,
> >>  			     const struct acpi_device_id *not_used);
> >> @@ -473,6 +474,8 @@ struct irq_penalty_info {
> >>  };
> >>  
> >>  static LIST_HEAD(acpi_irq_penalty_list);
> >> +static struct irq_penalty_info early_irq_infos[ACPI_PCI_LINK_MAX_EARLY_IRQINFO];
> >> +static int early_irq_info_counter;
> >>  
> >>  static int acpi_irq_get_penalty(int irq)
> >>  {
> >> @@ -507,10 +510,17 @@ static int acpi_irq_set_penalty(int irq, int new_penalty)
> >>  		}
> >>  	}
> >>  
> >> -	/* nope, let's allocate a slot for this IRQ */
> >> -	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
> >> -	if (!irq_info)
> >> -		return -ENOMEM;
> >> +	if (!acpi_gbl_permanent_mmap) {
> >> +		if (early_irq_info_counter < ARRAY_SIZE(early_irq_infos))
> >> +			irq_info = &early_irq_infos[early_irq_info_counter++];
> >> +		else
> >> +			return -ENOMEM;
> >> +	} else {
> >> +		/* nope, let's allocate a slot for this IRQ */
> >> +		irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
> >> +		if (!irq_info)
> >> +			return -ENOMEM;
> >> +	}
> >>  
> >>  	irq_info->irq = irq;
> >>  	irq_info->penalty = new_penalty;
> >> @@ -968,3 +978,4 @@ void __init acpi_pci_link_init(void)
> >>  	register_syscore_ops(&irqrouter_syscore_ops);
> >>  	acpi_scan_add_handler(&pci_link_handler);
> >>  }
> >> +
> >> -- 
> >> 1.8.2.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
> -- 
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

> There's so much code there, that I think all the code obscures the
> fact that there's almost nothing really happening.  In broad outline,
> I think we care about:
> 
>   - the legacy ISA IRQs, i.e., the contents of acpi_irq_isa_penalty[]
>   - acpi_irq_isa= from command line
>   - acpi_irq_pci= from command line
>   - which IRQ is used for SCI
>   - number of PCI Interrupt Link devices sharing an IRQ
> 
> I doubt we need any dynamic allocation at all to manage this.  We
> already have the acpi_irq_isa_penalty[] table statically allocated.
> The SCI IRQ is one word.  

Just to be clear, we have resized acpi_irq_penalty table to 16 and named it
acpi_irq_isa_penalty. We are dynamically allocating memory for the rest of 
the interrupts that is bigger than 16. 

The SCI interrupt that caused the failure is interrupt 22 in this case. The code
was trying to allocate a new entry with kzalloc. 22 won't fit into the 
acpi_irq_isa_penalty array. How do we handle such case? Is there a cap on the SCI
interrupt number? 

That's why, I was trying to reallocate some memory in this code.


> I bet the command-line stuff is only
> useful for the 16 ISA IRQs and could be merged into
> acpi_irq_isa_penalty[].  
> Same for acpi_penalize_isa_irq() and
> acpi_isa_irq_available().  

Agreed. No issues with ISA IRQs.

> We could easily compute the
> number of links sharing an IRQ by traversing acpi_link_list.

Sorry, I couldn't quite get this. Where would you do this?

> 
> I think only x86 cares about the first three items (legacy ISA IRQs
> and command-line args).  This should be reflected in the code.  Only
> x86 calls acpi_irq_penalty_init(), but that's pretty non-obvious.

Very true. It looks like somebody sneaked in some code there.

> 
> I think it would be better to completely rewrite this penalty stuff
> than to keep making it more complicated by fixing things in the
> existing design.
> 
OK. Let's find out what it needs to look like. I need guidance on what IRQs on
Intel systems look like. 

>>>> Reported-by: Nalla, Ravikanth <ravikanth.nalla@hpe.com>
>>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>>>> ---
>>>>  drivers/acpi/pci_link.c | 19 +++++++++++++++----
>>>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>>>> index fa28635..14fe3ca 100644
>>>> --- a/drivers/acpi/pci_link.c
>>>> +++ b/drivers/acpi/pci_link.c
>>>> @@ -47,6 +47,7 @@ ACPI_MODULE_NAME("pci_link");
>>>>  #define ACPI_PCI_LINK_FILE_INFO		"info"
>>>>  #define ACPI_PCI_LINK_FILE_STATUS	"state"
>>>>  #define ACPI_PCI_LINK_MAX_POSSIBLE	16
>>>> +#define ACPI_PCI_LINK_MAX_EARLY_IRQINFO 1024
>>>>  
>>>>  static int acpi_pci_link_add(struct acpi_device *device,
>>>>  			     const struct acpi_device_id *not_used);
>>>> @@ -473,6 +474,8 @@ struct irq_penalty_info {
>>>>  };
>>>>  
>>>>  static LIST_HEAD(acpi_irq_penalty_list);
>>>> +static struct irq_penalty_info early_irq_infos[ACPI_PCI_LINK_MAX_EARLY_IRQINFO];
>>>> +static int early_irq_info_counter;
>>>>  
>>>>  static int acpi_irq_get_penalty(int irq)
>>>>  {
>>>> @@ -507,10 +510,17 @@ static int acpi_irq_set_penalty(int irq, int new_penalty)
>>>>  		}
>>>>  	}
>>>>  
>>>> -	/* nope, let's allocate a slot for this IRQ */
>>>> -	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
>>>> -	if (!irq_info)
>>>> -		return -ENOMEM;
>>>> +	if (!acpi_gbl_permanent_mmap) {
>>>> +		if (early_irq_info_counter < ARRAY_SIZE(early_irq_infos))
>>>> +			irq_info = &early_irq_infos[early_irq_info_counter++];
>>>> +		else
>>>> +			return -ENOMEM;
>>>> +	} else {
>>>> +		/* nope, let's allocate a slot for this IRQ */
>>>> +		irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
>>>> +		if (!irq_info)
>>>> +			return -ENOMEM;
>>>> +	}
>>>>  
>>>>  	irq_info->irq = irq;
>>>>  	irq_info->penalty = new_penalty;
>>>> @@ -968,3 +978,4 @@ void __init acpi_pci_link_init(void)
>>>>  	register_syscore_ops(&irqrouter_syscore_ops);
>>>>  	acpi_scan_add_handler(&pci_link_handler);
>>>>  }
>>>> +
>>>> -- 
>>>> 1.8.2.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>> -- 
>> Sinan Kaya
>> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Bjorn Helgaas]

On Tue, Mar 01, 2016 at 01:49:34PM -0500, Sinan Kaya wrote:
> > There's so much code there, that I think all the code obscures the
> > fact that there's almost nothing really happening.  In broad outline,
> > I think we care about:
> > 
> >   - the legacy ISA IRQs, i.e., the contents of acpi_irq_isa_penalty[]
> >   - acpi_irq_isa= from command line
> >   - acpi_irq_pci= from command line
> >   - which IRQ is used for SCI
> >   - number of PCI Interrupt Link devices sharing an IRQ
> > 
> > I doubt we need any dynamic allocation at all to manage this.  We
> > already have the acpi_irq_isa_penalty[] table statically allocated.
> > The SCI IRQ is one word.  
> 
> Just to be clear, we have resized acpi_irq_penalty table to 16 and named it
> acpi_irq_isa_penalty. We are dynamically allocating memory for the rest of 
> the interrupts that is bigger than 16. 
> 
> The SCI interrupt that caused the failure is interrupt 22 in this case. The code
> was trying to allocate a new entry with kzalloc. 22 won't fit into the 
> acpi_irq_isa_penalty array. How do we handle such case? Is there a cap on the SCI
> interrupt number? 
> 
> That's why, I was trying to reallocate some memory in this code.

I don't think there's a restriction on what the SCI IRQ can be.  But
there is only one SCI IRQ, so all we have to do is keep track of what
it is, which only requires one word.

> > I bet the command-line stuff is only
> > useful for the 16 ISA IRQs and could be merged into
> > acpi_irq_isa_penalty[].  
> > Same for acpi_penalize_isa_irq() and
> > acpi_isa_irq_available().  
> 
> Agreed. No issues with ISA IRQs.
> 
> > We could easily compute the
> > number of links sharing an IRQ by traversing acpi_link_list.
> 
> Sorry, I couldn't quite get this. Where would you do this?

I've never been exactly clear on how these links work.  So pardon me
while I think out loud and bore you with what you already know
(correct me if I get this wrong):

  - A link device has a PCI wired interrupt (INTA, INTB, etc.) on its
    "downstream" end.

  - The link device has a set of possible interrupt controller inputs
    to which it can connect the PCI interrupt.  _PRS contains this
    set.

  - When we enable a PCI device's interrupt, Interrupt Pin from config
    space tells us which INTx it uses.  The _PRT tells us whether that
    INTx is connected to (a) a fixed GSI or (b) an Interrupt Link that
    can be configured to one of several interrupt controller inputs.

  - If the latter, we must select one of the interrupt controller
    inputs to use, i.e., one of the IRQs from _PRS, and enable the
    Link.

  - If the Link is already active, we probably shouldn't change its
    configuration because other devices might already be using it.

  - If the Link is inactive, we must choose an IRQ and activate it.
    We should be able to choose anything from _PRS (as long as the
    level & trigger attributes match), but we can try to reduce IRQ
    sharing by avoiding an IRQ that's already in use.

This IRQ selection process is where we use the penalty information.
In acpi_pci_link_allocate(), we iterate through the possible choices
(link->irq.possible[i]) and choose the one with the smallest penalty.

Here's a sketch of what I'm thinking the code could look like.  In x86
code:

  int pcibios_irq_penalty(int irq)
  {
    if (irq >= ACPI_MAX_ISA_IRQ)
      return 0;

    return acpi_irq_isa_penalty[irq] + acpi_irq_cmd_line_penalty[irq];
  }

In pci_link.c:

  static int sci_irq, sci_irq_penalty;

  void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
  {
    if (irq < 0)
      return;

    sci_irq = irq;
    if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
        polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
          sci_irq_penalty = infinite;  /* can't use for PCI at all */
    else
      sci_irq_penalty = PIRQ_PENALTY_PCI_USING;
  }

  static pci_irq_sharing_penalty(int irq)
  {
    struct acpi_pci_link *link;
    int penalty = 0;

    list_for_each_entry(link, &acpi_link_list, list) {

      /*
       * If a link is active, penalize its IRQ heavily so we try to choose
       * a different IRQ.
       */
      if (link->irq.active && link->irq.active == irq)
        penalty += PIRQ_PENALTY_PCI_USING;
      else {

        /*
         * If a link is inactive, penalize the IRQs it might use, but
         * not as severely.
         */ 
        for (i = 0; i < link->irq.possible_count; i++)
          if (link->irq.possible[i] == irq)
            penalty += PIRQ_PENALTY_PCI_POSSIBLE;
      }
    }

    return penalty;
  }

  int __weak pcibios_irq_penalty(int irq)
  {
    return 0;
  }

  static int acpi_irq_get_penalty(int irq)
  {
    int penalty;

    penalty = pcibios_irq_penalty(irq);

    if (irq == sci_irq)
      penalty += sci_irq_penalty;

    penalty += pci_irq_sharing_penalty(irq);
    return penalty;
  }

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

On 3/1/2016 2:43 PM, Bjorn Helgaas wrote:
> On Tue, Mar 01, 2016 at 01:49:34PM -0500, Sinan Kaya wrote:
>>> There's so much code there, that I think all the code obscures the
>>> fact that there's almost nothing really happening.  In broad outline,
>>> I think we care about:
>>>
>>>   - the legacy ISA IRQs, i.e., the contents of acpi_irq_isa_penalty[]
>>>   - acpi_irq_isa= from command line
>>>   - acpi_irq_pci= from command line
>>>   - which IRQ is used for SCI
>>>   - number of PCI Interrupt Link devices sharing an IRQ
>>>
>>> I doubt we need any dynamic allocation at all to manage this.  We
>>> already have the acpi_irq_isa_penalty[] table statically allocated.
>>> The SCI IRQ is one word.  
>>
>> Just to be clear, we have resized acpi_irq_penalty table to 16 and named it
>> acpi_irq_isa_penalty. We are dynamically allocating memory for the rest of 
>> the interrupts that is bigger than 16. 
>>
>> The SCI interrupt that caused the failure is interrupt 22 in this case. The code
>> was trying to allocate a new entry with kzalloc. 22 won't fit into the 
>> acpi_irq_isa_penalty array. How do we handle such case? Is there a cap on the SCI
>> interrupt number? 
>>
>> That's why, I was trying to reallocate some memory in this code.
> 
> I don't think there's a restriction on what the SCI IRQ can be.  But
> there is only one SCI IRQ, so all we have to do is keep track of what
> it is, which only requires one word.
> 
>>> I bet the command-line stuff is only
>>> useful for the 16 ISA IRQs and could be merged into
>>> acpi_irq_isa_penalty[].  
>>> Same for acpi_penalize_isa_irq() and
>>> acpi_isa_irq_available().  
>>
>> Agreed. No issues with ISA IRQs.
>>
>>> We could easily compute the
>>> number of links sharing an IRQ by traversing acpi_link_list.
>>
>> Sorry, I couldn't quite get this. Where would you do this?
> 
> I've never been exactly clear on how these links work.  So pardon me
> while I think out loud and bore you with what you already know
> (correct me if I get this wrong):
> 
>   - A link device has a PCI wired interrupt (INTA, INTB, etc.) on its
>     "downstream" end.
> 
>   - The link device has a set of possible interrupt controller inputs
>     to which it can connect the PCI interrupt.  _PRS contains this
>     set.
> 
>   - When we enable a PCI device's interrupt, Interrupt Pin from config
>     space tells us which INTx it uses.  The _PRT tells us whether that
>     INTx is connected to (a) a fixed GSI or (b) an Interrupt Link that
>     can be configured to one of several interrupt controller inputs.
> 
>   - If the latter, we must select one of the interrupt controller
>     inputs to use, i.e., one of the IRQs from _PRS, and enable the
>     Link.
> 
>   - If the Link is already active, we probably shouldn't change its
>     configuration because other devices might already be using it.
> 
>   - If the Link is inactive, we must choose an IRQ and activate it.
>     We should be able to choose anything from _PRS (as long as the
>     level & trigger attributes match), but we can try to reduce IRQ
>     sharing by avoiding an IRQ that's already in use.
> 

Really nice write up. We need to fold this into the code. It was never 
obvious.

I'll send something soon.

> This IRQ selection process is where we use the penalty information.
> In acpi_pci_link_allocate(), we iterate through the possible choices
> (link->irq.possible[i]) and choose the one with the smallest penalty.
> 
> Here's a sketch of what I'm thinking the code could look like.  In x86
> code:
> 
>   int pcibios_irq_penalty(int irq)
>   {
>     if (irq >= ACPI_MAX_ISA_IRQ)
>       return 0;
> 
>     return acpi_irq_isa_penalty[irq] + acpi_irq_cmd_line_penalty[irq];
>   }
> 

> In pci_link.c:
> 
>   static int sci_irq, sci_irq_penalty;
> 
>   void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
>   {
>     if (irq < 0)
>       return;
> 
>     sci_irq = irq;
>     if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
>         polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
>           sci_irq_penalty = infinite;  /* can't use for PCI at all */
>     else
>       sci_irq_penalty = PIRQ_PENALTY_PCI_USING;
>   }
> 
>   static pci_irq_sharing_penalty(int irq)
>   {
>     struct acpi_pci_link *link;
>     int penalty = 0;
> 
>     list_for_each_entry(link, &acpi_link_list, list) {
> 
>       /*
>        * If a link is active, penalize its IRQ heavily so we try to choose
>        * a different IRQ.
>        */
>       if (link->irq.active && link->irq.active == irq)
>         penalty += PIRQ_PENALTY_PCI_USING;
>       else {
> 
>         /*
>          * If a link is inactive, penalize the IRQs it might use, but
>          * not as severely.
>          */ 
>         for (i = 0; i < link->irq.possible_count; i++)
>           if (link->irq.possible[i] == irq)
>             penalty += PIRQ_PENALTY_PCI_POSSIBLE;
>       }
>     }
> 
>     return penalty;
>   }
> 
>   int __weak pcibios_irq_penalty(int irq)
>   {
>     return 0;
>   }
> 
>   static int acpi_irq_get_penalty(int irq)
>   {
>     int penalty;
> 
>     penalty = pcibios_irq_penalty(irq);
> 
>     if (irq == sci_irq)
>       penalty += sci_irq_penalty;
> 
>     penalty += pci_irq_sharing_penalty(irq);
>     return penalty;
>   }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

On 3/2/2016 1:31 PM, Sinan Kaya wrote:
> I don't think there's a restriction on what the SCI IRQ can be.  But
>> there is only one SCI IRQ, so all we have to do is keep track of what
>> it is, which only requires one word.

Taking a step back here and also some inspiration from your code, why don't we
fix the actual problem instead of redesigning the whole thing?

The code failed because we didn't account for the non-ISA SCI IRQs. Below code
does this.


diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index fa28635..99d0716 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -87,6 +87,7 @@ struct acpi_pci_link {
 
 static LIST_HEAD(acpi_link_list);
 static DEFINE_MUTEX(acpi_link_lock);
+static int sci_irq, sci_irq_penalty;
 
 /* --------------------------------------------------------------------------
                             PCI Link Device Management
@@ -481,6 +482,9 @@ static int acpi_irq_get_penalty(int irq)
 	if (irq < ACPI_MAX_ISA_IRQ)
 		return acpi_irq_isa_penalty[irq];
 
+	if (irq == sci_irq)
+		return sci_penalty;
+
 	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
 		if (irq_info->irq == irq)
 			return irq_info->penalty;
@@ -507,6 +511,11 @@ static int acpi_irq_set_penalty(int irq, int new_penalty)
 		}
 	}
 
+	if (irq == sci_irq) {
+		sci_penalty = penalty;
+		return 0;
+	}
+
 	/* nope, let's allocate a slot for this IRQ */
 	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
 	if (!irq_info)
@@ -900,6 +909,7 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
 	if (irq < 0)
 		return;
 
+	sci_irq = irq;
 	if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
 	    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
 		penalty = PIRQ_PENALTY_ISA_ALWAYS; 



-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

Taking another stab at it.

On 3/2/2016 10:14 PM, Sinan Kaya wrote:
> Taking a step back here and also some inspiration from your code, why don't we
> fix the actual problem instead of redesigning the whole thing?

I read your email multiple times. I think you want to move the x86 specific pieces
(ISA interrupts and its command line arguments) out of the drivers\acpi\pci_link.c.
Is this right?

- the legacy ISA IRQs, i.e., the contents of acpi_irq_isa_penalty[]
- acpi_irq_isa= from command line

 int pcibios_irq_penalty(int irq)
  {
    if (irq >= ACPI_MAX_ISA_IRQ)
      return 0;

    return acpi_irq_isa_penalty[irq] + acpi_irq_cmd_line_penalty[irq];
  }


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Bjorn Helgaas]

On Thu, Mar 03, 2016 at 09:48:09AM -0500, Sinan Kaya wrote:
> Taking another stab at it.
> 
> On 3/2/2016 10:14 PM, Sinan Kaya wrote:
> > Taking a step back here and also some inspiration from your code, why don't we
> > fix the actual problem instead of redesigning the whole thing?
> 
> I read your email multiple times. I think you want to move the x86 specific pieces
> (ISA interrupts and its command line arguments) out of the drivers\acpi\pci_link.c.
> Is this right?

That was my idea, but your minimal patch from last night looks awfully
attractive, and maybe it's not worth moving it to arch/x86.  I do think we
could simplify the code significantly by getting rid of the kzalloc and
acpi_irq_penalty_list from acpi_irq_set_penalty().  How about pushing on
that a little bit first, and see what it looks like then?

> - the legacy ISA IRQs, i.e., the contents of acpi_irq_isa_penalty[]
> - acpi_irq_isa= from command line
> 
>  int pcibios_irq_penalty(int irq)
>   {
>     if (irq >= ACPI_MAX_ISA_IRQ)
>       return 0;
> 
>     return acpi_irq_isa_penalty[irq] + acpi_irq_cmd_line_penalty[irq];
>   }
> 
> 
> -- 
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

On 3/3/2016 10:10 AM, Bjorn Helgaas wrote:
> That was my idea, but your minimal patch from last night looks awfully
> attractive, and maybe it's not worth moving it to arch/x86.  I do think we
> could simplify the code significantly by getting rid of the kzalloc and
> acpi_irq_penalty_list from acpi_irq_set_penalty().  How about pushing on
> that a little bit first, and see what it looks like then?

OK. Let me go that direction.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On 3/3/2016 10:12 AM, Sinan Kaya wrote:
> On 3/3/2016 10:10 AM, Bjorn Helgaas wrote:
>> That was my idea, but your minimal patch from last night looks awfully
>> attractive, and maybe it's not worth moving it to arch/x86.  I do think we
>> could simplify the code significantly by getting rid of the kzalloc and
>> acpi_irq_penalty_list from acpi_irq_set_penalty().  How about pushing on
>> that a little bit first, and see what it looks like then?
> 
> OK. Let me go that direction.
> 

How about this?

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[-- Attachment #2: 0001-acpi-pci-irq-account-for-early-penalty-assignment.patch --]
[-- Type: text/plain, Size: 3418 bytes --]

>From 6cc33747feb469fe4da2088f34e2c875a36f58f4 Mon Sep 17 00:00:00 2001
From: Sinan Kaya <okaya@codeaurora.org>
Date: Thu, 3 Mar 2016 10:14:22 -0500
Subject: [PATCH] acpi,pci,irq: account for early penalty assignment

---
 drivers/acpi/pci_link.c | 77 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 30 deletions(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index fa28635..09eea42 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -87,6 +87,7 @@ struct acpi_pci_link {

 static LIST_HEAD(acpi_link_list);
 static DEFINE_MUTEX(acpi_link_lock);
+static int sci_irq, sci_irq_penalty;

 /* --------------------------------------------------------------------------
                             PCI Link Device Management
@@ -466,56 +467,71 @@ static int acpi_irq_isa_penalty[ACPI_MAX_ISA_IRQ] = {
 	PIRQ_PENALTY_ISA_USED,		/* IRQ15 ide1 */
 };

-struct irq_penalty_info {
-	int irq;
-	int penalty;
-	struct list_head node;
-};
+static int acpi_irq_pci_sharing_penalty(int irq)
+{
+	struct acpi_pci_link *link;
+	int penalty = 0;
+	bool found = false;
+
+	list_for_each_entry(link, &acpi_link_list, list) {
+		/*
+		 * If a link is active, penalize its IRQ heavily
+		 * so we try to choose a different IRQ.
+		 */
+		if (link->irq.active && link->irq.active == irq) {
+			penalty += PIRQ_PENALTY_PCI_USING;
+			found = true;
+		} else {
+			int i;
+
+			/*
+			 * If a link is inactive, penalize the IRQs it
+			 * might, but not as severely.
+			 */
+			for (i = 0; i < link->irq.possible_count; i++) {
+				if (link->irq.possible[i] == irq) {
+					penalty += PIRQ_PENALTY_PCI_POSSIBLE /
+						link->irq.possible_count;
+					found = true;
+				}
+			}
+		}
+	}

-static LIST_HEAD(acpi_irq_penalty_list);
+	if (found)
+		return penalty;
+
+	return PIRQ_PENALTY_PCI_AVAILABLE;
+}

 static int acpi_irq_get_penalty(int irq)
 {
-	struct irq_penalty_info *irq_info;
-
 	if (irq < ACPI_MAX_ISA_IRQ)
 		return acpi_irq_isa_penalty[irq];

-	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
-		if (irq_info->irq == irq)
-			return irq_info->penalty;
-	}
+	if (irq == sci_irq)
+		return sci_irq_penalty;

-	return 0;
+	return acpi_irq_pci_sharing_penalty(irq);
 }

 static int acpi_irq_set_penalty(int irq, int new_penalty)
 {
-	struct irq_penalty_info *irq_info;
-
 	/* see if this is a ISA IRQ */
 	if (irq < ACPI_MAX_ISA_IRQ) {
 		acpi_irq_isa_penalty[irq] = new_penalty;
 		return 0;
 	}

-	/* next, try to locate from the dynamic list */
-	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
-		if (irq_info->irq == irq) {
-			irq_info->penalty  = new_penalty;
-			return 0;
-		}
+	if (irq == sci_irq) {
+		sci_irq_penalty = new_penalty;
+		return 0;
 	}

-	/* nope, let's allocate a slot for this IRQ */
-	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
-	if (!irq_info)
-		return -ENOMEM;
-
-	irq_info->irq = irq;
-	irq_info->penalty = new_penalty;
-	list_add_tail(&irq_info->node, &acpi_irq_penalty_list);
-
+	/*
+	 * This is the remaining PCI IRQs. They are calculated on the
+	 * flight in acpi_irq_get_penalty function.
+	 */
 	return 0;
 }

@@ -900,6 +916,7 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
 	if (irq < 0)
 		return;

+	sci_irq = irq;
 	if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
 	    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
 		penalty = PIRQ_PENALTY_ISA_ALWAYS;
--
1.8.2.1

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Bjorn Helgaas]

On Thu, Mar 03, 2016 at 12:29:01PM -0500, Sinan Kaya wrote:
> On 3/3/2016 10:12 AM, Sinan Kaya wrote:
> > On 3/3/2016 10:10 AM, Bjorn Helgaas wrote:
> >> That was my idea, but your minimal patch from last night looks awfully
> >> attractive, and maybe it's not worth moving it to arch/x86.  I do think we
> >> could simplify the code significantly by getting rid of the kzalloc and
> >> acpi_irq_penalty_list from acpi_irq_set_penalty().  How about pushing on
> >> that a little bit first, and see what it looks like then?
> > 
> > OK. Let me go that direction.
> > 
> 
> How about this?
> 
> -- 
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

> From 6cc33747feb469fe4da2088f34e2c875a36f58f4 Mon Sep 17 00:00:00 2001
> From: Sinan Kaya <okaya@codeaurora.org>
> Date: Thu, 3 Mar 2016 10:14:22 -0500
> Subject: [PATCH] acpi,pci,irq: account for early penalty assignment
> 
> ---
>  drivers/acpi/pci_link.c | 77 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 47 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index fa28635..09eea42 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -87,6 +87,7 @@ struct acpi_pci_link {
> 
>  static LIST_HEAD(acpi_link_list);
>  static DEFINE_MUTEX(acpi_link_lock);
> +static int sci_irq, sci_irq_penalty;
> 
>  /* --------------------------------------------------------------------------
>                              PCI Link Device Management
> @@ -466,56 +467,71 @@ static int acpi_irq_isa_penalty[ACPI_MAX_ISA_IRQ] = {
>  	PIRQ_PENALTY_ISA_USED,		/* IRQ15 ide1 */
>  };
> 
> -struct irq_penalty_info {
> -	int irq;
> -	int penalty;
> -	struct list_head node;
> -};
> +static int acpi_irq_pci_sharing_penalty(int irq)
> +{
> +	struct acpi_pci_link *link;
> +	int penalty = 0;
> +	bool found = false;
> +
> +	list_for_each_entry(link, &acpi_link_list, list) {
> +		/*
> +		 * If a link is active, penalize its IRQ heavily
> +		 * so we try to choose a different IRQ.
> +		 */
> +		if (link->irq.active && link->irq.active == irq) {
> +			penalty += PIRQ_PENALTY_PCI_USING;
> +			found = true;
> +		} else {
> +			int i;
> +
> +			/*
> +			 * If a link is inactive, penalize the IRQs it
> +			 * might, but not as severely.

s/might/might use/

> +			 */
> +			for (i = 0; i < link->irq.possible_count; i++) {
> +				if (link->irq.possible[i] == irq) {
> +					penalty += PIRQ_PENALTY_PCI_POSSIBLE /
> +						link->irq.possible_count;
> +					found = true;
> +				}
> +			}
> +		}
> +	}
> 
> -static LIST_HEAD(acpi_irq_penalty_list);
> +	if (found)
> +		return penalty;
> +
> +	return PIRQ_PENALTY_PCI_AVAILABLE;

It's a nit, but I don't think "#define PIRQ_PENALTY_PCI_AVAILABLE 0"
adds any readability.  If we dropped that, you could get rid of
"found" as well, and simply "return penalty" here.

> +}
> 
>  static int acpi_irq_get_penalty(int irq)
>  {
> -	struct irq_penalty_info *irq_info;
> -
>  	if (irq < ACPI_MAX_ISA_IRQ)
>  		return acpi_irq_isa_penalty[irq];
> 
> -	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
> -		if (irq_info->irq == irq)
> -			return irq_info->penalty;
> -	}
> +	if (irq == sci_irq)
> +		return sci_irq_penalty;
> 
> -	return 0;
> +	return acpi_irq_pci_sharing_penalty(irq);

I think there are two issues here that should be teased apart a bit
more:

  1) Trigger settings: If the IRQ is configured as anything other than
  level-triggered, active-low, we can't use it at all for a PCI
  interrupt, and we should return an "infinite" penalty.  We currently
  increase the penalty for the SCI IRQ if it's not level/low, but
  doesn't it apply to *all* IRQs, not just the SCI IRQ?
  
  It looks like we do something similar in the pcibios_lookup_irq()
  loop that uses can_request_irq().  If we used can_request_irq() in
  pci_link.c, would that mean we could get rid of the SCI
  trigger/polarity stuff and just keep track of the SCI IRQ?  I don't
  know whether the SCI IRQ is hooked into whatever can_request_irq()
  uses, but it seems like it *should* be.

  2) Sharing with other devices: It seems useful to me to accumulate
  the penalties because even an IRQ in the 0-15 range might be used by
  PCI devices.  Wouldn't we want to count those users in addition to
  whatever ISA users there might be?  E.g., something like this:

    penalty = 0;

    if (irq < ACPI_MAX_ISA_IRQ)
      penalty += acpi_irq_isa_penalty[irq];

    if (irq == sci_irq)
      penalty += PIRQ_PENALTY_PCI_USING;

    penalty += acpi_irq_pci_sharing_penalty(irq);
    return penalty;

>  }
> 
>  static int acpi_irq_set_penalty(int irq, int new_penalty)
>  {
> -	struct irq_penalty_info *irq_info;
> -
>  	/* see if this is a ISA IRQ */
>  	if (irq < ACPI_MAX_ISA_IRQ) {
>  		acpi_irq_isa_penalty[irq] = new_penalty;
>  		return 0;
>  	}
> 
> -	/* next, try to locate from the dynamic list */
> -	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
> -		if (irq_info->irq == irq) {
> -			irq_info->penalty  = new_penalty;
> -			return 0;
> -		}
> +	if (irq == sci_irq) {
> +		sci_irq_penalty = new_penalty;
> +		return 0;
>  	}
> 
> -	/* nope, let's allocate a slot for this IRQ */
> -	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
> -	if (!irq_info)
> -		return -ENOMEM;
> -
> -	irq_info->irq = irq;
> -	irq_info->penalty = new_penalty;
> -	list_add_tail(&irq_info->node, &acpi_irq_penalty_list);
> -
> +	/*
> +	 * This is the remaining PCI IRQs. They are calculated on the
> +	 * flight in acpi_irq_get_penalty function.
> +	 */
>  	return 0;
>  }

If we adopt the idea that we compute the penalties on the fly in
acpi_irq_get_penalty(), I don't think we need acpi_irq_penalty_init()
any more.  It does basically the same thing acpi_irq_get_penalty()
would do, and it's confusing to do it twice.

The acpi_irq_add_penalty() call in acpi_pci_link_allocate() should go
away for the same reason.

The acpi_irq_add_penalty() call in acpi_penalize_sci_irq() should go
away (assuming we can use can_request_irq() or something similar to
validate trigger settings).

I think acpi_irq_add_penalty() itself could go away, since the only
remaining user would be the command-line processing, which could just
set the penalty directly.

> 
> @@ -900,6 +916,7 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
>  	if (irq < 0)
>  		return;
> 
> +	sci_irq = irq;

Possibly acpi_penalize_sci_irq() itself could go away, since all we
really need is the SCI IRQ, and we might be able to just use
acpi_gbl_FADT.sci_interrupt (I'm not 100% sure sci_interrupt is
identical to a Linux IRQ number; we'd have to check that).

>  	if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
>  	    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
>  		penalty = PIRQ_PENALTY_ISA_ALWAYS;
> --
> 1.8.2.1
> 

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

On 3/4/2016 1:09 PM, Bjorn Helgaas wrote:
> On Thu, Mar 03, 2016 at 12:29:01PM -0500, Sinan Kaya wrote:
>> On 3/3/2016 10:12 AM, Sinan Kaya wrote:
>>> On 3/3/2016 10:10 AM, Bjorn Helgaas wrote:
>>>> That was my idea, but your minimal patch from last night looks awfully
>>>> attractive, and maybe it's not worth moving it to arch/x86.  I do think we
>>>> could simplify the code significantly by getting rid of the kzalloc and
>>>> acpi_irq_penalty_list from acpi_irq_set_penalty().  How about pushing on
>>>> that a little bit first, and see what it looks like then?
>>>
>>> OK. Let me go that direction.
>>>
>>
>> How about this?
>>
>> -- 
>> Sinan Kaya
>> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
> 
>> From 6cc33747feb469fe4da2088f34e2c875a36f58f4 Mon Sep 17 00:00:00 2001
>> From: Sinan Kaya <okaya@codeaurora.org>
>> Date: Thu, 3 Mar 2016 10:14:22 -0500
>> Subject: [PATCH] acpi,pci,irq: account for early penalty assignment
>>
>> ---
>>  drivers/acpi/pci_link.c | 77 ++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 47 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index fa28635..09eea42 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -87,6 +87,7 @@ struct acpi_pci_link {
>>
>>  static LIST_HEAD(acpi_link_list);
>>  static DEFINE_MUTEX(acpi_link_lock);
>> +static int sci_irq, sci_irq_penalty;
>>
>>  /* --------------------------------------------------------------------------
>>                              PCI Link Device Management
>> @@ -466,56 +467,71 @@ static int acpi_irq_isa_penalty[ACPI_MAX_ISA_IRQ] = {
>>  	PIRQ_PENALTY_ISA_USED,		/* IRQ15 ide1 */
>>  };
>>
>> -struct irq_penalty_info {
>> -	int irq;
>> -	int penalty;
>> -	struct list_head node;
>> -};
>> +static int acpi_irq_pci_sharing_penalty(int irq)
>> +{
>> +	struct acpi_pci_link *link;
>> +	int penalty = 0;
>> +	bool found = false;
>> +
>> +	list_for_each_entry(link, &acpi_link_list, list) {
>> +		/*
>> +		 * If a link is active, penalize its IRQ heavily
>> +		 * so we try to choose a different IRQ.
>> +		 */
>> +		if (link->irq.active && link->irq.active == irq) {
>> +			penalty += PIRQ_PENALTY_PCI_USING;
>> +			found = true;
>> +		} else {
>> +			int i;
>> +
>> +			/*
>> +			 * If a link is inactive, penalize the IRQs it
>> +			 * might, but not as severely.
> 
> s/might/might use/
OK

> 
>> +			 */
>> +			for (i = 0; i < link->irq.possible_count; i++) {
>> +				if (link->irq.possible[i] == irq) {
>> +					penalty += PIRQ_PENALTY_PCI_POSSIBLE /
>> +						link->irq.possible_count;
>> +					found = true;
>> +				}
>> +			}
>> +		}
>> +	}
>>
>> -static LIST_HEAD(acpi_irq_penalty_list);
>> +	if (found)
>> +		return penalty;
>> +
>> +	return PIRQ_PENALTY_PCI_AVAILABLE;
> 
> It's a nit, but I don't think "#define PIRQ_PENALTY_PCI_AVAILABLE 0"
> adds any readability.  If we dropped that, you could get rid of
> "found" as well, and simply "return penalty" here.
> 

Right, I never looked at what PIRQ_PENALTY_PCI_AVAILABLE value is. I was trying to 
match what current code is doing. I'll get rid of PIRQ_PENALTY_PCI_AVAILABLE 
altogether.


>> +}
>>
>>  static int acpi_irq_get_penalty(int irq)
>>  {
>> -	struct irq_penalty_info *irq_info;
>> -
>>  	if (irq < ACPI_MAX_ISA_IRQ)
>>  		return acpi_irq_isa_penalty[irq];
>>
>> -	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
>> -		if (irq_info->irq == irq)
>> -			return irq_info->penalty;
>> -	}
>> +	if (irq == sci_irq)
>> +		return sci_irq_penalty;
>>
>> -	return 0;
>> +	return acpi_irq_pci_sharing_penalty(irq);
> 
> I think there are two issues here that should be teased apart a bit
> more:
> 
>   1) Trigger settings: If the IRQ is configured as anything other than
>   level-triggered, active-low, we can't use it at all for a PCI
>   interrupt, and we should return an "infinite" penalty.  We currently
>   increase the penalty for the SCI IRQ if it's not level/low, but
>   doesn't it apply to *all* IRQs, not just the SCI IRQ?
>   

It makes sense for SCI as it is Intel specific.

Unfortunately, this cannot be done in an arch independent way. Of course,
ARM had to implement its own thing. While level-triggered, active-low is
good for intel world, it is not for the ARM world. ARM uses active-high
level triggered.

Of course, we could do something like this and check for level.

+       if (IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64))
+               polarity = ACPI_ACTIVE_HIGH;
+

Let me know what you think.

>   It looks like we do something similar in the pcibios_lookup_irq()
>   loop that uses can_request_irq().  If we used can_request_irq() in
>   pci_link.c, would that mean we could get rid of the SCI
>   trigger/polarity stuff and just keep track of the SCI IRQ?  I don't
>   know whether the SCI IRQ is hooked into whatever can_request_irq()
>   uses, but it seems like it *should* be.

Sorry, you lost me here. We are only tracking sci_irq and sci_penalty. 
Do you want to add an additional check here? something like this?

        if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
-           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
+           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW) &&
+           !can_request_irq(irq, IRQF_SHARED | IRQF_TRIGGER_LOW))

> 
>   2) Sharing with other devices: It seems useful to me to accumulate
>   the penalties because even an IRQ in the 0-15 range might be used by
>   PCI devices.  Wouldn't we want to count those users in addition to
>   whatever ISA users there might be?  E.g., something like this:
> 
>     penalty = 0;
> 
>     if (irq < ACPI_MAX_ISA_IRQ)
>       penalty += acpi_irq_isa_penalty[irq];
> 
>     if (irq == sci_irq)
>       penalty += PIRQ_PENALTY_PCI_USING;
> 
>     penalty += acpi_irq_pci_sharing_penalty(irq);
>     return penalty;
> 
>>  }

Sure, makes sense. Changed it like above.

>>
>>  static int acpi_irq_set_penalty(int irq, int new_penalty)
>>  {
>> -	struct irq_penalty_info *irq_info;
>> -
>>  	/* see if this is a ISA IRQ */
>>  	if (irq < ACPI_MAX_ISA_IRQ) {
>>  		acpi_irq_isa_penalty[irq] = new_penalty;
>>  		return 0;
>>  	}
>>
>> -	/* next, try to locate from the dynamic list */
>> -	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
>> -		if (irq_info->irq == irq) {
>> -			irq_info->penalty  = new_penalty;
>> -			return 0;
>> -		}
>> +	if (irq == sci_irq) {
>> +		sci_irq_penalty = new_penalty;
>> +		return 0;
>>  	}
>>
>> -	/* nope, let's allocate a slot for this IRQ */
>> -	irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
>> -	if (!irq_info)
>> -		return -ENOMEM;
>> -
>> -	irq_info->irq = irq;
>> -	irq_info->penalty = new_penalty;
>> -	list_add_tail(&irq_info->node, &acpi_irq_penalty_list);
>> -
>> +	/*
>> +	 * This is the remaining PCI IRQs. They are calculated on the
>> +	 * flight in acpi_irq_get_penalty function.
>> +	 */
>>  	return 0;
>>  }
> 
> If we adopt the idea that we compute the penalties on the fly in
> acpi_irq_get_penalty(), I don't think we need acpi_irq_penalty_init()
> any more.  It does basically the same thing acpi_irq_get_penalty()
> would do, and it's confusing to do it twice.

Agreed, I was going to take it out. I didn't want to get on it yet. Emptied
the function now.

> 
> The acpi_irq_add_penalty() call in acpi_pci_link_allocate() should go
> away for the same reason.

got it.

> 
> The acpi_irq_add_penalty() call in acpi_penalize_sci_irq() should go
> away (assuming we can use can_request_irq() or something similar to
> validate trigger settings).

@@ -917,13 +880,15 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
                return;

        sci_irq = irq;
+
        if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
-           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
+           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW) &&
+           !can_request_irq(irq, IRQF_SHARED | IRQF_TRIGGER_LOW))
                penalty = PIRQ_PENALTY_ISA_ALWAYS;
        else
                penalty = PIRQ_PENALTY_PCI_USING;

-       acpi_irq_add_penalty(irq, penalty);
+       sci_penalty = penalty;

> 
> I think acpi_irq_add_penalty() itself could go away, since the only
> remaining user would be the command-line processing, which could just
> set the penalty directly.

OK, this is how I changed acpi_penalize_isa_irq.

@@ -894,8 +857,8 @@ static int __init acpi_irq_penalty_update(char *str, int used)
 void acpi_penalize_isa_irq(int irq, int active)
 {
        if (irq >= 0)
-               acpi_irq_add_penalty(irq, active ?
-                       PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
+               acpi_irq_isa_penalty[irq] = active ?
+                       PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING;
 }


> 
>>
>> @@ -900,6 +916,7 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
>>  	if (irq < 0)
>>  		return;
>>
>> +	sci_irq = irq;
> 
> Possibly acpi_penalize_sci_irq() itself could go away, since all we
> really need is the SCI IRQ, and we might be able to just use
> acpi_gbl_FADT.sci_interrupt (I'm not 100% sure sci_interrupt is
> identical to a Linux IRQ number; we'd have to check that).

Is SCI IRQ exclusive in general? We are now keeping track of SCI IRQ and 
adding penalty when irq matches sci_irq in get_penalty function. 

How do we make sci_irq_penalty and acpi_penalize_sci_irq disappear?

> 
>>  	if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
>>  	    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
>>  		penalty = PIRQ_PENALTY_ISA_ALWAYS;
>> --
>> 1.8.2.1
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Bjorn Helgaas]

[+cc Thomas, irq_get_trigger_type() question below]

On Mon, Mar 07, 2016 at 11:55:43AM -0500, Sinan Kaya wrote:
> On 3/4/2016 1:09 PM, Bjorn Helgaas wrote:
> > On Thu, Mar 03, 2016 at 12:29:01PM -0500, Sinan Kaya wrote:
> >> On 3/3/2016 10:12 AM, Sinan Kaya wrote:
> >>> On 3/3/2016 10:10 AM, Bjorn Helgaas wrote:

> >> From 6cc33747feb469fe4da2088f34e2c875a36f58f4 Mon Sep 17 00:00:00 2001
> >> From: Sinan Kaya <okaya@codeaurora.org>
> >> Date: Thu, 3 Mar 2016 10:14:22 -0500
> >> Subject: [PATCH] acpi,pci,irq: account for early penalty assignment
> >>
> >> ---
> >>  drivers/acpi/pci_link.c | 77 ++++++++++++++++++++++++++++++-------------------
> >>  1 file changed, 47 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> >> index fa28635..09eea42 100644
> >> --- a/drivers/acpi/pci_link.c
> >> +++ b/drivers/acpi/pci_link.c

> >>  static int acpi_irq_get_penalty(int irq)
> >>  {
> >> -	struct irq_penalty_info *irq_info;
> >> -
> >>  	if (irq < ACPI_MAX_ISA_IRQ)
> >>  		return acpi_irq_isa_penalty[irq];
> >>
> >> -	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
> >> -		if (irq_info->irq == irq)
> >> -			return irq_info->penalty;
> >> -	}
> >> +	if (irq == sci_irq)
> >> +		return sci_irq_penalty;
> >>
> >> -	return 0;
> >> +	return acpi_irq_pci_sharing_penalty(irq);
> > 
> > I think there are two issues here that should be teased apart a bit
> > more:
> > 
> >   1) Trigger settings: If the IRQ is configured as anything other than
> >   level-triggered, active-low, we can't use it at all for a PCI
> >   interrupt, and we should return an "infinite" penalty.  We currently
> >   increase the penalty for the SCI IRQ if it's not level/low, but
> >   doesn't it apply to *all* IRQs, not just the SCI IRQ?
> 
> It makes sense for SCI as it is Intel specific.
> 
> Unfortunately, this cannot be done in an arch independent way. Of course,
> ARM had to implement its own thing. While level-triggered, active-low is
> good for intel world, it is not for the ARM world. ARM uses active-high
> level triggered.

I'm confused.  I don't think SCI is Intel-specific.  Per PCI Spec
r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low.
Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable,
level interrupt".

Are you saying SCI is active-high on ARM?  If so, I don't think that's
necessarily a huge problem, although we'd have to audit the ACPI code
to make sure we handle it correctly.

The point here is that a PCI Interrupt Link can only use an IRQ that
is level-triggered, active low.  If an IRQ is already set to any other
state, whether for an ISA device or for an active-high SCI, we can't
use it for a PCI Interrupt Link.

It'd be nice if there were a generic way we could figure out what the
trigger mode of an IRQ is.  I was hoping can_request_irq() was that
way, but I don't think it is, because it only looks at IRQF_SHARED,
not at IRQF_TRIGGER_LOW.

Maybe irq_get_trigger_type() is what we want?

> >   It looks like we do something similar in the pcibios_lookup_irq()
> >   loop that uses can_request_irq().  If we used can_request_irq() in
> >   pci_link.c, would that mean we could get rid of the SCI
> >   trigger/polarity stuff and just keep track of the SCI IRQ?  I don't
> >   know whether the SCI IRQ is hooked into whatever can_request_irq()
> >   uses, but it seems like it *should* be.
> 
> Sorry, you lost me here. We are only tracking sci_irq and sci_penalty. 
> Do you want to add an additional check here? something like this?
> 
>         if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
> -           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> +           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW) &&
> +           !can_request_irq(irq, IRQF_SHARED | IRQF_TRIGGER_LOW))

I'm saying:

  - If the IRQ trigger type is anything other than level/low, reject
    this IRQ, and

  - If this IRQ is the SCI IRQ, penalize the IRQ as though we have a
    PCI device already using it.

> > If we adopt the idea that we compute the penalties on the fly in
> > acpi_irq_get_penalty(), I don't think we need acpi_irq_penalty_init()
> > any more.  It does basically the same thing acpi_irq_get_penalty()
> > would do, and it's confusing to do it twice.
> 
> Agreed, I was going to take it out. I didn't want to get on it yet. Emptied
> the function now.

You might be able to do this incrementally, in several patches, and
I'd prefer that if it's possible.  It's much easier to review patches
if each one is as small as possible and changes only one thing at a
time.

> >> @@ -900,6 +916,7 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
> >>  	if (irq < 0)
> >>  		return;
> >>
> >> +	sci_irq = irq;
> > 
> > Possibly acpi_penalize_sci_irq() itself could go away, since all we
> > really need is the SCI IRQ, and we might be able to just use
> > acpi_gbl_FADT.sci_interrupt (I'm not 100% sure sci_interrupt is
> > identical to a Linux IRQ number; we'd have to check that).
> 
> Is SCI IRQ exclusive in general? We are now keeping track of SCI IRQ and 
> adding penalty when irq matches sci_irq in get_penalty function. 
> 
> How do we make sci_irq_penalty and acpi_penalize_sci_irq disappear?

I don't think the SCI IRQ needs to be exclusive.  The ACPI spec says
it should be sharable, as long as the other devices use a compatible
trigger mode (level/low per spec).

If we know the SCI IRQ, we don't need sci_irq_penalty or
acpi_penalize_sci_irq() because we can penalize an IRQ with the
PIRQ_PENALTY_PCI_USING, something like this:

  static int pci_compatible_trigger(int irq)
  {
    int type = irq_get_trigger_type(irq);

    return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE);
  }

  static unsigned int acpi_irq_get_penalty(int irq)
  {
    unsigned int penalty = 0;

    if (irq == acpi_gbl_FADT.sci_interrupt)
      penalty += PIRQ_PENALTY_PCI_USING;

    penalty += acpi_irq_pci_sharing_penalty(irq);
    return penalty;
  }

  static int acpi_pci_link_allocate(struct acpi_pci_link *link)
  {
    unsigned int best = ~0;
    ...

    for (i = (link->irq.possible_count - 1); i >= 0; i--) {
      candidate = link->irq.possible[i];
      if (!pci_compatible_trigger(candidate))
        continue;

      penalty = acpi_irq_get_penalty(candidate);
      if (penalty < best) {
        irq = candidate;
        best = penalty;
      }
    }
    ...
  }

This looks racy, because we test irq_get_trigger_type() without any
kind of locking, and later acpi_register_gsi() calls
irq_create_fwspec_mapping(), which looks like it sets the new trigger
type.  But I don't know how to fix that.

Bjorn

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Bjorn Helgaas]

[+cc Thomas for real, sorry]

On Mon, Mar 07, 2016 at 06:25:58PM -0600, Bjorn Helgaas wrote:
> [+cc Thomas, irq_get_trigger_type() question below]
> 
> On Mon, Mar 07, 2016 at 11:55:43AM -0500, Sinan Kaya wrote:
> > On 3/4/2016 1:09 PM, Bjorn Helgaas wrote:
> > > On Thu, Mar 03, 2016 at 12:29:01PM -0500, Sinan Kaya wrote:
> > >> On 3/3/2016 10:12 AM, Sinan Kaya wrote:
> > >>> On 3/3/2016 10:10 AM, Bjorn Helgaas wrote:
> 
> > >> From 6cc33747feb469fe4da2088f34e2c875a36f58f4 Mon Sep 17 00:00:00 2001
> > >> From: Sinan Kaya <okaya@codeaurora.org>
> > >> Date: Thu, 3 Mar 2016 10:14:22 -0500
> > >> Subject: [PATCH] acpi,pci,irq: account for early penalty assignment
> > >>
> > >> ---
> > >>  drivers/acpi/pci_link.c | 77 ++++++++++++++++++++++++++++++-------------------
> > >>  1 file changed, 47 insertions(+), 30 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> > >> index fa28635..09eea42 100644
> > >> --- a/drivers/acpi/pci_link.c
> > >> +++ b/drivers/acpi/pci_link.c
> 
> > >>  static int acpi_irq_get_penalty(int irq)
> > >>  {
> > >> -	struct irq_penalty_info *irq_info;
> > >> -
> > >>  	if (irq < ACPI_MAX_ISA_IRQ)
> > >>  		return acpi_irq_isa_penalty[irq];
> > >>
> > >> -	list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
> > >> -		if (irq_info->irq == irq)
> > >> -			return irq_info->penalty;
> > >> -	}
> > >> +	if (irq == sci_irq)
> > >> +		return sci_irq_penalty;
> > >>
> > >> -	return 0;
> > >> +	return acpi_irq_pci_sharing_penalty(irq);
> > > 
> > > I think there are two issues here that should be teased apart a bit
> > > more:
> > > 
> > >   1) Trigger settings: If the IRQ is configured as anything other than
> > >   level-triggered, active-low, we can't use it at all for a PCI
> > >   interrupt, and we should return an "infinite" penalty.  We currently
> > >   increase the penalty for the SCI IRQ if it's not level/low, but
> > >   doesn't it apply to *all* IRQs, not just the SCI IRQ?
> > 
> > It makes sense for SCI as it is Intel specific.
> > 
> > Unfortunately, this cannot be done in an arch independent way. Of course,
> > ARM had to implement its own thing. While level-triggered, active-low is
> > good for intel world, it is not for the ARM world. ARM uses active-high
> > level triggered.
> 
> I'm confused.  I don't think SCI is Intel-specific.  Per PCI Spec
> r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low.
> Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable,
> level interrupt".
> 
> Are you saying SCI is active-high on ARM?  If so, I don't think that's
> necessarily a huge problem, although we'd have to audit the ACPI code
> to make sure we handle it correctly.
> 
> The point here is that a PCI Interrupt Link can only use an IRQ that
> is level-triggered, active low.  If an IRQ is already set to any other
> state, whether for an ISA device or for an active-high SCI, we can't
> use it for a PCI Interrupt Link.
> 
> It'd be nice if there were a generic way we could figure out what the
> trigger mode of an IRQ is.  I was hoping can_request_irq() was that
> way, but I don't think it is, because it only looks at IRQF_SHARED,
> not at IRQF_TRIGGER_LOW.
> 
> Maybe irq_get_trigger_type() is what we want?
> 
> > >   It looks like we do something similar in the pcibios_lookup_irq()
> > >   loop that uses can_request_irq().  If we used can_request_irq() in
> > >   pci_link.c, would that mean we could get rid of the SCI
> > >   trigger/polarity stuff and just keep track of the SCI IRQ?  I don't
> > >   know whether the SCI IRQ is hooked into whatever can_request_irq()
> > >   uses, but it seems like it *should* be.
> > 
> > Sorry, you lost me here. We are only tracking sci_irq and sci_penalty. 
> > Do you want to add an additional check here? something like this?
> > 
> >         if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
> > -           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> > +           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW) &&
> > +           !can_request_irq(irq, IRQF_SHARED | IRQF_TRIGGER_LOW))
> 
> I'm saying:
> 
>   - If the IRQ trigger type is anything other than level/low, reject
>     this IRQ, and
> 
>   - If this IRQ is the SCI IRQ, penalize the IRQ as though we have a
>     PCI device already using it.
> 
> > > If we adopt the idea that we compute the penalties on the fly in
> > > acpi_irq_get_penalty(), I don't think we need acpi_irq_penalty_init()
> > > any more.  It does basically the same thing acpi_irq_get_penalty()
> > > would do, and it's confusing to do it twice.
> > 
> > Agreed, I was going to take it out. I didn't want to get on it yet. Emptied
> > the function now.
> 
> You might be able to do this incrementally, in several patches, and
> I'd prefer that if it's possible.  It's much easier to review patches
> if each one is as small as possible and changes only one thing at a
> time.
> 
> > >> @@ -900,6 +916,7 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
> > >>  	if (irq < 0)
> > >>  		return;
> > >>
> > >> +	sci_irq = irq;
> > > 
> > > Possibly acpi_penalize_sci_irq() itself could go away, since all we
> > > really need is the SCI IRQ, and we might be able to just use
> > > acpi_gbl_FADT.sci_interrupt (I'm not 100% sure sci_interrupt is
> > > identical to a Linux IRQ number; we'd have to check that).
> > 
> > Is SCI IRQ exclusive in general? We are now keeping track of SCI IRQ and 
> > adding penalty when irq matches sci_irq in get_penalty function. 
> > 
> > How do we make sci_irq_penalty and acpi_penalize_sci_irq disappear?
> 
> I don't think the SCI IRQ needs to be exclusive.  The ACPI spec says
> it should be sharable, as long as the other devices use a compatible
> trigger mode (level/low per spec).
> 
> If we know the SCI IRQ, we don't need sci_irq_penalty or
> acpi_penalize_sci_irq() because we can penalize an IRQ with the
> PIRQ_PENALTY_PCI_USING, something like this:
> 
>   static int pci_compatible_trigger(int irq)
>   {
>     int type = irq_get_trigger_type(irq);
> 
>     return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE);
>   }
> 
>   static unsigned int acpi_irq_get_penalty(int irq)
>   {
>     unsigned int penalty = 0;
> 
>     if (irq == acpi_gbl_FADT.sci_interrupt)
>       penalty += PIRQ_PENALTY_PCI_USING;
> 
>     penalty += acpi_irq_pci_sharing_penalty(irq);
>     return penalty;
>   }
> 
>   static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>   {
>     unsigned int best = ~0;
>     ...
> 
>     for (i = (link->irq.possible_count - 1); i >= 0; i--) {
>       candidate = link->irq.possible[i];
>       if (!pci_compatible_trigger(candidate))
>         continue;
> 
>       penalty = acpi_irq_get_penalty(candidate);
>       if (penalty < best) {
>         irq = candidate;
>         best = penalty;
>       }
>     }
>     ...
>   }
> 
> This looks racy, because we test irq_get_trigger_type() without any
> kind of locking, and later acpi_register_gsi() calls
> irq_create_fwspec_mapping(), which looks like it sets the new trigger
> type.  But I don't know how to fix that.
> 
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Thomas Gleixner]

On Mon, 7 Mar 2016, Bjorn Helgaas wrote:
> On Mon, Mar 07, 2016 at 11:55:43AM -0500, Sinan Kaya wrote:
> > It makes sense for SCI as it is Intel specific.
> > 
> > Unfortunately, this cannot be done in an arch independent way. Of course,
> > ARM had to implement its own thing. While level-triggered, active-low is
> > good for intel world, it is not for the ARM world. ARM uses active-high
> > level triggered.
> 
> I'm confused.  I don't think SCI is Intel-specific.  Per PCI Spec
> r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low.
> Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable,
> level interrupt".
> 
> Are you saying SCI is active-high on ARM?  If so, I don't think that's
> necessarily a huge problem, although we'd have to audit the ACPI code
> to make sure we handle it correctly.
> 
> The point here is that a PCI Interrupt Link can only use an IRQ that
> is level-triggered, active low.  If an IRQ is already set to any other
> state, whether for an ISA device or for an active-high SCI, we can't
> use it for a PCI Interrupt Link.
> 
> It'd be nice if there were a generic way we could figure out what the
> trigger mode of an IRQ is.  I was hoping can_request_irq() was that
> way, but I don't think it is, because it only looks at IRQF_SHARED,
> not at IRQF_TRIGGER_LOW.
> 
> Maybe irq_get_trigger_type() is what we want?

Yes, that gives you the trigger typ, if the interrupt is already set up.
 
>   static int pci_compatible_trigger(int irq)
>   {
>     int type = irq_get_trigger_type(irq);
> 
>     return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE);
>   }
> 
>   static unsigned int acpi_irq_get_penalty(int irq)
>   {
>     unsigned int penalty = 0;
> 
>     if (irq == acpi_gbl_FADT.sci_interrupt)
>       penalty += PIRQ_PENALTY_PCI_USING;
> 
>     penalty += acpi_irq_pci_sharing_penalty(irq);
>     return penalty;
>   }
> 
>   static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>   {
>     unsigned int best = ~0;
>     ...
> 
>     for (i = (link->irq.possible_count - 1); i >= 0; i--) {
>       candidate = link->irq.possible[i];
>       if (!pci_compatible_trigger(candidate))
>         continue;
> 
>       penalty = acpi_irq_get_penalty(candidate);
>       if (penalty < best) {
>         irq = candidate;
>         best = penalty;
>       }
>     }
>     ...
>   }
> 
> This looks racy, because we test irq_get_trigger_type() without any
> kind of locking, and later acpi_register_gsi() calls
> irq_create_fwspec_mapping(), which looks like it sets the new trigger
> type.  But I don't know how to fix that.

Right, if that pci link allocation code can be executed concurrent, then you
might end up with problem, but isn't that a problem even without
irq_get_trigger_type()?

Thanks,

	tglx

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Bjorn Helgaas]

On Tue, Mar 08, 2016 at 09:22:13AM +0100, Thomas Gleixner wrote:
> On Mon, 7 Mar 2016, Bjorn Helgaas wrote:
> > On Mon, Mar 07, 2016 at 11:55:43AM -0500, Sinan Kaya wrote:
> > > It makes sense for SCI as it is Intel specific.
> > > 
> > > Unfortunately, this cannot be done in an arch independent way. Of course,
> > > ARM had to implement its own thing. While level-triggered, active-low is
> > > good for intel world, it is not for the ARM world. ARM uses active-high
> > > level triggered.
> > 
> > I'm confused.  I don't think SCI is Intel-specific.  Per PCI Spec
> > r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low.
> > Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable,
> > level interrupt".
> > 
> > Are you saying SCI is active-high on ARM?  If so, I don't think that's
> > necessarily a huge problem, although we'd have to audit the ACPI code
> > to make sure we handle it correctly.
> > 
> > The point here is that a PCI Interrupt Link can only use an IRQ that
> > is level-triggered, active low.  If an IRQ is already set to any other
> > state, whether for an ISA device or for an active-high SCI, we can't
> > use it for a PCI Interrupt Link.
> > 
> > It'd be nice if there were a generic way we could figure out what the
> > trigger mode of an IRQ is.  I was hoping can_request_irq() was that
> > way, but I don't think it is, because it only looks at IRQF_SHARED,
> > not at IRQF_TRIGGER_LOW.
> > 
> > Maybe irq_get_trigger_type() is what we want?
> 
> Yes, that gives you the trigger typ, if the interrupt is already set up.
>  
> >   static int pci_compatible_trigger(int irq)
> >   {
> >     int type = irq_get_trigger_type(irq);
> > 
> >     return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE);
> >   }
> > 
> >   static unsigned int acpi_irq_get_penalty(int irq)
> >   {
> >     unsigned int penalty = 0;
> > 
> >     if (irq == acpi_gbl_FADT.sci_interrupt)
> >       penalty += PIRQ_PENALTY_PCI_USING;
> > 
> >     penalty += acpi_irq_pci_sharing_penalty(irq);
> >     return penalty;
> >   }
> > 
> >   static int acpi_pci_link_allocate(struct acpi_pci_link *link)
> >   {
> >     unsigned int best = ~0;
> >     ...
> > 
> >     for (i = (link->irq.possible_count - 1); i >= 0; i--) {
> >       candidate = link->irq.possible[i];
> >       if (!pci_compatible_trigger(candidate))
> >         continue;
> > 
> >       penalty = acpi_irq_get_penalty(candidate);
> >       if (penalty < best) {
> >         irq = candidate;
> >         best = penalty;
> >       }
> >     }
> >     ...
> >   }
> > 
> > This looks racy, because we test irq_get_trigger_type() without any
> > kind of locking, and later acpi_register_gsi() calls
> > irq_create_fwspec_mapping(), which looks like it sets the new trigger
> > type.  But I don't know how to fix that.
> 
> Right, if that pci link allocation code can be executed concurrent, then you
> might end up with problem, but isn't that a problem even without
> irq_get_trigger_type()?

Yes.  It's not a new problem, I just noticed it since we're thinking
more about the details of what's happening here.

Bjorn

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

>>>> I think there are two issues here that should be teased apart a bit
>>>> more:
>>>>
>>>>   1) Trigger settings: If the IRQ is configured as anything other than
>>>>   level-triggered, active-low, we can't use it at all for a PCI
>>>>   interrupt, and we should return an "infinite" penalty.  We currently
>>>>   increase the penalty for the SCI IRQ if it's not level/low, but
>>>>   doesn't it apply to *all* IRQs, not just the SCI IRQ?
>>>
>>> It makes sense for SCI as it is Intel specific.
>>>
>>> Unfortunately, this cannot be done in an arch independent way. Of course,
>>> ARM had to implement its own thing. While level-triggered, active-low is
>>> good for intel world, it is not for the ARM world. ARM uses active-high
>>> level triggered.
>>
>> I'm confused.  I don't think SCI is Intel-specific.  Per PCI Spec
>> r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low.
>> Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable,
>> level interrupt".
>>
>> Are you saying SCI is active-high on ARM?  If so, I don't think that's
>> necessarily a huge problem, although we'd have to audit the ACPI code
>> to make sure we handle it correctly.

We don't have an SCI interrupt on ARM. That's why, I assumed it is an Intel specific
interrupt. However, all legacy interrupts are active-high level sensitive. This is a
limitation of the ARM GIC Interrupt Controller.

Here is what a PCI Link object looks like.

Device(LN0D){
	Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
	Name(_UID, 4)
	Name(_PRS, ResourceTemplate(){
		Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {123}
	})
	Method(_DIS) {}
	Method(_CRS) { Return (_PRS) }
	Method(_SRS, 1) {}
} 

>>
>> The point here is that a PCI Interrupt Link can only use an IRQ that
>> is level-triggered, active low.  If an IRQ is already set to any other
>> state, whether for an ISA device or for an active-high SCI, we can't
>> use it for a PCI Interrupt Link.

Unfortunately, this still doesn't hold. 

A patch is long overdue for this series. I'll post v3. We can go from there.


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Rafael J. Wysocki]

On Tue, Mar 8, 2016 at 8:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>
>>>>> I think there are two issues here that should be teased apart a bit
>>>>> more:
>>>>>
>>>>>   1) Trigger settings: If the IRQ is configured as anything other than
>>>>>   level-triggered, active-low, we can't use it at all for a PCI
>>>>>   interrupt, and we should return an "infinite" penalty.  We currently
>>>>>   increase the penalty for the SCI IRQ if it's not level/low, but
>>>>>   doesn't it apply to *all* IRQs, not just the SCI IRQ?
>>>>
>>>> It makes sense for SCI as it is Intel specific.
>>>>
>>>> Unfortunately, this cannot be done in an arch independent way. Of course,
>>>> ARM had to implement its own thing. While level-triggered, active-low is
>>>> good for intel world, it is not for the ARM world. ARM uses active-high
>>>> level triggered.
>>>
>>> I'm confused.  I don't think SCI is Intel-specific.  Per PCI Spec
>>> r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low.
>>> Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable,
>>> level interrupt".

That's correct.

The SCI isn't Intel-specific or even x86-specific for that matter.
However, it is not available on hardware-reduced ACPI platforms which
are all ARM using ACPI.

>>> Are you saying SCI is active-high on ARM?  If so, I don't think that's
>>> necessarily a huge problem, although we'd have to audit the ACPI code
>>> to make sure we handle it correctly.
>
> We don't have an SCI interrupt on ARM. That's why, I assumed it is an Intel specific
> interrupt. However, all legacy interrupts are active-high level sensitive. This is a
> limitation of the ARM GIC Interrupt Controller.
>
> Here is what a PCI Link object looks like.
>
> Device(LN0D){
>         Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
>         Name(_UID, 4)
>         Name(_PRS, ResourceTemplate(){
>                 Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {123}
>         })
>         Method(_DIS) {}
>         Method(_CRS) { Return (_PRS) }
>         Method(_SRS, 1) {}
> }
>
>>>
>>> The point here is that a PCI Interrupt Link can only use an IRQ that
>>> is level-triggered, active low.  If an IRQ is already set to any other
>>> state, whether for an ISA device or for an active-high SCI, we can't
>>> use it for a PCI Interrupt Link.
>
> Unfortunately, this still doesn't hold.

I think that the original active-low requirement is related to the
fact that those IRQ can be shared.  If they aren't shared, I guess the
point is slightly moot.

Thanks,
Rafael

Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment

[Sinan Kaya]

Hi Bjorn,

On 3/8/2016 2:04 PM, Sinan Kaya wrote:
>>> The point here is that a PCI Interrupt Link can only use an IRQ that
>>> >> is level-triggered, active low.  If an IRQ is already set to any other
>>> >> state, whether for an ISA device or for an active-high SCI, we can't
>>> >> use it for a PCI Interrupt Link.
> Unfortunately, this still doesn't hold. 
> 
> A patch is long overdue for this series. I'll post v3. We can go from there.

I just posted a new series and changed the title. Let's continue the discussion there.

[PATCH 1/4] acpi,pci,irq: reduce resource requirements
...

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project