[PATCH 0/2] PCI/AER: Remove module parameters

[PATCH 0/2] PCI/AER: Remove module parameters

[Bjorn Helgaas]

I'd like to remove the aerdriver.nosourceid and aerdriver.forceload kernel
parameters.  They seem like basically debugging things that are not really
supportable upstream.

"nosourceid" may have some legitimate uses, but I'd rather it were done via
quirks with the flag Jon Derrick recently added so it happens
automatically, without users having to specify the parameter.

I don't *know* of any actual users of either of these, but obviously that
doesn't mean there are none.  Comments welcome.

---

Bjorn Helgaas (2):
      PCI/AER: Remove aerdriver.nosourceid kernel parameter
      PCI/AER: Remove aerdriver.forceload kernel parameter


 Documentation/PCI/pcieaer-howto.txt |   26 +++++++++-----------------
 drivers/pci/pcie/aer/aerdrv.c       |    7 +------
 drivers/pci/pcie/aer/aerdrv.h       |    8 --------
 drivers/pci/pcie/aer/aerdrv_core.c  |   31 ++++---------------------------
 4 files changed, 14 insertions(+), 58 deletions(-)

[PATCH 1/2] PCI/AER: Remove aerdriver.nosourceid kernel parameter

[Bjorn Helgaas]

The aerdriver.nosourceid kernel parameter was intended for working around
broken chipsets don't supply the source ID for AER events.  We recently
added PCI_BUS_FLAGS_NO_AERSID, which can be set by quirks for the same
purpose.

Remove the aerdriver.nosourceid kernel parameter.  For anything other than
debugging, asking users to find and use kernel parameters is a poor user
experience.  Instead, we should add PCI_BUS_FLAGS_NO_AERSID quirks for any
hardware that needs it.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 Documentation/PCI/pcieaer-howto.txt |    4 ----
 drivers/pci/pcie/aer/aerdrv_core.c  |   12 ++++--------
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/Documentation/PCI/pcieaer-howto.txt b/Documentation/PCI/pcieaer-howto.txt
index b4987c0..4956df3 100644
--- a/Documentation/PCI/pcieaer-howto.txt
+++ b/Documentation/PCI/pcieaer-howto.txt
@@ -61,10 +61,6 @@ be initiated although firmwares have no _OSC support. To enable the
 walkaround, pls. add aerdriver.forceload=y to kernel boot parameter line
 when booting kernel. Note that forceload=n by default.
 
-nosourceid, another parameter of type bool, can be used when broken
-hardware (mostly chipsets) has root ports that cannot obtain the reporting
-source ID. nosourceid=n by default.
-
 2.3 AER error output
 When a PCI-E AER error is captured, an error message will be outputted to
 console. If it's a correctable error, it is outputted as a warning.
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 8f5e14c..f8a9b17 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -28,9 +28,7 @@
 #include "aerdrv.h"
 
 static bool forceload;
-static bool nosourceid;
 module_param(forceload, bool, 0);
-module_param(nosourceid, bool, 0);
 
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
@@ -132,8 +130,7 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 	 * When bus id is equal to 0, it might be a bad id
 	 * reported by root port.
 	 */
-	if (!nosourceid &&
-	    (PCI_BUS_NUM(e_info->id) != 0) &&
+	if ((PCI_BUS_NUM(e_info->id) != 0) &&
 	    !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
 		/* Device ID match? */
 		if (e_info->id == ((dev->bus->number << 8) | dev->devfn))
@@ -146,11 +143,10 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 
 	/*
 	 * When either
-	 *      1) nosourceid==y;
-	 *      2) bus id is equal to 0. Some ports might lose the bus
+	 *      1) bus id is equal to 0. Some ports might lose the bus
 	 *              id of error source id;
-	 *      3) bus flag PCI_BUS_FLAGS_NO_AERSID is set
-	 *      4) There are multiple errors and prior ID comparing fails;
+	 *      2) bus flag PCI_BUS_FLAGS_NO_AERSID is set
+	 *      3) There are multiple errors and prior ID comparing fails;
 	 * We check AER status registers to find possible reporter.
 	 */
 	if (atomic_read(&dev->enable_cnt) == 0)

[PATCH 2/2] PCI/AER: Remove aerdriver.forceload kernel parameter

[Bjorn Helgaas]

Per the PCI Firmware spec, r3.0, sec 4.5.1, on ACPI systems, the OS must
not use AER unless _OSC is present and _OSC grants AER control to the OS.
The aerdriver.forceload kernel parameter was a way to enable Linux AER
support on ACPI systems that lack _OSC or fail to grant control the the OS.

Enabling Linux AER support when the firmware doesn't want us to is a recipe
for problems, e.g., the firmware might be handling AER itself.

Remove the aerdriver.forceload kernel parameter and related supporting
code.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 Documentation/PCI/pcieaer-howto.txt |   22 +++++++++-------------
 drivers/pci/pcie/aer/aerdrv.c       |    7 +------
 drivers/pci/pcie/aer/aerdrv.h       |    8 --------
 drivers/pci/pcie/aer/aerdrv_core.c  |   19 -------------------
 4 files changed, 10 insertions(+), 46 deletions(-)

diff --git a/Documentation/PCI/pcieaer-howto.txt b/Documentation/PCI/pcieaer-howto.txt
index 4956df3..ea8cafb 100644
--- a/Documentation/PCI/pcieaer-howto.txt
+++ b/Documentation/PCI/pcieaer-howto.txt
@@ -49,21 +49,17 @@ depends on CONFIG_PCIEPORTBUS, so pls. set CONFIG_PCIEPORTBUS=y and
 CONFIG_PCIEAER = y.
 
 2.2 Load PCI Express AER Root Driver
-There is a case where a system has AER support in BIOS. Enabling the AER
-Root driver and having AER support in BIOS may result unpredictable
-behavior. To avoid this conflict, a successful load of the AER Root driver
-requires ACPI _OSC support in the BIOS to allow the AER Root driver to
-request for native control of AER. See the PCI FW 3.0 Specification for
-details regarding OSC usage. Currently, lots of firmwares don't provide
-_OSC support while they use PCI Express. To support such firmwares,
-forceload, a parameter of type bool, could enable AER to continue to
-be initiated although firmwares have no _OSC support. To enable the
-walkaround, pls. add aerdriver.forceload=y to kernel boot parameter line
-when booting kernel. Note that forceload=n by default.
+
+Some systems have AER support in firmware. Enabling Linux AER support at
+the same time the firmware handles AER may result in unpredictable
+behavior. Therefore, Linux does not handle AER events unless the firmware
+grants AER control to the OS via the ACPI _OSC method. See the PCI FW 3.0
+Specification for details regarding _OSC usage.
 
 2.3 AER error output
-When a PCI-E AER error is captured, an error message will be outputted to
-console. If it's a correctable error, it is outputted as a warning.
+
+When a PCIe AER error is captured, an error message will be output to
+console. If it's a correctable error, it is output as a warning.
 Otherwise, it is printed as an error. So users could choose different
 log level to filter out correctable error messages.
 
diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 48d21e0..08ce257 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -70,7 +70,7 @@ static int pcie_aer_disable;
 
 void pci_no_aer(void)
 {
-	pcie_aer_disable = 1;	/* has priority over 'forceload' */
+	pcie_aer_disable = 1;
 }
 
 bool pci_aer_available(void)
@@ -304,11 +304,6 @@ static int aer_probe(struct pcie_device *dev)
 	struct aer_rpc *rpc;
 	struct device *device = &dev->device;
 
-	/* Init */
-	status = aer_init(dev);
-	if (status)
-		return status;
-
 	/* Alloc rpc data structure */
 	rpc = aer_alloc_rpc(dev);
 	if (!rpc) {
diff --git a/drivers/pci/pcie/aer/aerdrv.h b/drivers/pci/pcie/aer/aerdrv.h
index 945c939..f15ca8d 100644
--- a/drivers/pci/pcie/aer/aerdrv.h
+++ b/drivers/pci/pcie/aer/aerdrv.h
@@ -105,7 +105,6 @@ static inline pci_ers_result_t merge_result(enum pci_ers_result orig,
 }
 
 extern struct bus_type pcie_port_bus_type;
-int aer_init(struct pcie_device *dev);
 void aer_isr(struct work_struct *work);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
 void aer_print_port_info(struct pci_dev *dev, struct aer_err_info *info);
@@ -121,11 +120,4 @@ static inline int pcie_aer_get_firmware_first(struct pci_dev *pci_dev)
 	return 0;
 }
 #endif
-
-static inline void pcie_aer_force_firmware_first(struct pci_dev *pci_dev,
-						 int enable)
-{
-	pci_dev->__aer_firmware_first = !!enable;
-	pci_dev->__aer_firmware_first_valid = 1;
-}
 #endif /* _AERDRV_H_ */
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index f8a9b17..8262527 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -27,9 +27,6 @@
 #include <linux/kfifo.h>
 #include "aerdrv.h"
 
-static bool forceload;
-module_param(forceload, bool, 0);
-
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 
@@ -811,19 +808,3 @@ void aer_isr(struct work_struct *work)
 		aer_isr_one_error(p_device, &e_src);
 	mutex_unlock(&rpc->rpc_mutex);
 }
-
-/**
- * aer_init - provide AER initialization
- * @dev: pointer to AER pcie device
- *
- * Invoked when AER service driver is loaded.
- */
-int aer_init(struct pcie_device *dev)
-{
-	if (forceload) {
-		dev_printk(KERN_DEBUG, &dev->device,
-			   "aerdrv forceload requested.\n");
-		pcie_aer_force_firmware_first(dev->port, 0);
-	}
-	return 0;
-}

Re: [PATCH 0/2] PCI/AER: Remove module parameters

[Bjorn Helgaas]

On Tue, Sep 06, 2016 at 05:19:22PM -0500, Bjorn Helgaas wrote:
> I'd like to remove the aerdriver.nosourceid and aerdriver.forceload kernel
> parameters.  They seem like basically debugging things that are not really
> supportable upstream.
> 
> "nosourceid" may have some legitimate uses, but I'd rather it were done via
> quirks with the flag Jon Derrick recently added so it happens
> automatically, without users having to specify the parameter.
> 
> I don't *know* of any actual users of either of these, but obviously that
> doesn't mean there are none.  Comments welcome.
> 
> ---
> 
> Bjorn Helgaas (2):
>       PCI/AER: Remove aerdriver.nosourceid kernel parameter
>       PCI/AER: Remove aerdriver.forceload kernel parameter
> 
> 
>  Documentation/PCI/pcieaer-howto.txt |   26 +++++++++-----------------
>  drivers/pci/pcie/aer/aerdrv.c       |    7 +------
>  drivers/pci/pcie/aer/aerdrv.h       |    8 --------
>  drivers/pci/pcie/aer/aerdrv_core.c  |   31 ++++---------------------------
>  4 files changed, 14 insertions(+), 58 deletions(-)

I'm going to go out on a limb a little bit and apply these to pci/aer for
v4.9.  Please let me know if you trip over any issues.

RE: [PATCH 0/2] PCI/AER: Remove module parameters

[Tang, CQ]

Bjorn,
     Can we extend this aerdriver to support AER from a root complex integrated endpoint? Root complex event collector has the same job as root port in this case.

     It looks that both port driver and aer driver check PCI_EXP_TYPE_ROOT_PORT/PCI_EXP_TYPE_UPSTREAM/PCI_EXP_TYPE_DOWNSTREAM only.
portdrv_pci.c:127, aerdrv.c:86

      Is it just a simple addition of PCI_EXP_TYPE_RC_EC to above two places?  Is there any difference to recover a root complex integrated endpoint? (compared with devices connected to root port).

Thanks.
--CQ

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> Sent: Wednesday, September 14, 2016 1:30 PM
> To: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org; Lance Ortiz <lance.ortiz@hpe.com>; Betty Dall
> <betty.dall@hpe.com>; Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>;
> Chen Gong <gong.chen@linux.intel.com>; KeithBuschkeith.busch@intel.com;
> Rafael J. Wysocki <rjw@rjwysocki.net>; linux-kernel@vger.kernel.org; Shuah
> Khan <shuahkh@osg.samsung.com>; Huang, Ying <ying.huang@intel.com>;
> Vijay Mohan Pandarathil <vijaymohan.pandarathil@hpe.com>; Derrick,
> Jonathan <jonathan.derrick@intel.com>
> Subject: Re: [PATCH 0/2] PCI/AER: Remove module parameters
> 
> On Tue, Sep 06, 2016 at 05:19:22PM -0500, Bjorn Helgaas wrote:
> > I'd like to remove the aerdriver.nosourceid and aerdriver.forceload
> > kernel parameters.  They seem like basically debugging things that are
> > not really supportable upstream.
> >
> > "nosourceid" may have some legitimate uses, but I'd rather it were
> > done via quirks with the flag Jon Derrick recently added so it happens
> > automatically, without users having to specify the parameter.
> >
> > I don't *know* of any actual users of either of these, but obviously
> > that doesn't mean there are none.  Comments welcome.
> >
> > ---
> >
> > Bjorn Helgaas (2):
> >       PCI/AER: Remove aerdriver.nosourceid kernel parameter
> >       PCI/AER: Remove aerdriver.forceload kernel parameter
> >
> >
> >  Documentation/PCI/pcieaer-howto.txt |   26 +++++++++-----------------
> >  drivers/pci/pcie/aer/aerdrv.c       |    7 +------
> >  drivers/pci/pcie/aer/aerdrv.h       |    8 --------
> >  drivers/pci/pcie/aer/aerdrv_core.c  |   31 ++++---------------------------
> >  4 files changed, 14 insertions(+), 58 deletions(-)
> 
> I'm going to go out on a limb a little bit and apply these to pci/aer for v4.9.
> Please let me know if you trip over any issues.
> --
> 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

Re: [PATCH 0/2] PCI/AER: Remove module parameters

[Bjorn Helgaas]

On Sat, Sep 17, 2016 at 04:49:11AM +0000, Tang, CQ wrote:
> Bjorn,
>      Can we extend this aerdriver to support AER from a root complex integrated endpoint? Root complex event collector has the same job as root port in this case.

Seems like a reasonable idea.

>      It looks that both port driver and aer driver check PCI_EXP_TYPE_ROOT_PORT/PCI_EXP_TYPE_UPSTREAM/PCI_EXP_TYPE_DOWNSTREAM only.
> portdrv_pci.c:127, aerdrv.c:86
> 
>       Is it just a simple addition of PCI_EXP_TYPE_RC_EC to above two places?  Is there any difference to recover a root complex integrated endpoint? (compared with devices connected to root port).

It may well be that the extension would be as simple as you suggest.
If you want to test and post a patch to that effect, I'll review it
more carefully.

Bjorn

> > -----Original Message-----
> > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> > Sent: Wednesday, September 14, 2016 1:30 PM
> > To: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: linux-pci@vger.kernel.org; Lance Ortiz <lance.ortiz@hpe.com>; Betty Dall
> > <betty.dall@hpe.com>; Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>;
> > Chen Gong <gong.chen@linux.intel.com>; KeithBuschkeith.busch@intel.com;
> > Rafael J. Wysocki <rjw@rjwysocki.net>; linux-kernel@vger.kernel.org; Shuah
> > Khan <shuahkh@osg.samsung.com>; Huang, Ying <ying.huang@intel.com>;
> > Vijay Mohan Pandarathil <vijaymohan.pandarathil@hpe.com>; Derrick,
> > Jonathan <jonathan.derrick@intel.com>
> > Subject: Re: [PATCH 0/2] PCI/AER: Remove module parameters
> > 
> > On Tue, Sep 06, 2016 at 05:19:22PM -0500, Bjorn Helgaas wrote:
> > > I'd like to remove the aerdriver.nosourceid and aerdriver.forceload
> > > kernel parameters.  They seem like basically debugging things that are
> > > not really supportable upstream.
> > >
> > > "nosourceid" may have some legitimate uses, but I'd rather it were
> > > done via quirks with the flag Jon Derrick recently added so it happens
> > > automatically, without users having to specify the parameter.
> > >
> > > I don't *know* of any actual users of either of these, but obviously
> > > that doesn't mean there are none.  Comments welcome.
> > >
> > > ---
> > >
> > > Bjorn Helgaas (2):
> > >       PCI/AER: Remove aerdriver.nosourceid kernel parameter
> > >       PCI/AER: Remove aerdriver.forceload kernel parameter
> > >
> > >
> > >  Documentation/PCI/pcieaer-howto.txt |   26 +++++++++-----------------
> > >  drivers/pci/pcie/aer/aerdrv.c       |    7 +------
> > >  drivers/pci/pcie/aer/aerdrv.h       |    8 --------
> > >  drivers/pci/pcie/aer/aerdrv_core.c  |   31 ++++---------------------------
> > >  4 files changed, 14 insertions(+), 58 deletions(-)
> > 
> > I'm going to go out on a limb a little bit and apply these to pci/aer for v4.9.
> > Please let me know if you trip over any issues.
> > --
> > 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