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/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.