On Wed, 21 Sep 2022 08:33:03 -0700
Dave Jiang <dave.ji...@intel.com> wrote:
> The mock cxl mem devs needs a way to go into "locked" status to simulate
> when the platform is rebooted. Add a sysfs mechanism so the device security
> state is set to "locked" and the frozen state bits are cleared.
>
> Signed-off-by: Dave Jiang <dave.ji...@intel.com>
Hi Dave
A few minor comments below.
> ---
> tools/testing/cxl/test/cxl.c | 52
> ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 6dd286a52839..7f76f494a0d4 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -628,6 +628,45 @@ static void mock_companion(struct acpi_device *adev,
> struct device *dev)
> #define SZ_512G (SZ_64G * 8)
> #endif
>
> +static ssize_t security_lock_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct cxl_mock_mem_pdata *mdata = dev_get_platdata(dev);
> +
> + return sysfs_emit(buf, "%s\n", mdata->security_state &
> CXL_PMEM_SEC_STATE_LOCKED ?
> + "locked" : "unlocked");
It's called lock. So 1 or 0 seems sufficient to me rather than needing strings.
Particularly when you use an int to lock it.
> +}
> +
> +static ssize_t security_lock_store(struct device *dev, struct
> device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct cxl_mock_mem_pdata *mdata = dev_get_platdata(dev);
> + u32 mask = CXL_PMEM_SEC_STATE_FROZEN | CXL_PMEM_SEC_STATE_USER_PLIMIT |
> + CXL_PMEM_SEC_STATE_MASTER_PLIMIT;
> + int val;
> +
> + if (kstrtoint(buf, 0, &val) < 0)
> + return -EINVAL;
> +
> + if (val == 1) {
> + if (!(mdata->security_state & CXL_PMEM_SEC_STATE_USER_PASS_SET))
> + return -ENXIO;
> + mdata->security_state |= CXL_PMEM_SEC_STATE_LOCKED;
> + mdata->security_state &= ~mask;
> + } else {
> + return -EINVAL;
> + }
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(security_lock);
> +
> +static struct attribute *cxl_mock_mem_attrs[] = {
> + &dev_attr_security_lock.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(cxl_mock_mem);
> +
> static __init int cxl_test_init(void)
> {
> struct cxl_mock_mem_pdata *mem_pdata;
> @@ -757,6 +796,11 @@ static __init int cxl_test_init(void)
> platform_device_put(pdev);
> goto err_mem;
> }
> +
> + rc = device_add_groups(&pdev->dev, cxl_mock_mem_groups);
Can we just set pdev->dev.groups? and avoid dynamic part of this or need to
remove them manually? I can't immediately find an example of this for
a platform_device but it's done for lots of other types.
> + if (rc)
> + goto err_mem;
> +
> cxl_mem[i] = pdev;
> }
>
> @@ -811,8 +855,12 @@ static __exit void cxl_test_exit(void)
> int i;
>
> platform_device_unregister(cxl_acpi);
> - for (i = ARRAY_SIZE(cxl_mem) - 1; i >= 0; i--)
> - platform_device_unregister(cxl_mem[i]);
> + for (i = ARRAY_SIZE(cxl_mem) - 1; i >= 0; i--) {
> + struct platform_device *pdev = cxl_mem[i];
> +
> + device_remove_groups(&pdev->dev, cxl_mock_mem_groups);
> + platform_device_unregister(pdev);
> + }
> for (i = ARRAY_SIZE(cxl_switch_dport) - 1; i >= 0; i--)
> platform_device_unregister(cxl_switch_dport[i]);
> for (i = ARRAY_SIZE(cxl_switch_uport) - 1; i >= 0; i--)
>
>
