Re: [PATCH 1/2] libcxl: add accessors for Get Alert Configuration CCI command output

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

On Fri, 2022-10-21 at 07:30 +0000, Verma, Vishal L wrote:
> On Mon, 2022-10-03 at 16:23 -0700, Jonathan Zhang wrote:
> > From: Jonathan Zhang <jonzhang@meta.com>
> > 
> > CXL 3.0 spec section 8.2.9.8.3.2 "Get Alert Configuration
> > (Opcode 4201h) defines the get-alert-config command to
> > retrieve the devices's critical alert and programmable
> > warning configuration.
> > 
> > Add the methods to issue the command and get the fields
> > defined.
> > 
> > Signed-off-by: Jonathan Zhang <jonzhang@meta.com>
> > ---
> >  Documentation/cxl/lib/libcxl.txt |   1 +
> >  cxl/lib/libcxl.c                 | 120 +++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym               |  23 ++++++
> >  cxl/lib/private.h                |  28 ++++++++
> >  cxl/libcxl.h                     |  19 +++++
> >  5 files changed, 191 insertions(+)
> 
> Hi Jonathan,
> 
> Sorry for the delay in getting to these - a few small comments below.
> 
> > 
> > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> > index fd2962a..dec3641 100644
> > --- a/Documentation/cxl/lib/libcxl.txt
> > +++ b/Documentation/cxl/lib/libcxl.txt
> > @@ -121,6 +121,7 @@ information this call requires root / CAP_SYS_ADMIN.
> >  struct cxl_cmd *cxl_cmd_new_raw(struct cxl_memdev *memdev, int opcode);
> >  struct cxl_cmd *cxl_cmd_new_identify(struct cxl_memdev *memdev);
> >  struct cxl_cmd *cxl_cmd_new_get_health_info(struct cxl_memdev *memdev);
> > +struct cxl_cmd *cxl_cmd_new_get_alert_config(struct cxl_memdev *memdev);
> >  struct cxl_cmd *cxl_cmd_new_read_label(struct cxl_memdev *memdev,
> >                                         unsigned int offset, unsigned int length);
> >  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev, void *buf,
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index e8c5d44..ed5616c 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -3140,6 +3140,126 @@ do {                                                                    \
> >         return !!(c->field & mask);                                     \
> >  } while(0)
> >  
> > +CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_alert_config(
> > +               struct cxl_memdev *memdev)
> > +{
> > +       return cxl_cmd_new_generic(memdev, CXL_MEM_COMMAND_ID_GET_ALERT_CONFIG);
> > +}
> > +
> > +#define cmd_alert_get_valid_alerts_field(c, m)                                 \
> > +       cmd_get_field_u8_mask(c, get_alert_config, GET_ALERT_CONFIG, valid_alerts, m)
> > +
> > +CXL_EXPORT int cxl_cmd_alert_config_get_life_used_prog_warn_threshold_valid(struct cxl_cmd *cmd)
> 
> Generally throughout these patches, because of the (unavoidably) long
> names, these lines are well over 80 chars. For cases where a #define
> for example can't be broken down further, that's okay. But a lot of
> these can be split. We have a .clang-format for the project, and that
> should do the right thing in all these cases.
> 
> <..>
> > 
> > diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> > index 8bb91e0..7c1e261 100644
> > --- a/cxl/lib/libcxl.sym
> > +++ b/cxl/lib/libcxl.sym
> > @@ -217,3 +217,26 @@ global:
> >         cxl_decoder_get_max_available_extent;
> >         cxl_decoder_get_region;
> >  } LIBCXL_2;
> > +
> > +LIBCXL_4 {
> > +global:
> > +       cxl_cmd_new_get_alert_config;
> > +       cxl_cmd_alert_config_get_life_used_prog_warn_threshold_valid;
> > +       cxl_cmd_alert_config_get_dev_over_temp_prog_warn_threshold_valid;
> > +       cxl_cmd_alert_config_get_dev_under_temp_prog_warn_threshold_valid;
> > +       cxl_cmd_alert_config_get_corr_vol_mem_err_prog_warn_threshold_valid;
> > +       cxl_cmd_alert_config_get_corr_pers_mem_err_prog_warn_threshold_valid;
> 
> I think for all the 'valid' accessors, the 'get' can be dropped.
> cxl_cmd_alert_config_<something>_valid is a reasonable API that asks
> whether something is valid. 
> 
> > +       cxl_cmd_alert_config_get_life_used_prog_warn_threshold_prog;
> > +       cxl_cmd_alert_config_get_dev_over_temp_prog_warn_threshold_prog;
> > +       cxl_cmd_alert_config_get_dev_under_temp_prog_warn_threshold_prog;
> > +       cxl_cmd_alert_config_get_corr_vol_mem_err_prog_warn_threshold_prog;
> > +       cxl_cmd_alert_config_get_corr_pers_mem_err_prog_warn_threshold_prog;

Only realized this after hitting send - same comment here about the
double 'prog', 'pmem' etc (from my patch 2 email) here. Additionally
the 'get' can be dropped since the responds with a boolean. So this can
be: 

  cxl_cmd_alert_config_corrected_pmem_error_prog_warn_threshold_writable

> > +       cxl_cmd_alert_config_get_life_used_crit_alert_threshold;
> > +       cxl_cmd_alert_config_get_life_used_prog_warn_threshold;
> > +       cxl_cmd_alert_config_get_dev_over_temp_crit_alert_threshold;
> > +       cxl_cmd_alert_config_get_dev_under_temp_crit_alert_threshold;
> > +       cxl_cmd_alert_config_get_dev_over_temp_prog_warn_threshold;
> > +       cxl_cmd_alert_config_get_dev_under_temp_prog_warn_threshold;
> > +       cxl_cmd_alert_config_get_corr_vol_mem_err_prog_warn_threshold;
> > +       cxl_cmd_alert_config_get_corr_pers_mem_err_prog_warn_threshold;
> 
> These are okay to have the get, since we are getting a threshold value.
> 
> 
>