Re: [PATCH v2 16/19] tools/testing/cxl: add mechanism to lock mem device for testing

[Dave Jiang <dave.jiang@intel.com>]

On 11/7/2022 7:56 AM, Jonathan Cameron wrote:
> On Wed, 21 Sep 2022 08:33:03 -0700
> Dave Jiang <dave.jiang@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.jiang@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.

ok

> 
>> +}
>> +
>> +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.

ok

> 
> 
>> +		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--)
>>
>>
> 
>