[PATCH 0/4] PCI: aer_inject cleanups and verbosity

[PATCH 0/4] PCI: aer_inject cleanups and verbosity

[Jean Delvare]

As discussed earlier, here are a few cleanups and verbosity adjustments
to the aer_inject driver.

[PATCH 1/4] PCI: aer_inject: Fix error codes
[PATCH 2/4] PCI: aer_inject: Use dev_warn()
[PATCH 3/4] PCI: aer_inject: Log actual error causes
[PATCH 4/4] PCI: aer_inject: Log error injections

-- 
Jean Delvare
SUSE L3 Support

[PATCH 1/4] PCI: aer_inject: Fix error codes

[Jean Delvare]

EPERM means "Operation not permitted", which doesn't reflect the lack
of support for AER. EPROTONOSUPPORT (Protocol not supported) is a
better choice or error code if the device or its root port lack
support for AER.

Likewise, EINVAL means "Invalid argument", which is not suitable for
cases where the AER error device is missing or unusable. ENODEV and
EPROTONOSUPPORT, respectively, fit better.

Suggested-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Prarit Bhargava <prarit@redhat.com>
---
 drivers/pci/pcie/aer/aer_inject.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux-4.5-rc4.orig/drivers/pci/pcie/aer/aer_inject.c	2016-02-18 11:23:03.741503472 +0100
+++ linux-4.5-rc4/drivers/pci/pcie/aer/aer_inject.c	2016-02-18 13:18:18.999397788 +0100
@@ -340,7 +340,7 @@ static int aer_inject(struct aer_error_i
 
 	pos_cap_err = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 	if (!pos_cap_err) {
-		ret = -EPERM;
+		ret = -EPROTONOSUPPORT;
 		goto out_put;
 	}
 	pci_read_config_dword(dev, pos_cap_err + PCI_ERR_UNCOR_SEVER, &sever);
@@ -350,7 +350,7 @@ static int aer_inject(struct aer_error_i
 
 	rp_pos_cap_err = pci_find_ext_capability(rpdev, PCI_EXT_CAP_ID_ERR);
 	if (!rp_pos_cap_err) {
-		ret = -EPERM;
+		ret = -EPROTONOSUPPORT;
 		goto out_put;
 	}
 
@@ -458,12 +458,12 @@ static int aer_inject(struct aer_error_i
 	if (find_aer_device(rpdev, &edev)) {
 		if (!get_service_data(edev)) {
 			printk(KERN_WARNING "AER service is not initialized\n");
-			ret = -EINVAL;
+			ret = -EPROTONOSUPPORT;
 			goto out_put;
 		}
 		aer_irq(-1, edev);
 	} else
-		ret = -EINVAL;
+		ret = -ENODEV;
 out_put:
 	kfree(err_alloc);
 	kfree(rperr_alloc);

-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/4] PCI: aer_inject: Use dev_warn()

[Jean Delvare]

dev_warn() is better than printk(LOG_WARNING...) as it records which
device the message relates to. Also add a prefix "aer_inject:" to
help differentiate real errors from injected errors.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aer/aer_inject.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

--- linux-4.5-rc4.orig/drivers/pci/pcie/aer/aer_inject.c	2016-02-18 11:41:48.738010173 +0100
+++ linux-4.5-rc4/drivers/pci/pcie/aer/aer_inject.c	2016-02-18 12:50:56.927581343 +0100
@@ -25,6 +25,7 @@
 #include <linux/fs.h>
 #include <linux/uaccess.h>
 #include <linux/stddef.h>
+#include <linux/device.h>
 #include "aerdrv.h"
 
 /* Override the existing corrected and uncorrected error masks */
@@ -397,14 +398,16 @@ static int aer_inject(struct aer_error_i
 	if (!aer_mask_override && einj->cor_status &&
 	    !(einj->cor_status & ~cor_mask)) {
 		ret = -EINVAL;
-		printk(KERN_WARNING "The correctable error(s) is masked by device\n");
+		dev_warn(&dev->dev,
+			 "aer_inject: The correctable error(s) is masked by device\n");
 		spin_unlock_irqrestore(&inject_lock, flags);
 		goto out_put;
 	}
 	if (!aer_mask_override && einj->uncor_status &&
 	    !(einj->uncor_status & ~uncor_mask)) {
 		ret = -EINVAL;
-		printk(KERN_WARNING "The uncorrectable error(s) is masked by device\n");
+		dev_warn(&dev->dev,
+			 "aer_inject: The uncorrectable error(s) is masked by device\n");
 		spin_unlock_irqrestore(&inject_lock, flags);
 		goto out_put;
 	}
@@ -457,7 +460,8 @@ static int aer_inject(struct aer_error_i
 
 	if (find_aer_device(rpdev, &edev)) {
 		if (!get_service_data(edev)) {
-			printk(KERN_WARNING "AER service is not initialized\n");
+			dev_warn(&edev->device,
+				 "aer_inject: AER service is not initialized\n");
 			ret = -EPROTONOSUPPORT;
 			goto out_put;
 		}

-- 
Jean Delvare
SUSE L3 Support

[PATCH 3/4] PCI: aer_inject: Log actual error causes

[Jean Delvare]

The aer_inject driver is very quiet. In most cases, it merely returns
an error code to user-space, leaving the user with little clue about
the actual reason for the failure.

So, log error messages for 4 of the most frequent causes of failure:
* Can't find the root port of the specified device.
* Device doesn't support AER.
* Root port doesn't support AER.
* AER device not found.
This gives the user a chance to understand why aer-inject failed.

Based on a preliminary patch by Thomas Renninger.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: Thomas Renninger <trenn@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aer/aer_inject.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--- linux-4.5-rc4.orig/drivers/pci/pcie/aer/aer_inject.c	2016-02-18 12:50:56.927581343 +0100
+++ linux-4.5-rc4/drivers/pci/pcie/aer/aer_inject.c	2016-02-18 12:53:18.432587496 +0100
@@ -335,12 +335,14 @@ static int aer_inject(struct aer_error_i
 		return -ENODEV;
 	rpdev = pcie_find_root_port(dev);
 	if (!rpdev) {
+		dev_err(&dev->dev, "aer_inject: Root port not found\n");
 		ret = -ENODEV;
 		goto out_put;
 	}
 
 	pos_cap_err = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 	if (!pos_cap_err) {
+		dev_err(&dev->dev, "aer_inject: Device doesn't support AER\n");
 		ret = -EPROTONOSUPPORT;
 		goto out_put;
 	}
@@ -351,6 +353,8 @@ static int aer_inject(struct aer_error_i
 
 	rp_pos_cap_err = pci_find_ext_capability(rpdev, PCI_EXT_CAP_ID_ERR);
 	if (!rp_pos_cap_err) {
+		dev_err(&rpdev->dev,
+			"aer_inject: Root port doesn't support AER\n");
 		ret = -EPROTONOSUPPORT;
 		goto out_put;
 	}
@@ -466,8 +470,10 @@ static int aer_inject(struct aer_error_i
 			goto out_put;
 		}
 		aer_irq(-1, edev);
-	} else
+	} else {
+		dev_err(&rpdev->dev, "aer_inject: AER device not found\n");
 		ret = -ENODEV;
+	}
 out_put:
 	kfree(err_alloc);
 	kfree(rperr_alloc);

-- 
Jean Delvare
SUSE L3 Support

[PATCH 4/4] PCI: aer_inject: Log error injections

[Jean Delvare]

Log successful error injections so that injected errors can be
differentiated from real errors.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
Note: none of my systems support AER injection so this is untested.

 drivers/pci/pcie/aer/aer_inject.c |    3 +++
 1 file changed, 3 insertions(+)

--- linux-4.5-rc4.orig/drivers/pci/pcie/aer/aer_inject.c	2016-02-18 13:20:02.063933626 +0100
+++ linux-4.5-rc4/drivers/pci/pcie/aer/aer_inject.c	2016-02-18 13:20:02.515936151 +0100
@@ -469,6 +469,9 @@ static int aer_inject(struct aer_error_i
 			ret = -EPROTONOSUPPORT;
 			goto out_put;
 		}
+		dev_info(&edev->device,
+			 "aer_inject: Injecting errors %08x/%08x into device %s\n",
+			 einj->cor_status, einj->uncor_status, pci_name(dev));
 		aer_irq(-1, edev);
 	} else {
 		dev_err(&rpdev->dev, "aer_inject: AER device not found\n");

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/4] PCI: aer_inject cleanups and verbosity

[Bjorn Helgaas]

On Thu, Feb 18, 2016 at 01:09:28PM +0100, Jean Delvare wrote:
> As discussed earlier, here are a few cleanups and verbosity adjustments
> to the aer_inject driver.
> 
> [PATCH 1/4] PCI: aer_inject: Fix error codes
> [PATCH 2/4] PCI: aer_inject: Use dev_warn()
> [PATCH 3/4] PCI: aer_inject: Log actual error causes
> [PATCH 4/4] PCI: aer_inject: Log error injections

Applied all four to pci/aer for v4.6, thanks, Jean!