Groups > linux.kernel > #1311891 > unrolled thread

[PATCH] pci: fix unavailable irq number 255 reported by BIOS

Started by	Chen Fan <chen.fan.fnst@cn.fujitsu.com>
First post	2016-01-19 03:00 +0100
Last post	2016-01-20 06:10 +0100
Articles	18 — 7 participants

Back to article view | Back to linux.kernel

  [PATCH] pci: fix unavailable irq number 255 reported by BIOS Chen Fan <chen.fan.fnst@cn.fujitsu.com> - 2016-01-19 03:00 +0100
    Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-19 14:50 +0100
      Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-19 15:30 +0100
        Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Sinan Kaya <okaya@codeaurora.org> - 2016-01-19 15:50 +0100
          Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS "Rafael J. Wysocki" <rafael@kernel.org> - 2016-01-19 16:40 +0100
            Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Sinan Kaya <okaya@codeaurora.org> - 2016-01-19 17:00 +0100
              Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS "Rafael J. Wysocki" <rafael@kernel.org> - 2016-01-19 17:10 +0100
            Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Chen Fan <chen.fan.fnst@cn.fujitsu.com> - 2016-01-20 06:10 +0100
      Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Bjorn Helgaas <helgaas@kernel.org> - 2016-01-20 01:30 +0100
        Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Chen Fan <chen.fan.fnst@cn.fujitsu.com> - 2016-01-20 05:30 +0100
          Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Bjorn Helgaas <helgaas@kernel.org> - 2016-01-20 18:20 +0100
            Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Chen Fan <chen.fan.fnst@cn.fujitsu.com> - 2016-01-21 09:10 +0100
        Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-01-21 15:40 +0100
          Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS "Rafael J. Wysocki" <rafael@kernel.org> - 2016-01-22 00:00 +0100
            Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Bjorn Helgaas <helgaas@kernel.org> - 2016-01-22 19:00 +0100
              Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS David Daney <ddaney.cavm@gmail.com> - 2016-01-22 20:30 +0100
                Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS "Rafael J. Wysocki" <rafael@kernel.org> - 2016-01-23 03:10 +0100
      Re: [PATCH] pci: fix unavailable irq number 255 reported by BIOS Chen Fan <chen.fan.fnst@cn.fujitsu.com> - 2016-01-20 06:10 +0100

#1311891 — [PATCH] pci: fix unavailable irq number 255 reported by BIOS

From	Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Date	2016-01-19 03:00 +0100
Subject	[PATCH] pci: fix unavailable irq number 255 reported by BIOS
Message-ID	<qStKq-Kn-11@gated-at.bofh.it>

In our environment, when enable Secure boot, we found an abnormal
phenomenon as following call trace shows. after investigation, we
found the firmware assigned an irq number 255 which means unknown
or no connection in PCI local spec for i801_smbus, meanwhile the
ACPI didn't configure the pci irq routing. and the 255 irq number
was assigned for megasa msix without IRQF_SHARED. then in this case
during i801_smbus probe, the i801_smbus driver would request irq with
bad irq number 255. but the 255 irq number was assigned for memgasa
with MSIX enable. which will cause request_irq fails, and call trace
shows, actually, we should expose the error early, rather than in request
irq, here we simply fix the problem by return err when find the irq is
255.

See the call trace:

 [   32.459195] ipmi device interface
 [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
 [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
 [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
 [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
 [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
 [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
 [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
 [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
 [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
 [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
 [   32.851178] Workqueue: events work_for_cpu_fn
 [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
 [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
 [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
 [   32.851247] Call Trace:
 [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
 [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
 [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
 [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
 [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
 [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
 [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
 [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
 [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
 [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
 [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
 [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
 [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
 [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
 [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
 [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
 [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
 [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
 		if (acpi_isa_register_gsi(dev))
 			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
 				 pin_name(pin));
+		rc = 0;
+		/*
+		 * Excluding the BIOS report the value 255, which meaning
+		 * "unknown" or "no connection" in PCI Local Bus Specification
+		 * Revision 3.0 February 3, 2004, P223.
+		 */
+		if (dev->irq == 0xFF)
+			rc = -EINVAL;
 
 		kfree(entry);
-		return 0;
+		return rc;
 	}
 
 	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
-- 
1.9.3

[toc] | [next] | [standalone]

#1312250

From	"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date	2016-01-19 14:50 +0100
Message-ID	<qSEPw-8d-19@gated-at.bofh.it>
In reply to	#1311891

On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
> In our environment, when enable Secure boot, we found an abnormal
> phenomenon as following call trace shows. after investigation, we
> found the firmware assigned an irq number 255 which means unknown
> or no connection in PCI local spec for i801_smbus, meanwhile the
> ACPI didn't configure the pci irq routing. and the 255 irq number
> was assigned for megasa msix without IRQF_SHARED. then in this case
> during i801_smbus probe, the i801_smbus driver would request irq with
> bad irq number 255. but the 255 irq number was assigned for memgasa
> with MSIX enable. which will cause request_irq fails, and call trace
> shows, actually, we should expose the error early, rather than in request
> irq, here we simply fix the problem by return err when find the irq is
> 255.
> 
> See the call trace:
> 
>  [   32.459195] ipmi device interface
>  [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>  [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>  [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>  [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>  [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>  [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>  [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>  [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>  [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>  [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>  [   32.851178] Workqueue: events work_for_cpu_fn
>  [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>  [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>  [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>  [   32.851247] Call Trace:
>  [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>  [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>  [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>  [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>  [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>  [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>  [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>  [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>  [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>  [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>  [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>  [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>  [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>  [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>  [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>  [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>  [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>  [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>  		if (acpi_isa_register_gsi(dev))
>  			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>  				 pin_name(pin));
> +		rc = 0;
> +		/*
> +		 * Excluding the BIOS report the value 255, which meaning
> +		 * "unknown" or "no connection" in PCI Local Bus Specification
> +		 * Revision 3.0 February 3, 2004, P223.

You mean the footnote on page 223 talking about the Interrupt Line values, right?

> +		 */
> +		if (dev->irq == 0xFF)
> +			rc = -EINVAL;
>  
>  		kfree(entry);
> -		return 0;
> +		return rc;
>  	}
>  
>  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
> 

Well, if you look at acpi_isa_register_gsi(), you'll see that it
actually does the check you're adding, so maybe the following should
be done instead?

---
 drivers/acpi/pci_irq.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/pci_irq.c
===================================================================
--- linux-pm.orig/drivers/acpi/pci_irq.c
+++ linux-pm/drivers/acpi/pci_irq.c
@@ -436,12 +436,13 @@ int acpi_pci_irq_enable(struct pci_dev *
 	 * driver reported one, then use it. Exit in any case.
 	 */
 	if (gsi < 0) {
-		if (acpi_isa_register_gsi(dev))
+		rc = acpi_isa_register_gsi(dev);
+		if (rc)
 			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
 				 pin_name(pin));
 
 		kfree(entry);
-		return 0;
+		return rc;
 	}
 
 	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);

[toc] | [prev] | [next] | [standalone]

#1312281

From	"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date	2016-01-19 15:30 +0100
Message-ID	<qSFse-Eb-11@gated-at.bofh.it>
In reply to	#1312250

On Tuesday, January 19, 2016 02:43:30 PM Rafael J. Wysocki wrote:
> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
> > In our environment, when enable Secure boot, we found an abnormal
> > phenomenon as following call trace shows. after investigation, we
> > found the firmware assigned an irq number 255 which means unknown
> > or no connection in PCI local spec for i801_smbus, meanwhile the
> > ACPI didn't configure the pci irq routing. and the 255 irq number
> > was assigned for megasa msix without IRQF_SHARED. then in this case
> > during i801_smbus probe, the i801_smbus driver would request irq with
> > bad irq number 255. but the 255 irq number was assigned for memgasa
> > with MSIX enable. which will cause request_irq fails, and call trace
> > shows, actually, we should expose the error early, rather than in request
> > irq, here we simply fix the problem by return err when find the irq is
> > 255.
> > 
> > See the call trace:
> > 
> >  [   32.459195] ipmi device interface
> >  [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
> >  [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
> >  [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
> >  [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
> >  [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
> >  [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
> >  [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
> >  [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
> >  [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
> >  [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
> >  [   32.851178] Workqueue: events work_for_cpu_fn
> >  [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
> >  [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
> >  [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
> >  [   32.851247] Call Trace:
> >  [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
> >  [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
> >  [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
> >  [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
> >  [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
> >  [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
> >  [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
> >  [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
> >  [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
> >  [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
> >  [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
> >  [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
> >  [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
> >  [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
> >  [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
> >  [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
> >  [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
> >  [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
> > 
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
> > --- a/drivers/acpi/pci_irq.c
> > +++ b/drivers/acpi/pci_irq.c
> > @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
> >  		if (acpi_isa_register_gsi(dev))
> >  			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
> >  				 pin_name(pin));
> > +		rc = 0;
> > +		/*
> > +		 * Excluding the BIOS report the value 255, which meaning
> > +		 * "unknown" or "no connection" in PCI Local Bus Specification
> > +		 * Revision 3.0 February 3, 2004, P223.
> 
> You mean the footnote on page 223 talking about the Interrupt Line values, right?
> 
> > +		 */
> > +		if (dev->irq == 0xFF)
> > +			rc = -EINVAL;
> >  
> >  		kfree(entry);
> > -		return 0;
> > +		return rc;
> >  	}
> >  
> >  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
> > 
> 
> Well, if you look at acpi_isa_register_gsi(), you'll see that it
> actually does the check you're adding, so maybe the following should
> be done instead?
> 
> ---
>  drivers/acpi/pci_irq.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/acpi/pci_irq.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_irq.c
> +++ linux-pm/drivers/acpi/pci_irq.c
> @@ -436,12 +436,13 @@ int acpi_pci_irq_enable(struct pci_dev *
>  	 * driver reported one, then use it. Exit in any case.
>  	 */
>  	if (gsi < 0) {
> -		if (acpi_isa_register_gsi(dev))
> +		rc = acpi_isa_register_gsi(dev);
> +		if (rc)
>  			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>  				 pin_name(pin));
>  
>  		kfree(entry);
> -		return 0;
> +		return rc;
>  	}
>  
>  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
> 
> --

Alternatively, to be entirely on the safe side and avoid possible regressions
from returning errors when they were not returned previously, we can
just special case the 0xff value as you did, but in a slightly cleaner way:

---
 drivers/acpi/pci_irq.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/pci_irq.c
===================================================================
--- linux-pm.orig/drivers/acpi/pci_irq.c
+++ linux-pm/drivers/acpi/pci_irq.c
@@ -436,12 +436,18 @@ int acpi_pci_irq_enable(struct pci_dev *
 	 * driver reported one, then use it. Exit in any case.
 	 */
 	if (gsi < 0) {
-		if (acpi_isa_register_gsi(dev))
+		/*
+		 * The Interrupt Line value of 0xff is defined to mean "unknown"
+		 * or "no connection" (PCI 3.0, Section 6.2.4, footnote on page
+		 * 223), so return an error in that case.
+		 */
+		rc = dev->irq == 0xff ? -EINVAL : 0;
+		if (rc || acpi_isa_register_gsi(dev))
 			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
 				 pin_name(pin));
 
 		kfree(entry);
-		return 0;
+		return rc;
 	}
 
 	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);

[toc] | [prev] | [next] | [standalone]

#1312307

From	Sinan Kaya <okaya@codeaurora.org>
Date	2016-01-19 15:50 +0100
Message-ID	<qSFLB-Lg-33@gated-at.bofh.it>
In reply to	#1312281

On 1/19/2016 9:20 AM, Rafael J. Wysocki wrote:
> On Tuesday, January 19, 2016 02:43:30 PM Rafael J. Wysocki wrote:
>> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>>> In our environment, when enable Secure boot, we found an abnormal
>>> phenomenon as following call trace shows. after investigation, we
>>> found the firmware assigned an irq number 255 which means unknown
>>> or no connection in PCI local spec for i801_smbus, meanwhile the
>>> ACPI didn't configure the pci irq routing. and the 255 irq number
>>> was assigned for megasa msix without IRQF_SHARED. then in this case
>>> during i801_smbus probe, the i801_smbus driver would request irq with
>>> bad irq number 255. but the 255 irq number was assigned for memgasa
>>> with MSIX enable. which will cause request_irq fails, and call trace
>>> shows, actually, we should expose the error early, rather than in request
>>> irq, here we simply fix the problem by return err when find the irq is
>>> 255.
>>>
>>> See the call trace:
>>>
>>>  [   32.459195] ipmi device interface
>>>  [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>>  [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>>  [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>>  [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>>  [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>>  [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>>  [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>>  [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>>  [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>>  [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>>  [   32.851178] Workqueue: events work_for_cpu_fn
>>>  [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>>  [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>>  [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>>  [   32.851247] Call Trace:
>>>  [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>>  [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>>  [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>>  [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>>  [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>>  [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>>  [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>>  [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>>  [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>>  [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>>  [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>>  [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>  [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>>  [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>  [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>>  [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>>>  [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>>>  [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
>>>
>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
>>> --- a/drivers/acpi/pci_irq.c
>>> +++ b/drivers/acpi/pci_irq.c
>>> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>>  		if (acpi_isa_register_gsi(dev))
>>>  			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>>  				 pin_name(pin));
>>> +		rc = 0;
>>> +		/*
>>> +		 * Excluding the BIOS report the value 255, which meaning
>>> +		 * "unknown" or "no connection" in PCI Local Bus Specification
>>> +		 * Revision 3.0 February 3, 2004, P223.
>>
>> You mean the footnote on page 223 talking about the Interrupt Line values, right?

"Unknown" does not necessarily mean invalid. I have a platform that is using 255 as a valid legacy
interrupt on PCI Express.

>>
>>> +		 */
>>> +		if (dev->irq == 0xFF)
>>> +			rc = -EINVAL;
>>>  
>>>  		kfree(entry);
>>> -		return 0;
>>> +		return rc;
>>>  	}
>>>  
>>>  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>>>
>>
>> Well, if you look at acpi_isa_register_gsi(), you'll see that it
>> actually does the check you're adding, so maybe the following should
>> be done instead?
>>
>> ---
>>  drivers/acpi/pci_irq.c |    5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/pci_irq.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/pci_irq.c
>> +++ linux-pm/drivers/acpi/pci_irq.c
>> @@ -436,12 +436,13 @@ int acpi_pci_irq_enable(struct pci_dev *
>>  	 * driver reported one, then use it. Exit in any case.
>>  	 */
>>  	if (gsi < 0) {
>> -		if (acpi_isa_register_gsi(dev))
>> +		rc = acpi_isa_register_gsi(dev);
>> +		if (rc)
>>  			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>  				 pin_name(pin));
>>  
>>  		kfree(entry);
>> -		return 0;
>> +		return rc;
>>  	}
>>  
>>  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>>
>> --
> 
> Alternatively, to be entirely on the safe side and avoid possible regressions
> from returning errors when they were not returned previously, we can
> just special case the 0xff value as you did, but in a slightly cleaner way:
> 

Is the issue limited to ISA? If yes, can we limit the change with ISA/PCI adapters only?

Is there a way to know if the system is PCIe vs. PCI?

> ---
>  drivers/acpi/pci_irq.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/acpi/pci_irq.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_irq.c
> +++ linux-pm/drivers/acpi/pci_irq.c
> @@ -436,12 +436,18 @@ int acpi_pci_irq_enable(struct pci_dev *
>  	 * driver reported one, then use it. Exit in any case.
>  	 */
>  	if (gsi < 0) {
> -		if (acpi_isa_register_gsi(dev))
> +		/*
> +		 * The Interrupt Line value of 0xff is defined to mean "unknown"
> +		 * or "no connection" (PCI 3.0, Section 6.2.4, footnote on page
> +		 * 223), so return an error in that case.
> +		 */
> +		rc = dev->irq == 0xff ? -EINVAL : 0;
> +		if (rc || acpi_isa_register_gsi(dev))
>  			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>  				 pin_name(pin));
>  
>  		kfree(entry);
> -		return 0;
> +		return rc;
>  	}
>  
>  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
> 
> --
> 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
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1312331

From	"Rafael J. Wysocki" <rafael@kernel.org>
Date	2016-01-19 16:40 +0100
Message-ID	<qSGxX-1lu-5@gated-at.bofh.it>
In reply to	#1312307

On Tue, Jan 19, 2016 at 3:48 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 1/19/2016 9:20 AM, Rafael J. Wysocki wrote:
>> On Tuesday, January 19, 2016 02:43:30 PM Rafael J. Wysocki wrote:
>>> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>>>> In our environment, when enable Secure boot, we found an abnormal
>>>> phenomenon as following call trace shows. after investigation, we
>>>> found the firmware assigned an irq number 255 which means unknown
>>>> or no connection in PCI local spec for i801_smbus, meanwhile the
>>>> ACPI didn't configure the pci irq routing. and the 255 irq number
>>>> was assigned for megasa msix without IRQF_SHARED. then in this case
>>>> during i801_smbus probe, the i801_smbus driver would request irq with
>>>> bad irq number 255. but the 255 irq number was assigned for memgasa
>>>> with MSIX enable. which will cause request_irq fails, and call trace
>>>> shows, actually, we should expose the error early, rather than in request
>>>> irq, here we simply fix the problem by return err when find the irq is
>>>> 255.
>>>>
>>>> See the call trace:
>>>>
>>>>  [   32.459195] ipmi device interface
>>>>  [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>>>  [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>>>  [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>>>  [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>>>  [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>>>  [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>>>  [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>>>  [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>>>  [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>>>  [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>>>  [   32.851178] Workqueue: events work_for_cpu_fn
>>>>  [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>>>  [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>>>  [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>>>  [   32.851247] Call Trace:
>>>>  [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>>>  [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>>>  [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>>>  [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>>>  [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>>>  [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>>>  [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>>>  [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>>>  [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>>>  [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>>>  [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>>>  [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>  [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>>>  [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>  [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>>>  [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>>>>  [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>>>>  [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
>>>>
>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
>>>> --- a/drivers/acpi/pci_irq.c
>>>> +++ b/drivers/acpi/pci_irq.c
>>>> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>>>             if (acpi_isa_register_gsi(dev))
>>>>                     dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>>>                              pin_name(pin));
>>>> +           rc = 0;
>>>> +           /*
>>>> +            * Excluding the BIOS report the value 255, which meaning
>>>> +            * "unknown" or "no connection" in PCI Local Bus Specification
>>>> +            * Revision 3.0 February 3, 2004, P223.
>>>
>>> You mean the footnote on page 223 talking about the Interrupt Line values, right?
>
> "Unknown" does not necessarily mean invalid. I have a platform that is using 255 as a valid legacy
> interrupt on PCI Express.

So first off this is about the Interrupt Line value not about an
interrupt vector.

Second, the footnote in question is talking about x86 PCs, so if your
platform is not one of them, there is no connection here.

Which means that the change should be limited to x86 probably.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1312353

From	Sinan Kaya <okaya@codeaurora.org>
Date	2016-01-19 17:00 +0100
Message-ID	<qSGRl-1sS-11@gated-at.bofh.it>
In reply to	#1312331

On 1/19/2016 10:39 AM, Rafael J. Wysocki wrote:
> On Tue, Jan 19, 2016 at 3:48 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 1/19/2016 9:20 AM, Rafael J. Wysocki wrote:
>>> On Tuesday, January 19, 2016 02:43:30 PM Rafael J. Wysocki wrote:
>>>> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>>>>> In our environment, when enable Secure boot, we found an abnormal
>>>>> phenomenon as following call trace shows. after investigation, we
>>>>> found the firmware assigned an irq number 255 which means unknown
>>>>> or no connection in PCI local spec for i801_smbus, meanwhile the
>>>>> ACPI didn't configure the pci irq routing. and the 255 irq number
>>>>> was assigned for megasa msix without IRQF_SHARED. then in this case
>>>>> during i801_smbus probe, the i801_smbus driver would request irq with
>>>>> bad irq number 255. but the 255 irq number was assigned for memgasa
>>>>> with MSIX enable. which will cause request_irq fails, and call trace
>>>>> shows, actually, we should expose the error early, rather than in request
>>>>> irq, here we simply fix the problem by return err when find the irq is
>>>>> 255.
>>>>>
>>>>> See the call trace:
>>>>>
>>>>>  [   32.459195] ipmi device interface
>>>>>  [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>>>>  [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>>>>  [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>>>>  [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>>>>  [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>>>>  [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>>>>  [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>>>>  [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>>>>  [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>>>>  [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>>>>  [   32.851178] Workqueue: events work_for_cpu_fn
>>>>>  [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>>>>  [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>>>>  [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>>>>  [   32.851247] Call Trace:
>>>>>  [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>>>>  [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>>>>  [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>>>>  [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>>>>  [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>>>>  [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>>>>  [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>>>>  [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>>>>  [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>>>>  [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>>>>  [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>>>>  [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>>  [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>>>>  [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>>  [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>>>>  [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>>>>>  [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>>>>>  [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
>>>>>
>>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
>>>>> --- a/drivers/acpi/pci_irq.c
>>>>> +++ b/drivers/acpi/pci_irq.c
>>>>> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>>>>             if (acpi_isa_register_gsi(dev))
>>>>>                     dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>>>>                              pin_name(pin));
>>>>> +           rc = 0;
>>>>> +           /*
>>>>> +            * Excluding the BIOS report the value 255, which meaning
>>>>> +            * "unknown" or "no connection" in PCI Local Bus Specification
>>>>> +            * Revision 3.0 February 3, 2004, P223.
>>>>
>>>> You mean the footnote on page 223 talking about the Interrupt Line values, right?
>>
>> "Unknown" does not necessarily mean invalid. I have a platform that is using 255 as a valid legacy
>> interrupt on PCI Express.
> 
> So first off this is about the Interrupt Line value not about an
> interrupt vector.

Got it. Just to be clear, I assume this is not the value that code reads from the ACPI table. 

+		rc = dev->irq == 0xff ? -EINVAL : 0;

I was nervous to see this check in common code. 

> 
> Second, the footnote in question is talking about x86 PCs, so if your
> platform is not one of them, there is no connection here.
> 
> Which means that the change should be limited to x86 probably.
> 
> Thanks,
> Rafael
> --
> 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
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1312359

From	"Rafael J. Wysocki" <rafael@kernel.org>
Date	2016-01-19 17:10 +0100
Message-ID	<qSH12-1Nb-63@gated-at.bofh.it>
In reply to	#1312353

On Tue, Jan 19, 2016 at 4:51 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 1/19/2016 10:39 AM, Rafael J. Wysocki wrote:
>> On Tue, Jan 19, 2016 at 3:48 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>> On 1/19/2016 9:20 AM, Rafael J. Wysocki wrote:
>>>> On Tuesday, January 19, 2016 02:43:30 PM Rafael J. Wysocki wrote:
>>>>> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>>>>>> In our environment, when enable Secure boot, we found an abnormal
>>>>>> phenomenon as following call trace shows. after investigation, we
>>>>>> found the firmware assigned an irq number 255 which means unknown
>>>>>> or no connection in PCI local spec for i801_smbus, meanwhile the
>>>>>> ACPI didn't configure the pci irq routing. and the 255 irq number
>>>>>> was assigned for megasa msix without IRQF_SHARED. then in this case
>>>>>> during i801_smbus probe, the i801_smbus driver would request irq with
>>>>>> bad irq number 255. but the 255 irq number was assigned for memgasa
>>>>>> with MSIX enable. which will cause request_irq fails, and call trace
>>>>>> shows, actually, we should expose the error early, rather than in request
>>>>>> irq, here we simply fix the problem by return err when find the irq is
>>>>>> 255.
>>>>>>
>>>>>> See the call trace:
>>>>>>
>>>>>>  [   32.459195] ipmi device interface
>>>>>>  [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>>>>>  [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>>>>>  [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>>>>>  [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>>>>>  [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>>>>>  [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>>>>>  [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>>>>>  [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>>>>>  [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>>>>>  [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>>>>>  [   32.851178] Workqueue: events work_for_cpu_fn
>>>>>>  [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>>>>>  [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>>>>>  [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>>>>>  [   32.851247] Call Trace:
>>>>>>  [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>>>>>  [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>>>>>  [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>>>>>  [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>>>>>  [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>>>>>  [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>>>>>  [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>>>>>  [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>>>>>  [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>>>>>  [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>>>>>  [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>>>>>  [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>>>  [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>>>>>  [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>>>  [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>>>>>  [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>>>>>>  [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>>>>>>  [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
>>>>>>
>>>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
>>>>>> --- a/drivers/acpi/pci_irq.c
>>>>>> +++ b/drivers/acpi/pci_irq.c
>>>>>> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>>>>>             if (acpi_isa_register_gsi(dev))
>>>>>>                     dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>>>>>                              pin_name(pin));
>>>>>> +           rc = 0;
>>>>>> +           /*
>>>>>> +            * Excluding the BIOS report the value 255, which meaning
>>>>>> +            * "unknown" or "no connection" in PCI Local Bus Specification
>>>>>> +            * Revision 3.0 February 3, 2004, P223.
>>>>>
>>>>> You mean the footnote on page 223 talking about the Interrupt Line values, right?
>>>
>>> "Unknown" does not necessarily mean invalid. I have a platform that is using 255 as a valid legacy
>>> interrupt on PCI Express.
>>
>> So first off this is about the Interrupt Line value not about an
>> interrupt vector.
>
> Got it. Just to be clear, I assume this is not the value that code reads from the ACPI table.
>
> +               rc = dev->irq == 0xff ? -EINVAL : 0;
>
> I was nervous to see this check in common code.

No, this value is read from the PCI register, but the interpretation
of it is arch-specific according to the spec.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1312879

From	Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Date	2016-01-20 06:10 +0100
Message-ID	<qSTbQ-1M2-23@gated-at.bofh.it>
In reply to	#1312331

On 01/19/2016 11:39 PM, Rafael J. Wysocki wrote:
> On Tue, Jan 19, 2016 at 3:48 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 1/19/2016 9:20 AM, Rafael J. Wysocki wrote:
>>> On Tuesday, January 19, 2016 02:43:30 PM Rafael J. Wysocki wrote:
>>>> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>>>>> In our environment, when enable Secure boot, we found an abnormal
>>>>> phenomenon as following call trace shows. after investigation, we
>>>>> found the firmware assigned an irq number 255 which means unknown
>>>>> or no connection in PCI local spec for i801_smbus, meanwhile the
>>>>> ACPI didn't configure the pci irq routing. and the 255 irq number
>>>>> was assigned for megasa msix without IRQF_SHARED. then in this case
>>>>> during i801_smbus probe, the i801_smbus driver would request irq with
>>>>> bad irq number 255. but the 255 irq number was assigned for memgasa
>>>>> with MSIX enable. which will cause request_irq fails, and call trace
>>>>> shows, actually, we should expose the error early, rather than in request
>>>>> irq, here we simply fix the problem by return err when find the irq is
>>>>> 255.
>>>>>
>>>>> See the call trace:
>>>>>
>>>>>   [   32.459195] ipmi device interface
>>>>>   [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>>>>   [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>>>>   [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>>>>   [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>>>>   [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>>>>   [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>>>>   [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>>>>   [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>>>>   [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>>>>   [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>>>>   [   32.851178] Workqueue: events work_for_cpu_fn
>>>>>   [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>>>>   [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>>>>   [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>>>>   [   32.851247] Call Trace:
>>>>>   [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>>>>   [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>>>>   [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>>>>   [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>>>>   [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>>>>   [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>>>>   [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>>>>   [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>>>>   [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>>>>   [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>>>>   [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>>>>   [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>>   [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>>>>   [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>>   [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>>>>   [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>>>>>   [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>>>>>   [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
>>>>>
>>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
>>>>> --- a/drivers/acpi/pci_irq.c
>>>>> +++ b/drivers/acpi/pci_irq.c
>>>>> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>>>>              if (acpi_isa_register_gsi(dev))
>>>>>                      dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>>>>                               pin_name(pin));
>>>>> +           rc = 0;
>>>>> +           /*
>>>>> +            * Excluding the BIOS report the value 255, which meaning
>>>>> +            * "unknown" or "no connection" in PCI Local Bus Specification
>>>>> +            * Revision 3.0 February 3, 2004, P223.
>>>> You mean the footnote on page 223 talking about the Interrupt Line values, right?
>> "Unknown" does not necessarily mean invalid. I have a platform that is using 255 as a valid legacy
>> interrupt on PCI Express.
> So first off this is about the Interrupt Line value not about an
> interrupt vector.
>
> Second, the footnote in question is talking about x86 PCs, so if your
> platform is not one of them, there is no connection here.
>
> Which means that the change should be limited to x86 probably.
That's right, we should wrap this code in arch x86.

Thanks,
Chen

>
> Thanks,
> Rafael
>
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1312601

From	Bjorn Helgaas <helgaas@kernel.org>
Date	2016-01-20 01:30 +0100
Message-ID	<qSOOT-6Wg-91@gated-at.bofh.it>
In reply to	#1312250

[+cc Jiang]

Hi Chen,

On Tue, Jan 19, 2016 at 02:43:30PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
> > In our environment, when enable Secure boot, we found an abnormal

This has more information than necessary.  I don't think Secure Boot is
really relevant, and nor are the timestamps and stack addresses below.

> > phenomenon as following call trace shows. after investigation, we
> > found the firmware assigned an irq number 255 which means unknown
> > or no connection in PCI local spec for i801_smbus, meanwhile the
> > ACPI didn't configure the pci irq routing. and the 255 irq number
> > was assigned for megasa msix without IRQF_SHARED. then in this case
> > during i801_smbus probe, the i801_smbus driver would request irq with
> > bad irq number 255. but the 255 irq number was assigned for memgasa
> > with MSIX enable. which will cause request_irq fails, and call trace
> > shows, actually, we should expose the error early, rather than in request
> > irq, here we simply fix the problem by return err when find the irq is
> > 255.

> > See the call trace:
> > 
> >  [   32.459195] ipmi device interface
> >  [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
> >  [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
> >  [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
> >  [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
> >  [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
> >  [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
> >  [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
> >  [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
> >  [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
> >  [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
> >  [   32.851178] Workqueue: events work_for_cpu_fn
> >  [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
> >  [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
> >  [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
> >  [   32.851247] Call Trace:
> >  [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
> >  [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
> >  [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
> >  [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
> >  [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
> >  [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
> >  [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
> >  [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
> >  [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
> >  [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
> >  [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
> >  [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
> >  [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
> >  [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
> >  [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
> >  [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16

Since the Interrupt Line register is writable and might contain any
value, it would be nice if Linux could at least tolerate anything
firmware might leave there without a backtrace, even if we end up not
being able to use the device.

Your patch changes the acpi_pci_irq_enable() return value from 0 to
-EINVAL for this case.  You're running v3.10, and this change probably
makes pci_enable_device() fail.  I suppose the user-visible effect is
that with your patch,

  - there's no backtrace,
  - i801_smbus fails with "Failed to enable SMBus PCI device" instead
    of with "Failed to allocate irq 255", and
  - i801_smbus fails even if no other device is using IRQ 255, instead
    of "succeeding" and using an IRQ 255 that probably doesn't work
    (this seems like maybe the most important difference)

Jiang has changed this path with 890e4847587f ("PCI: Add
pcibios_alloc_irq() and pcibios_free_irq()"), so I think on newer
kernels, we'll never even call the i801_smbus probe function.

What behavior are you looking for from i801_smbus?  Decline to claim
the device?  Try to use the device without interrupts?  Try to figure
out an interrupt in some other way?

I'm not 100% sure that 890e4847587f does the right thing by preventing
a driver from claiming a device where we can't set up an IRQ.  It's
conceivable that a driver could still operate a device even without an
IRQ.

> >  [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
> >  [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
> > 
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
> > --- a/drivers/acpi/pci_irq.c
> > +++ b/drivers/acpi/pci_irq.c
> > @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
> >  		if (acpi_isa_register_gsi(dev))
> >  			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
> >  				 pin_name(pin));
> > +		rc = 0;
> > +		/*
> > +		 * Excluding the BIOS report the value 255, which meaning
> > +		 * "unknown" or "no connection" in PCI Local Bus Specification
> > +		 * Revision 3.0 February 3, 2004, P223.
> 
> You mean the footnote on page 223 talking about the Interrupt Line values, right?
> 
> > +		 */
> > +		if (dev->irq == 0xFF)
> > +			rc = -EINVAL;
> >  
> >  		kfree(entry);
> > -		return 0;
> > +		return rc;
> >  	}
> >  
> >  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
> > 
> 
> Well, if you look at acpi_isa_register_gsi(), you'll see that it
> actually does the check you're adding, so maybe the following should
> be done instead?
> 
> ---
>  drivers/acpi/pci_irq.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/acpi/pci_irq.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_irq.c
> +++ linux-pm/drivers/acpi/pci_irq.c
> @@ -436,12 +436,13 @@ int acpi_pci_irq_enable(struct pci_dev *
>  	 * driver reported one, then use it. Exit in any case.
>  	 */
>  	if (gsi < 0) {
> -		if (acpi_isa_register_gsi(dev))
> +		rc = acpi_isa_register_gsi(dev);
> +		if (rc)
>  			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>  				 pin_name(pin));
>  
>  		kfree(entry);
> -		return 0;
> +		return rc;
>  	}
>  
>  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
> 
> --
> 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

[toc] | [prev] | [next] | [standalone]

#1312864

From	Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Date	2016-01-20 05:30 +0100
Message-ID	<qSSz8-1f8-21@gated-at.bofh.it>
In reply to	#1312601

On 01/20/2016 08:24 AM, Bjorn Helgaas wrote:
> [+cc Jiang]
>
> Hi Chen,
>
> On Tue, Jan 19, 2016 at 02:43:30PM +0100, Rafael J. Wysocki wrote:
>> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>>> In our environment, when enable Secure boot, we found an abnormal
> This has more information than necessary.  I don't think Secure Boot is
> really relevant, and nor are the timestamps and stack addresses below.
I just think enable the Secure Boot, probably the firmware assigned
a 0xff interrupt to the device which unauthenticated.
>
>>> phenomenon as following call trace shows. after investigation, we
>>> found the firmware assigned an irq number 255 which means unknown
>>> or no connection in PCI local spec for i801_smbus, meanwhile the
>>> ACPI didn't configure the pci irq routing. and the 255 irq number
>>> was assigned for megasa msix without IRQF_SHARED. then in this case
>>> during i801_smbus probe, the i801_smbus driver would request irq with
>>> bad irq number 255. but the 255 irq number was assigned for memgasa
>>> with MSIX enable. which will cause request_irq fails, and call trace
>>> shows, actually, we should expose the error early, rather than in request
>>> irq, here we simply fix the problem by return err when find the irq is
>>> 255.
>>> See the call trace:
>>>
>>>   [   32.459195] ipmi device interface
>>>   [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>>   [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>>   [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>>   [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>>   [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>>   [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>>   [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>>   [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>>   [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>>   [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>>   [   32.851178] Workqueue: events work_for_cpu_fn
>>>   [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>>   [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>>   [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>>   [   32.851247] Call Trace:
>>>   [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>>   [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>>   [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>>   [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>>   [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>>   [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>>   [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>>   [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>>   [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>>   [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>>   [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>>   [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>   [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>>   [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>   [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>>   [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
> Since the Interrupt Line register is writable and might contain any
> value, it would be nice if Linux could at least tolerate anything
> firmware might leave there without a backtrace, even if we end up not
> being able to use the device.
>
> Your patch changes the acpi_pci_irq_enable() return value from 0 to
> -EINVAL for this case.  You're running v3.10, and this change probably
> makes pci_enable_device() fail.  I suppose the user-visible effect is
> that with your patch,
>
>    - there's no backtrace,
>    - i801_smbus fails with "Failed to enable SMBus PCI device" instead
>      of with "Failed to allocate irq 255", and
>    - i801_smbus fails even if no other device is using IRQ 255, instead
>      of "succeeding" and using an IRQ 255 that probably doesn't work
>      (this seems like maybe the most important difference)
>
> Jiang has changed this path with 890e4847587f ("PCI: Add
> pcibios_alloc_irq() and pcibios_free_irq()"), so I think on newer
> kernels, we'll never even call the i801_smbus probe function.
no, on newer kernels, this phenomenon also probably appearance,
with this patch 890e4847587f change, it didn't change the
acpi_pci_irq_enable() return value, with the problem it also return 0,
and then still call __pci_device_probe() to do i801_smbus probe
function in pci_device_probe() function.

>
> What behavior are you looking for from i801_smbus?  Decline to claim
> the device?  Try to use the device without interrupts?  Try to figure
> out an interrupt in some other way?
I think if BIOS assigned 0xff interrupt line to device, and kernel can't 
look
up a valid interrupt for the device, we should not allow to use the device.
>
> I'm not 100% sure that 890e4847587f does the right thing by preventing
> a driver from claiming a device where we can't set up an IRQ.  It's
> conceivable that a driver could still operate a device even without an
> IRQ.
I don't understanding, does without IRQ for device still work?

Thanks,
Chen

>
>>>   [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>>>   [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
>>>
>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
>>> --- a/drivers/acpi/pci_irq.c
>>> +++ b/drivers/acpi/pci_irq.c
>>> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>>   		if (acpi_isa_register_gsi(dev))
>>>   			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>>   				 pin_name(pin));
>>> +		rc = 0;
>>> +		/*
>>> +		 * Excluding the BIOS report the value 255, which meaning
>>> +		 * "unknown" or "no connection" in PCI Local Bus Specification
>>> +		 * Revision 3.0 February 3, 2004, P223.
>> You mean the footnote on page 223 talking about the Interrupt Line values, right?
>>
>>> +		 */
>>> +		if (dev->irq == 0xFF)
>>> +			rc = -EINVAL;
>>>   
>>>   		kfree(entry);
>>> -		return 0;
>>> +		return rc;
>>>   	}
>>>   
>>>   	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>>>
>> Well, if you look at acpi_isa_register_gsi(), you'll see that it
>> actually does the check you're adding, so maybe the following should
>> be done instead?
>>
>> ---
>>   drivers/acpi/pci_irq.c |    5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/pci_irq.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/pci_irq.c
>> +++ linux-pm/drivers/acpi/pci_irq.c
>> @@ -436,12 +436,13 @@ int acpi_pci_irq_enable(struct pci_dev *
>>   	 * driver reported one, then use it. Exit in any case.
>>   	 */
>>   	if (gsi < 0) {
>> -		if (acpi_isa_register_gsi(dev))
>> +		rc = acpi_isa_register_gsi(dev);
>> +		if (rc)
>>   			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>   				 pin_name(pin));
>>   
>>   		kfree(entry);
>> -		return 0;
>> +		return rc;
>>   	}
>>   
>>   	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>>
>> --
>> 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
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1313383

From	Bjorn Helgaas <helgaas@kernel.org>
Date	2016-01-20 18:20 +0100
Message-ID	<qT4Ah-1fm-1@gated-at.bofh.it>
In reply to	#1312864

On Wed, Jan 20, 2016 at 12:21:24PM +0800, Chen Fan wrote:
> 
> On 01/20/2016 08:24 AM, Bjorn Helgaas wrote:
> >[+cc Jiang]
> >
> >Hi Chen,
> >
> >On Tue, Jan 19, 2016 at 02:43:30PM +0100, Rafael J. Wysocki wrote:
> >>On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
> >>>In our environment, when enable Secure boot, we found an abnormal
> >This has more information than necessary.  I don't think Secure Boot is
> >really relevant, and nor are the timestamps and stack addresses below.
> I just think enable the Secure Boot, probably the firmware assigned
> a 0xff interrupt to the device which unauthenticated.

The important thing is that you're changing the way we handle
Interrupt Line being 0xff.  That affects more than just Secure Boot
users.  It's fine to mention Secure Boot later, as one example of an
affected scenario.

I don't know anything about Secure Boot, but setting Interrupt Line to
0xff would obviously not be a robust way of hiding an unauthenticated
device.  But it sounds like you're just speculating about that anyway.

> >>>phenomenon as following call trace shows. after investigation, we
> >>>found the firmware assigned an irq number 255 which means unknown
> >>>or no connection in PCI local spec for i801_smbus, meanwhile the
> >>>ACPI didn't configure the pci irq routing. and the 255 irq number
> >>>was assigned for megasa msix without IRQF_SHARED. then in this case
> >>>during i801_smbus probe, the i801_smbus driver would request irq with
> >>>bad irq number 255. but the 255 irq number was assigned for memgasa
> >>>with MSIX enable. which will cause request_irq fails, and call trace
> >>>shows, actually, we should expose the error early, rather than in request
> >>>irq, here we simply fix the problem by return err when find the irq is
> >>>255.
> >>>See the call trace:
> >>>
> >>>  [   32.459195] ipmi device interface
> >>>  [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
> >>>  [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
> >>>  [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
> >>>  [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
> >>>  [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
> >>>  [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
> >>>  [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
> >>>  [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
> >>>  [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
> >>>  [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
> >>>  [   32.851178] Workqueue: events work_for_cpu_fn
> >>>  [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
> >>>  [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
> >>>  [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
> >>>  [   32.851247] Call Trace:
> >>>  [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
> >>>  [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
> >>>  [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
> >>>  [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
> >>>  [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
> >>>  [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
> >>>  [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
> >>>  [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
> >>>  [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
> >>>  [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
> >>>  [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
> >>>  [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
> >>>  [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
> >>>  [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
> >>>  [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
> >>>  [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
> >Since the Interrupt Line register is writable and might contain any
> >value, it would be nice if Linux could at least tolerate anything
> >firmware might leave there without a backtrace, even if we end up not
> >being able to use the device.
> >
> >Your patch changes the acpi_pci_irq_enable() return value from 0 to
> >-EINVAL for this case.  You're running v3.10, and this change probably
> >makes pci_enable_device() fail.  I suppose the user-visible effect is
> >that with your patch,
> >
> >   - there's no backtrace,
> >   - i801_smbus fails with "Failed to enable SMBus PCI device" instead
> >     of with "Failed to allocate irq 255", and
> >   - i801_smbus fails even if no other device is using IRQ 255, instead
> >     of "succeeding" and using an IRQ 255 that probably doesn't work
> >     (this seems like maybe the most important difference)
> >
> >Jiang has changed this path with 890e4847587f ("PCI: Add
> >pcibios_alloc_irq() and pcibios_free_irq()"), so I think on newer
> >kernels, we'll never even call the i801_smbus probe function.
>
> no, on newer kernels, this phenomenon also probably appearance,
> with this patch 890e4847587f change, it didn't change the
> acpi_pci_irq_enable() return value, with the problem it also return 0,
> and then still call __pci_device_probe() to do i801_smbus probe
> function in pci_device_probe() function.

I meant that *with your patch*, newer kernels won't call the
i801_smbus probe function.

> >What behavior are you looking for from i801_smbus?  Decline to claim
> >the device?  Try to use the device without interrupts?  Try to figure
> >out an interrupt in some other way?
>
> I think if BIOS assigned 0xff interrupt line to device, and kernel
> can't look
> up a valid interrupt for the device, we should not allow to use the device.
> >
> >I'm not 100% sure that 890e4847587f does the right thing by preventing
> >a driver from claiming a device where we can't set up an IRQ.  It's
> >conceivable that a driver could still operate a device even without an
> >IRQ.
>
> I don't understanding, does without IRQ for device still work?

Polling drivers do not need IRQs.  The PCI core has no idea whether a
driver is interrupt-driven or polling, so we can't assume that a
device with no IRQ is useless.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1313987

From	Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Date	2016-01-21 09:10 +0100
Message-ID	<qTitA-2wH-3@gated-at.bofh.it>
In reply to	#1313383

On 01/21/2016 01:12 AM, Bjorn Helgaas wrote:
> On Wed, Jan 20, 2016 at 12:21:24PM +0800, Chen Fan wrote:
>> On 01/20/2016 08:24 AM, Bjorn Helgaas wrote:
>>> [+cc Jiang]
>>>
>>> Hi Chen,
>>>
>>> On Tue, Jan 19, 2016 at 02:43:30PM +0100, Rafael J. Wysocki wrote:
>>>> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>>>>> In our environment, when enable Secure boot, we found an abnormal
>>> This has more information than necessary.  I don't think Secure Boot is
>>> really relevant, and nor are the timestamps and stack addresses below.
>> I just think enable the Secure Boot, probably the firmware assigned
>> a 0xff interrupt to the device which unauthenticated.
> The important thing is that you're changing the way we handle
> Interrupt Line being 0xff.  That affects more than just Secure Boot
> users.  It's fine to mention Secure Boot later, as one example of an
> affected scenario.
>
> I don't know anything about Secure Boot, but setting Interrupt Line to
> 0xff would obviously not be a robust way of hiding an unauthenticated
> device.  But it sounds like you're just speculating about that anyway.
>
>>>>> phenomenon as following call trace shows. after investigation, we
>>>>> found the firmware assigned an irq number 255 which means unknown
>>>>> or no connection in PCI local spec for i801_smbus, meanwhile the
>>>>> ACPI didn't configure the pci irq routing. and the 255 irq number
>>>>> was assigned for megasa msix without IRQF_SHARED. then in this case
>>>>> during i801_smbus probe, the i801_smbus driver would request irq with
>>>>> bad irq number 255. but the 255 irq number was assigned for memgasa
>>>>> with MSIX enable. which will cause request_irq fails, and call trace
>>>>> shows, actually, we should expose the error early, rather than in request
>>>>> irq, here we simply fix the problem by return err when find the irq is
>>>>> 255.
>>>>> See the call trace:
>>>>>
>>>>>   [   32.459195] ipmi device interface
>>>>>   [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>>>>   [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>>>>   [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>>>>   [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>>>>   [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>>>>   [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>>>>   [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>>>>   [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>>>>   [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>>>>   [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>>>>   [   32.851178] Workqueue: events work_for_cpu_fn
>>>>>   [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>>>>   [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>>>>   [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>>>>   [   32.851247] Call Trace:
>>>>>   [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>>>>   [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>>>>   [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>>>>   [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>>>>   [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>>>>   [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>>>>   [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>>>>   [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>>>>   [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>>>>   [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>>>>   [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>>>>   [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>>   [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>>>>   [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>>>   [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>>>>   [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>>> Since the Interrupt Line register is writable and might contain any
>>> value, it would be nice if Linux could at least tolerate anything
>>> firmware might leave there without a backtrace, even if we end up not
>>> being able to use the device.
>>>
>>> Your patch changes the acpi_pci_irq_enable() return value from 0 to
>>> -EINVAL for this case.  You're running v3.10, and this change probably
>>> makes pci_enable_device() fail.  I suppose the user-visible effect is
>>> that with your patch,
>>>
>>>    - there's no backtrace,
>>>    - i801_smbus fails with "Failed to enable SMBus PCI device" instead
>>>      of with "Failed to allocate irq 255", and
>>>    - i801_smbus fails even if no other device is using IRQ 255, instead
>>>      of "succeeding" and using an IRQ 255 that probably doesn't work
>>>      (this seems like maybe the most important difference)
>>>
>>> Jiang has changed this path with 890e4847587f ("PCI: Add
>>> pcibios_alloc_irq() and pcibios_free_irq()"), so I think on newer
>>> kernels, we'll never even call the i801_smbus probe function.
>> no, on newer kernels, this phenomenon also probably appearance,
>> with this patch 890e4847587f change, it didn't change the
>> acpi_pci_irq_enable() return value, with the problem it also return 0,
>> and then still call __pci_device_probe() to do i801_smbus probe
>> function in pci_device_probe() function.
> I meant that *with your patch*, newer kernels won't call the
> i801_smbus probe function.
>
>>> What behavior are you looking for from i801_smbus?  Decline to claim
>>> the device?  Try to use the device without interrupts?  Try to figure
>>> out an interrupt in some other way?
>> I think if BIOS assigned 0xff interrupt line to device, and kernel
>> can't look
>> up a valid interrupt for the device, we should not allow to use the device.
>>> I'm not 100% sure that 890e4847587f does the right thing by preventing
>>> a driver from claiming a device where we can't set up an IRQ.  It's
>>> conceivable that a driver could still operate a device even without an
>>> IRQ.
>> I don't understanding, does without IRQ for device still work?
> Polling drivers do not need IRQs.  The PCI core has no idea whether a
> driver is interrupt-driven or polling, so we can't assume that a
> device with no IRQ is useless.
Got it, I observed the smbus driver has changed to polling when request_irq
failed on new kernel.
can we use a broken_irq flag in pci_dev to mark the device irq if invalid ?
then if a device broken_irq set, we don't need to call request_irq and
directly return failure. of course, maybe we need to check this for all 
devices.

BTW, can we skip the 0xff irq number when allocating irq in x86 arch ?

Thanks,
Chen


>
> Bjorn
>
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1314229

From	Cao jin <caoj.fnst@cn.fujitsu.com>
Date	2016-01-21 15:40 +0100
Message-ID	<qToz0-6v1-17@gated-at.bofh.it>
In reply to	#1312601

Hi,

     IMHO, I think maybe modification on i801_smbus driver is easier.

     Because when i801_smbus request_irq using pci_dev->irq, this 
pci_dev->irq seems still holds the value read from register( 
pci_setup_device->pci_read_irq), if the value is 255, it is invalid in 
register, but when request_irq, 255 is a valid value(with 
CONFIG_X86_IO_APIC & CONFIG_PCI_MSI defined). so I guess maybe we can do 
a judgement in i801_smbus, like:

     if (pci_dev->irq == 255) {

         conversion to another irq # which maybe still not be used
     }


On 01/20/2016 08:24 AM, Bjorn Helgaas wrote:
> [+cc Jiang]
>
> Hi Chen,
>
> On Tue, Jan 19, 2016 at 02:43:30PM +0100, Rafael J. Wysocki wrote:
>> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>>> In our environment, when enable Secure boot, we found an abnormal
>
> This has more information than necessary.  I don't think Secure Boot is
> really relevant, and nor are the timestamps and stack addresses below.
>
>>> phenomenon as following call trace shows. after investigation, we
>>> found the firmware assigned an irq number 255 which means unknown
>>> or no connection in PCI local spec for i801_smbus, meanwhile the
>>> ACPI didn't configure the pci irq routing. and the 255 irq number
>>> was assigned for megasa msix without IRQF_SHARED. then in this case
>>> during i801_smbus probe, the i801_smbus driver would request irq with
>>> bad irq number 255. but the 255 irq number was assigned for memgasa
>>> with MSIX enable. which will cause request_irq fails, and call trace
>>> shows, actually, we should expose the error early, rather than in request
>>> irq, here we simply fix the problem by return err when find the irq is
>>> 255.
>
>>> See the call trace:
>>>
>>>   [   32.459195] ipmi device interface
>>>   [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>>   [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>>   [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>>   [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>>   [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>>   [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>>   [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>>   [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>>   [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>>   [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>>   [   32.851178] Workqueue: events work_for_cpu_fn
>>>   [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>>   [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>>   [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>>   [   32.851247] Call Trace:
>>>   [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>>   [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>>   [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>>   [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>>   [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>>   [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>>   [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>>   [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>>   [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>>   [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>>   [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>>   [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>   [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>>   [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>>   [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>>   [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>
> Since the Interrupt Line register is writable and might contain any
> value, it would be nice if Linux could at least tolerate anything
> firmware might leave there without a backtrace, even if we end up not
> being able to use the device.
>
> Your patch changes the acpi_pci_irq_enable() return value from 0 to
> -EINVAL for this case.  You're running v3.10, and this change probably
> makes pci_enable_device() fail.  I suppose the user-visible effect is
> that with your patch,
>
>    - there's no backtrace,
>    - i801_smbus fails with "Failed to enable SMBus PCI device" instead
>      of with "Failed to allocate irq 255", and
>    - i801_smbus fails even if no other device is using IRQ 255, instead
>      of "succeeding" and using an IRQ 255 that probably doesn't work
>      (this seems like maybe the most important difference)
>
> Jiang has changed this path with 890e4847587f ("PCI: Add
> pcibios_alloc_irq() and pcibios_free_irq()"), so I think on newer
> kernels, we'll never even call the i801_smbus probe function.
>
> What behavior are you looking for from i801_smbus?  Decline to claim
> the device?  Try to use the device without interrupts?  Try to figure
> out an interrupt in some other way?
>
> I'm not 100% sure that 890e4847587f does the right thing by preventing
> a driver from claiming a device where we can't set up an IRQ.  It's
> conceivable that a driver could still operate a device even without an
> IRQ.
>
>>>   [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>>>   [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
>>>
>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
>>> --- a/drivers/acpi/pci_irq.c
>>> +++ b/drivers/acpi/pci_irq.c
>>> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>>   		if (acpi_isa_register_gsi(dev))
>>>   			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>>   				 pin_name(pin));
>>> +		rc = 0;
>>> +		/*
>>> +		 * Excluding the BIOS report the value 255, which meaning
>>> +		 * "unknown" or "no connection" in PCI Local Bus Specification
>>> +		 * Revision 3.0 February 3, 2004, P223.
>>
>> You mean the footnote on page 223 talking about the Interrupt Line values, right?
>>
>>> +		 */
>>> +		if (dev->irq == 0xFF)
>>> +			rc = -EINVAL;
>>>
>>>   		kfree(entry);
>>> -		return 0;
>>> +		return rc;
>>>   	}
>>>
>>>   	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>>>
>>
>> Well, if you look at acpi_isa_register_gsi(), you'll see that it
>> actually does the check you're adding, so maybe the following should
>> be done instead?
>>
>> ---
>>   drivers/acpi/pci_irq.c |    5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/pci_irq.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/pci_irq.c
>> +++ linux-pm/drivers/acpi/pci_irq.c
>> @@ -436,12 +436,13 @@ int acpi_pci_irq_enable(struct pci_dev *
>>   	 * driver reported one, then use it. Exit in any case.
>>   	 */
>>   	if (gsi < 0) {
>> -		if (acpi_isa_register_gsi(dev))
>> +		rc = acpi_isa_register_gsi(dev);
>> +		if (rc)
>>   			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>   				 pin_name(pin));
>>
>>   		kfree(entry);
>> -		return 0;
>> +		return rc;
>>   	}
>>
>>   	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>>
>> --
>> 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
>
>
> .
>

-- 
Yours Sincerely,

Cao jin

[toc] | [prev] | [next] | [standalone]

#1314604

From	"Rafael J. Wysocki" <rafael@kernel.org>
Date	2016-01-22 00:00 +0100
Message-ID	<qTwmU-3nT-53@gated-at.bofh.it>
In reply to	#1314229

On Thu, Jan 21, 2016 at 3:41 PM, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> Hi,
>
>     IMHO, I think maybe modification on i801_smbus driver is easier.
>
>     Because when i801_smbus request_irq using pci_dev->irq, this
> pci_dev->irq seems still holds the value read from register(
> pci_setup_device->pci_read_irq), if the value is 255, it is invalid in
> register,

Right.

Which is why the PCI core should not leak it into the driver's ->probe callback.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1315176

From	Bjorn Helgaas <helgaas@kernel.org>
Date	2016-01-22 19:00 +0100
Message-ID	<qTOa7-7nC-7@gated-at.bofh.it>
In reply to	#1314604

On Thu, Jan 21, 2016 at 11:58:26PM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 21, 2016 at 3:41 PM, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> > Hi,
> >
> >     IMHO, I think maybe modification on i801_smbus driver is easier.
> >
> >     Because when i801_smbus request_irq using pci_dev->irq, this
> > pci_dev->irq seems still holds the value read from register(
> > pci_setup_device->pci_read_irq), if the value is 255, it is invalid in
> > register,
> 
> Right.
> 
> Which is why the PCI core should not leak it into the driver's ->probe callback.

Is there a reserved IRQ value we could use to mean "invalid"?

I guess we have NR_IRQS as a ceiling, so the range of valid IRQs would be
[0 .. NR_IRQS - 1].  It looks like irq_desc() and a few drivers already
rely on NR_IRQS being the bound:

  lpc32xx_kscan_probe
  lpc32xx_nand_probe
  pcmcia_setup_isa_irq
  lpc32xx_rtc_probe
  apbuart_verify_port
  ar933x_uart_verify_port
  lqasc_verify_port

So I guess we could use ~0 as "invalid IRQ", and maybe the PCI core could
set dev->irq to ~0 in these cases, and drivers like i801_smbus could check
for that.  Maybe a wrapper like irq_valid() would be useful.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1315235

From	David Daney <ddaney.cavm@gmail.com>
Date	2016-01-22 20:30 +0100
Message-ID	<qTPzc-8tD-21@gated-at.bofh.it>
In reply to	#1315176

On 01/22/2016 09:53 AM, Bjorn Helgaas wrote:
> On Thu, Jan 21, 2016 at 11:58:26PM +0100, Rafael J. Wysocki wrote:
>> On Thu, Jan 21, 2016 at 3:41 PM, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>>> Hi,
>>>
>>>      IMHO, I think maybe modification on i801_smbus driver is easier.
>>>
>>>      Because when i801_smbus request_irq using pci_dev->irq, this
>>> pci_dev->irq seems still holds the value read from register(
>>> pci_setup_device->pci_read_irq), if the value is 255, it is invalid in
>>> register,
>>
>> Right.
>>
>> Which is why the PCI core should not leak it into the driver's ->probe callback.
>
> Is there a reserved IRQ value we could use to mean "invalid"?

In many (most) cases, zero indicates no irq.




>
> I guess we have NR_IRQS as a ceiling, so the range of valid IRQs would be
> [0 .. NR_IRQS - 1].  It looks like irq_desc() and a few drivers already
> rely on NR_IRQS being the bound:
>
>    lpc32xx_kscan_probe
>    lpc32xx_nand_probe
>    pcmcia_setup_isa_irq
>    lpc32xx_rtc_probe
>    apbuart_verify_port
>    ar933x_uart_verify_port
>    lqasc_verify_port
>
> So I guess we could use ~0 as "invalid IRQ", and maybe the PCI core could
> set dev->irq to ~0 in these cases, and drivers like i801_smbus could check
> for that.  Maybe a wrapper like irq_valid() would be useful.
>
> Bjorn
>
>

[toc] | [prev] | [next] | [standalone]

#1315487

From	"Rafael J. Wysocki" <rafael@kernel.org>
Date	2016-01-23 03:10 +0100
Message-ID	<qTVOi-4k3-9@gated-at.bofh.it>
In reply to	#1315235

On Fri, Jan 22, 2016 at 8:23 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 01/22/2016 09:53 AM, Bjorn Helgaas wrote:
>>
>> On Thu, Jan 21, 2016 at 11:58:26PM +0100, Rafael J. Wysocki wrote:
>>>
>>> On Thu, Jan 21, 2016 at 3:41 PM, Cao jin <caoj.fnst@cn.fujitsu.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>>      IMHO, I think maybe modification on i801_smbus driver is easier.
>>>>
>>>>      Because when i801_smbus request_irq using pci_dev->irq, this
>>>> pci_dev->irq seems still holds the value read from register(
>>>> pci_setup_device->pci_read_irq), if the value is 255, it is invalid in
>>>> register,
>>>
>>>
>>> Right.
>>>
>>> Which is why the PCI core should not leak it into the driver's ->probe
>>> callback.
>>
>>
>> Is there a reserved IRQ value we could use to mean "invalid"?
>
>
> In many (most) cases, zero indicates no irq.

Zero is a valid timer IRQ on x86, though, so it's better not to give
any special meaning to it in general.

Using ~0 as suggested by Bjorn should work as it would cause
request_irq() to return -EINVAL if passed to it AFAICS.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1312877

From	Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Date	2016-01-20 06:10 +0100
Message-ID	<qSTbQ-1M2-29@gated-at.bofh.it>
In reply to	#1312250

On 01/19/2016 09:43 PM, Rafael J. Wysocki wrote:
> On Tuesday, January 19, 2016 09:45:13 AM Chen Fan wrote:
>> In our environment, when enable Secure boot, we found an abnormal
>> phenomenon as following call trace shows. after investigation, we
>> found the firmware assigned an irq number 255 which means unknown
>> or no connection in PCI local spec for i801_smbus, meanwhile the
>> ACPI didn't configure the pci irq routing. and the 255 irq number
>> was assigned for megasa msix without IRQF_SHARED. then in this case
>> during i801_smbus probe, the i801_smbus driver would request irq with
>> bad irq number 255. but the 255 irq number was assigned for memgasa
>> with MSIX enable. which will cause request_irq fails, and call trace
>> shows, actually, we should expose the error early, rather than in request
>> irq, here we simply fix the problem by return err when find the irq is
>> 255.
>>
>> See the call trace:
>>
>>   [   32.459195] ipmi device interface
>>   [   32.612907] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
>>   [   32.800459] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k-rh
>>   [   32.818319] ixgbe: Copyright (c) 1999-2014 Intel Corporation.
>>   [   32.844009] lpc_ich 0001:80:1f.0: I/O space for ACPI uninitialized
>>   [   32.850093] i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143)
>>   [   32.851134] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C
>>   [   32.851136] i801_smbus 0000:00:1f.3: PCI INT C: no GSI
>>   [   32.851164] genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa
>>   [   32.851168] CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1
>>   [   32.851170] Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5
>>   [   32.851178] Workqueue: events work_for_cpu_fn
>>   [   32.851208]  ffff88086c330b00 00000000e233a9df ffff88086d57bca0 ffffffff81603f36
>>   [   32.851227]  ffff88086d57bcf8 ffffffff8110d23a ffff88686fe02000 0000000000000246
>>   [   32.851246]  ffff88086a9a8c00 00000000e233a9df ffffffffa00ad220 0000000000000080
>>   [   32.851247] Call Trace:
>>   [   32.851261]  [<ffffffff81603f36>] dump_stack+0x19/0x1b
>>   [   32.851271]  [<ffffffff8110d23a>] __setup_irq+0x54a/0x570
>>   [   32.851282]  [<ffffffffa00ad220>] ? i801_check_pre.isra.5+0xe0/0xe0 [i2c_i801]
>>   [   32.851289]  [<ffffffff8110d3bc>] request_threaded_irq+0xcc/0x170
>>   [   32.851298]  [<ffffffffa00ae87f>] i801_probe+0x32f/0x508 [i2c_i801]
>>   [   32.851308]  [<ffffffff81308385>] local_pci_probe+0x45/0xa0
>>   [   32.851315]  [<ffffffff8108bfd4>] work_for_cpu_fn+0x14/0x20
>>   [   32.851323]  [<ffffffff8108f0ab>] process_one_work+0x17b/0x470
>>   [   32.851330]  [<ffffffff81090003>] worker_thread+0x293/0x400
>>   [   32.851338]  [<ffffffff8108fd70>] ? rescuer_thread+0x400/0x400
>>   [   32.851346]  [<ffffffff8109726f>] kthread+0xcf/0xe0
>>   [   32.851353]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>   [   32.851362]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
>>   [   32.851369]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
>>   [   32.851373] i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16
>>   [   32.851435] i801_smbus: probe of 0000:00:1f.3 failed with error -16
>>   [   33.180145] ixgbe 0000:5a:00.0: Multiq[   33.240538] ixgbe 0000:5a:00.0: (PCI Express:03:e0
>>   [   33.280826] ixgbe 0000:5a:00.0: MAC: 3, PHY: 0, PBA No: 000000-000
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.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 d30184c..d2f47f8 100644
>> --- a/drivers/acpi/pci_irq.c
>> +++ b/drivers/acpi/pci_irq.c
>> @@ -439,9 +439,17 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>   		if (acpi_isa_register_gsi(dev))
>>   			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>   				 pin_name(pin));
>> +		rc = 0;
>> +		/*
>> +		 * Excluding the BIOS report the value 255, which meaning
>> +		 * "unknown" or "no connection" in PCI Local Bus Specification
>> +		 * Revision 3.0 February 3, 2004, P223.
> You mean the footnote on page 223 talking about the Interrupt Line values, right?
>
>> +		 */
>> +		if (dev->irq == 0xFF)
>> +			rc = -EINVAL;
>>   
>>   		kfree(entry);
>> -		return 0;
>> +		return rc;
>>   	}
>>   
>>   	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>>
> Well, if you look at acpi_isa_register_gsi(), you'll see that it
> actually does the check you're adding, so maybe the following should
> be done instead?
no, the function acpi_isa_register_gsi() which check
the  dev->irq > 0 && (dev->irq <= 0xF), so if the dev->irq is 0xff,
it should return 0 directly. right?

>
> ---
>   drivers/acpi/pci_irq.c |    5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/acpi/pci_irq.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_irq.c
> +++ linux-pm/drivers/acpi/pci_irq.c
> @@ -436,12 +436,13 @@ int acpi_pci_irq_enable(struct pci_dev *
>   	 * driver reported one, then use it. Exit in any case.
>   	 */
>   	if (gsi < 0) {
> -		if (acpi_isa_register_gsi(dev))
> +		rc = acpi_isa_register_gsi(dev);
> +		if (rc)
>   			dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>   				 pin_name(pin));
>   
>   		kfree(entry);
> -		return 0;
> +		return rc;
>   	}
>   
>   	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>
>
>
> .
>

[toc] | [prev] | [standalone]

csiph-web

[PATCH] pci: fix unavailable irq number 255 reported by BIOS

Contents

#1311891 — [PATCH] pci: fix unavailable irq number 255 reported by BIOS

#1312250

#1312281

#1312307

#1312331

#1312353

#1312359

#1312879

#1312601

#1312864

#1313383

#1313987

#1314229

#1314604

#1315176

#1315235

#1315487

#1312877