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

[Ryan Grimm <grimm@linux.vnet.ibm.com>]

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
>