Re: [PATCH v3 1/3] cxl: Change 'struct cxl_memdev_state' *_perf_list to single 'struct cxl_dpa_perf'

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

On 2/2/24 08:40, Dave Jiang wrote:
> 
> 
> On 2/1/24 22:28, Dan Williams wrote:
>> Wonjae Lee wrote:
>>> On Thu, Feb 01, 2024 at 02:47:29PM -0700, Dave Jiang wrote:
>>>> In order to address the issue with being able to expose qos_class sysfs
>>>> attributes under 'ram' and 'pmem' sub-directories, the attributes must
>>>> be defined as static attributes rather than under driver->dev_groups.
>>>> To avoid implementing locking for accessing the 'struct cxl_dpa_perf`
>>>> lists, convert the list to a single 'struct cxl_dpa_perf' entry in
>>>> preparation to move the attributes to statically defined.
>>>>
>>>> While theoretically a partition may have multiple qos_class via CDAT, this
>>>> has not been encountered with testing on available hardware. The code is
>>>> simplified for now to not support the complex case until a use case is
>>>> needed to support that.
>>>>
>>>> Link: https://lore.kernel.org/linux-cxl/65b200ba228f_2d43c29468@dwillia2-mobl3.amr.corp.intel.com.notmuch/
>>>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>> v3:
>>>> - Add to commit log about simplification (Dan)
>>>> - Remove check for dev->driver (Dan)
>>>> - Remove check for invalid qos_class (Dan)
>>>> ---
>>>>  drivers/cxl/core/cdat.c | 81 ++++++++++++-----------------------------
>>>>  drivers/cxl/core/mbox.c |  4 +-
>>>>  drivers/cxl/cxlmem.h    | 10 ++---
>>>>  drivers/cxl/mem.c       | 28 ++------------
>>>>  4 files changed, 33 insertions(+), 90 deletions(-)
>>>>
>> [..]
>>>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>>>> index 6fe11546889f..55b82dfd794b 100644
>>>> --- a/drivers/cxl/core/cdat.c
>>>> +++ b/drivers/cxl/core/cdat.c
>>>> @@ -293,24 +270,25 @@ static int match_cxlrd_qos_class(struct device *dev, void *data)
>>>>   return 0;
>>>>  }
>>>>
>>>> +static void reset_dpa_perf(struct cxl_dpa_perf *dpa_perf)
>>>> +{
>>>> + memset(&dpa_perf, 0, sizeof(*dpa_perf));
>>>
>>> Hello,
>>>
>>> I think you meant dpa_perf instead of &dpa_perf, right?
>>>
>>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>>> index 5c93bf9d5253..7091619f12a9 100644
>>> --- a/drivers/cxl/core/cdat.c
>>> +++ b/drivers/cxl/core/cdat.c
>>> @@ -272,7 +272,7 @@ static int match_cxlrd_qos_class(struct device *dev, void *data)
>>>
>>>  static void reset_dpa_perf(struct cxl_dpa_perf *dpa_perf)
>>>  {
>>> -       memset(&dpa_perf, 0, sizeof(*dpa_perf));
>>> +       memset(dpa_perf, 0, sizeof(*dpa_perf));
>>
>> Good catch!
>>
>> ...or even better kill this function and just do:
>>
>>     *dpa_perf = { 0 };
> 
> We need to reinit the qos_class to -1 as well. 
>


This should do it right? The rest should be zeroed. 

	*dpa_perf = (struct cxl_dpa_perf) {
		.qos_class = CXL_QOS_CLASS_INVALID,
	};