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

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

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