Message-ID: <20161019224405.GA14915@localhost>
Date:   Wed, 19 Oct 2016 17:44:05 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Sinan Kaya <okaya@...eaurora.org>
Cc:     linux-acpi@...r.kernel.org, rjw@...ysocki.net, bhelgaas@...gle.com,
        ravikanth.nalla@....com, linux@...nbow-software.org,
        timur@...eaurora.org, cov@...eaurora.org, jcm@...hat.com,
        alex.williamson@...hat.com, linux-pci@...r.kernel.org,
        agross@...eaurora.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, wim@....tudelft.nl,
        linux-arm-kernel@...ts.infradead.org, Len Brown <lenb@...nel.org>
Subject: Re: [PATCH V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA
 interrupts

On Tue, Oct 18, 2016 at 08:32:44AM -0700, Sinan Kaya wrote:
> Sorry, I think I didn't have enough morning coffee.
> 
> Looking at these again and trying to be specific.
> 
> On 10/18/2016 8:20 AM, Sinan Kaya wrote:
> > It seems wrong to me that we call acpi_irq_get_penalty() from
> >> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
> >> should just manipulate acpi_isa_irq_penalty[irq] directly.
> >> 
> >> acpi_irq_penalty_update() is for command-line parameters, so it certainly
> >> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> >> acpi_link_list should be empty at the time we process the command-line
> >> parameters).
> 
> Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
> any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty.
> 
> If this is broken, then we need special care so that we don't assign
> dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
> results in returning incorrect penalty as
> 
> acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * sci_penalty.
> 
> Now that we added sci_penalty into the acpi_irq_get_penalty function,
> calling acpi_irq_get_penalty is not correct anymore. This line here needs to
> be replaced with acpi_isa_irq_penalty[irq] as you suggested.
> 
>                 if (used)
>                         new_penalty = acpi_irq_get_penalty(irq) +
>                                         PIRQ_PENALTY_ISA_USED;
>                 else
>                         new_penalty = 0;
> 
>                 acpi_isa_irq_penalty[irq] = new_penalty;
> 
> 
> >> 
> >> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> >> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
> >> the acpi_irq_pci_sharing_penalty() value at all.
> >> 
> 
> Same problem here. This line will be broken after the sci_penalty change.
> 
>                 acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
>                   (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);

I think the fragility of this code is an indication that we have a
design problem, so I want to step back from the nitty gritty details
for a bit and look at the overall design.

Let me restate the overall problem: We have a PCI device connected to
an interrupt link.  The interrupt link can be connected to one of
several IRQs, and we want to choose one of those IRQs to minimize IRQ
sharing.

That means we need information about which IRQs are used.
Historically we've started with a compiled-in table of common ISA IRQ
usage, and we also collect information about which IRQs are used and
which *might* be used.  So we have the following inputs:

  - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
    values.  ACPI is *supposed* to tell us about all these usages, so
    I don't know why we have the table.  But it's been there as long
    as I can remember.  The table is probably x86-specific, but we
    keep it in the supposedly generic pci_link.c.

  - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
    acpi_irq_pci().  I suppose these are for cases where we can't
    figure things out automatically.  I would resist adding parameters
    like this today (I would treat the need for them as a bug and look
    for a fix or a quirk), but we might be stuck with these.

  - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).

  - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
    acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
    NOT include interrupt link (PNP0C0F) devices because we don't
    handle them as PNPACPI devices.  I think this is related to the
    fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].

  - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
    information from _CRS and _PRS.  This seems sub-optimal and
    possibly buggy.

  - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
    acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
    call this on x86.  If _PRS exists, we penalize each possible IRQ.
    If there's no _PRS but _CRS contains an active IRQ, we penalize
    it.

  - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
    enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
    penalize an IRQ if it appears in _CRS or _PRS of any link device
    in the system.
    
    For IRQs 0-15, this overlaps with the penalization done at
    boot-time by acpi_irq_penalty_init(): if a device has _PRS, we'll
    add the "possible" penalty twice (once in acpi_irq_penalty_init()
    and again in acpi_irq_pci_sharing_penalty()), and the "using"
    penalty once (in acpi_irq_pci_sharing_penalty()).

    If a device has no _PRS but has _CRS, the "using" penalty is
    applied twice (once in once in acpi_irq_penalty_init() and again
    in acpi_irq_pci_sharing_penalty())

I think this whole thing is baroque and grotesque.

Here's a strawman idea:

  - Maintain a mapping of (IRQ, penalty).  Initially all penalties are
    zero.  This is for *all* IRQs, not just ISA ones.  This could be a
    linked list, but the structure is not important as long as we can
    add things dynamically.

  - Add a "acpi_penalize_irq()" function similar to
    acpi_penalize_isa_irq(), but not restricted to ISA, so we can
    increase the penalty for any IRQ, and maybe we can specify how
    much penalty to add.

  - Make acpi_irq_get_penalty() a simple lookup in the mapping.  No
    iterating through all link devices.

  - If we think the compiled-in penalties are really necessary, move
    the table to x86 code and add a boot-time loop to use
    acpi_penalize_irq() to penalize these IRQs.  Same for the
    command-line options.

  - Change acpi_penalize_sci_irq() to use acpi_penalize_irq().
    Probably the mapping needs to pay attention to trigger/polarity
    somehow, too.

  - Figure out how to make the ACPI core use acpi_penalize_irq() to
    based on the _CRS and _PRS of every ACPI device, including
    PNPACPI, PNP0C0F, etc.  Then we can remove acpi_irq_penalty_init().

  - Change acpi_pci_link_set() to use acpi_penalize_irq() for the IRQ
    it is enabling.  Conceptually maybe this should be done in the
    acpi_set_current_resources() path so it happens whenever we use
    _CRS to enable an IRQ on *any* ACPI device.

I think the biggest issue is figuring out how to get the ACPI core to
look at the _CRS for *all* devices.  If we could do that, I think it
could substantially simplify this code.

Bjorn

Powered by blists - more mailing lists