On 21-09-09 11:06:53, Dan Williams wrote:
> On Thu, Sep 9, 2021 at 9:20 AM Ben Widawsky <[email protected]> wrote:
> >
> > On 21-09-08 22:12:15, Dan Williams wrote:
> > > Commit 0b9159d0ff21 ("cxl/pci: Store memory capacity values") missed
> > > updating the kernel-doc for 'struct cxl_mem' leading to the following
> > > warnings:
> > >
> > > ./scripts/kernel-doc -v drivers/cxl/cxlmem.h 2>&1 | grep warn
> > > drivers/cxl/cxlmem.h:107: warning: Function parameter or member
> > > 'total_bytes' not described in 'cxl_mem'
> > > drivers/cxl/cxlmem.h:107: warning: Function parameter or member
> > > 'volatile_only_bytes' not described in 'cxl_mem'
> > > drivers/cxl/cxlmem.h:107: warning: Function parameter or member
> > > 'persistent_only_bytes' not described in 'cxl_mem'
> > > drivers/cxl/cxlmem.h:107: warning: Function parameter or member
> > > 'partition_align_bytes' not described in 'cxl_mem'
> > > drivers/cxl/cxlmem.h:107: warning: Function parameter or member
> > > 'active_volatile_bytes' not described in 'cxl_mem'
> > > drivers/cxl/cxlmem.h:107: warning: Function parameter or member
> > > 'active_persistent_bytes' not described in 'cxl_mem'
> > > drivers/cxl/cxlmem.h:107: warning: Function parameter or member
> > > 'next_volatile_bytes' not described in 'cxl_mem'
> > > drivers/cxl/cxlmem.h:107: warning: Function parameter or member
> > > 'next_persistent_bytes' not described in 'cxl_mem'
> > >
> > > Also, it is redundant to describe those same parameters in the
> > > kernel-doc for cxl_mem_get_partition_info(). Given the only user of that
> > > routine updates the values in @cxlm, just do that implicitly internal to
> > > the helper.
> > >
> > > Cc: Ira Weiny <[email protected]>
> > > Reported-by: Ben Widawsky <[email protected]>
> > > Signed-off-by: Dan Williams <[email protected]>
> > > ---
> > > drivers/cxl/cxlmem.h | 15 +++++++++++++--
> > > drivers/cxl/pci.c | 35 +++++++++++------------------------
> > > 2 files changed, 24 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > index d5334df83fb2..c6fce966084a 100644
> > > --- a/drivers/cxl/cxlmem.h
> > > +++ b/drivers/cxl/cxlmem.h
> > > @@ -78,8 +78,19 @@ devm_cxl_add_memdev(struct cxl_mem *cxlm,
> > > * @mbox_mutex: Mutex to synchronize mailbox access.
> > > * @firmware_version: Firmware version for the memory device.
> > > * @enabled_cmds: Hardware commands found enabled in CEL.
> > > - * @pmem_range: Persistent memory capacity information.
> > > - * @ram_range: Volatile memory capacity information.
> > > + * @pmem_range: Active Persistent memory capacity configuration
> > > + * @ram_range: Active Volatile memory capacity configuration
> > > + * @total_bytes: sum of all possible capacities
> > > + * @volatile_only_bytes: hard volatile capacity
> > > + * @persistent_only_bytes: hard persistent capacity
> > > + * @partition_align_bytes: soft setting for configurable capacity
see below... How about:
"alignment size for partition-able capacity"
> > > + * @active_volatile_bytes: sum of hard + soft volatile
> > > + * @active_persistent_bytes: sum of hard + soft persistent
> >
> > Looking at this now, probably makes sense to create some helper macros or
> > inline
> > functions to calculate these as needed, rather than storing them in the
> > structure.
>
> Perhaps, I would need to look deeper into what is worth caching vs
> what is suitable to be recalculated. Do you have a proposal here?
I think it's worth being considered... however, this suggestion was my mistake.
I thought there was a way to infer the active capacities just from identify, but
there isn't. This leads me to request an update to the kdoc above which was how
I got confused.
>
>
> >
> > > + * @next_volatile_bytes: volatile capacity change pending device reset
> > > + * @next_persistent_bytes: persistent capacity change pending device
> > > reset
> > > + *
> > > + * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> > > + * details on capacity parameters.
> > > */
> > > struct cxl_mem {
> > > struct device *dev;
> > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > index c1e1d12e24b6..8077d907e7d3 100644
> > > --- a/drivers/cxl/pci.c
> > > +++ b/drivers/cxl/pci.c
> > > @@ -1262,11 +1262,7 @@ static struct cxl_mbox_get_supported_logs
> > > *cxl_get_gsl(struct cxl_mem *cxlm)
> > >
> > > /**
> > > * cxl_mem_get_partition_info - Get partition info
> > > - * @cxlm: The device to act on
> > > - * @active_volatile_bytes: returned active volatile capacity
> > > - * @active_persistent_bytes: returned active persistent capacity
> > > - * @next_volatile_bytes: return next volatile capacity
> > > - * @next_persistent_bytes: return next persistent capacity
> > > + * @cxlm: cxl_mem instance to update partition info
> > > *
> > > * Retrieve the current partition info for the device specified. If not
> > > 0, the
> > > * 'next' values are pending and take affect on next cold reset.
> > > @@ -1275,11 +1271,7 @@ static struct cxl_mbox_get_supported_logs
> > > *cxl_get_gsl(struct cxl_mem *cxlm)
> > > *
> > > * See CXL @8.2.9.5.2.1 Get Partition Info
> > > */
> > > -static int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> > > - u64 *active_volatile_bytes,
> > > - u64 *active_persistent_bytes,
> > > - u64 *next_volatile_bytes,
> > > - u64 *next_persistent_bytes)
> > > +static int cxl_mem_get_partition_info(struct cxl_mem *cxlm)
> > > {
> > > struct cxl_mbox_get_partition_info {
> > > __le64 active_volatile_cap;
> > > @@ -1294,15 +1286,14 @@ static int cxl_mem_get_partition_info(struct
> > > cxl_mem *cxlm,
> > > if (rc)
> > > return rc;
> > >
> > > - *active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap);
> > > - *active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap);
> > > - *next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap);
> > > - *next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap);
> > > -
> > > - *active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> > > - *active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> > > - *next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> > > - *next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> > > + cxlm->active_volatile_bytes =
> > > + le64_to_cpu(pi.active_volatile_cap) *
> > > CXL_CAPACITY_MULTIPLIER;
> > > + cxlm->active_persistent_bytes =
> > > + le64_to_cpu(pi.active_persistent_cap) *
> > > CXL_CAPACITY_MULTIPLIER;
> > > + cxlm->next_volatile_bytes =
> > > + le64_to_cpu(pi.next_volatile_cap) * CXL_CAPACITY_MULTIPLIER;
> > > + cxlm->next_persistent_bytes =
> > > + le64_to_cpu(pi.next_volatile_cap) * CXL_CAPACITY_MULTIPLIER;
> >
> > Personally, I prefer the more functional style implementation. I guess if
> > you
> > wanted to make the change, my preference would be to kill
> > cxl_mem_get_partition_info() entirely. Up to you though...
>
> I was bringing this function in line with the precedent we already set
> with cxl_mem_identify() that caches the result in @cxlm. Are you
> saying you want to change that style too?
>
> I feel like caching device attributes is idiomatic. Look at all the
> PCI attributes that are cached in "struct pci_device" that could be
> re-read or re-calculated rather than cached. In general I think a
> routine that returns 4 values is better off filling in a structure.
Caching is totally fine, I was just suggesting keeping the side effects out of
the innocuous sounding cxl_mem_get_partition_info(). I think I originally
authored authored cxl_mem_identify(), so mea culpa. I just realized it after
seeing the removal that I liked Ira's functional style.
Perhaps simply renaming these functions is the right solution (ie. save it for a
rainy day). Populate is not my favorite verb, but as an example:
static int cxl_mem_populate_partition_info
static int cxl_mem_populate_indentify
