[PATCH 1/3] CXL: Add image control to sysfs

[PATCH 1/3] CXL: Add image control to sysfs

[Ryan Grimm]

reset_image_select identifies whether a PERST will cause the image to be
flashed to the card.  And if so, which image.

Valid entries are: "none", "user" and "factory".

A value of "none" means PERST will not cause the image to be flashed.  A power
cycle to the pcie slot is required to load the image.

"user" loads the user provided image and "factory" loads the factory image upon
PERST.

sysfs updates the cxl struct in the driver then calls cxl_update_image_control
to write the vals in the VSEC.

Signed-off-by: Ryan Grimm <grimm@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/sysfs-class-cxl |  9 +++++++
 drivers/misc/cxl/cxl.h                    |  1 +
 drivers/misc/cxl/pci.c                    | 35 +++++++++++++++++++++++++++
 drivers/misc/cxl/sysfs.c                  | 39 +++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl
index 554405e..d23d1c7 100644
--- a/Documentation/ABI/testing/sysfs-class-cxl
+++ b/Documentation/ABI/testing/sysfs-class-cxl
@@ -127,3 +127,12 @@ Contact:        linuxppc-dev@lists.ozlabs.org
 Description:    read only
                 Will return "user" or "factory" depending on the image loaded
                 onto the card.
+
+What:           /sys/class/cxl/<card>/load_image_on_perst
+Date:           December 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    read/write
+                Value of "none" means PERST will not cause image to be loaded
+                to the card.  A power cycle is required to load the image.  
+                Value of "user" and "factory" means PERST will cause either the
+                user or factory image to be loaded.
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 0df0438..518c4c6 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -488,6 +488,7 @@ void cxl_release_one_irq(struct cxl *adapter, int hwirq);
 int cxl_alloc_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter, unsigned int num);
 void cxl_release_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter);
 int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq, unsigned int virq);
+int cxl_update_image_control(struct cxl *adapter);
 
 /* common == phyp + powernv */
 struct cxl_process_element_common {
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index f801c28..26cacc1 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -362,6 +362,41 @@ int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq,
 	return pnv_cxl_ioda_msi_setup(dev, hwirq, virq);
 }
 
+int cxl_update_image_control(struct cxl *adapter)
+{
+	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
+	int rc;
+	int vsec;
+	u8 image_state;
+
+	if (!(vsec = find_cxl_vsec(dev))) {
+		dev_err(&dev->dev, "ABORTING: CXL VSEC not found!\n");
+		return -ENODEV;
+	}
+
+	if ((rc = CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state))) {
+		dev_err(&dev->dev, "failed to read image state: %i\n", rc);
+		return rc;
+	}
+
+	if (adapter->perst_loads_image)
+		image_state |= CXL_VSEC_PERST_LOADS_IMAGE;
+	else
+		image_state &= ~CXL_VSEC_PERST_LOADS_IMAGE;
+
+	if (adapter->perst_select_user)
+		image_state |= CXL_VSEC_PERST_SELECT_USER;
+	else
+		image_state &= ~CXL_VSEC_PERST_SELECT_USER;
+
+	if ((rc = CXL_WRITE_VSEC_IMAGE_STATE(dev, vsec, image_state))) {
+		dev_err(&dev->dev, "failed to update image control: %i\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
 int cxl_alloc_one_irq(struct cxl *adapter)
 {
 	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
index 461bdbd..ed4ad46 100644
--- a/drivers/misc/cxl/sysfs.c
+++ b/drivers/misc/cxl/sysfs.c
@@ -56,11 +56,50 @@ static ssize_t image_loaded_show(struct device *device,
 	return scnprintf(buf, PAGE_SIZE, "factory\n");
 }
 
+static ssize_t load_image_on_perst_show(struct device *device,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+
+	if (!adapter->perst_loads_image)
+		return scnprintf(buf, PAGE_SIZE, "none\n");
+
+	if (adapter->perst_select_user)
+		return scnprintf(buf, PAGE_SIZE, "user\n");
+	return scnprintf(buf, PAGE_SIZE, "factory\n");
+}
+
+static ssize_t load_image_on_perst_store(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	int rc;
+
+	if (!strncmp(buf, "none", 4))
+		adapter->perst_loads_image = false;
+	else if (!strncmp(buf, "user", 4)) {
+		adapter->perst_select_user = true;
+		adapter->perst_loads_image = true;
+	} else if (!strncmp(buf, "factory", 7)) {
+		adapter->perst_select_user = false;
+		adapter->perst_loads_image = true;
+	} else
+		return -EINVAL;
+
+	if ((rc = cxl_update_image_control(adapter)))
+		return rc;
+
+	return count;
+}
+
 static struct device_attribute adapter_attrs[] = {
 	__ATTR_RO(caia_version),
 	__ATTR_RO(psl_revision),
 	__ATTR_RO(base_image),
 	__ATTR_RO(image_loaded),
+	__ATTR_RW(load_image_on_perst),
 };
 
 
-- 
1.9.1

[PATCH 2/3] CXL: Enable CAPP recovery

[Ryan Grimm]

Turning snoops on is the last step in CAPP recovery.  Sapphire is expected to
have reinitialized the PHB and done the previous recovery steps.

Add mode argument to opal call to do this.  Driver can turn snoops off although
it does not currently.

Signed-off-by: Ryan Grimm <grimm@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/opal.h           | 8 ++++++++
 arch/powerpc/include/asm/pnv-pci.h        | 2 +-
 arch/powerpc/platforms/powernv/pci-ioda.c | 6 +++---
 drivers/misc/cxl/pci.c                    | 8 +++++++-
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index eb95b67..2baf8a5 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -595,6 +595,14 @@ enum {
 	OPAL_PHB3_NUM_PEST_REGS = 256
 };
 
+/* CAPI modes for PHB */
+enum {
+	OPAL_PHB_CAPI_MODE_PCIE         = 0,
+	OPAL_PHB_CAPI_MODE_CAPI         = 1,
+	OPAL_PHB_CAPI_MODE_SNOOP_OFF    = 2,
+	OPAL_PHB_CAPI_MODE_SNOOP_ON     = 3,
+};
+
 struct OpalIoPhbErrorCommon {
 	__be32 version;
 	__be32 ioType;
diff --git a/arch/powerpc/include/asm/pnv-pci.h b/arch/powerpc/include/asm/pnv-pci.h
index f09a22f..3c00d64 100644
--- a/arch/powerpc/include/asm/pnv-pci.h
+++ b/arch/powerpc/include/asm/pnv-pci.h
@@ -13,7 +13,7 @@
 #include <linux/pci.h>
 #include <misc/cxl.h>
 
-int pnv_phb_to_cxl(struct pci_dev *dev);
+int pnv_phb_to_cxl_mode(struct pci_dev *dev, uint64_t mode);
 int pnv_cxl_ioda_msi_setup(struct pci_dev *dev, unsigned int hwirq,
 			   unsigned int virq);
 int pnv_cxl_alloc_hwirqs(struct pci_dev *dev, int num);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index fac88ed..5d52d6f 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1468,7 +1468,7 @@ struct device_node *pnv_pci_to_phb_node(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pnv_pci_to_phb_node);
 
-int pnv_phb_to_cxl(struct pci_dev *dev)
+int pnv_phb_to_cxl_mode(struct pci_dev *dev, uint64_t mode)
 {
 	struct pci_controller *hose = pci_bus_to_host(dev->bus);
 	struct pnv_phb *phb = hose->private_data;
@@ -1481,13 +1481,13 @@ int pnv_phb_to_cxl(struct pci_dev *dev)
 
 	pe_info(pe, "Switching PHB to CXL\n");
 
-	rc = opal_pci_set_phb_cxl_mode(phb->opal_id, 1, pe->pe_number);
+	rc = opal_pci_set_phb_cxl_mode(phb->opal_id, mode, pe->pe_number);
 	if (rc)
 		dev_err(&dev->dev, "opal_pci_set_phb_cxl_mode failed: %i\n", rc);
 
 	return rc;
 }
-EXPORT_SYMBOL(pnv_phb_to_cxl);
+EXPORT_SYMBOL(pnv_phb_to_cxl_mode);
 
 /* Find PHB for cxl dev and allocate MSI hwirqs?
  * Returns the absolute hardware IRQ number
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 26cacc1..0f546f6 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -924,9 +924,15 @@ static struct cxl *cxl_init_adapter(struct pci_dev *dev)
 	if ((rc = init_implementation_adapter_regs(adapter, dev)))
 		goto err3;
 
-	if ((rc = pnv_phb_to_cxl(dev)))
+	if ((rc = pnv_phb_to_cxl_mode(dev, OPAL_PHB_CAPI_MODE_CAPI)))
 		goto err3;
 
+	/* If recovery happened, the last step is to turn on snooping.
+	 * In the non-recovery case this has no effect */
+	if ((rc = pnv_phb_to_cxl_mode(dev, OPAL_PHB_CAPI_MODE_SNOOP_ON))) {
+		goto err3;
+	}
+
 	if ((rc = cxl_register_psl_err_irq(adapter)))
 		goto err3;
 
-- 
1.9.1

Re: [PATCH 2/3] CXL: Enable CAPP recovery

[Ian Munsie]

Acked-by: Ian Munsie <imunsie@au1.ibm.com>

[PATCH 3/3] CXL: Add ability to reset the card

[Ryan Grimm]

Adds reset to sysfs which will PERST the card.  If load_image_on_perst is set
to "user" or "factory", the PERST will cause that image to be loaded.

load_image_on_perst is set to "user" for production.

"none" could be used for debugging.  The PSL trace arrays are preserved which
then can be read through debugfs.

PERST also triggers CAPP recovery.  An HMI comes in, which is handled by EEH.
EEH unbinds the driver, calls into Sapphire to reinitialize the PHB, then
rebinds the driver.

Signed-off-by: Ryan Grimm <grimm@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/sysfs-class-cxl | 14 ++++++++--
 drivers/misc/cxl/cxl.h                    |  1 +
 drivers/misc/cxl/pci.c                    | 44 +++++++++++++++++++++++++++++--
 drivers/misc/cxl/sysfs.c                  | 13 +++++++++
 4 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl
index d23d1c7..043bfbf 100644
--- a/Documentation/ABI/testing/sysfs-class-cxl
+++ b/Documentation/ABI/testing/sysfs-class-cxl
@@ -133,6 +133,16 @@ Date:           December 2014
 Contact:        linuxppc-dev@lists.ozlabs.org
 Description:    read/write
                 Value of "none" means PERST will not cause image to be loaded
-                to the card.  A power cycle is required to load the image.  
+                to the card.  A power cycle is required to load the image.
+                "none" is useful for debugging so the contents of the trace 
+                arrays are preserved.
                 Value of "user" and "factory" means PERST will cause either the
-                user or factory image to be loaded.
+                user or factory image to be loaded.  "user" is default and 
+                should be used in production.  
+
+What:           /sys/class/cxl/<card>/reset
+Date:           October 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    write only
+                Writing 1 will issue a PERST to card which may cause the card
+                to reload the FPGA depending on load_image_on_perst.
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 518c4c6..6a6a487 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -489,6 +489,7 @@ int cxl_alloc_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter, unsig
 void cxl_release_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter);
 int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq, unsigned int virq);
 int cxl_update_image_control(struct cxl *adapter);
+int cxl_reset(struct cxl *adapter);
 
 /* common == phyp + powernv */
 struct cxl_process_element_common {
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 0f546f6..5137ee5 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -21,6 +21,7 @@
 #include <asm/msi_bitmap.h>
 #include <asm/pci-bridge.h> /* for struct pci_controller */
 #include <asm/pnv-pci.h>
+#include <asm/io.h>
 
 #include "cxl.h"
 
@@ -742,6 +743,42 @@ static void cxl_remove_afu(struct cxl_afu *afu)
 	device_unregister(&afu->dev);
 }
 
+int cxl_reset(struct cxl *adapter)
+{
+	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
+	int rc;
+	int i;
+	u32 val;
+
+	dev_info(&dev->dev, "CXL reset\n");
+
+	for (i = 0; i < adapter->slices; i++)
+		cxl_remove_afu(adapter->afu[i]);
+
+	/* pcie_warm_reset requests a fundamental pci reset which includes a
+	 * PERST assert/deassert.  PERST triggers a loading of the image
+	 * if "user" or "factory" is selected in sysfs */
+	if ((rc = pci_set_pcie_reset_state(dev, pcie_warm_reset))) {
+		dev_err(&dev->dev, "cxl: pcie_warm_reset failed\n");
+		return rc;
+	}
+
+	/* the PERST done above fences the PHB.  So, reset depends on EEH
+	 * to unbind the driver, tell Sapphire to reinit the PHB, and rebind
+	 * the driver.  Do an mmio read explictly to ensure EEH notices the
+	 * fenced PHB.  Retry for a few seconds before giving up. */
+	i = 0;
+	while (((val = mmio_read32be(adapter->p1_mmio)) != 0xffffffff) &&
+		(i < 5)) {
+		msleep(500);
+		i++;
+	}
+
+	if (val != 0xffffffff)
+		dev_err(&dev->dev, "cxl: PERST failed to trigger EEH\n");
+
+	return rc;
+}
 
 static int cxl_map_adapter_regs(struct cxl *adapter, struct pci_dev *dev)
 {
@@ -806,8 +843,8 @@ static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev)
 	CXL_READ_VSEC_BASE_IMAGE(dev, vsec, &adapter->base_image);
 	CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state);
 	adapter->user_image_loaded = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
-	adapter->perst_loads_image = !!(image_state & CXL_VSEC_PERST_LOADS_IMAGE);
-	adapter->perst_select_user = !!(image_state & CXL_VSEC_PERST_SELECT_USER);
+	adapter->perst_loads_image = true;
+	adapter->perst_select_user = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
 
 	CXL_READ_VSEC_NAFUS(dev, vsec, &adapter->slices);
 	CXL_READ_VSEC_AFU_DESC_OFF(dev, vsec, &afu_desc_off);
@@ -915,6 +952,9 @@ static struct cxl *cxl_init_adapter(struct pci_dev *dev)
 	if ((rc = cxl_vsec_looks_ok(adapter, dev)))
 		goto err2;
 
+	if ((rc = cxl_update_image_control(adapter)))
+		goto err2;
+
 	if ((rc = cxl_map_adapter_regs(adapter, dev)))
 		goto err2;
 
diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
index ed4ad46..c4febcf 100644
--- a/drivers/misc/cxl/sysfs.c
+++ b/drivers/misc/cxl/sysfs.c
@@ -56,6 +56,18 @@ static ssize_t image_loaded_show(struct device *device,
 	return scnprintf(buf, PAGE_SIZE, "factory\n");
 }
 
+static ssize_t reset_adapter_store(struct device *device,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	int rc;
+
+	if ((rc = cxl_reset(adapter)))
+		return rc;
+	return count;
+}
+
 static ssize_t load_image_on_perst_show(struct device *device,
 				 struct device_attribute *attr,
 				 char *buf)
@@ -100,6 +112,7 @@ static struct device_attribute adapter_attrs[] = {
 	__ATTR_RO(base_image),
 	__ATTR_RO(image_loaded),
 	__ATTR_RW(load_image_on_perst),
+	__ATTR(reset, S_IWUSR, NULL, reset_adapter_store),
 };
 
 
-- 
1.9.1

Re: [PATCH 3/3] CXL: Add ability to reset the card

[Ian Munsie]

Hi Ryan,

Excerpts from Ryan Grimm's message of 2015-01-16 09:27:17 +1100:
> Adds reset to sysfs which will PERST the card.  If load_image_on_perst is set
> to "user" or "factory", the PERST will cause that image to be loaded.
> 
> load_image_on_perst is set to "user" for production.

While it generally will be "user" for production, some cards may only
ship with only a factory image, which is then going to be the image used
for production.  It might be better to say that it will default to
whichever image the card has already loaded.


>                  Value of "none" means PERST will not cause image to be loaded
> -                to the card.  A power cycle is required to load the image.  
> +                to the card.  A power cycle is required to load the image.
> +                "none" is useful for debugging so the contents of the trace 
> +                arrays are preserved.
>                  Value of "user" and "factory" means PERST will cause either the
> -                user or factory image to be loaded.
> +                user or factory image to be loaded.  "user" is default and 
> +                should be used in production.  

git am spotted some whitespace at the end of a couple of lines here



> +    /* pcie_warm_reset requests a fundamental pci reset which includes a
> +     * PERST assert/deassert.  PERST triggers a loading of the image
> +     * if "user" or "factory" is selected in sysfs */
> +    if ((rc = pci_set_pcie_reset_state(dev, pcie_warm_reset))) {
> +        dev_err(&dev->dev, "cxl: pcie_warm_reset failed\n");
> +        return rc;
> +    }
> +
> +    /* the PERST done above fences the PHB.  So, reset depends on EEH
> +     * to unbind the driver, tell Sapphire to reinit the PHB, and rebind
> +     * the driver.  Do an mmio read explictly to ensure EEH notices the
> +     * fenced PHB.  Retry for a few seconds before giving up. */

Great, thanks for adding the explanations here :)


> @@ -806,8 +843,8 @@ static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev)
>      CXL_READ_VSEC_BASE_IMAGE(dev, vsec, &adapter->base_image);
>      CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state);
>      adapter->user_image_loaded = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
> -    adapter->perst_loads_image = !!(image_state & CXL_VSEC_PERST_LOADS_IMAGE);
> -    adapter->perst_select_user = !!(image_state & CXL_VSEC_PERST_SELECT_USER);
> +    adapter->perst_loads_image = true;
> +    adapter->perst_select_user = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
<snip>
> +    if ((rc = cxl_update_image_control(adapter)))
> +        goto err2;

Please move these two hunks into a separate patch (can be first in the
series) along with the cxl_update_image_control() function from patch 1
- I'd like to get this backported to stable, which will be simpler if it
is in it's own patch.


> +static ssize_t reset_adapter_store(struct device *device,
> +                   struct device_attribute *attr,
> +                   const char *buf, size_t count)
> +{
> +    struct cxl *adapter = to_cxl_adapter(device);
> +    int rc;
> +
> +    if ((rc = cxl_reset(adapter)))
> +        return rc;
> +    return count;
> +}

Please add a check here so the reset only occurs when a "1" is written
to this file to match the documentation.


Cheers,
-Ian

Re: [PATCH 3/3] CXL: Add ability to reset the card

[Ryan Grimm]

On 01/15/2015 11:27 PM, Ian Munsie wrote:
> Hi Ryan,
>
> Excerpts from Ryan Grimm's message of 2015-01-16 09:27:17 +1100:
>> Adds reset to sysfs which will PERST the card.  If load_image_on_perst is set
>> to "user" or "factory", the PERST will cause that image to be loaded.
>>
>> load_image_on_perst is set to "user" for production.
>
> While it generally will be "user" for production, some cards may only
> ship with only a factory image, which is then going to be the image used
> for production.  It might be better to say that it will default to
> whichever image the card has already loaded.

K, yeah.

>
>
>>                   Value of "none" means PERST will not cause image to be loaded
>> -                to the card.  A power cycle is required to load the image.
>> +                to the card.  A power cycle is required to load the image.
>> +                "none" is useful for debugging so the contents of the trace
>> +                arrays are preserved.
>>                   Value of "user" and "factory" means PERST will cause either the
>> -                user or factory image to be loaded.
>> +                user or factory image to be loaded.  "user" is default and
>> +                should be used in production.
>
> git am spotted some whitespace at the end of a couple of lines here
>

Fixed...darn whitespace.

>
>
>> +    /* pcie_warm_reset requests a fundamental pci reset which includes a
>> +     * PERST assert/deassert.  PERST triggers a loading of the image
>> +     * if "user" or "factory" is selected in sysfs */
>> +    if ((rc = pci_set_pcie_reset_state(dev, pcie_warm_reset))) {
>> +        dev_err(&dev->dev, "cxl: pcie_warm_reset failed\n");
>> +        return rc;
>> +    }
>> +
>> +    /* the PERST done above fences the PHB.  So, reset depends on EEH
>> +     * to unbind the driver, tell Sapphire to reinit the PHB, and rebind
>> +     * the driver.  Do an mmio read explictly to ensure EEH notices the
>> +     * fenced PHB.  Retry for a few seconds before giving up. */
>
> Great, thanks for adding the explanations here :)
>
>
>> @@ -806,8 +843,8 @@ static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev)
>>       CXL_READ_VSEC_BASE_IMAGE(dev, vsec, &adapter->base_image);
>>       CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state);
>>       adapter->user_image_loaded = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
>> -    adapter->perst_loads_image = !!(image_state & CXL_VSEC_PERST_LOADS_IMAGE);
>> -    adapter->perst_select_user = !!(image_state & CXL_VSEC_PERST_SELECT_USER);
>> +    adapter->perst_loads_image = true;
>> +    adapter->perst_select_user = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
> <snip>
>> +    if ((rc = cxl_update_image_control(adapter)))
>> +        goto err2;
>
> Please move these two hunks into a separate patch (can be first in the
> series) along with the cxl_update_image_control() function from patch 1
> - I'd like to get this backported to stable, which will be simpler if it
> is in it's own patch.
>
>

K, I completely agree.

>> +static ssize_t reset_adapter_store(struct device *device,
>> +                   struct device_attribute *attr,
>> +                   const char *buf, size_t count)
>> +{
>> +    struct cxl *adapter = to_cxl_adapter(device);
>> +    int rc;
>> +
>> +    if ((rc = cxl_reset(adapter)))
>> +        return rc;
>> +    return count;
>> +}
>
> Please add a check here so the reset only occurs when a "1" is written
> to this file to match the documentation.
>

Yeah, it's probably good to do that :)

-Ryan

>
> Cheers,
> -Ian
>

Re: [PATCH 1/3] CXL: Add image control to sysfs

[Ian Munsie]

Hi Ryan,

Excerpts from Ryan Grimm's message of 2015-01-16 09:27:15 +1100:
> reset_image_select identifies whether a PERST will cause the image to be

s/reset_image_select/load_image_on_perst/

> +What:           /sys/class/cxl/<card>/load_image_on_perst
> +Date:           December 2014
> +Contact:        linuxppc-dev@lists.ozlabs.org
> +Description:    read/write
> +                Value of "none" means PERST will not cause image to be loaded
> +                to the card.  A power cycle is required to load the image.  

git am spotted some whitespace at the end of this line.


> +int cxl_update_image_control(struct cxl *adapter)

Would you be able to pull this function out into a separate patch (can
be first in the series so everything still compiles after each patch)
along with the change to the image_state defaults from patch 3? It's a
fix that I think would be good to be backported to stable, which will be
simpler if it's in a separate patch.



Otherwise, looks good :)

Acked-by: Ian Munsie <imunsie@au1.ibm.com>

Re: [PATCH 1/3] CXL: Add image control to sysfs

[Ryan Grimm]

On 01/15/2015 11:15 PM, Ian Munsie wrote:
> Hi Ryan,
>
> Excerpts from Ryan Grimm's message of 2015-01-16 09:27:15 +1100:
>> reset_image_select identifies whether a PERST will cause the image to be
>
> s/reset_image_select/load_image_on_perst/
>

Ah, thanks, fixed.

>> +What:           /sys/class/cxl/<card>/load_image_on_perst
>> +Date:           December 2014
>> +Contact:        linuxppc-dev@lists.ozlabs.org
>> +Description:    read/write
>> +                Value of "none" means PERST will not cause image to be loaded
>> +                to the card.  A power cycle is required to load the image.
>
> git am spotted some whitespace at the end of this line.
>
>

Thanks, fixed again.

>> +int cxl_update_image_control(struct cxl *adapter)
>
> Would you be able to pull this function out into a separate patch (can
> be first in the series so everything still compiles after each patch)
> along with the change to the image_state defaults from patch 3? It's a
> fix that I think would be good to be backported to stable, which will be
> simpler if it's in a separate patch.
>

Good idea.  Yeah, pulling in the change from patch 3 makes sense.

Sure, done.

>
>
> Otherwise, looks good :)

Great, thanks for reviewing!~

-Ryan


>
> Acked-by: Ian Munsie <imunsie@au1.ibm.com>
>

[PATCH 1/3] CXL: Add image control to sysfs

[Ryan Grimm]

Add reset_loads_image and reset_image_select to sysfs.

reset_image_select identifies which image will be loaded to the card on the
next PERST.  Valid entries are: "user" and "factory".

reset_loads_image defines functionality on a PERST.  Value of 0 means PERST
will not cause image load.  A power cycle is required to load the image.  Value
of 1 means PERST will cause image load.

sysfs updates the cxl struct in the driver then calls cxl_update_image_control
to write the vals in the VSEC.

Signed-off-by: Ryan Grimm <grimm@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/sysfs-class-cxl | 15 ++++++++
 drivers/misc/cxl/cxl.h                    |  1 +
 drivers/misc/cxl/pci.c                    | 35 ++++++++++++++++++
 drivers/misc/cxl/sysfs.c                  | 60 +++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl
index 554405e..134cfaf 100644
--- a/Documentation/ABI/testing/sysfs-class-cxl
+++ b/Documentation/ABI/testing/sysfs-class-cxl
@@ -127,3 +127,18 @@ Contact:        linuxppc-dev@lists.ozlabs.org
 Description:    read only
                 Will return "user" or "factory" depending on the image loaded
                 onto the card.
+
+What:           /sys/class/cxl/<card>/reset_image_select
+Date:           December 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    read/write
+                Identifies which image will be loaded to the card on the next
+                PERST.  Valid entries are: "user" and "factory".
+
+What:           /sys/class/cxl/<card>/reset_loads_image
+Date:           December 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    read/write
+                Value of 0 means PERST will not cause image load.  A power
+                cycle is required to load the image.  Value of 1 means PERST
+                will cause image load.
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 0df0438..518c4c6 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -488,6 +488,7 @@ void cxl_release_one_irq(struct cxl *adapter, int hwirq);
 int cxl_alloc_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter, unsigned int num);
 void cxl_release_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter);
 int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq, unsigned int virq);
+int cxl_update_image_control(struct cxl *adapter);
 
 /* common == phyp + powernv */
 struct cxl_process_element_common {
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index f801c28..26cacc1 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -362,6 +362,41 @@ int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq,
 	return pnv_cxl_ioda_msi_setup(dev, hwirq, virq);
 }
 
+int cxl_update_image_control(struct cxl *adapter)
+{
+	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
+	int rc;
+	int vsec;
+	u8 image_state;
+
+	if (!(vsec = find_cxl_vsec(dev))) {
+		dev_err(&dev->dev, "ABORTING: CXL VSEC not found!\n");
+		return -ENODEV;
+	}
+
+	if ((rc = CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state))) {
+		dev_err(&dev->dev, "failed to read image state: %i\n", rc);
+		return rc;
+	}
+
+	if (adapter->perst_loads_image)
+		image_state |= CXL_VSEC_PERST_LOADS_IMAGE;
+	else
+		image_state &= ~CXL_VSEC_PERST_LOADS_IMAGE;
+
+	if (adapter->perst_select_user)
+		image_state |= CXL_VSEC_PERST_SELECT_USER;
+	else
+		image_state &= ~CXL_VSEC_PERST_SELECT_USER;
+
+	if ((rc = CXL_WRITE_VSEC_IMAGE_STATE(dev, vsec, image_state))) {
+		dev_err(&dev->dev, "failed to update image control: %i\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
 int cxl_alloc_one_irq(struct cxl *adapter)
 {
 	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
index 461bdbd..06f554b 100644
--- a/drivers/misc/cxl/sysfs.c
+++ b/drivers/misc/cxl/sysfs.c
@@ -56,11 +56,71 @@ static ssize_t image_loaded_show(struct device *device,
 	return scnprintf(buf, PAGE_SIZE, "factory\n");
 }
 
+static ssize_t reset_loads_image_show(struct device *device,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	return sprintf(buf, "%d\n", adapter->perst_loads_image);
+}
+
+static ssize_t reset_loads_image_store(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	unsigned long val;
+	int rc;
+
+        if (kstrtoul(buf, 0, &val) < 0)
+                return -EINVAL;
+
+        adapter->perst_loads_image = !!val;
+	if ((rc = cxl_update_image_control(adapter)))
+		return rc;
+
+	return count;
+}
+
+static ssize_t reset_image_select_show(struct device *device,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+
+	if (adapter->perst_select_user)
+		return scnprintf(buf, PAGE_SIZE, "user\n");
+
+	return scnprintf(buf, PAGE_SIZE, "factory\n");
+}
+
+static ssize_t reset_image_select_store(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	int rc;
+
+	if (!strncmp(buf, "user", 4))
+		adapter->perst_select_user = true;
+	else if (!strncmp(buf, "factory", 7))
+		adapter->perst_select_user = false;
+	else
+                return -EINVAL;
+
+	if ((rc = cxl_update_image_control(adapter)))
+		return rc;
+
+	return count;
+}
+
 static struct device_attribute adapter_attrs[] = {
 	__ATTR_RO(caia_version),
 	__ATTR_RO(psl_revision),
 	__ATTR_RO(base_image),
 	__ATTR_RO(image_loaded),
+	__ATTR_RW(reset_loads_image),
+	__ATTR_RW(reset_image_select),
 };
 
 
-- 
1.9.1

Re: [PATCH 1/3] CXL: Add image control to sysfs

[Ian Munsie]

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100:
> Add reset_loads_image and reset_image_select to sysfs.
> 
> reset_image_select identifies which image will be loaded to the card on the
> next PERST.  Valid entries are: "user" and "factory".
> 
> reset_loads_image defines functionality on a PERST.  Value of 0 means PERST
> will not cause image load.  A power cycle is required to load the image.  Value
> of 1 means PERST will cause image load.
> 
> sysfs updates the cxl struct in the driver then calls cxl_update_image_control
> to write the vals in the VSEC.

Let's combine both of these into a single sysfs file, with "none",
"user" and "factory" options and have the show & read functions handle
mapping those three options to the two bits in the register.

Of the two names I'd probably go with reset_image_select.

> +What:           /sys/class/cxl/<card>/reset_loads_image
> +Date:           December 2014
> +Contact:        linuxppc-dev@lists.ozlabs.org
> +Description:    read/write
> +                Value of 0 means PERST will not cause image load.  A power
> +                cycle is required to load the image.  Value of 1 means PERST
> +                will cause image load.

It also seems to be that having this disabled also means that PERST
doesn't fully reset the card. Might want to clarify that somewhat and
recommend it only be disabled for debugging purposes (e.g. to retain
the contents of the PSL trace arrays across a reset), and to always
enable it for production.

At the moment we don't set it at boot - we just go with whatever the
card is already set to do. I'm thinking it might be a good idea to
always set this bit on boot so the only time it's disabled is if a user
has explicitly gone and disabled it.

> +static ssize_t reset_loads_image_show(struct device *device,
> +                 struct device_attribute *attr,
> +                 char *buf)
> +{
> +    struct cxl *adapter = to_cxl_adapter(device);
> +    return sprintf(buf, "%d\n", adapter->perst_loads_image);

We've used scnprintf for the other sysfs reads in this file, why sprintf
here?

> +static ssize_t reset_loads_image_store(struct device *device,
> +                 struct device_attribute *attr,
> +                 const char *buf, size_t count)
> +{
> +    struct cxl *adapter = to_cxl_adapter(device);
> +    unsigned long val;
> +    int rc;
> +
> +        if (kstrtoul(buf, 0, &val) < 0)
> +                return -EINVAL;
> +
> +        adapter->perst_loads_image = !!val;
> +    if ((rc = cxl_update_image_control(adapter)))
> +        return rc;

Seems to be some indentation mismatches here - some lines are using
spaces other are using tabs. Please use tabs for everything.

> +static ssize_t reset_image_select_store(struct device *device,
> +                 struct device_attribute *attr,
> +                 const char *buf, size_t count)
> +{
> +    struct cxl *adapter = to_cxl_adapter(device);
> +    int rc;
> +
> +    if (!strncmp(buf, "user", 4))
> +        adapter->perst_select_user = true;
> +    else if (!strncmp(buf, "factory", 7))
> +        adapter->perst_select_user = false;
> +    else
> +                return -EINVAL;

More indentation mismatches here.


Cheers,
-Ian

Re: [PATCH 1/3] CXL: Add image control to sysfs

[Ian Munsie]

Excerpts from Ian Munsie's message of 2015-01-15 15:41:24 +1100:
> At the moment we don't set it at boot - we just go with whatever the
> card is already set to do. I'm thinking it might be a good idea to
> always set this bit on boot so the only time it's disabled is if a user
> has explicitly gone and disabled it.

While I think of it - if we change this on boot we should also change
reset_image_select to match the currently loaded image. e.g. if
reset_loads_image has defaulted to off and reset_image_select has
defaulted to factory, but the user image has been loaded - that way we
avoid unexpectedly switching to factory if the card gets reset.

Cheers,
-Ian

Re: [PATCH 1/3] CXL: Add image control to sysfs

[Ian Munsie]

> While I think of it - if we change this on boot we should also change
> reset_image_select to match the currently loaded image. e.g. if
> reset_loads_image has defaulted to off and reset_image_select has
> defaulted to factory, but the user image has been loaded - that way we
> avoid unexpectedly switching to factory if the card gets reset.

Nevermind - I see you have done exactly this in patch 3 :-)

-Ian

Re: [PATCH 1/3] CXL: Add image control to sysfs

[Michael Ellerman]

On Thu, 2015-01-15 at 15:41 +1100, Ian Munsie wrote:
> Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100:
> > Add reset_loads_image and reset_image_select to sysfs.
> > 
> > reset_image_select identifies which image will be loaded to the card on the
> > next PERST.  Valid entries are: "user" and "factory".
> > 
> > reset_loads_image defines functionality on a PERST.  Value of 0 means PERST
> > will not cause image load.  A power cycle is required to load the image.  Value
> > of 1 means PERST will cause image load.
> > 
> > sysfs updates the cxl struct in the driver then calls cxl_update_image_control
> > to write the vals in the VSEC.
> 
> Let's combine both of these into a single sysfs file, with "none",
> "user" and "factory" options and have the show & read functions handle
> mapping those three options to the two bits in the register.
> 
> Of the two names I'd probably go with reset_image_select.

Three words, all can be verbs, two can be nouns, it's not too clear.

Maybe "load_image_on_perst" ?

cheers

Re: [PATCH 1/3] CXL: Add image control to sysfs

[Ian Munsie]

Excerpts from Michael Ellerman's message of 2015-01-15 16:07:17 +1100:
> > Of the two names I'd probably go with reset_image_select.
> 
> Three words, all can be verbs, two can be nouns, it's not too clear.
> 
> Maybe "load_image_on_perst" ?

Works for me :)

Cheers,
-Ian

Re: [PATCH 1/3] CXL: Add image control to sysfs

[Ryan Grimm]

Ian,

Thanks for reviewing!

On 01/14/2015 11:41 PM, Ian Munsie wrote:
> Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100:
>> Add reset_loads_image and reset_image_select to sysfs.
>>
>> reset_image_select identifies which image will be loaded to the card on the
>> next PERST.  Valid entries are: "user" and "factory".
>>
>> reset_loads_image defines functionality on a PERST.  Value of 0 means PERST
>> will not cause image load.  A power cycle is required to load the image.  Value
>> of 1 means PERST will cause image load.
>>
>> sysfs updates the cxl struct in the driver then calls cxl_update_image_control
>> to write the vals in the VSEC.
>
> Let's combine both of these into a single sysfs file, with "none",
> "user" and "factory" options and have the show & read functions handle
> mapping those three options to the two bits in the register.
>

I like that idea!

> Of the two names I'd probably go with reset_image_select.
>
>> +What:           /sys/class/cxl/<card>/reset_loads_image
>> +Date:           December 2014
>> +Contact:        linuxppc-dev@lists.ozlabs.org
>> +Description:    read/write
>> +                Value of 0 means PERST will not cause image load.  A power
>> +                cycle is required to load the image.  Value of 1 means PERST
>> +                will cause image load.
>
> It also seems to be that having this disabled also means that PERST
> doesn't fully reset the card. Might want to clarify that somewhat and
> recommend it only be disabled for debugging purposes (e.g. to retain
> the contents of the PSL trace arrays across a reset), and to always
> enable it for production.
>

Yeah, that is the main reason you'd disable it.  Will add that info to 
the doc.

> At the moment we don't set it at boot - we just go with whatever the
> card is already set to do. I'm thinking it might be a good idea to
> always set this bit on boot so the only time it's disabled is if a user
> has explicitly gone and disabled it.
>
>> +static ssize_t reset_loads_image_show(struct device *device,
>> +                 struct device_attribute *attr,
>> +                 char *buf)
>> +{
>> +    struct cxl *adapter = to_cxl_adapter(device);
>> +    return sprintf(buf, "%d\n", adapter->perst_loads_image);
>
> We've used scnprintf for the other sysfs reads in this file, why sprintf
> here?
>

No reason, scnprintf is safer so fixed.

>> +static ssize_t reset_loads_image_store(struct device *device,
>> +                 struct device_attribute *attr,
>> +                 const char *buf, size_t count)
>> +{
>> +    struct cxl *adapter = to_cxl_adapter(device);
>> +    unsigned long val;
>> +    int rc;
>> +
>> +        if (kstrtoul(buf, 0, &val) < 0)
>> +                return -EINVAL;
>> +
>> +        adapter->perst_loads_image = !!val;
>> +    if ((rc = cxl_update_image_control(adapter)))
>> +        return rc;
>
> Seems to be some indentation mismatches here - some lines are using
> spaces other are using tabs. Please use tabs for everything.
>

Fixed.

>> +static ssize_t reset_image_select_store(struct device *device,
>> +                 struct device_attribute *attr,
>> +                 const char *buf, size_t count)
>> +{
>> +    struct cxl *adapter = to_cxl_adapter(device);
>> +    int rc;
>> +
>> +    if (!strncmp(buf, "user", 4))
>> +        adapter->perst_select_user = true;
>> +    else if (!strncmp(buf, "factory", 7))
>> +        adapter->perst_select_user = false;
>> +    else
>> +                return -EINVAL;
>
> More indentation mismatches here.
>
>

Fixed.

-Ryan

> Cheers,
> -Ian
>