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

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

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;
> +       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.