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

[Dan Williams <dan.j.williams@intel.com>]

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