[PATCH 0/4] powerpc/eeh: More precisely error injection

[PATCH 0/4] powerpc/eeh: More precisely error injection

[Gavin Shan]

Previously, we inject PCI errors through various debugfs entries in
/sys/kernel/debug/powerpc/PCIxxxx/. Unfortunately, the PCI errors
are injected with PHB granularity. It's hard to injection errors to
specified PE. The series of patches adds one more debugfs entry, which
allows to inject errors to specified PE for testing purpose.

echo "pe_no:0:function:0:0" > /sys/kernel/debug/powerpc/PCIxxxx/err_injct

The frequently used "function" would be:
    0 : MMIO read
    4 : CFG read
    6 : MMIO write
    10: CFG write

Gavin Shan (1):
  powerpc/powernv: Clear PAPR error injection registers

Mike Qiu (3):
  powerpc/powernv: Sync header with firmware
  powerpc/eeh: Introduce eeh_ops::err_inject
  powerpc/powernv: Add error injection debugfs entry

 arch/powerpc/include/asm/eeh.h                 |   2 +
 arch/powerpc/include/asm/opal.h                |  30 +++++++
 arch/powerpc/platforms/powernv/eeh-ioda.c      | 115 +++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/eeh-powernv.c   |  26 ++++++
 arch/powerpc/platforms/powernv/opal-wrappers.S |   1 +
 arch/powerpc/platforms/powernv/pci.h           |   2 +
 arch/powerpc/platforms/pseries/eeh_pseries.c   |   1 +
 7 files changed, 177 insertions(+)

-- 
1.8.3.2

[PATCH 1/4] powerpc/powernv: Sync header with firmware

[Gavin Shan]

From: Mike Qiu <qiudayu@linux.vnet.ibm.com>

The patch synchronizes firmware header file (opal.h) for PCI error
injection.

Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/opal.h                | 30 ++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal-wrappers.S |  1 +
 2 files changed, 31 insertions(+)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 4593a93..9113653 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -148,6 +148,7 @@ struct opal_sg_list {
 #define OPAL_SET_PARAM				90
 #define OPAL_DUMP_RESEND			91
 #define OPAL_DUMP_INFO2				94
+#define OPAL_PCI_ERR_INJCT			96
 #define OPAL_PCI_EEH_FREEZE_SET			97
 #define OPAL_HANDLE_HMI				98
 #define OPAL_REGISTER_DUMP_REGION		101
@@ -200,6 +201,33 @@ enum OpalPciErrorSeverity {
 	OPAL_EEH_SEV_INF	= 5
 };
 
+enum OpalErrinjctType {
+	OpalErrinjctTypeIoaBusError	= 0,
+	OpalErrinjctTypeIoaBusError64	= 1,
+
+	/* IoaBusError & IoaBusError64 */
+	OpalEjtIoaLoadMemAddr		= 0,
+	OpalEjtIoaLoadMemData		= 1,
+	OpalEjtIoaLoadIoAddr		= 2,
+	OpalEjtIoaLoadIoData		= 3,
+	OpalEjtIoaLoadConfigAddr	= 4,
+	OpalEjtIoaLoadConfigData	= 5,
+	OpalEjtIoaStoreMemAddr		= 6,
+	OpalEjtIoaStoreMemData		= 7,
+	OpalEjtIoaStoreIoAddr		= 8,
+	OpalEjtIoaStoreIoData		= 9,
+	OpalEjtIoaStoreConfigAddr	= 10,
+	OpalEjtIoaStoreConfigData	= 11,
+	OpalEjtIoaDmaReadMemAddr	= 12,
+	OpalEjtIoaDmaReadMemData	= 13,
+	OpalEjtIoaDmaReadMemMaster	= 14,
+	OpalEjtIoaDmaReadMemTarget	= 15,
+	OpalEjtIoaDmaWriteMemAddr	= 16,
+	OpalEjtIoaDmaWriteMemData	= 17,
+	OpalEjtIoaDmaWriteMemMaster	= 18,
+	OpalEjtIoaDmaWriteMemTarget	= 19,
+};
+
 enum OpalShpcAction {
 	OPAL_SHPC_GET_LINK_STATE = 0,
 	OPAL_SHPC_GET_SLOT_STATE = 1
@@ -825,6 +853,8 @@ int64_t opal_pci_eeh_freeze_clear(uint64_t phb_id, uint64_t pe_number,
 				  uint64_t eeh_action_token);
 int64_t opal_pci_eeh_freeze_set(uint64_t phb_id, uint64_t pe_number,
 				uint64_t eeh_action_token);
+int64_t opal_pci_err_inject(uint64_t phb_id, uint32_t pe_no, uint32_t type,
+			    uint32_t function, uint64_t addr, uint64_t mask);
 int64_t opal_pci_shpc(uint64_t phb_id, uint64_t shpc_action, uint8_t *state);
 
 
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index 5718855..9c68501 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -184,6 +184,7 @@ OPAL_CALL(opal_register_exception_handler,	OPAL_REGISTER_OPAL_EXCEPTION_HANDLER)
 OPAL_CALL(opal_pci_eeh_freeze_status,		OPAL_PCI_EEH_FREEZE_STATUS);
 OPAL_CALL(opal_pci_eeh_freeze_clear,		OPAL_PCI_EEH_FREEZE_CLEAR);
 OPAL_CALL(opal_pci_eeh_freeze_set,		OPAL_PCI_EEH_FREEZE_SET);
+OPAL_CALL(opal_pci_err_inject,			OPAL_PCI_ERR_INJCT);
 OPAL_CALL(opal_pci_shpc,			OPAL_PCI_SHPC);
 OPAL_CALL(opal_pci_phb_mmio_enable,		OPAL_PCI_PHB_MMIO_ENABLE);
 OPAL_CALL(opal_pci_set_phb_mem_window,		OPAL_PCI_SET_PHB_MEM_WINDOW);
-- 
1.8.3.2

[PATCH 2/4] powerpc/eeh: Introduce eeh_ops::err_inject

[Gavin Shan]

From: Mike Qiu <qiudayu@linux.vnet.ibm.com>

The patch introduces eeh_ops::err_inject(), which allows to inject
specified errors to indicated PE for testing purpose. The functionality
isn't support on pSeries platform. On PowerNV, the functionality
relies on OPAL API opal_pci_err_inject().

Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/eeh.h               |  2 ++
 arch/powerpc/platforms/powernv/eeh-ioda.c    | 38 ++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/eeh-powernv.c | 26 +++++++++++++++++++
 arch/powerpc/platforms/powernv/pci.h         |  2 ++
 arch/powerpc/platforms/pseries/eeh_pseries.c |  1 +
 5 files changed, 69 insertions(+)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index f98b1b5..1951d07 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -199,6 +199,8 @@ struct eeh_ops {
 	int (*wait_state)(struct eeh_pe *pe, int max_wait);
 	int (*get_log)(struct eeh_pe *pe, int severity, char *drv_log, unsigned long len);
 	int (*configure_bridge)(struct eeh_pe *pe);
+	int (*err_inject)(struct eeh_pe *pe, int type, int function,
+			  unsigned long addr, unsigned long mask);
 	int (*read_config)(struct device_node *dn, int where, int size, u32 *val);
 	int (*write_config)(struct device_node *dn, int where, int size, u32 val);
 	int (*next_error)(struct eeh_pe **pe);
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index f3027b9..18099c5 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -673,6 +673,43 @@ static int ioda_eeh_configure_bridge(struct eeh_pe *pe)
 	return 0;
 }
 
+static int ioda_eeh_err_inject(struct eeh_pe *pe, int type, int function,
+			       unsigned long addr, unsigned long mask)
+{
+	struct pci_controller *hose = pe->phb;
+	struct pnv_phb *phb = hose->private_data;
+	s64 ret;
+
+	/* Sanity check on error type */
+	if (type < OpalErrinjctTypeIoaBusError   ||
+	    type > OpalErrinjctTypeIoaBusError64 ||
+	    function < OpalEjtIoaLoadMemAddr     ||
+	    function > OpalEjtIoaDmaWriteMemTarget) {
+		pr_warn("%s: Invalid error type %d-%d\n",
+			__func__, type, function);
+		return -ERANGE;
+	}
+
+	/* Firmware supports error injection ? */
+	ret = opal_check_token(OPAL_PCI_ERR_INJCT);
+	if (ret != OPAL_TOKEN_PRESENT) {
+		pr_warn("%s: Firmware not support error injection (%lld)\n",
+			__func__, ret);
+		return -ENXIO;
+	}
+
+	/* Do error injection */
+	ret = opal_pci_err_inject(phb->opal_id, pe->addr,
+				  type, function, addr, mask);
+	if (ret != OPAL_SUCCESS) {
+		pr_warn("%s: Failure %lld injecting error to PHB#%x-PE#%x\n",
+			__func__, ret, hose->global_number, pe->addr);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static void ioda_eeh_hub_diag_common(struct OpalIoP7IOCErrorData *data)
 {
 	/* GEM */
@@ -994,5 +1031,6 @@ struct pnv_eeh_ops ioda_eeh_ops = {
 	.reset			= ioda_eeh_reset,
 	.get_log		= ioda_eeh_get_log,
 	.configure_bridge	= ioda_eeh_configure_bridge,
+	.err_inject		= ioda_eeh_err_inject,
 	.next_error		= ioda_eeh_next_error
 };
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index fd7a16f..f5b1424 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -359,6 +359,31 @@ static int powernv_eeh_configure_bridge(struct eeh_pe *pe)
 }
 
 /**
+ * powernv_pe_err_inject - Inject specified error to the indicated PE
+ * @pe: the indicated PE
+ * @type: error type
+ * @function: specific error type
+ * @addr: address
+ * @mask: address mask
+ *
+ * The routine is called to inject specified error, which is
+ * determined by @type and @function, to the indicated PE for
+ * testing purpose.
+ */
+static int powernv_eeh_err_inject(struct eeh_pe *pe, int type, int function,
+				  unsigned long addr, unsigned long mask)
+{
+	struct pci_controller *hose = pe->phb;
+	struct pnv_phb *phb = hose->private_data;
+	int ret = -EEXIST;
+
+	if (phb->eeh_ops && phb->eeh_ops->err_inject)
+		ret = phb->eeh_ops->err_inject(pe, type, function, addr, mask);
+
+	return ret;
+}
+
+/**
  * powernv_eeh_next_error - Retrieve next EEH error to handle
  * @pe: Affected PE
  *
@@ -414,6 +439,7 @@ static struct eeh_ops powernv_eeh_ops = {
 	.wait_state             = powernv_eeh_wait_state,
 	.get_log                = powernv_eeh_get_log,
 	.configure_bridge       = powernv_eeh_configure_bridge,
+	.err_inject		= powernv_eeh_err_inject,
 	.read_config            = pnv_pci_cfg_read,
 	.write_config           = pnv_pci_cfg_write,
 	.next_error		= powernv_eeh_next_error,
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 48494d4..b54af34 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -85,6 +85,8 @@ struct pnv_eeh_ops {
 	int (*get_log)(struct eeh_pe *pe, int severity,
 		       char *drv_log, unsigned long len);
 	int (*configure_bridge)(struct eeh_pe *pe);
+	int (*err_inject)(struct eeh_pe *pe, int type, int function,
+			  unsigned long addr, unsigned long mask);
 	int (*next_error)(struct eeh_pe **pe);
 };
 #endif /* CONFIG_EEH */
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index b645dc6..4fc5ff9 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -731,6 +731,7 @@ static struct eeh_ops pseries_eeh_ops = {
 	.wait_state		= pseries_eeh_wait_state,
 	.get_log		= pseries_eeh_get_log,
 	.configure_bridge       = pseries_eeh_configure_bridge,
+	.err_inject		= NULL,
 	.read_config		= pseries_eeh_read_config,
 	.write_config		= pseries_eeh_write_config,
 	.next_error		= NULL,
-- 
1.8.3.2

[PATCH 3/4] powerpc/powernv: Add error injection debugfs entry

[Gavin Shan]

From: Mike Qiu <qiudayu@linux.vnet.ibm.com>

The patch adds debugfs file (/sys/kernel/debug/powerpc/PCIxxxx/
err_injct), which accepts following formated string, to support
error injection. It will be used to support userland utility
"errinjct" in future.

  "pe_no:0:function:address:mask" - 32-bits PCI errors
  "pe_no:1:function:address:mask" - 64-bits PCI errors

Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/eeh-ioda.c | 52 +++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 18099c5..4d40093 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -66,6 +66,54 @@ static struct notifier_block ioda_eeh_nb = {
 };
 
 #ifdef CONFIG_DEBUG_FS
+static ssize_t ioda_eeh_ei_write(struct file *filp,
+				 const char __user *user_buf,
+				 size_t count, loff_t *ppos)
+{
+	struct pci_controller *hose = filp->private_data;
+	struct pnv_phb *phb = hose->private_data;
+	struct eeh_dev *edev;
+	struct eeh_pe *pe;
+	int pe_no, type, func;
+	unsigned long addr, mask;
+	char buf[50];
+	int ret;
+
+	if (!phb->eeh_ops || !phb->eeh_ops->err_inject)
+		return -ENXIO;
+
+	ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
+	if (!ret)
+		return -EFAULT;
+
+	/* Retrieve parameters */
+	ret = sscanf(buf, "%x:%x:%x:%lx:%lx",
+		     &pe_no, &type, &func, &addr, &mask);
+	if (ret != 5)
+		return -EINVAL;
+
+	/* Retrieve PE */
+	edev = kzalloc(sizeof(*edev), GFP_KERNEL);
+	if (!edev)
+		return -ENOMEM;
+	edev->phb = hose;
+	edev->pe_config_addr = pe_no;
+	pe = eeh_pe_get(edev);
+	kfree(edev);
+	if (!pe)
+		return -ENODEV;
+
+	/* Do error injection */
+	ret = phb->eeh_ops->err_inject(pe, type, func, addr, mask);
+	return ret < 0 ? ret : count;
+}
+
+static const struct file_operations ioda_eeh_ei_fops = {
+	.open   = simple_open,
+	.llseek = no_llseek,
+	.write  = ioda_eeh_ei_write,
+};
+
 static int ioda_eeh_dbgfs_set(void *data, int offset, u64 val)
 {
 	struct pci_controller *hose = data;
@@ -152,6 +200,10 @@ static int ioda_eeh_post_init(struct pci_controller *hose)
 	if (!phb->has_dbgfs && phb->dbgfs) {
 		phb->has_dbgfs = 1;
 
+		debugfs_create_file("err_injct", 0200,
+				    phb->dbgfs, hose,
+				    &ioda_eeh_ei_fops);
+
 		debugfs_create_file("err_injct_outbound", 0600,
 				    phb->dbgfs, hose,
 				    &ioda_eeh_outb_dbgfs_ops);
-- 
1.8.3.2

[PATCH 4/4] powerpc/powernv: Clear PAPR error injection registers

[Gavin Shan]

The frozen state on one specific PE is probably caused by error
injection, which is done with help of PAPR error injection registers.
According to the hardware spec, those registers should be cleared
automatically after one-shot frozen PE. However, that's not always
true, at least on P7IOC of Firebird-L. So we have to clear them
before doing PE reset to avoid recursive EEH errors at recovery
stage.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/eeh-ioda.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 4d40093..729e445 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -682,6 +682,31 @@ static int ioda_eeh_reset(struct eeh_pe *pe, int option)
 	if (pe->type & EEH_PE_PHB) {
 		ret = ioda_eeh_phb_reset(hose, option);
 	} else {
+		struct pnv_phb *phb;
+		s64 rc;
+
+		/*
+		 * The frozen PE might be caused by PAPR error injection
+		 * registers, which are expected to be cleared after hitting
+		 * frozen PE as stated in the hardware spec. Unfortunately,
+		 * that's not true on P7IOC. So we have to clear it manually
+		 * to avoid recursive EEH errors during recovery.
+		 */
+		phb = hose->private_data;
+		if (phb->model == PNV_PHB_MODEL_P7IOC &&
+		    (option == EEH_RESET_HOT ||
+		    option == EEH_RESET_FUNDAMENTAL)) {
+			rc = opal_pci_reset(phb->opal_id,
+					    OPAL_PHB_ERROR,
+					    OPAL_ASSERT_RESET);
+			if (rc != OPAL_SUCCESS) {
+				pr_warn("%s: Failure %lld clearing "
+					"error injection registers\n",
+					__func__, rc);
+				return -EIO;
+			}
+		}
+
 		bus = eeh_pe_bus_get(pe);
 		if (pci_is_root_bus(bus) ||
 		    pci_is_root_bus(bus->parent))
-- 
1.8.3.2

Re: [2/4] powerpc/eeh: Introduce eeh_ops::err_inject

[Michael Ellerman]

On Tue, 2014-26-08 at 07:56:17 UTC, Gavin Shan wrote:
> From: Mike Qiu <qiudayu@linux.vnet.ibm.com>
> 
> +
> +	/* Firmware supports error injection ? */
> +	ret = opal_check_token(OPAL_PCI_ERR_INJCT);
> +	if (ret != OPAL_TOKEN_PRESENT) {
> +		pr_warn("%s: Firmware not support error injection (%lld)\n",
> +			__func__, ret);

This doesn't build, we dropped OPAL_TOKEN_PRESENT.

I've replaced it with:

       if (!opal_check_token(OPAL_PCI_ERR_INJCT)) {
               pr_warn("%s: Firmware doesn't support error injection (%lld)\n",
                        __func__, ret);

cheers

Re: [1/4] powerpc/powernv: Sync header with firmware

[Michael Ellerman]

On Tue, 2014-26-08 at 07:56:16 UTC, Gavin Shan wrote:
> From: Mike Qiu <qiudayu@linux.vnet.ibm.com>
> 
> The patch synchronizes firmware header file (opal.h) for PCI error
> injection.
> 
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 4593a93..9113653 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -200,6 +201,33 @@ enum OpalPciErrorSeverity {
>  	OPAL_EEH_SEV_INF	= 5
>  };
>  
> +enum OpalErrinjctType {
> +	OpalErrinjctTypeIoaBusError	= 0,
> +	OpalErrinjctTypeIoaBusError64	= 1,
> +
> +	/* IoaBusError & IoaBusError64 */
> +	OpalEjtIoaLoadMemAddr		= 0,
> +	OpalEjtIoaLoadMemData		= 1,
> +	OpalEjtIoaLoadIoAddr		= 2,
> +	OpalEjtIoaLoadIoData		= 3,
> +	OpalEjtIoaLoadConfigAddr	= 4,
> +	OpalEjtIoaLoadConfigData	= 5,
> +	OpalEjtIoaStoreMemAddr		= 6,
> +	OpalEjtIoaStoreMemData		= 7,
> +	OpalEjtIoaStoreIoAddr		= 8,
> +	OpalEjtIoaStoreIoData		= 9,
> +	OpalEjtIoaStoreConfigAddr	= 10,
> +	OpalEjtIoaStoreConfigData	= 11,
> +	OpalEjtIoaDmaReadMemAddr	= 12,
> +	OpalEjtIoaDmaReadMemData	= 13,
> +	OpalEjtIoaDmaReadMemMaster	= 14,
> +	OpalEjtIoaDmaReadMemTarget	= 15,
> +	OpalEjtIoaDmaWriteMemAddr	= 16,
> +	OpalEjtIoaDmaWriteMemData	= 17,
> +	OpalEjtIoaDmaWriteMemMaster	= 18,
> +	OpalEjtIoaDmaWriteMemTarget	= 19,
> +};

I realise these come from the skiboot source, but they're just too ugly.

Please use kernel style naming, like most of the rest of the file, eg:

	OPAL_ERR_INJECT_IOA_BUS_ERR

Also this enum seems to contain two separate types, the first two values are
the "type", and the rest are "functions".

The only usage I see is:

	/* Sanity check on error type */
	if (type < OpalErrinjctTypeIoaBusError   ||
	    type > OpalErrinjctTypeIoaBusError64 ||
	    function < OpalEjtIoaLoadMemAddr     ||
	    function > OpalEjtIoaDmaWriteMemTarget) {
		pr_warn("%s: Invalid error type %d-%d\n",
			__func__, type, function);
		return -ERANGE;
	}

So we could also just do:

# define OPAL_ERR_INJECT_TYPE_MIN	0
# define OPAL_ERR_INJECT_TYPE_MAX	1

# define OPAL_ERR_INJECT_FUNC_MIN	0
# define OPAL_ERR_INJECT_FUNC_MAX	19

	if (type < OPAL_ERR_INJECT_TYPE_MIN ||
	    type > OPAL_ERR_INJECT_TYPE_MAX ||
	    function < OPAL_ERR_INJECT_FUNC_MIN ||
	    function > OPAL_ERR_INJECT_FUNC_MIN)
	{
		pr_warn("%s: Invalid error type %d-%d\n", __func__, type, function);
		return -ERANGE;
	}


cheers

Re: [1/4] powerpc/powernv: Sync header with firmware

[Gavin Shan]

On Thu, Sep 25, 2014 at 02:27:47PM +1000, Michael Ellerman wrote:
>On Tue, 2014-26-08 at 07:56:16 UTC, Gavin Shan wrote:
>> From: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>> 
>> The patch synchronizes firmware header file (opal.h) for PCI error
>> injection.
>> 
>> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
>> index 4593a93..9113653 100644
>> --- a/arch/powerpc/include/asm/opal.h
>> +++ b/arch/powerpc/include/asm/opal.h
>> @@ -200,6 +201,33 @@ enum OpalPciErrorSeverity {
>>  	OPAL_EEH_SEV_INF	= 5
>>  };
>>  
>> +enum OpalErrinjctType {
>> +	OpalErrinjctTypeIoaBusError	= 0,
>> +	OpalErrinjctTypeIoaBusError64	= 1,
>> +
>> +	/* IoaBusError & IoaBusError64 */
>> +	OpalEjtIoaLoadMemAddr		= 0,
>> +	OpalEjtIoaLoadMemData		= 1,
>> +	OpalEjtIoaLoadIoAddr		= 2,
>> +	OpalEjtIoaLoadIoData		= 3,
>> +	OpalEjtIoaLoadConfigAddr	= 4,
>> +	OpalEjtIoaLoadConfigData	= 5,
>> +	OpalEjtIoaStoreMemAddr		= 6,
>> +	OpalEjtIoaStoreMemData		= 7,
>> +	OpalEjtIoaStoreIoAddr		= 8,
>> +	OpalEjtIoaStoreIoData		= 9,
>> +	OpalEjtIoaStoreConfigAddr	= 10,
>> +	OpalEjtIoaStoreConfigData	= 11,
>> +	OpalEjtIoaDmaReadMemAddr	= 12,
>> +	OpalEjtIoaDmaReadMemData	= 13,
>> +	OpalEjtIoaDmaReadMemMaster	= 14,
>> +	OpalEjtIoaDmaReadMemTarget	= 15,
>> +	OpalEjtIoaDmaWriteMemAddr	= 16,
>> +	OpalEjtIoaDmaWriteMemData	= 17,
>> +	OpalEjtIoaDmaWriteMemMaster	= 18,
>> +	OpalEjtIoaDmaWriteMemTarget	= 19,
>> +};
>
>I realise these come from the skiboot source, but they're just too ugly.
>
>Please use kernel style naming, like most of the rest of the file, eg:
>
>	OPAL_ERR_INJECT_IOA_BUS_ERR
>
>Also this enum seems to contain two separate types, the first two values are
>the "type", and the rest are "functions".
>

Yes, two separate types: One is major error type, another one is
specific error type in that domain.

>The only usage I see is:
>
>	/* Sanity check on error type */
>	if (type < OpalErrinjctTypeIoaBusError   ||
>	    type > OpalErrinjctTypeIoaBusError64 ||
>	    function < OpalEjtIoaLoadMemAddr     ||
>	    function > OpalEjtIoaDmaWriteMemTarget) {
>		pr_warn("%s: Invalid error type %d-%d\n",
>			__func__, type, function);
>		return -ERANGE;
>	}
>
>So we could also just do:
>
># define OPAL_ERR_INJECT_TYPE_MIN	0
># define OPAL_ERR_INJECT_TYPE_MAX	1
>
># define OPAL_ERR_INJECT_FUNC_MIN	0
># define OPAL_ERR_INJECT_FUNC_MAX	19
>
>	if (type < OPAL_ERR_INJECT_TYPE_MIN ||
>	    type > OPAL_ERR_INJECT_TYPE_MAX ||
>	    function < OPAL_ERR_INJECT_FUNC_MIN ||
>	    function > OPAL_ERR_INJECT_FUNC_MIN)
>	{
>		pr_warn("%s: Invalid error type %d-%d\n", __func__, type, function);
>		return -ERANGE;
>	}
>

Ok. I'll take this and put it into next revision.

Thanks,
Gavin

Re: [1/4] powerpc/powernv: Sync header with firmware

[Gavin Shan]

On Thu, Sep 25, 2014 at 02:27:47PM +1000, Michael Ellerman wrote:
>On Tue, 2014-26-08 at 07:56:16 UTC, Gavin Shan wrote:
>> From: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>> 
>> The patch synchronizes firmware header file (opal.h) for PCI error
>> injection.
>> 
>> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
>> index 4593a93..9113653 100644
>> --- a/arch/powerpc/include/asm/opal.h
>> +++ b/arch/powerpc/include/asm/opal.h
>> @@ -200,6 +201,33 @@ enum OpalPciErrorSeverity {
>>  	OPAL_EEH_SEV_INF	= 5
>>  };
>>  
>> +enum OpalErrinjctType {
>> +	OpalErrinjctTypeIoaBusError	= 0,
>> +	OpalErrinjctTypeIoaBusError64	= 1,
>> +
>> +	/* IoaBusError & IoaBusError64 */
>> +	OpalEjtIoaLoadMemAddr		= 0,
>> +	OpalEjtIoaLoadMemData		= 1,
>> +	OpalEjtIoaLoadIoAddr		= 2,
>> +	OpalEjtIoaLoadIoData		= 3,
>> +	OpalEjtIoaLoadConfigAddr	= 4,
>> +	OpalEjtIoaLoadConfigData	= 5,
>> +	OpalEjtIoaStoreMemAddr		= 6,
>> +	OpalEjtIoaStoreMemData		= 7,
>> +	OpalEjtIoaStoreIoAddr		= 8,
>> +	OpalEjtIoaStoreIoData		= 9,
>> +	OpalEjtIoaStoreConfigAddr	= 10,
>> +	OpalEjtIoaStoreConfigData	= 11,
>> +	OpalEjtIoaDmaReadMemAddr	= 12,
>> +	OpalEjtIoaDmaReadMemData	= 13,
>> +	OpalEjtIoaDmaReadMemMaster	= 14,
>> +	OpalEjtIoaDmaReadMemTarget	= 15,
>> +	OpalEjtIoaDmaWriteMemAddr	= 16,
>> +	OpalEjtIoaDmaWriteMemData	= 17,
>> +	OpalEjtIoaDmaWriteMemMaster	= 18,
>> +	OpalEjtIoaDmaWriteMemTarget	= 19,
>> +};
>
>I realise these come from the skiboot source, but they're just too ugly.
>
>Please use kernel style naming, like most of the rest of the file, eg:
>
>	OPAL_ERR_INJECT_IOA_BUS_ERR
>

After thinking it for more, I decided to take this way because the specfic
function types will potentially be used when we're going to support error
injection from userland (QEMU). 

The new revision has folded all your comments (for other patches as well).
Michael, I'm not sure it's good way to send all my patches for 3.18, or I
just need send revised ones?

Thanks,
Gavin

>Also this enum seems to contain two separate types, the first two values are
>the "type", and the rest are "functions".
>
>The only usage I see is:
>
>	/* Sanity check on error type */
>	if (type < OpalErrinjctTypeIoaBusError   ||
>	    type > OpalErrinjctTypeIoaBusError64 ||
>	    function < OpalEjtIoaLoadMemAddr     ||
>	    function > OpalEjtIoaDmaWriteMemTarget) {
>		pr_warn("%s: Invalid error type %d-%d\n",
>			__func__, type, function);
>		return -ERANGE;
>	}
>
>So we could also just do:
>
># define OPAL_ERR_INJECT_TYPE_MIN	0
># define OPAL_ERR_INJECT_TYPE_MAX	1
>
># define OPAL_ERR_INJECT_FUNC_MIN	0
># define OPAL_ERR_INJECT_FUNC_MAX	19
>
>	if (type < OPAL_ERR_INJECT_TYPE_MIN ||
>	    type > OPAL_ERR_INJECT_TYPE_MAX ||
>	    function < OPAL_ERR_INJECT_FUNC_MIN ||
>	    function > OPAL_ERR_INJECT_FUNC_MIN)
>	{
>		pr_warn("%s: Invalid error type %d-%d\n", __func__, type, function);
>		return -ERANGE;
>	}
>

Re: [1/4] powerpc/powernv: Sync header with firmware

[Stewart Smith]

Michael Ellerman <mpe@ellerman.id.au> writes:
>> +	OpalEjtIoaDmaWriteMemTarget	= 19,
>> +};
>
> I realise these come from the skiboot source, but they're just too ugly.
>
> Please use kernel style naming, like most of the rest of the file, eg:
>
> 	OPAL_ERR_INJECT_IOA_BUS_ERR

You know what, I think I'd feel better if we changed skiboot source to
be like this too. Many of the other enums in skiboot are kernel style
and not camel. So, I'm going to go do that, then kernel and firmware can
match.

Re: [1/4] powerpc/powernv: Sync header with firmware

[Gavin Shan]

On Fri, Oct 03, 2014 at 03:30:31PM +1000, Stewart Smith wrote:
>Michael Ellerman <mpe@ellerman.id.au> writes:
>>> +	OpalEjtIoaDmaWriteMemTarget	= 19,
>>> +};
>>
>> I realise these come from the skiboot source, but they're just too ugly.
>>
>> Please use kernel style naming, like most of the rest of the file, eg:
>>
>> 	OPAL_ERR_INJECT_IOA_BUS_ERR
>
>You know what, I think I'd feel better if we changed skiboot source to
>be like this too. Many of the other enums in skiboot are kernel style
>and not camel. So, I'm going to go do that, then kernel and firmware can
>match.

Yes, for the PCI error types/functions, I sent one patch to make skiboot
looks same to kernel yesterday.

Thanks,
Gavin