Re: [ndctl PATCH v2 3/6] libcxl: return the partition alignment field in bytes

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Thu, 2022-01-13 at 08:34 -0800, Alison Schofield wrote:
> On Thu, Jan 13, 2022 at 08:31:49AM -0800, Alison Schofield wrote:
> > 
> > On Tue, Jan 11, 2022 at 10:33:31PM -0800, alison.schofield@intel.com wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > Per the CXL specification, the partition alignment field reports
> > > the alignment value in multiples of 256MB. In the libcxl API, values
> > > for all capacity fields are defined to return bytes.
> > > 
> > > Update the partition alignment accessor to return bytes so that it
> > > is in sync with other capacity related fields.
> > > 
> > > Rename the function to reflect the new return value:
> > > cxl_cmd_identify_get_partition_align_bytes()
> > 
> > Vishal,
> > Just realized that the cxl_identify_get_partition_align() API was released
> > in ndctl-v72. Does that mean NAK on changing the name here?
> > Alison
> 
> That question was incomplete! Does that mean NAK on changing what it
> returns too?  Or are 'we' early enough in cxl-cli to make such a change?

Ah yep - changing the name is definitely a no go. I think changing what
it returns is okay - I see it as a bug fix. The least-surprise return
is bytes, and returning anything else was a bug. Luckily we don't need
to change the return type :)

> 
> > 
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > ---
> > >  cxl/lib/libcxl.c   | 4 ++--
> > >  cxl/lib/libcxl.sym | 2 +-
> > >  cxl/libcxl.h       | 2 +-
> > >  3 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > > index 1fd584a..823bcb0 100644
> > > --- a/cxl/lib/libcxl.c
> > > +++ b/cxl/lib/libcxl.c
> > > @@ -1078,7 +1078,7 @@ CXL_EXPORT int cxl_cmd_identify_get_fw_rev(struct cxl_cmd *cmd, char *fw_rev,
> > >  	return 0;
> > >  }
> > >  
> > > -CXL_EXPORT unsigned long long cxl_cmd_identify_get_partition_align(
> > > +CXL_EXPORT unsigned long long cxl_cmd_identify_get_partition_align_bytes(
> > >  		struct cxl_cmd *cmd)
> > >  {
> > >  	struct cxl_cmd_identify *id =
> > > @@ -1089,7 +1089,7 @@ CXL_EXPORT unsigned long long cxl_cmd_identify_get_partition_align(
> > >  	if (cmd->status < 0)
> > >  		return cmd->status;
> > >  
> > > -	return le64_to_cpu(id->partition_align);
> > > +	return le64_to_cpu(id->partition_align) * CXL_CAPACITY_MULTIPLIER;
> > >  }
> > >  
> > >  CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
> > > diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> > > index b7e969f..1e2cf05 100644
> > > --- a/cxl/lib/libcxl.sym
> > > +++ b/cxl/lib/libcxl.sym
> > > @@ -34,7 +34,7 @@ global:
> > >  	cxl_cmd_identify_get_total_bytes;
> > >  	cxl_cmd_identify_get_volatile_only_bytes;
> > >  	cxl_cmd_identify_get_persistent_only_bytes;
> > > -	cxl_cmd_identify_get_partition_align;
> > > +	cxl_cmd_identify_get_partition_align_bytes;
> > >  	cxl_cmd_identify_get_label_size;
> > >  	cxl_cmd_new_get_health_info;
> > >  	cxl_cmd_health_info_get_maintenance_needed;
> > > diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> > > index 46f99fb..f19ed4f 100644
> > > --- a/cxl/libcxl.h
> > > +++ b/cxl/libcxl.h
> > > @@ -71,7 +71,7 @@ int cxl_cmd_identify_get_fw_rev(struct cxl_cmd *cmd, char *fw_rev, int fw_len);
> > >  unsigned long long cxl_cmd_identify_get_total_bytes(struct cxl_cmd *cmd);
> > >  unsigned long long cxl_cmd_identify_get_volatile_only_bytes(struct cxl_cmd *cmd);
> > >  unsigned long long cxl_cmd_identify_get_persistent_only_bytes(struct cxl_cmd *cmd);
> > > -unsigned long long cxl_cmd_identify_get_partition_align(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_identify_get_partition_align_bytes(struct cxl_cmd *cmd);
> > >  unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd);
> > >  struct cxl_cmd *cxl_cmd_new_get_health_info(struct cxl_memdev *memdev);
> > >  int cxl_cmd_health_info_get_maintenance_needed(struct cxl_cmd *cmd);
> > > -- 
> > > 2.31.1
> > > 
> > 
>