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: <469134d3-3950-0d89-2599-64f917f09781@codeaurora.org>
Date:   Thu, 13 Oct 2016 16:16:28 -0400
From:   Sinan Kaya <okaya@...eaurora.org>
To:     Bjorn Helgaas <helgaas@...nel.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-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, wim@....tudelft.nl,
        Len Brown <lenb@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/3] Revert "ACPI,PCI,IRQ: reduce static IRQ array size
 to 16"

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.



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.