diff mbox

kernel-4.7 bug in Intel sound and/or ACPI

Message ID 57686D67.8010201@codeaurora.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Sinan Kaya June 20, 2016, 10:25 p.m. UTC
On 6/20/2016 5:25 PM, Bjorn Helgaas wrote:
> [+cc Sinan]
> 
> On Mon, Jun 20, 2016 at 03:02:57AM +0200, Rafael J. Wysocki wrote:
>> You should CC the linux-pci too (done now)
>>
>> On Monday, June 20, 2016 02:35:30 AM Wim Osterholt wrote:
>>> L.S.
>>> up to vanilla kernel-4.6.2 sound was working fine.
>>> Switching to kernel-4.7.0-rc3 made sound disappear. No /dev/mixer etc.
>>> There appears to be a bug in the Intel sound driver and/or ACPI driver.
>>> Dmesg shows interesting lines like:
>>> [   11.498592] ACPI: PCI Interrupt Link [LNKB] enabled at IRQ 7
>>> [   11.498605] PCI: setting IRQ 7 as level-triggered
>>> [   11.543903] parport0: PC-style at 0x378 (0x778), irq 7 [PCSPP,TRISTATE,EPP]
>>> [   12.757735] genirq: Flags mismatch irq 7. 00000080 (snd_intel8x0m) vs. 00000000 (parport0)
>>> [   12.757751] snd_intel8x0m 0000:00:1f.6: unable to grab IRQ 7
>>> [   12.757875] snd_intel8x0m: probe of 0000:00:1f.6 failed with error -16
>>> [   12.900171] genirq: Flags mismatch irq 7. 00000080 (snd_intel8x0) vs. 00000000 (parport0)
>>> [   12.900189] snd_intel8x0 0000:00:1f.5: unable to grab IRQ 7
>>> [   12.900389] snd_intel8x0: probe of 0000:00:1f.5 failed with error -16
>>> [   12.922554] genirq: Flags mismatch irq 7. 00000080 (ipw2200) vs. 00000000 (parport0)
>>> [   12.922559] ipw2200: Error allocating IRQ 7
>>>
>>> If I boot kernel-4.7.0-rc3 with acpi=off  then sound is back!
>>>
>>> If you need the full dmesg outputs then please get them here:
>>> http://webserver.djo.tudelft.nl/dmesg460+ACPI
>>> http://webserver.djo.tudelft.nl/dmesg473+ACPI
>>> http://webserver.djo.tudelft.nl/dmesg473noACPI
>>> (with excuses for the silly hostname which is out of our control)
> 
> I think snd_intel8x0 (00:1f.5) is connected to LNKB, and in v4.6 LNKB
> was set to IRQ 5, while in v4.7 LNKB is connected to IRQ7.  It looks
> like IRQ7 is already in use by parport, which doesn't want to share
> it.
> 
> This might be related to Sinan's changes to pci_link.c, which
> were merged for 4.7:
> 
>   9e5ed6d1fb87 ACPI,PCI,IRQ: remove SCI penalize function
>   1fcb6a813c4f ACPI,PCI,IRQ: remove redundant code in acpi_irq_penalty_init()
>   5c5087a55390 ACPI,PCI,IRQ: reduce static IRQ array size to 16
>   103544d86976 ACPI,PCI,IRQ: reduce resource requirements
> 
> I don't think we intended to change any behavior with those patches,
> but maybe we did.  Could you confirm/deny that those patches are
> related?  If they're not, bisection might be the quickest way to
> find the problem.
> 

I agree. The intention was not to change the existing behavior. 
I am trying to decode what this means.

[    0.305642] ACPI: PCI Interrupt Link [LNKA] (IRQs 9 10 *11)
[    0.306365] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 7) *11
[    0.307078] ACPI: PCI Interrupt Link [LNKC] (IRQs 9 10 *11)
[    0.307791] ACPI: PCI Interrupt Link [LNKD] (IRQs 5 7 9 10 *11)

It looks like the syntax is ((possible irqs) *active irq)

It looks like both 5 and 7 are possible for LNKB and 11 is the active IRQ. 

Since 5 and 7 are listed as possible, I don't understand what makes 7
not a good IRQ. 

Anyhow, I did some code inspection. I am seeing some problems around the
acpi_irq_get_penalty function. The acpi_irq_get_penalty function
is both a get and set function for PCI IRQs. However, it looks like
we are accounting the penalty twice when using ISA IRQs.

Can you try the following and see if it makes any difference?


> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Comments

wim June 21, 2016, 12:47 p.m. UTC | #1
> Can you try the following and see if it makes any difference?
> 
> 
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -500,7 +500,7 @@ static int acpi_irq_get_penalty(int irq)
>         int penalty = 0;
> 
>         if (irq < ACPI_MAX_ISA_IRQS)
> -               penalty += acpi_isa_irq_penalty[irq];
> +               return acpi_isa_irq_penalty[irq];
> 
>         /*
>         * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict
> @@ -586,6 +586,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>                             acpi_device_bid(link->device));
>                 return -ENODEV;
>         } else {
> +               if (irq < ACPI_MAX_ISA_IRQS)
> +                       acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> +                                                       PIRQ_PENALTY_PCI_USING;
> +
> 
> 
> 
> > Bjorn


I tried this on kernel 4.7.0-rc4, but that didn't help. It still tried to
grab irq7.


Regards, Wim.


----- wim@djo.tudelft.nl -----

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sinan Kaya June 21, 2016, 1:40 p.m. UTC | #2
On 6/21/2016 8:47 AM, Wim Osterholt wrote:
>> Can you try the following and see if it makes any difference?
>>
>>
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -500,7 +500,7 @@ static int acpi_irq_get_penalty(int irq)
>>         int penalty = 0;
>>
>>         if (irq < ACPI_MAX_ISA_IRQS)
>> -               penalty += acpi_isa_irq_penalty[irq];
>> +               return acpi_isa_irq_penalty[irq];
>>
>>         /*
>>         * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict
>> @@ -586,6 +586,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>>                             acpi_device_bid(link->device));
>>                 return -ENODEV;
>>         } else {
>> +               if (irq < ACPI_MAX_ISA_IRQS)
>> +                       acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
>> +                                                       PIRQ_PENALTY_PCI_USING;
>> +
>>
>>
>>
>>> Bjorn
> 
> 
> I tried this on kernel 4.7.0-rc4, but that didn't help. It still tried to
> grab irq7.
> 
> 

Thanks, It was a guess with no proof.

Let's undo the change above and start adding some print statements to collect
data from your system.

Can you add this to the end of acpi_irq_get_penalty function and then send
the output?

	pr_info("%s:%d irq = %d penalty = %d\n", __func__, __LINE__, irq,
		penalty);
 


> Regards, Wim.
> 
> 
> ----- wim@djo.tudelft.nl -----
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
wim June 21, 2016, 10:13 p.m. UTC | #3
On Tue, Jun 21, 2016 at 09:40:10AM -0400, Sinan Kaya wrote:
> 
> Thanks, It was a guess with no proof.
> 
> Let's undo the change above and start adding some print statements to collect
> data from your system.
> 
> Can you add this to the end of acpi_irq_get_penalty function and then send
> the output?
> 
> 	pr_info("%s:%d irq = %d penalty = %d\n", __func__, __LINE__, irq,
> 		penalty);
>  

This produced some 60 lines extra. Too much to include here.
The entire dmesg file is here:
http://webserver.djo.tudelft.nl/dmesg474+printpenalty


Regards, Wim.


----- wim@djo.tudelft.nl -----

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sinan Kaya June 23, 2016, 3:54 a.m. UTC | #4
On 2016-06-21 18:13, Wim Osterholt wrote:
> On Tue, Jun 21, 2016 at 09:40:10AM -0400, Sinan Kaya wrote:
>> 
>> Thanks, It was a guess with no proof.
>> 
>> Let's undo the change above and start adding some print statements to 
>> collect
>> data from your system.
>> 
>> Can you add this to the end of acpi_irq_get_penalty function and then 
>> send
>> the output?
>> 
>> 	pr_info("%s:%d irq = %d penalty = %d\n", __func__, __LINE__, irq,
>> 		penalty);
>> 
> 
> This produced some 60 lines extra. Too much to include here.
> The entire dmesg file is here:
> http://webserver.djo.tudelft.nl/dmesg474+printpenalty

Thanks, let's go back to 4.6 and add a very similar printf to every 
single place where the array is modified and also right before the 
enabled message.

I am trying to find a system with similar characteristics for debug



> 
> 
> Regards, Wim.
> 
> 
> ----- wim@djo.tudelft.nl -----
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
wim June 23, 2016, 2:12 p.m. UTC | #5
On Wed, Jun 22, 2016 at 11:54:39PM -0400, okaya@codeaurora.org wrote:
> On 2016-06-21 18:13, Wim Osterholt wrote:
> >> 
> >> 	pr_info("%s:%d irq = %d penalty = %d\n", __func__, __LINE__, irq,
> >> 		penalty);
> >> 
> > 
> > This produced some 60 lines extra....
> 
> Thanks, let's go back to 4.6 and add a very similar printf to every 
> single place where the array is modified and also right before the 
> enabled message.
> 

I don't get this right.
Assuming that you're still talking about the same file, I find a few
instances of 'enabled', most of them in if-statements and one where it might
be set, so it looks. However, that's already in a printk statement.
I don't know about arrays and even less where these are set. Even worse, I
don't know what to put in a 'similar' line if you don't mean 'exactly the
same'.
So please state file and line numbers and the line to be inserted.



Groeten, Wim.


----- wim@djo.tudelft.nl -----

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sinan Kaya June 23, 2016, 2:55 p.m. UTC | #6
On 6/23/2016 10:12 AM, Wim Osterholt wrote:
> On Wed, Jun 22, 2016 at 11:54:39PM -0400, okaya@codeaurora.org wrote:
>> On 2016-06-21 18:13, Wim Osterholt wrote:
>>>>
>>>> 	pr_info("%s:%d irq = %d penalty = %d\n", __func__, __LINE__, irq,
>>>> 		penalty);
>>>>
>>>
>>> This produced some 60 lines extra....
>>
>> Thanks, let's go back to 4.6 and add a very similar printf to every 
>> single place where the array is modified and also right before the 
>> enabled message.
>>
> 
> I don't get this right.
> Assuming that you're still talking about the same file, I find a few
> instances of 'enabled', most of them in if-statements and one where it might
> be set, so it looks. However, that's already in a printk statement.
> I don't know about arrays and even less where these are set. Even worse, I
> don't know what to put in a 'similar' line if you don't mean 'exactly the
> same'.
> So please state file and line numbers and the line to be inserted.
> 

Sure, let me get a patch for you. I was hoping to do it yesterday. 
I ran out of time. I typed the message from my phone. 

> 
> 
> Groeten, Wim.
> 
> 
> ----- wim@djo.tudelft.nl -----
>
diff mbox

Patch

--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -500,7 +500,7 @@  static int acpi_irq_get_penalty(int irq)
        int penalty = 0;

        if (irq < ACPI_MAX_ISA_IRQS)
-               penalty += acpi_isa_irq_penalty[irq];
+               return acpi_isa_irq_penalty[irq];

        /*
        * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict
@@ -586,6 +586,10 @@  static int acpi_pci_link_allocate(struct acpi_pci_link *link)
                            acpi_device_bid(link->device));
                return -ENODEV;
        } else {
+               if (irq < ACPI_MAX_ISA_IRQS)
+                       acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
+                                                       PIRQ_PENALTY_PCI_USING;
+