Re: [PATCH v2 3/6] coresight: configfs: Modify config files to allow userspace use

Hi Mathieu,

On Fri, 28 Jan 2022 at 18:33, Mathieu Poirier mathieu.poirier@linaro.org wrote:

On Tue, Nov 30, 2021 at 10:00:57PM +0000, Mike Leach wrote:

...

Update coresight-config.h and the coresight-config-file.c & .h to allow use in userspace programs.

Use __KERNEL__ defines to filter out driver only structures and elements so that user space programs can use the descriptor structures.

Abstract memory allocation in coresight-config-file.c to allow read file functions to be run in userspace and kernel drivers.

Signed-off-by: Mike Leach mike.leach@linaro.org

.../coresight/coresight-config-file.c | 96 +++++++++++++------ .../coresight/coresight-config-file.h | 33 +++++++ .../hwtracing/coresight/coresight-config.h | 21 ++++ 3 files changed, 122 insertions(+), 28 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-config-file.c b/drivers/hwtracing/coresight/coresight-config-file.c index 3fd001938324..77a64b54280d 100644 --- a/drivers/hwtracing/coresight/coresight-config-file.c +++ b/drivers/hwtracing/coresight/coresight-config-file.c @@ -4,10 +4,58 @@

• Author: Mike Leach mike.leach@linaro.org

*/

-#include "coresight-config.h" #include "coresight-config-file.h"

+/*

• • To allow reuse of this source in tools, define memory allocation fns according
• • to build environment.
• */

+#ifdef __KERNEL__ #include "coresight-syscfg.h"

+static void *cscfg_calloc(size_t num, size_t size) +{

• return devm_kcalloc(cscfg_device(), num, size, GFP_KERNEL);
  

+}

+static char *cscfg_strdup(const char *str) +{

• return devm_kstrdup(cscfg_device(), str, GFP_KERNEL);
  

+}

+static void *cscfg_zalloc(size_t size) +{

• return devm_kzalloc(cscfg_device(), size, GFP_KERNEL);
  

+}

+#else

+#include <stddef.h> +#include <string.h> +#include <stdlib.h>

+static void *cscfg_calloc(size_t num, size_t size) +{

• return calloc(num, size);
  

+}

+static char *cscfg_strdup(const char *str) +{

• return strdup(str);
  

+}

+static void *cscfg_zalloc(size_t size) +{

• void *ptr = malloc(size);
  

• if (ptr)
  

•         memset(ptr, 0, size);
  

• return ptr;
  

+}

+#endif

#define cscfg_extract_u64(val64) { \ val64 = *(u64 *)(buffer + used); \ used += sizeof(u64); \ @@ -80,6 +128,7 @@ static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf struct cscfg_file_elem_str *elem_str) { int used = *buf_used;

• const u8 *str;
  
  if ((buflen - used) < sizeof(u16))
          return -EINVAL;
  

@@ -89,11 +138,13 @@ static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf if ((buflen - used) < elem_str->str_len) return -EINVAL;

• str = buffer + used;
  

• /* check for 0 termination */
  

• if (buffer[elem_str->str_len - 1] != 0)
  

• if (str[elem_str->str_len - 1] != 0)
          return -EINVAL;
  
  

• elem_str->str = devm_kstrdup(cscfg_device(), (char *)buffer, GFP_KERNEL);
  

• elem_str->str = cscfg_strdup((char *)str);
  used += elem_str->str_len;
  
  *buf_used = used;
  

@@ -104,14 +155,12 @@ static int cscfg_file_alloc_desc_arrays(struct cscfg_fs_load_descs *desc_arrays, int nr_features) { /* arrays are 0 terminated - max of 1 config & nr_features features */

• desc_arrays->config_descs = devm_kcalloc(cscfg_device(), 2,
  

•                                          sizeof(struct cscfg_config_desc *),
  

•                                          GFP_KERNEL);
  

• desc_arrays->config_descs = cscfg_calloc(2, sizeof(struct cscfg_config_desc *));
  if (!desc_arrays->config_descs)
          return -ENOMEM;
  

• desc_arrays->feat_descs = devm_kcalloc(cscfg_device(), nr_features + 1,
  

•                                        sizeof(struct cscfg_feature_desc *),
  

•                                        GFP_KERNEL);
  

• desc_arrays->feat_descs = cscfg_calloc(nr_features + 1,
  

•                                        sizeof(struct cscfg_feature_desc *));
  if (!desc_arrays->feat_descs)
          return -ENOMEM;
  return 0;
  

@@ -138,8 +187,7 @@ static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int * return 0;

  /* we have a config - allocate the descriptor */

• config_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_config_desc),
  

•                            GFP_KERNEL);
  

• config_desc = cscfg_zalloc(sizeof(struct cscfg_config_desc));
  if (!config_desc)
          return -ENOMEM;
  
  

@@ -165,8 +213,7 @@ static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int * /* read the array of 64bit presets if present */ nr_preset_vals = config_desc->nr_total_params * config_desc->nr_presets; if (nr_preset_vals) {

•         presets = devm_kcalloc(cscfg_device(), nr_preset_vals,
  

•                                sizeof(u64), GFP_KERNEL);
  

•         presets = cscfg_calloc(nr_preset_vals, sizeof(u64));
          if (!presets)
                  return -ENOMEM;
  
  

@@ -181,10 +228,8 @@ static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int *

  /* read the array of feature names referenced by the config */
  if (config_desc->nr_feat_refs) {

•         config_desc->feat_ref_names = devm_kcalloc(cscfg_device(),
  

•                                                    config_desc->nr_feat_refs,
  

•                                                    sizeof(char *),
  

•                                                    GFP_KERNEL);
  

•         config_desc->feat_ref_names = cscfg_calloc(config_desc->nr_feat_refs,
  

•                                                    sizeof(char *));
          if (!config_desc->feat_ref_names)
                  return -ENOMEM;
  
  

@@ -262,8 +307,7 @@ static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int u32 val32;

  /* allocate the feature descriptor object */

• feat_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_feature_desc),
  

•                          GFP_KERNEL);
  

• feat_desc = cscfg_zalloc(sizeof(struct cscfg_feature_desc));
  if (!feat_desc)
          return -ENOMEM;
  
  

@@ -302,10 +346,8 @@ static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int nr_regs_bytes = ((sizeof(u32) + sizeof(u64)) * feat_desc->nr_regs); if ((buflen - used) < nr_regs_bytes) return -EINVAL;

•         feat_desc->regs_desc = devm_kcalloc(cscfg_device(),
  

•                                             feat_desc->nr_regs,
  

•                                             sizeof(struct cscfg_regval_desc),
  

•                                             GFP_KERNEL);
  

•         feat_desc->regs_desc = cscfg_calloc(feat_desc->nr_regs,
  

•                                             sizeof(struct cscfg_regval_desc));
          if (!feat_desc->regs_desc)
                  return -ENOMEM;
  
  

@@ -319,10 +361,8 @@ static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int

  /* parameter descriptors - string + 64 bit value */
  if (feat_desc->nr_params) {

•         feat_desc->params_desc = devm_kcalloc(cscfg_device(),
  

•                                               feat_desc->nr_params,
  

•                                               sizeof(struct cscfg_parameter_desc),
  

•                                               GFP_KERNEL);
  

•         feat_desc->params_desc = cscfg_calloc(feat_desc->nr_params,
  

•                                               sizeof(struct cscfg_parameter_desc));
          if (!feat_desc->params_desc)
                  return -ENOMEM;
          for (i = 0; i < feat_desc->nr_params; i++) {
  

@@ -399,7 +439,7 @@ int cscfg_file_read_buffer(const u8 *buffer, const int buflen, if (err) return err; }

• return used;
  

• return 0;
  

}

int cscfg_file_read_buffer_first_name(const u8 *buffer, const int buflen, diff --git a/drivers/hwtracing/coresight/coresight-config-file.h b/drivers/hwtracing/coresight/coresight-config-file.h index 03899f7d94c9..04c365e5109b 100644 --- a/drivers/hwtracing/coresight/coresight-config-file.h +++ b/drivers/hwtracing/coresight/coresight-config-file.h @@ -7,6 +7,39 @@ #ifndef _CORESIGHT_CORESIGHT_CONFIG_FILE_H #define _CORESIGHT_CORESIGHT_CONFIG_FILE_H

+#include <linux/types.h>

+#ifndef __KERNEL__ +/*

• • allow the user space programs to include the coresight config headers
• • to use the _desc structures, and reuse the read code
• */

+#ifndef u8 +typedef __u8 u8; +typedef __u16 u16; +typedef __u32 u32; +typedef __u64 u64; +#endif

+/* ARRAY SIZE is useful too */ +#ifndef ARRAY_SIZE +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) +#endif

+/* define EINVAL for user space */ +#ifndef EINVAL +#define EINVAL 22 +#endif

+#ifndef ENOMEM +#define ENOMEM 12 +#endif

+#endif

+#include "coresight-config.h"

/*

• Structures to represent configuration descriptors in a memory buffer
• to serialise to and from files

diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 770986316bc2..2136393df710 100644 --- a/drivers/hwtracing/coresight/coresight-config.h +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -7,7 +7,14 @@ #ifndef _CORESIGHT_CORESIGHT_CONFIG_H #define _CORESIGHT_CORESIGHT_CONFIG_H

+/*

• • Filter out kernel only portions of the file to allow user space programs
• • to use the descriptor definitions.
• */

+#ifdef __KERNEL__ #include <linux/coresight.h> +#endif

#include <linux/types.h>

/* CoreSight Configuration Management - component and system wide configuration */ @@ -103,14 +110,18 @@ struct cscfg_regval_desc { struct cscfg_feature_desc { const char *name; const char *description; +#ifdef __KERNEL__ struct list_head item; +#endif u32 match_flags; int nr_params; struct cscfg_parameter_desc *params_desc; int nr_regs; struct cscfg_regval_desc *regs_desc; +#ifdef __KERNEL__ void *load_owner; struct config_group *fs_group; +#endif };

That will quickly become a maintenance nightmare. I suggest the following:

I take your point. I was trying to avoid having a new structure, but allow the config buffer reader to use the same code in both kernel and userspace - good for testing and avoid maintaing two copies of the same code.

struct cscfg_feature_desc { struct list_head item; void *load_owner; struct config_group *fs_group; struct cscfg_feature_udesc *udesc; };

struct cscfg_feature_udesc { u32 version; const char *name; const char *description; u32 match_flags; int nr_params; struct cscfg_parameter_desc *params_desc; int nr_regs; struct cscfg_regval_desc *regs_desc; };

Structure cscfg_feature_udesc should likely be under include/uapi/linux/coresight-config.h

I don't mid addiing a udesc set of structures - but wanted to avoid changing the kernel structure. A change such as above would have huge ripple effects throughout all the existing kernel code.

It may be better to have separate user and kernel structures but try to get more abstraction into the reader algorithm.

I'll revisit this

Thanks

Mike

...

/** @@ -138,16 +149,20 @@ struct cscfg_feature_desc { struct cscfg_config_desc { const char *name; const char *description; +#ifdef __KERNEL__ struct list_head item; +#endif int nr_feat_refs; const char **feat_ref_names; int nr_presets; int nr_total_params; const u64 *presets; /* nr_presets * nr_total_params */ +#ifdef __KERNEL__ struct dev_ext_attribute *event_ea; atomic_t active_cnt; void *load_owner; struct config_group *fs_group; +#endif };

/** @@ -162,11 +177,16 @@ struct cscfg_config_desc {

• @feat_descs: array of feature descriptor pointers.

*/ struct cscfg_fs_load_descs { +#ifdef __KERNEL__ void *owner_info; +#endif struct cscfg_config_desc **config_descs; struct cscfg_feature_desc **feat_descs; };

Same comment as above.

...

+/* remainder of header is used by the kernel drivers only */ +#ifdef __KERNEL__

/**

• config register instance - part of a loaded feature.

•                        maps register values to csdev driver structures
  

@@ -274,4 +294,5 @@ void cscfg_csdev_disable_config(struct cscfg_config_csdev *config_csdev); /* reset a feature to default values */ void cscfg_reset_feat(struct cscfg_feature_csdev *feat_csdev);

+#endif /* __KERNEL__ */

#endif /* _CORESIGHT_CORESIGHT_CONFIG_H */

2.17.1