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.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
