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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 2 Feb 2024 08:51:01 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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,
> 	};
> 

yes