On Thu, Oct 13, 2016 at 10:16 PM, Sinan Kaya <okaya@xxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
