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
> > + * @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?
>
> > + * @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.
