Re: [PATCH] acpi: add support for extended IRQ to PCI link

[Andy Shevchenko]

On Sun, Nov 8, 2015 at 6:07 PM, Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
> The ACPI compiler uses the extended format when
> used interrupt numbers are greater than 256.
> The PCI link code currently only supports simple
> interrupt format. The IRQ numbers are represented
> using 32 bits when extended IRQ syntax. This patch
> changes the interrupt number type to 32 bits and
> places an upper limit of 1020 as possible interrupt
> id. Additional checks have been placed to prevent
> out of bounds writes.

In commit messages and in comments I see this narrow lines, any reason
to make them short, except increasing number of lines?

> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -1,6 +1,7 @@
>  /*
>   *  pci_link.c - ACPI PCI Interrupt Link Device Driver ($Revision: 34 $)
>   *
> + *  Copyright (c) 2015, The Linux Foundation. All rights reserved.
>   *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@xxxxxxxxx>
>   *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh@xxxxxxxxx>
>   *  Copyright (C) 2002       Dominik Brodowski <devel@xxxxxxxx>
> @@ -67,12 +68,12 @@ static struct acpi_scan_handler pci_link_handler = {
>   * later even the link is disable. Instead, we just repick the active irq
>   */
>  struct acpi_pci_link_irq {
> -       u8 active;              /* Current IRQ */
> +       u32 active;             /* Current IRQ */
>         u8 triggering;          /* All IRQs */
>         u8 polarity;            /* All IRQs */
>         u8 resource_type;
>         u8 possible_count;
> -       u8 possible[ACPI_PCI_LINK_MAX_POSSIBLE];
> +       u32 possible[ACPI_PCI_LINK_MAX_POSSIBLE];
>         u8 initialized:1;
>         u8 reserved:7;
>  };
> @@ -437,7 +438,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
>   * enabled system.
>   */
>
> -#define ACPI_MAX_IRQS          256
> +#define ACPI_MAX_IRQS          1020
>  #define ACPI_MAX_ISA_IRQ       16
>
>  #define PIRQ_PENALTY_PCI_AVAILABLE     (0)
> @@ -493,7 +494,8 @@ int __init acpi_irq_penalty_init(void)
>                                             penalty;
>                         }
>
> -               } else if (link->irq.active) {
> +               } else if (link->irq.active &&
> +                       (link->irq.active < ACPI_MAX_IRQS)) {
>                         acpi_irq_penalty[link->irq.active] +=
>                             PIRQ_PENALTY_PCI_POSSIBLE;
>                 }
> @@ -542,14 +544,19 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>                 irq = link->irq.possible[link->irq.possible_count - 1];
>
>         if (acpi_irq_balance || !link->irq.active) {
> -               /*
> -                * Select the best IRQ.  This is done in reverse to promote
> -                * the use of IRQs 9, 10, 11, and >15.
> -                */
> -               for (i = (link->irq.possible_count - 1); i >= 0; i--) {
> -                       if (acpi_irq_penalty[irq] >
> -                           acpi_irq_penalty[link->irq.possible[i]])
> -                               irq = link->irq.possible[i];
> +
> +               if (irq < ACPI_MAX_IRQS) {
> +                       /*
> +                        * Select the best IRQ.  This is done in reverse to
> +                        * promote the use of IRQs 9, 10, 11, and >15.
> +                        */
> +                       for (i = (link->irq.possible_count - 1); i >= 0;
> +                               i--) {

Why not

if ((acpi_irq_balance || !link->irq.active) && irq < ACPI_MAX_IRQS) {
 int i = link->irq.possible_count;

 while (--i) {
 â
 }
}

> +                               if ((link->irq.possible[i] < ACPI_MAX_IRQS) &&
> +                                   (acpi_irq_penalty[irq] >
> +                                   acpi_irq_penalty[link->irq.possible[i]]))
> +                                       irq = link->irq.possible[i];
> +                       }
>                 }
>         }
>         if (acpi_irq_penalty[irq] >= PIRQ_PENALTY_ISA_ALWAYS) {

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/