linux-kernel - Re: [PATCH V2 1/3] Revert "ACPI,PCI,IRQ: reduce static IRQ array size to 16"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0jw0KERMXuXcD63GqsfcJmwZ7MbrAx-A_dVqK-xdXBXhw@mail.gmail.com>
Date:   Thu, 13 Oct 2016 23:01:11 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Sinan Kaya <okaya@...eaurora.org>
Cc:     Bjorn Helgaas <helgaas@...nel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Bjorn Helgaas <bhelgaas@...gle.com>, ravikanth.nalla@....com,
        Ondrej Zary <linux@...nbow-software.org>,
        Timur Tabi <timur@...eaurora.org>,
        Christopher Covington <cov@...eaurora.org>,
        Jon Masters <jcm@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Linux PCI <linux-pci@...r.kernel.org>,
        Andy Gross <agross@...eaurora.org>,
        linux-arm-msm@...r.kernel.org,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>, wim@....tudelft.nl,
        Len Brown <lenb@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 1/3] Revert "ACPI,PCI,IRQ: reduce static IRQ array size
 to 16"

On Thu, Oct 13, 2016 at 10:16 PM, Sinan Kaya <okaya@...eaurora.org> wrote:
> On 10/13/2016 4:02 PM, Bjorn Helgaas wrote:
>> On Thu, Oct 13, 2016 at 03:36:11PM -0400, Sinan Kaya wrote:
>>> On 10/13/2016 2:15 PM, Bjorn Helgaas wrote:
>>>> It seems like the problem is that we removed acpi_penalize_sci_irq(),
>>>> which told us the polarity and trigger mode.  We tried to get that
>>>> information via irq_get_trigger_type(), but that didn't work in this
>>>> case because we use the acpi_irq_get_penalty() path before the SCI is
>>>> registered.
>>>>
>>>> It makes sense to me to add acpi_penalize_sci_irq() back in, which is
>>>> what patch [3/3] does.
>>>>
>>>> I don't understand how *this* patch, which basically just increases
>>>> the penalty array size from 16 to 256, helps fix the problem.  It
>>>> seems like this patch should only matter if the SCI were some IRQ
>>>> between 16 and 255.
>>>
>>>
>>> I see your point. The original code supported 256 interrupts.
>>>
>>> The machine where we had the problem had an SCI interrupt of 11. So,
>>> this patch does not necessarily fix anything for this machine alone.
>>> However, to be safe; I wanted to go back to the old behavior to fix
>>> the SCI issue for all existing platforms.
>>
>> I saw a previous email that said the SCI interrupt could not be
>> greater than 256, but I don't know where that restriction is.  I'm
>> pretty sure the FADT field is 2 bytes, which would mean it could be up
>> to 65535.
>>
>> To fix this problem, I think we only need to fix the penalty for the
>> SCI interrupt.  It seems better to add a single "sci_penalty"
>> variable, set it to PIRQ_PENALTY_PCI_USING if it's level/low or
>> PIRQ_PENALTY_ISA_ALWAYS otherwise, and add "sci_penalty" in when
>> appropriate.  That should fix it for *any* SCI IRQ, not just those
>> less than 256, and we don't have to add these extra penalty table
>> entries that are all unused (except possibly for one entry if we have
>> an SCI in the 16-255 range).
>>
>> Something like this:
>>
>>   static int sci_irq, sci_penalty;
>>
>>   void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
>>   {
>>     sci = irq;
>>     if (trigger == ACPI_MADT_TRIGGER_LEVEL &&
>>         polarity == ACPI_MADT_POLARITY_ACTIVE_LOW)
>>       sci_penalty = PIRQ_PENALTY_PCI_USING;
>>     else
>>       sci_penalty = PIRQ_PENALTY_ISA_ALWAYS;
>>   }
>>
>>   static int acpi_irq_get_penalty(int irq)
>>   {
>>     int penalty = 0;
>>
>>     if (irq == sci_irq)
>>       penalty += sci_penalty;
>>     ...
>>   }
>>
>> One could argue that ACPI devices can use IRQs above 15, and we should
>> handle penalties for them, too.  But the table is the wrong mechanism
>> for that, because it handles penalties for IRQs < 256, but IRQs above
>> that would mysteriously be handled differently.
>>
>> Bjorn
>>
>
> I agree this is a better approach. I think my math was wrong when figuring
> out what a max SCI interrupt could be.

OK

I will be expecting a new patch (or a new series) then.

Thanks,
Rafael