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

[Ian Munsie <imunsie@au1.ibm.com>]

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