Re: [PATCH 1/2] ACPI / PCI: don't allow PCI devices without irq to be enabled

Re: [PATCH 1/2] ACPI / PCI: don't allow PCI devices without irq to be enabled

[Rafael J. Wysocki]

On Thursday, August 08, 2013 03:35:13 PM Heikki Krogerus wrote:
> If there is no ACPI entry for the irq, returning error from
> acpi_pci_enable_irq() if the irq is 0.
> 
> Prarit Bhargava reported an issue where he noticed that his
> Dell PowerEdge 840 has buggy BIOS that does not supply ACPI
> entries for irq with some devices. That lead into kernel
> generating a warning "genirq: Flags mismatch irq 0...". This
> will fix that issue.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Tested-by: Prarit Bhargava <prarit@redhat.com>

Hi Bjorn,

Any objections to this?

Rafael


> ---
>  drivers/acpi/pci_irq.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> index 41c5e1b..902ed1c 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -419,8 +419,16 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>  	 */
>  	if (gsi < 0) {
>  		u32 dev_gsi;
> +
> +		/* Not accepting 0 as valid PCI IRQ. */
> +		if (!dev->irq) {
> +			dev_err(&dev->dev, "PCI INT %c: no IRQ\n",
> +				pin_name(pin));
> +			return -ENOENT;
> +		}
> +
>  		/* Interrupt Line values above 0xF are forbidden */
> -		if (dev->irq > 0 && (dev->irq <= 0xF) &&
> +		if ((dev->irq <= 0xF) &&
>  		    (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0)) {
>  			dev_warn(&dev->dev, "PCI INT %c: no GSI - using ISA IRQ %d\n",
>  				 pin_name(pin), dev->irq);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] ACPI / PCI: don't allow PCI devices without irq to be enabled

[Bjorn Helgaas]

On Thu, Aug 8, 2013 at 7:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, August 08, 2013 03:35:13 PM Heikki Krogerus wrote:
>> If there is no ACPI entry for the irq, returning error from
>> acpi_pci_enable_irq() if the irq is 0.
>>
>> Prarit Bhargava reported an issue where he noticed that his
>> Dell PowerEdge 840 has buggy BIOS that does not supply ACPI
>> entries for irq with some devices. That lead into kernel
>> generating a warning "genirq: Flags mismatch irq 0...". This
>> will fix that issue.
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Tested-by: Prarit Bhargava <prarit@redhat.com>
>
> Hi Bjorn,
>
> Any objections to this?

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

>> ---
>>  drivers/acpi/pci_irq.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
>> index 41c5e1b..902ed1c 100644
>> --- a/drivers/acpi/pci_irq.c
>> +++ b/drivers/acpi/pci_irq.c
>> @@ -419,8 +419,16 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>        */
>>       if (gsi < 0) {
>>               u32 dev_gsi;
>> +
>> +             /* Not accepting 0 as valid PCI IRQ. */
>> +             if (!dev->irq) {
>> +                     dev_err(&dev->dev, "PCI INT %c: no IRQ\n",
>> +                             pin_name(pin));
>> +                     return -ENOENT;
>> +             }
>> +
>>               /* Interrupt Line values above 0xF are forbidden */
>> -             if (dev->irq > 0 && (dev->irq <= 0xF) &&
>> +             if ((dev->irq <= 0xF) &&
>>                   (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0)) {
>>                       dev_warn(&dev->dev, "PCI INT %c: no GSI - using ISA IRQ %d\n",
>>                                pin_name(pin), dev->irq);
>>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] ACPI / PCI: don't allow PCI devices without irq to be enabled

[Yinghai Lu]

On Mon, Aug 12, 2013 at 2:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Aug 8, 2013 at 7:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Thursday, August 08, 2013 03:35:13 PM Heikki Krogerus wrote:
>>> If there is no ACPI entry for the irq, returning error from
>>> acpi_pci_enable_irq() if the irq is 0.
>>>
>>> Prarit Bhargava reported an issue where he noticed that his
>>> Dell PowerEdge 840 has buggy BIOS that does not supply ACPI
>>> entries for irq with some devices. That lead into kernel
>>> generating a warning "genirq: Flags mismatch irq 0...". This
>>> will fix that issue.
>>>
>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> Tested-by: Prarit Bhargava <prarit@redhat.com>
>>
>> Hi Bjorn,
>>
>> Any objections to this?
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

some USB3 host controller does not have intx configured, but still
work with MSI?

Yinghai

Re: [PATCH 1/2] ACPI / PCI: don't allow PCI devices without irq to be enabled

[Bjorn Helgaas]

On Mon, Aug 12, 2013 at 3:32 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Aug 12, 2013 at 2:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Thu, Aug 8, 2013 at 7:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> On Thursday, August 08, 2013 03:35:13 PM Heikki Krogerus wrote:
>>>> If there is no ACPI entry for the irq, returning error from
>>>> acpi_pci_enable_irq() if the irq is 0.
>>>>
>>>> Prarit Bhargava reported an issue where he noticed that his
>>>> Dell PowerEdge 840 has buggy BIOS that does not supply ACPI
>>>> entries for irq with some devices. That lead into kernel
>>>> generating a warning "genirq: Flags mismatch irq 0...". This
>>>> will fix that issue.
>>>>
>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>> Tested-by: Prarit Bhargava <prarit@redhat.com>
>>>
>>> Hi Bjorn,
>>>
>>> Any objections to this?
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> some USB3 host controller does not have intx configured, but still
> work with MSI?

Huh, OK, I take back my ack, at least while we investigate this.

Bjorn

Re: [PATCH 1/2] ACPI / PCI: don't allow PCI devices without irq to be enabled

[Bjorn Helgaas]

On Mon, Aug 12, 2013 at 4:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Aug 12, 2013 at 3:32 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Mon, Aug 12, 2013 at 2:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Thu, Aug 8, 2013 at 7:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>>> On Thursday, August 08, 2013 03:35:13 PM Heikki Krogerus wrote:
>>>>> If there is no ACPI entry for the irq, returning error from
>>>>> acpi_pci_enable_irq() if the irq is 0.
>>>>>
>>>>> Prarit Bhargava reported an issue where he noticed that his
>>>>> Dell PowerEdge 840 has buggy BIOS that does not supply ACPI
>>>>> entries for irq with some devices. That lead into kernel
>>>>> generating a warning "genirq: Flags mismatch irq 0...". This
>>>>> will fix that issue.
>>>>>
>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>> Tested-by: Prarit Bhargava <prarit@redhat.com>
>>>>
>>>> Hi Bjorn,
>>>>
>>>> Any objections to this?
>>>
>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> some USB3 host controller does not have intx configured, but still
>> work with MSI?
>
> Huh, OK, I take back my ack, at least while we investigate this.

Is anybody looking into this?  I'm afraid I gave the impression that
*I* would investigate this.  But I don't really have enough
information, and it would be better if the patch author and the
reporter could investigate it first.

It would help if there were a bugzilla with complete dmesg log, acpidump, etc.

Bjorn

Re: [PATCH 1/2] ACPI / PCI: don't allow PCI devices without irq to be enabled

[Prarit Bhargava]

On 08/12/2013 06:32 PM, Bjorn Helgaas wrote:
> On Mon, Aug 12, 2013 at 3:32 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Mon, Aug 12, 2013 at 2:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Thu, Aug 8, 2013 at 7:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>>> On Thursday, August 08, 2013 03:35:13 PM Heikki Krogerus wrote:
>>>>> If there is no ACPI entry for the irq, returning error from
>>>>> acpi_pci_enable_irq() if the irq is 0.
>>>>>
>>>>> Prarit Bhargava reported an issue where he noticed that his
>>>>> Dell PowerEdge 840 has buggy BIOS that does not supply ACPI
>>>>> entries for irq with some devices. That lead into kernel
>>>>> generating a warning "genirq: Flags mismatch irq 0...". This
>>>>> will fix that issue.
>>>>>
>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>> Tested-by: Prarit Bhargava <prarit@redhat.com>
>>>>
>>>> Hi Bjorn,
>>>>
>>>> Any objections to this?
>>>
>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> some USB3 host controller does not have intx configured, but still
>> work with MSI?
> 
> Huh, OK, I take back my ack, at least while we investigate this.
> 

Bjorn, as suggested I've opened up a BZ here:

https://bugzilla.kernel.org/show_bug.cgi?id=62521

I've only cc'd yourself and Rafael.  Heikki -- I didn't find an account under
your intel.com address ...

P.

Re: [PATCH 2/2] ACPI / PCI: remove an extra check from acpi_pci_irq_enable()

[Rafael J. Wysocki]

On Thursday, August 08, 2013 03:35:14 PM Heikki Krogerus wrote:
> acpi_isa_irq_to_gsi() will check if the irq is within
> accepted rage so there is no need to check it separately.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Hi Bjorn,

Does this look OK to you?

Rafael


> ---
>  drivers/acpi/pci_irq.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> index 902ed1c..e71e7fa 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -427,9 +427,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>  			return -ENOENT;
>  		}
>  
> -		/* Interrupt Line values above 0xF are forbidden */
> -		if ((dev->irq <= 0xF) &&
> -		    (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0)) {
> +		if (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0) {
>  			dev_warn(&dev->dev, "PCI INT %c: no GSI - using ISA IRQ %d\n",
>  				 pin_name(pin), dev->irq);
>  			acpi_register_gsi(&dev->dev, dev_gsi,
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] ACPI / PCI: remove an extra check from acpi_pci_irq_enable()

[Bjorn Helgaas]

On Thu, Aug 8, 2013 at 7:58 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, August 08, 2013 03:35:14 PM Heikki Krogerus wrote:
>> acpi_isa_irq_to_gsi() will check if the irq is within
>> accepted rage so there is no need to check it separately.
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
> Hi Bjorn,
>
> Does this look OK to you?

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

>> ---
>>  drivers/acpi/pci_irq.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
>> index 902ed1c..e71e7fa 100644
>> --- a/drivers/acpi/pci_irq.c
>> +++ b/drivers/acpi/pci_irq.c
>> @@ -427,9 +427,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>                       return -ENOENT;
>>               }
>>
>> -             /* Interrupt Line values above 0xF are forbidden */
>> -             if ((dev->irq <= 0xF) &&
>> -                 (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0)) {
>> +             if (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0) {
>>                       dev_warn(&dev->dev, "PCI INT %c: no GSI - using ISA IRQ %d\n",
>>                                pin_name(pin), dev->irq);
>>                       acpi_register_gsi(&dev->dev, dev_gsi,
>>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/2] ACPI: return error in acpi_pci_irq_enable when there is no irq

[Rafael J. Wysocki]

On Friday, September 13, 2013 08:52:18 AM Prarit Bhargava wrote:
> 
> On 08/08/2013 08:35 AM, Heikki Krogerus wrote:
> > Hi,
> > 
> > This is the fix for the Dell PowerEdge 840 issue where nothing
> > allocated an irq for it's PCH if I understood correctly. The issue was
> > reported by Prarit Bhargava.
> > 
> > The second patch is just a minor cleanup.
> > 
> > Thanks,
> > 
> > 
> > Heikki Krogerus (2):
> >   ACPI / PCI: don't allow PCI devices without irq to be enabled
> >   ACPI / PCI: remove an extra check from acpi_pci_irq_enable()
> > 
> 
> Rafael, is this in your queue?

No, it is not, because Bjorn wanted to debug this issue deeper as far as I
could understand.

Bjorn?

Re: [PATCH 0/2] ACPI: return error in acpi_pci_irq_enable when there is no irq

[Bjorn Helgaas]

[+cc Yinghai]

On Fri, Sep 13, 2013 at 5:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, September 13, 2013 08:52:18 AM Prarit Bhargava wrote:
>>
>> On 08/08/2013 08:35 AM, Heikki Krogerus wrote:
>> > Hi,
>> >
>> > This is the fix for the Dell PowerEdge 840 issue where nothing
>> > allocated an irq for it's PCH if I understood correctly. The issue was
>> > reported by Prarit Bhargava.
>> >
>> > The second patch is just a minor cleanup.
>> >
>> > Thanks,
>> >
>> >
>> > Heikki Krogerus (2):
>> >   ACPI / PCI: don't allow PCI devices without irq to be enabled
>> >   ACPI / PCI: remove an extra check from acpi_pci_irq_enable()
>> >
>>
>> Rafael, is this in your queue?
>
> No, it is not, because Bjorn wanted to debug this issue deeper as far as I
> could understand.
>
> Bjorn?

Yes.  Yinghai pointed out a possible issue ("some USB3 host controller
does not have intx configured, but still work with MSI?").  I can't
explain to you right now what that means, but I did look at it enough
to think that there indeed might be a problem there.

So I asked [1] for a bugzilla with more info (dmesg, acpidump) so
somebody could look at it.  Actually, I was hoping Heikki or Prarit
would look into it and address Yinghai's concern.  Maybe there's no
problem there, but we should at least look and explain *why* it's not
a problem.  And I would really like a kernel.org bugzilla with the
info, in case this change turns out to have unintended consequences
and we have to debug it again later.

Bjorn

[1] http://lkml.kernel.org/r/CAErSpo7KDY87QPLNwbufeUAFGBtvyGteDmk6AxqFXBOX0RaObw@mail.gmail.com

Re: [PATCH 0/2] ACPI: return error in acpi_pci_irq_enable when there is no irq

[Heikki Krogerus]

On Fri, Sep 13, 2013 at 05:55:40PM -0600, Bjorn Helgaas wrote:
> On Fri, Sep 13, 2013 at 5:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, September 13, 2013 08:52:18 AM Prarit Bhargava wrote:
> >>
> >> Rafael, is this in your queue?
> >
> > No, it is not, because Bjorn wanted to debug this issue deeper as far as I
> > could understand.
> >
> > Bjorn?
> 
> Yes.  Yinghai pointed out a possible issue ("some USB3 host controller
> does not have intx configured, but still work with MSI?").  I can't
> explain to you right now what that means, but I did look at it enough
> to think that there indeed might be a problem there.
> 
> So I asked [1] for a bugzilla with more info (dmesg, acpidump) so
> somebody could look at it.  Actually, I was hoping Heikki or Prarit
> would look into it and address Yinghai's concern.  Maybe there's no
> problem there, but we should at least look and explain *why* it's not
> a problem.  And I would really like a kernel.org bugzilla with the
> info, in case this change turns out to have unintended consequences
> and we have to debug it again later.

I'll ask Sarah (xHCI maintaner) for help in case she knows something
and create a bug for this if needed.

I don't understand much about PCI, I need to study it a bit. Why would
it be a problem to get the xHCI working with only MSI?

Thanks,

-- 
heikki

Re: [PATCH 0/2] ACPI: return error in acpi_pci_irq_enable when there is no irq

[Heikki Krogerus]

Hi,

On Mon, Sep 16, 2013 at 10:40:00AM +0300, Heikki Krogerus wrote:
> On Fri, Sep 13, 2013 at 05:55:40PM -0600, Bjorn Helgaas wrote:
> > On Fri, Sep 13, 2013 at 5:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Friday, September 13, 2013 08:52:18 AM Prarit Bhargava wrote:
> > >>
> > >> Rafael, is this in your queue?
> > >
> > > No, it is not, because Bjorn wanted to debug this issue deeper as far as I
> > > could understand.
> > >
> > > Bjorn?
> > 
> > Yes.  Yinghai pointed out a possible issue ("some USB3 host controller
> > does not have intx configured, but still work with MSI?").  I can't
> > explain to you right now what that means, but I did look at it enough
> > to think that there indeed might be a problem there.
> > 
> > So I asked [1] for a bugzilla with more info (dmesg, acpidump) so
> > somebody could look at it.  Actually, I was hoping Heikki or Prarit
> > would look into it and address Yinghai's concern.  Maybe there's no
> > problem there, but we should at least look and explain *why* it's not
> > a problem.  And I would really like a kernel.org bugzilla with the
> > info, in case this change turns out to have unintended consequences
> > and we have to debug it again later.
> 
> I'll ask Sarah (xHCI maintaner) for help in case she knows something
> and create a bug for this if needed.
> 
> I don't understand much about PCI, I need to study it a bit. Why would
> it be a problem to get the xHCI working with only MSI?

Sorry but I'll take it back. I don't understand the problem, so I
can't really file a bug.

Just in case there is a misunderstanding, I'll explain the situation
again. Right now there is no issue with xHCI controller driver. Prarit
has Dell machine that apparently has buggy BIOS. He suggested a WA [1]
for his issue, but that WA broke the xHCI on my Haswell machine. This
patch fixed his issue and left my xHCI controller working.

Maybe it would make sense to create a bug for the original issue with
the buggy BIOS on Prarit's Dell. Prarit, you need to file that one!

[1] http://permalink.gmane.org/gmane.linux.kernel.pci/23962

-- 
heikki