[v3,1/5] coresight: configfs: Add in functionality for load via configfs

Commit Message

Mike Leach April 14, 2022, 6:44 a.m. UTC

Add in functionality to allow load via configfs.

define a binary file format and provide a reader for that format
that will create and populate configuration and feature structures
use by the driver infrastructure.

Signed-off-by: Mike Leach <[e-mail]>
---
 drivers/hwtracing/coresight/Makefile          |   2 +-
 .../coresight/coresight-config-file.c         | 432 ++++++++++++++++++
 .../coresight/coresight-config-file.h         | 117 +++++
 .../hwtracing/coresight/coresight-config.h    |  15 +
 .../hwtracing/coresight/coresight-syscfg.h    |   1 +
 5 files changed, 566 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-config-file.c
 create mode 100644 drivers/hwtracing/coresight/coresight-config-file.h

Comments

Mathieu Poirier May 11, 2022, 5:58 p.m. UTC | #1

Good day,

On Thu, Apr 14, 2022 at 07:44:53AM +0100, Mike Leach wrote:
> Add in functionality to allow load via configfs.
> 
> define a binary file format and provide a reader for that format
> that will create and populate configuration and feature structures
> use by the driver infrastructure.
> 
> Signed-off-by: Mike Leach <[e-mail]>
> ---
>  drivers/hwtracing/coresight/Makefile          |   2 +-
>  .../coresight/coresight-config-file.c         | 432 ++++++++++++++++++
>  .../coresight/coresight-config-file.h         | 117 +++++
>  .../hwtracing/coresight/coresight-config.h    |  15 +
>  .../hwtracing/coresight/coresight-syscfg.h    |   1 +
>  5 files changed, 566 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.c
>  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.h
> 
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index b6c4a48140ec..5de2bb79f4ac 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -6,7 +6,7 @@ obj-$(CONFIG_CORESIGHT) += coresight.o
>  coresight-y := coresight-core.o  coresight-etm-perf.o coresight-platform.o \
>  		coresight-sysfs.o coresight-syscfg.o coresight-config.o \
>  		coresight-cfg-preload.o coresight-cfg-afdo.o \
> -		coresight-syscfg-configfs.o
> +		coresight-syscfg-configfs.o coresight-config-file.o
>  obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
>  coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
>  		      coresight-tmc-etr.o
> diff --git a/drivers/hwtracing/coresight/coresight-config-file.c b/drivers/hwtracing/coresight/coresight-config-file.c
> new file mode 100644
> index 000000000000..3fd001938324
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-config-file.c
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> + * Author: Mike Leach <[e-mail]>
> + */
> +
> +#include "coresight-config.h"
> +#include "coresight-config-file.h"
> +#include "coresight-syscfg.h"
> +
> +#define cscfg_extract_u64(val64) {	\
> +	val64 = *(u64 *)(buffer + used); \
> +	used += sizeof(u64); \
> +	}
> +
> +#define cscfg_extract_u32(val32) { \
> +	val32 = *(u32 *)(buffer + used); \
> +	used += sizeof(u32); \
> +	}
> +
> +#define cscfg_extract_u16(val16) { \
> +	val16 = *(u16 *)(buffer + used); \
> +	used += sizeof(u16); \
> +	}
> +
> +#define cscfg_extract_u8(val8) {	\
> +	val8 = *(buffer + used); \
> +	used++; \
> +	}
> +
> +static int cscfg_file_read_hdr(const u8 *buffer, const int buflen, int *buf_used,
> +			       struct cscfg_file_header *hdr)
> +{
> +	/* file header always at the start of the buffer */
> +	int used = 0;
> +
> +	if (buflen < sizeof(struct cscfg_file_header))
> +		return -EINVAL;
> +
> +	cscfg_extract_u32(hdr->magic_version);
> +	if (hdr->magic_version != CSCFG_FILE_MAGIC_VERSION)
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(hdr->length);
> +	if (hdr->length > buflen)
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(hdr->nr_features);
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_hdr(const u8 *buffer, const int buflen, int *buf_used,
> +				    struct cscfg_file_elem_header *elem_hdr)
> +{
> +	int used = *buf_used;
> +
> +	if ((buflen - used) < (sizeof(u16) + sizeof(u8)))
> +		return -EINVAL;
> +
> +	/* read length and check enough buffer remains for this element */
> +	elem_hdr->elem_length = *(u16 *)(buffer + used);
> +	if ((buflen - used) < elem_hdr->elem_length)
> +		return -EINVAL;
> +	/* don't use extract fn as we update used _after_ the comparison */
> +	used += sizeof(u16);
> +
> +	/* read type and validate */
> +	cscfg_extract_u8(elem_hdr->elem_type);
> +	if ((elem_hdr->elem_type < CSCFG_FILE_ELEM_TYPE_FEAT) ||
> +	    (elem_hdr->elem_type > CSCFG_FILE_ELEM_TYPE_CFG))
> +		return -EINVAL;
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf_used,
> +				    struct cscfg_file_elem_str *elem_str)
> +{
> +	int used = *buf_used;
> +
> +	if ((buflen - used) < sizeof(u16))
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(elem_str->str_len);
> +
> +	if ((buflen - used) < elem_str->str_len)
> +		return -EINVAL;
> +
> +	/* check for 0 termination */
> +	if (buffer[elem_str->str_len - 1] != 0)

As far as I can see @buffer is never incremented, and @elem_str->str_len is the
length of the upcoming series of characters in the buffer.  As such either this
will always reference the start of @buffer and it is a bug or I am missing
something very subtle.  In the latter case please add a comment to underline
what is happening.

> +		return -EINVAL;
> +
> +	elem_str->str = devm_kstrdup(cscfg_device(), (char *)buffer, GFP_KERNEL);
> +	used += elem_str->str_len;
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +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);
> +	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);
> +	if (!desc_arrays->feat_descs)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int *buf_used,
> +				       struct cscfg_fs_load_descs *desc_arrays)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	struct cscfg_config_desc *config_desc;
> +	int used = *buf_used, nr_preset_vals, nr_preset_bytes, i;
> +	int err = 0;
> +	u64 *presets;
> +
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> +		return 0;
> +
> +	/* we have a config - allocate the descriptor */
> +	config_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_config_desc),
> +				   GFP_KERNEL);
> +	if (!config_desc)
> +		return -ENOMEM;
> +
> +	/* read the name string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	config_desc->name = elem_str.str;
> +
> +	/* read the description string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	config_desc->description = elem_str.str;
> +
> +	/* read in some values */
> +	if ((buflen - used) < sizeof(u64))
> +		return -EINVAL;
> +	cscfg_extract_u16(config_desc->nr_presets);
> +	cscfg_extract_u32(config_desc->nr_total_params);
> +	cscfg_extract_u16(config_desc->nr_feat_refs);
> +
> +	/* 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);
> +		if (!presets)
> +			return -ENOMEM;
> +
> +		nr_preset_bytes = sizeof(u64) * nr_preset_vals;
> +		if ((buflen - used) < nr_preset_bytes)
> +			return -EINVAL;
> +
> +		memcpy(presets, (buffer + used), nr_preset_bytes);
> +		config_desc->presets = presets;
> +		used += nr_preset_bytes;
> +	}
> +
> +	/* 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);
> +		if (!config_desc->feat_ref_names)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < config_desc->nr_feat_refs; i++) {
> +			err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +			if (err)
> +				return err;
> +			config_desc->feat_ref_names[i] = elem_str.str;
> +		}
> +	}
> +
> +	desc_arrays->config_descs[0] = config_desc;
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +/* just read the config name - if there is a config at this position */

Please add a description of the return behavior.  Here I have to correlate with
the code in cscfg_file_read_buffer_first_name() that it is not an error to find
something else than a CSCFG_FILE_ELEM_TYPE_CFG type.

More comments coming tomorrow.

Thanks,
Mathieu

> +static int cscfg_file_read_elem_config_name(const u8 *buffer, const int buflen, int *buf_used,
> +					    struct cscfg_file_elem_str *elem_str)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	int used = *buf_used;
> +	int err;
> +
> +	elem_str->str_len = 0;
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> +		return 0;
> +
> +	/* read the name string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> +	if (err)
> +		return err;
> +	*buf_used = used;
> +
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_param(const u8 *buffer, const int buflen, int *buf_used,
> +				      struct cscfg_parameter_desc *param_desc)
> +{
> +	struct cscfg_file_elem_str elem_str;
> +	int err = 0, used = *buf_used;
> +
> +	/* parameter name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	param_desc->name = elem_str.str;
> +
> +	/* parameter value */
> +	if ((buflen - used) < sizeof(u64))
> +		return -EINVAL;
> +	cscfg_extract_u64(param_desc->value);
> +
> +	*buf_used = used;
> +	return err;
> +}
> +
> +static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int *buf_used,
> +					struct cscfg_fs_load_descs *desc_arrays,
> +					const int feat_idx)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	struct cscfg_feature_desc *feat_desc;
> +	struct cscfg_regval_desc *p_reg_desc;
> +	int used = *buf_used, err, i, nr_regs_bytes;
> +	u32 val32;
> +
> +	/* allocate the feature descriptor object */
> +	feat_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_feature_desc),
> +				 GFP_KERNEL);
> +	if (!feat_desc)
> +		return -ENOMEM;
> +
> +	/* read and check the element header */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> +		return -EINVAL;
> +
> +	/* read the feature name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	feat_desc->name = elem_str.str;
> +
> +	/* read the description string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	feat_desc->description = elem_str.str;
> +
> +	/*
> +	 * read in some values
> +	 * [u32 value: match_flags]
> +	 * [u16 value: nr_regs]	    - number of registers.
> +	 * [u16 value: nr_params]   - number of parameters.
> +	 */
> +	cscfg_extract_u32(feat_desc->match_flags);
> +	cscfg_extract_u16(feat_desc->nr_regs);
> +	cscfg_extract_u16(feat_desc->nr_params);
> +
> +	/* register descriptors  - 32 bit + 64 bit value */
> +	if (feat_desc->nr_regs) {
> +		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);
> +		if (!feat_desc->regs_desc)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < feat_desc->nr_regs; i++) {
> +			cscfg_extract_u32(val32);
> +			p_reg_desc = (struct cscfg_regval_desc *)&feat_desc->regs_desc[i];
> +			CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_reg_desc);
> +			cscfg_extract_u64(feat_desc->regs_desc[i].val64);
> +		}
> +	}
> +
> +	/* 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);
> +		if (!feat_desc->params_desc)
> +			return -ENOMEM;
> +		for (i = 0; i < feat_desc->nr_params; i++) {
> +			err = cscfg_file_read_elem_param(buffer, buflen, &used,
> +							 &feat_desc->params_desc[i]);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	desc_arrays->feat_descs[feat_idx] = feat_desc;
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +/* just read the feature name - if there is a feature at this position */
> +static int cscfg_file_read_elem_feat_name(const u8 *buffer, const int buflen, int *buf_used,
> +					  struct cscfg_file_elem_str *elem_str)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	int used = *buf_used;
> +	int err;
> +
> +	elem_str->str_len = 0;
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> +		return -EINVAL;
> +
> +	/* read the feature name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> +	if (err)
> +		return err;
> +	*buf_used = used;
> +
> +	return 0;
> +}
> +
> +/*
> + * Read a buffer and create the configuration and feature
> + * descriptors to load into the cscfg system
> + */
> +int cscfg_file_read_buffer(const u8 *buffer, const int buflen,
> +			   struct cscfg_fs_load_descs *desc_arrays)
> +{
> +	struct cscfg_file_header hdr;
> +	int used = 0, err, i;
> +
> +	/* read in the file header */
> +	err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> +	if (err)
> +		return err;
> +
> +	/* allocate the memory for the descriptor pointer arrays */
> +	err = cscfg_file_alloc_desc_arrays(desc_arrays, hdr.nr_features);
> +	if (err)
> +		return err;
> +
> +	/* read elements */
> +
> +	/* first element could be a config so check */
> +	err = cscfg_file_read_elem_config(buffer, buflen, &used, desc_arrays);
> +	if (err)
> +		return err;
> +
> +	/* now read and populate all the feature descriptors */
> +	for (i = 0; i < hdr.nr_features; i++) {
> +		err = cscfg_file_read_elem_feature(buffer, buflen, &used, desc_arrays, i);
> +		if (err)
> +			return err;
> +	}
> +	return used;
> +}
> +
> +int cscfg_file_read_buffer_first_name(const u8 *buffer, const int buflen,
> +				      const char **name)
> +{
> +	struct cscfg_file_header hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	int used = 0, err = 0;
> +
> +	*name = NULL;
> +
> +	/* read in the file header */
> +	err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> +	if (err)
> +		return err;
> +
> +	err = cscfg_file_read_elem_config_name(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +
> +	/* no config string - get first feature name */
> +	if (!elem_str.str_len) {
> +		err = cscfg_file_read_elem_feat_name(buffer, buflen, &used, &elem_str);
> +		if (err)
> +			return err;
> +	}
> +	if (elem_str.str_len)
> +		*name = elem_str.str;
> +	return err;
> +}
> diff --git a/drivers/hwtracing/coresight/coresight-config-file.h b/drivers/hwtracing/coresight/coresight-config-file.h
> new file mode 100644
> index 000000000000..591f4c2c4be9
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-config-file.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> + * Author: Mike Leach <[e-mail]>
> + */
> +
> +#ifndef _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> +#define _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> +
> +/*
> + * Structures to represent configuration descriptors in a memory buffer
> + * to serialise to and from files
> + *
> + * File structure - for loading a configuration + features
> + * from configfs.
> + *
> + * [cscfg_file_header]	- mandatory
> + * [CONFIG_ELEM]	- optional - only one permitted,
> + * [FEATURE_ELEM] * [cscfg_file_header.nr_features]
> + *			- optional - file valid with config only.
> + *
> + * Invalid file if no config or features.
> + *
> + *
> + * File structure for [CONFIG_ELEM]:
> + *
> + * [cscfg_file_elem_header] - header length value to end of feature strings.
> + * [cscfg_file_elem_str]    - name of the configuration
> + * [cscfg_file_elem_str]    - description of configuration
> + * [u16 value - nr_presets]
> + * [u32 value - nr_total_params]
> + * [u16 value - nr_feat_refs]
> + * [u64 values] * (nr_presets * nr_total_params)
> + * [cscfg_file_elem_str] * nr_feat_refs
> + *
> + * Only one configuration per file.
> + *
> + * File structure for a [FEATURE_ELEM]
> + *
> + * [cscfg_file_elem_header] - header length is total bytes to end of param structures.
> + * [cscfg_file_elem_str]    - feature name.
> + * [cscfg_file_elem_str]    - feature description.
> + * [u32 value: match_flags]
> + * [u16 value: nr_regs]	    - number of registers.
> + * [u16 value: nr_params]   - number of parameters.
> + * [cscfg_regval_desc struct] * nr_regs
> + * [PARAM_ELEM] * nr_params
> + *
> + * File structure for [PARAM_ELEM]
> + *
> + * [cscfg_file_elem_str]    - parameter name.
> + * [u64 value: param_value] - initial value.
> + */
> +
> +/* major element types - configurations and features */
> +
> +#define CSCFG_FILE_ELEM_TYPE_FEAT	0x1
> +#define CSCFG_FILE_ELEM_TYPE_CFG	0x2
> +
> +#define CSCFG_FILE_MAGIC_VERSION	0xC5CF0001
> +
> +#define CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_desc) \
> +	{ \
> +	p_desc->type = (val32 >> 24) & 0xFF; \
> +	p_desc->offset = (val32 >> 12) & 0xFFF; \
> +	p_desc->hw_info = val32 & 0xFFF; \
> +	}
> +
> +#define CSCFG_FILE_REG_DESC_INFO_TO_U32(val32, p_desc) \
> +	{ \
> +	val32 = p_desc->hw_info & 0xFFF; \
> +	val32 |= ((p_desc->offset & 0xFFF) << 12); \
> +	val32 |= ((p_desc->type & 0xFF) << 24); \
> +	}
> +
> +/* binary attributes in configfs need a max size - declare a value for this. */
> +#define CSCFG_FILE_MAXSIZE	16384
> +
> +/* limit string sizes */
> +#define CSCFG_FILE_STR_MAXSIZE	1024
> +
> +/**
> + * file header.
> + *
> + * @magic_version: magic number / version for file/buffer format.
> + * @length       : total length of all data in the buffer.
> + * @nr_features  : total number of features in the buffer.
> + */
> +struct cscfg_file_header {
> +	u32 magic_version;
> +	u16 length;
> +	u16 nr_features;
> +};
> +
> +/**
> + * element header
> + *
> + * @elem_length: total length of this element
> + * @elem_type  : type of this element - one of CSCFG_FILE_ELEM_TYPE.. defines.
> + */
> +struct cscfg_file_elem_header {
> +	u16 elem_length;
> +	u8 elem_type;
> +};
> +
> +/**
> + * string file element.
> + *
> + * @str_len: length of string buffer including 0 terminator
> + * @str    : string buffer - 0 terminated.
> + */
> +struct cscfg_file_elem_str {
> +	u16 str_len;
> +	char *str;
> +};
> +
> +#endif /* _CORESIGHT_CORESIGHT_CONFIG_FILE_H */
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index 2e1670523461..9cd3c26ce023 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -85,6 +85,21 @@ struct cscfg_regval_desc {
>  	};
>  };
>  
> +/**
> + * Dynamically loaded descriptor arrays loaded via configfs.
> + *
> + * For builtin or module loaded configurations / features these are
> + * statically defined at compile time. For configfs we create the arrays
> + * dynamically so need a structure to handle this.
> + *
> + * @config_descs:	array of config descriptor pointers.
> + * @feat_descs:		array of feature descriptor pointers.
> + */
> +struct cscfg_fs_load_descs {
> +	struct cscfg_config_desc **config_descs;
> +	struct cscfg_feature_desc **feat_descs;
> +};
> +
>  /**
>   * Device feature descriptor - combination of registers and parameters to
>   * program a device to implement a specific complex function.
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index 9106ffab4833..6a6e33585be9 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -66,6 +66,7 @@ struct cscfg_registered_csdev {
>  enum cscfg_load_owner_type {
>  	CSCFG_OWNER_PRELOAD,
>  	CSCFG_OWNER_MODULE,
> +	CSCFG_OWNER_CONFIGFS,
>  };
>  
>  /**
> -- 
> 2.17.1
>

Mathieu Poirier May 12, 2022, 5:54 p.m. UTC | #2

On Thu, Apr 14, 2022 at 07:44:53AM +0100, Mike Leach wrote:
> Add in functionality to allow load via configfs.
> 
> define a binary file format and provide a reader for that format
> that will create and populate configuration and feature structures
> use by the driver infrastructure.
> 
> Signed-off-by: Mike Leach <[e-mail]>
> ---
>  drivers/hwtracing/coresight/Makefile          |   2 +-
>  .../coresight/coresight-config-file.c         | 432 ++++++++++++++++++
>  .../coresight/coresight-config-file.h         | 117 +++++
>  .../hwtracing/coresight/coresight-config.h    |  15 +
>  .../hwtracing/coresight/coresight-syscfg.h    |   1 +
>  5 files changed, 566 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.c
>  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.h
> 
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index b6c4a48140ec..5de2bb79f4ac 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -6,7 +6,7 @@ obj-$(CONFIG_CORESIGHT) += coresight.o
>  coresight-y := coresight-core.o  coresight-etm-perf.o coresight-platform.o \
>  		coresight-sysfs.o coresight-syscfg.o coresight-config.o \
>  		coresight-cfg-preload.o coresight-cfg-afdo.o \
> -		coresight-syscfg-configfs.o
> +		coresight-syscfg-configfs.o coresight-config-file.o
>  obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
>  coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
>  		      coresight-tmc-etr.o
> diff --git a/drivers/hwtracing/coresight/coresight-config-file.c b/drivers/hwtracing/coresight/coresight-config-file.c
> new file mode 100644
> index 000000000000..3fd001938324
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-config-file.c
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> + * Author: Mike Leach <[e-mail]>
> + */
> +
> +#include "coresight-config.h"
> +#include "coresight-config-file.h"
> +#include "coresight-syscfg.h"
> +
> +#define cscfg_extract_u64(val64) {	\
> +	val64 = *(u64 *)(buffer + used); \
> +	used += sizeof(u64); \
> +	}
> +
> +#define cscfg_extract_u32(val32) { \
> +	val32 = *(u32 *)(buffer + used); \
> +	used += sizeof(u32); \
> +	}
> +
> +#define cscfg_extract_u16(val16) { \
> +	val16 = *(u16 *)(buffer + used); \
> +	used += sizeof(u16); \
> +	}
> +
> +#define cscfg_extract_u8(val8) {	\
> +	val8 = *(buffer + used); \
> +	used++; \
> +	}
> +
> +static int cscfg_file_read_hdr(const u8 *buffer, const int buflen, int *buf_used,
> +			       struct cscfg_file_header *hdr)
> +{
> +	/* file header always at the start of the buffer */
> +	int used = 0;
> +
> +	if (buflen < sizeof(struct cscfg_file_header))
> +		return -EINVAL;
> +
> +	cscfg_extract_u32(hdr->magic_version);
> +	if (hdr->magic_version != CSCFG_FILE_MAGIC_VERSION)
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(hdr->length);
> +	if (hdr->length > buflen)
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(hdr->nr_features);
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_hdr(const u8 *buffer, const int buflen, int *buf_used,
> +				    struct cscfg_file_elem_header *elem_hdr)
> +{
> +	int used = *buf_used;
> +
> +	if ((buflen - used) < (sizeof(u16) + sizeof(u8)))
> +		return -EINVAL;
> +
> +	/* read length and check enough buffer remains for this element */
> +	elem_hdr->elem_length = *(u16 *)(buffer + used);
> +	if ((buflen - used) < elem_hdr->elem_length)
> +		return -EINVAL;
> +	/* don't use extract fn as we update used _after_ the comparison */
> +	used += sizeof(u16);
> +
> +	/* read type and validate */
> +	cscfg_extract_u8(elem_hdr->elem_type);
> +	if ((elem_hdr->elem_type < CSCFG_FILE_ELEM_TYPE_FEAT) ||
> +	    (elem_hdr->elem_type > CSCFG_FILE_ELEM_TYPE_CFG))
> +		return -EINVAL;
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf_used,
> +				    struct cscfg_file_elem_str *elem_str)
> +{
> +	int used = *buf_used;
> +
> +	if ((buflen - used) < sizeof(u16))
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(elem_str->str_len);
> +
> +	if ((buflen - used) < elem_str->str_len)
> +		return -EINVAL;
> +
> +	/* check for 0 termination */
> +	if (buffer[elem_str->str_len - 1] != 0)
> +		return -EINVAL;
> +
> +	elem_str->str = devm_kstrdup(cscfg_device(), (char *)buffer, GFP_KERNEL);
> +	used += elem_str->str_len;
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +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);
> +	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);
> +	if (!desc_arrays->feat_descs)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int *buf_used,
> +				       struct cscfg_fs_load_descs *desc_arrays)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	struct cscfg_config_desc *config_desc;
> +	int used = *buf_used, nr_preset_vals, nr_preset_bytes, i;
> +	int err = 0;
> +	u64 *presets;
> +
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> +		return 0;
> +
> +	/* we have a config - allocate the descriptor */
> +	config_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_config_desc),
> +				   GFP_KERNEL);
> +	if (!config_desc)
> +		return -ENOMEM;
> +
> +	/* read the name string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	config_desc->name = elem_str.str;
> +
> +	/* read the description string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	config_desc->description = elem_str.str;
> +
> +	/* read in some values */
> +	if ((buflen - used) < sizeof(u64))
> +		return -EINVAL;
> +	cscfg_extract_u16(config_desc->nr_presets);
> +	cscfg_extract_u32(config_desc->nr_total_params);
> +	cscfg_extract_u16(config_desc->nr_feat_refs);
> +
> +	/* 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);
> +		if (!presets)
> +			return -ENOMEM;
> +
> +		nr_preset_bytes = sizeof(u64) * nr_preset_vals;
> +		if ((buflen - used) < nr_preset_bytes)
> +			return -EINVAL;
> +
> +		memcpy(presets, (buffer + used), nr_preset_bytes);
> +		config_desc->presets = presets;
> +		used += nr_preset_bytes;
> +	}
> +
> +	/* 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);
> +		if (!config_desc->feat_ref_names)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < config_desc->nr_feat_refs; i++) {
> +			err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +			if (err)
> +				return err;
> +			config_desc->feat_ref_names[i] = elem_str.str;
> +		}
> +	}
> +
> +	desc_arrays->config_descs[0] = config_desc;
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +/* just read the config name - if there is a config at this position */
> +static int cscfg_file_read_elem_config_name(const u8 *buffer, const int buflen, int *buf_used,
> +					    struct cscfg_file_elem_str *elem_str)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	int used = *buf_used;
> +	int err;
> +
> +	elem_str->str_len = 0;
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> +		return 0;
> +
> +	/* read the name string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> +	if (err)
> +		return err;
> +	*buf_used = used;
> +
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_param(const u8 *buffer, const int buflen, int *buf_used,
> +				      struct cscfg_parameter_desc *param_desc)
> +{
> +	struct cscfg_file_elem_str elem_str;
> +	int err = 0, used = *buf_used;
> +
> +	/* parameter name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	param_desc->name = elem_str.str;
> +
> +	/* parameter value */
> +	if ((buflen - used) < sizeof(u64))
> +		return -EINVAL;
> +	cscfg_extract_u64(param_desc->value);
> +
> +	*buf_used = used;
> +	return err;
> +}
> +
> +static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int *buf_used,
> +					struct cscfg_fs_load_descs *desc_arrays,
> +					const int feat_idx)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	struct cscfg_feature_desc *feat_desc;
> +	struct cscfg_regval_desc *p_reg_desc;
> +	int used = *buf_used, err, i, nr_regs_bytes;
> +	u32 val32;
> +
> +	/* allocate the feature descriptor object */
> +	feat_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_feature_desc),
> +				 GFP_KERNEL);
> +	if (!feat_desc)
> +		return -ENOMEM;
> +
> +	/* read and check the element header */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> +		return -EINVAL;
> +
> +	/* read the feature name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	feat_desc->name = elem_str.str;
> +
> +	/* read the description string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	feat_desc->description = elem_str.str;
> +
> +	/*
> +	 * read in some values
> +	 * [u32 value: match_flags]
> +	 * [u16 value: nr_regs]	    - number of registers.
> +	 * [u16 value: nr_params]   - number of parameters.
> +	 */

I expected the same kind of check found in cscfg_file_read_elem_config().

I am done with patch, moving on with the rest in the coming days.

Thanks,
Mathieu

> +	cscfg_extract_u32(feat_desc->match_flags);
> +	cscfg_extract_u16(feat_desc->nr_regs);
> +	cscfg_extract_u16(feat_desc->nr_params);
> +
> +	/* register descriptors  - 32 bit + 64 bit value */
> +	if (feat_desc->nr_regs) {
> +		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);
> +		if (!feat_desc->regs_desc)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < feat_desc->nr_regs; i++) {
> +			cscfg_extract_u32(val32);
> +			p_reg_desc = (struct cscfg_regval_desc *)&feat_desc->regs_desc[i];
> +			CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_reg_desc);
> +			cscfg_extract_u64(feat_desc->regs_desc[i].val64);
> +		}
> +	}
> +
> +	/* 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);
> +		if (!feat_desc->params_desc)
> +			return -ENOMEM;
> +		for (i = 0; i < feat_desc->nr_params; i++) {
> +			err = cscfg_file_read_elem_param(buffer, buflen, &used,
> +							 &feat_desc->params_desc[i]);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	desc_arrays->feat_descs[feat_idx] = feat_desc;
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +/* just read the feature name - if there is a feature at this position */
> +static int cscfg_file_read_elem_feat_name(const u8 *buffer, const int buflen, int *buf_used,
> +					  struct cscfg_file_elem_str *elem_str)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	int used = *buf_used;
> +	int err;
> +
> +	elem_str->str_len = 0;
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> +		return -EINVAL;
> +
> +	/* read the feature name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> +	if (err)
> +		return err;
> +	*buf_used = used;
> +
> +	return 0;
> +}
> +
> +/*
> + * Read a buffer and create the configuration and feature
> + * descriptors to load into the cscfg system
> + */
> +int cscfg_file_read_buffer(const u8 *buffer, const int buflen,
> +			   struct cscfg_fs_load_descs *desc_arrays)
> +{
> +	struct cscfg_file_header hdr;
> +	int used = 0, err, i;
> +
> +	/* read in the file header */
> +	err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> +	if (err)
> +		return err;
> +
> +	/* allocate the memory for the descriptor pointer arrays */
> +	err = cscfg_file_alloc_desc_arrays(desc_arrays, hdr.nr_features);
> +	if (err)
> +		return err;
> +
> +	/* read elements */
> +
> +	/* first element could be a config so check */
> +	err = cscfg_file_read_elem_config(buffer, buflen, &used, desc_arrays);
> +	if (err)
> +		return err;
> +
> +	/* now read and populate all the feature descriptors */
> +	for (i = 0; i < hdr.nr_features; i++) {
> +		err = cscfg_file_read_elem_feature(buffer, buflen, &used, desc_arrays, i);
> +		if (err)
> +			return err;
> +	}
> +	return used;
> +}
> +
> +int cscfg_file_read_buffer_first_name(const u8 *buffer, const int buflen,
> +				      const char **name)
> +{
> +	struct cscfg_file_header hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	int used = 0, err = 0;
> +
> +	*name = NULL;
> +
> +	/* read in the file header */
> +	err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> +	if (err)
> +		return err;
> +
> +	err = cscfg_file_read_elem_config_name(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +
> +	/* no config string - get first feature name */
> +	if (!elem_str.str_len) {
> +		err = cscfg_file_read_elem_feat_name(buffer, buflen, &used, &elem_str);
> +		if (err)
> +			return err;
> +	}
> +	if (elem_str.str_len)
> +		*name = elem_str.str;
> +	return err;
> +}
> diff --git a/drivers/hwtracing/coresight/coresight-config-file.h b/drivers/hwtracing/coresight/coresight-config-file.h
> new file mode 100644
> index 000000000000..591f4c2c4be9
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-config-file.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> + * Author: Mike Leach <[e-mail]>
> + */
> +
> +#ifndef _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> +#define _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> +
> +/*
> + * Structures to represent configuration descriptors in a memory buffer
> + * to serialise to and from files
> + *
> + * File structure - for loading a configuration + features
> + * from configfs.
> + *
> + * [cscfg_file_header]	- mandatory
> + * [CONFIG_ELEM]	- optional - only one permitted,
> + * [FEATURE_ELEM] * [cscfg_file_header.nr_features]
> + *			- optional - file valid with config only.
> + *
> + * Invalid file if no config or features.
> + *
> + *
> + * File structure for [CONFIG_ELEM]:
> + *
> + * [cscfg_file_elem_header] - header length value to end of feature strings.
> + * [cscfg_file_elem_str]    - name of the configuration
> + * [cscfg_file_elem_str]    - description of configuration
> + * [u16 value - nr_presets]
> + * [u32 value - nr_total_params]
> + * [u16 value - nr_feat_refs]
> + * [u64 values] * (nr_presets * nr_total_params)
> + * [cscfg_file_elem_str] * nr_feat_refs
> + *
> + * Only one configuration per file.
> + *
> + * File structure for a [FEATURE_ELEM]
> + *
> + * [cscfg_file_elem_header] - header length is total bytes to end of param structures.
> + * [cscfg_file_elem_str]    - feature name.
> + * [cscfg_file_elem_str]    - feature description.
> + * [u32 value: match_flags]
> + * [u16 value: nr_regs]	    - number of registers.
> + * [u16 value: nr_params]   - number of parameters.
> + * [cscfg_regval_desc struct] * nr_regs
> + * [PARAM_ELEM] * nr_params
> + *
> + * File structure for [PARAM_ELEM]
> + *
> + * [cscfg_file_elem_str]    - parameter name.
> + * [u64 value: param_value] - initial value.
> + */
> +
> +/* major element types - configurations and features */
> +
> +#define CSCFG_FILE_ELEM_TYPE_FEAT	0x1
> +#define CSCFG_FILE_ELEM_TYPE_CFG	0x2
> +
> +#define CSCFG_FILE_MAGIC_VERSION	0xC5CF0001
> +
> +#define CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_desc) \
> +	{ \
> +	p_desc->type = (val32 >> 24) & 0xFF; \
> +	p_desc->offset = (val32 >> 12) & 0xFFF; \
> +	p_desc->hw_info = val32 & 0xFFF; \
> +	}
> +
> +#define CSCFG_FILE_REG_DESC_INFO_TO_U32(val32, p_desc) \
> +	{ \
> +	val32 = p_desc->hw_info & 0xFFF; \
> +	val32 |= ((p_desc->offset & 0xFFF) << 12); \
> +	val32 |= ((p_desc->type & 0xFF) << 24); \
> +	}
> +
> +/* binary attributes in configfs need a max size - declare a value for this. */
> +#define CSCFG_FILE_MAXSIZE	16384
> +
> +/* limit string sizes */
> +#define CSCFG_FILE_STR_MAXSIZE	1024
> +
> +/**
> + * file header.
> + *
> + * @magic_version: magic number / version for file/buffer format.
> + * @length       : total length of all data in the buffer.
> + * @nr_features  : total number of features in the buffer.
> + */
> +struct cscfg_file_header {
> +	u32 magic_version;
> +	u16 length;
> +	u16 nr_features;
> +};
> +
> +/**
> + * element header
> + *
> + * @elem_length: total length of this element
> + * @elem_type  : type of this element - one of CSCFG_FILE_ELEM_TYPE.. defines.
> + */
> +struct cscfg_file_elem_header {
> +	u16 elem_length;
> +	u8 elem_type;
> +};
> +
> +/**
> + * string file element.
> + *
> + * @str_len: length of string buffer including 0 terminator
> + * @str    : string buffer - 0 terminated.
> + */
> +struct cscfg_file_elem_str {
> +	u16 str_len;
> +	char *str;
> +};
> +
> +#endif /* _CORESIGHT_CORESIGHT_CONFIG_FILE_H */
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index 2e1670523461..9cd3c26ce023 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -85,6 +85,21 @@ struct cscfg_regval_desc {
>  	};
>  };
>  
> +/**
> + * Dynamically loaded descriptor arrays loaded via configfs.
> + *
> + * For builtin or module loaded configurations / features these are
> + * statically defined at compile time. For configfs we create the arrays
> + * dynamically so need a structure to handle this.
> + *
> + * @config_descs:	array of config descriptor pointers.
> + * @feat_descs:		array of feature descriptor pointers.
> + */
> +struct cscfg_fs_load_descs {
> +	struct cscfg_config_desc **config_descs;
> +	struct cscfg_feature_desc **feat_descs;
> +};
> +
>  /**
>   * Device feature descriptor - combination of registers and parameters to
>   * program a device to implement a specific complex function.
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index 9106ffab4833..6a6e33585be9 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -66,6 +66,7 @@ struct cscfg_registered_csdev {
>  enum cscfg_load_owner_type {
>  	CSCFG_OWNER_PRELOAD,
>  	CSCFG_OWNER_MODULE,
> +	CSCFG_OWNER_CONFIGFS,
>  };
>  
>  /**
> -- 
> 2.17.1
>

Mike Leach May 16, 2022, 12:43 p.m. UTC | #3

Hi Mathieu,

On Wed, 11 May 2022 at 18:58, Mathieu Poirier
<[e-mail]> wrote:
>
> Good day,
>
> On Thu, Apr 14, 2022 at 07:44:53AM +0100, Mike Leach wrote:
> > Add in functionality to allow load via configfs.
> >
> > define a binary file format and provide a reader for that format
> > that will create and populate configuration and feature structures
> > use by the driver infrastructure.
> >
> > Signed-off-by: Mike Leach <[e-mail]>
> > ---
> >  drivers/hwtracing/coresight/Makefile          |   2 +-
> >  .../coresight/coresight-config-file.c         | 432 ++++++++++++++++++
> >  .../coresight/coresight-config-file.h         | 117 +++++
> >  .../hwtracing/coresight/coresight-config.h    |  15 +
> >  .../hwtracing/coresight/coresight-syscfg.h    |   1 +
> >  5 files changed, 566 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.c
> >  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.h
> >
> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> > index b6c4a48140ec..5de2bb79f4ac 100644
> > --- a/drivers/hwtracing/coresight/Makefile
> > +++ b/drivers/hwtracing/coresight/Makefile
> > @@ -6,7 +6,7 @@ obj-$(CONFIG_CORESIGHT) += coresight.o
> >  coresight-y := coresight-core.o  coresight-etm-perf.o coresight-platform.o \
> >               coresight-sysfs.o coresight-syscfg.o coresight-config.o \
> >               coresight-cfg-preload.o coresight-cfg-afdo.o \
> > -             coresight-syscfg-configfs.o
> > +             coresight-syscfg-configfs.o coresight-config-file.o
> >  obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
> >  coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
> >                     coresight-tmc-etr.o
> > diff --git a/drivers/hwtracing/coresight/coresight-config-file.c b/drivers/hwtracing/coresight/coresight-config-file.c
> > new file mode 100644
> > index 000000000000..3fd001938324
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-config-file.c
> > @@ -0,0 +1,432 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> > + * Author: Mike Leach <[e-mail]>
> > + */
> > +
> > +#include "coresight-config.h"
> > +#include "coresight-config-file.h"
> > +#include "coresight-syscfg.h"
> > +
> > +#define cscfg_extract_u64(val64) {   \
> > +     val64 = *(u64 *)(buffer + used); \
> > +     used += sizeof(u64); \
> > +     }
> > +
> > +#define cscfg_extract_u32(val32) { \
> > +     val32 = *(u32 *)(buffer + used); \
> > +     used += sizeof(u32); \
> > +     }
> > +
> > +#define cscfg_extract_u16(val16) { \
> > +     val16 = *(u16 *)(buffer + used); \
> > +     used += sizeof(u16); \
> > +     }
> > +
> > +#define cscfg_extract_u8(val8) {     \
> > +     val8 = *(buffer + used); \
> > +     used++; \
> > +     }
> > +
> > +static int cscfg_file_read_hdr(const u8 *buffer, const int buflen, int *buf_used,
> > +                            struct cscfg_file_header *hdr)
> > +{
> > +     /* file header always at the start of the buffer */
> > +     int used = 0;
> > +
> > +     if (buflen < sizeof(struct cscfg_file_header))
> > +             return -EINVAL;
> > +
> > +     cscfg_extract_u32(hdr->magic_version);
> > +     if (hdr->magic_version != CSCFG_FILE_MAGIC_VERSION)
> > +             return -EINVAL;
> > +
> > +     cscfg_extract_u16(hdr->length);
> > +     if (hdr->length > buflen)
> > +             return -EINVAL;
> > +
> > +     cscfg_extract_u16(hdr->nr_features);
> > +
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +static int cscfg_file_read_elem_hdr(const u8 *buffer, const int buflen, int *buf_used,
> > +                                 struct cscfg_file_elem_header *elem_hdr)
> > +{
> > +     int used = *buf_used;
> > +
> > +     if ((buflen - used) < (sizeof(u16) + sizeof(u8)))
> > +             return -EINVAL;
> > +
> > +     /* read length and check enough buffer remains for this element */
> > +     elem_hdr->elem_length = *(u16 *)(buffer + used);
> > +     if ((buflen - used) < elem_hdr->elem_length)
> > +             return -EINVAL;
> > +     /* don't use extract fn as we update used _after_ the comparison */
> > +     used += sizeof(u16);
> > +
> > +     /* read type and validate */
> > +     cscfg_extract_u8(elem_hdr->elem_type);
> > +     if ((elem_hdr->elem_type < CSCFG_FILE_ELEM_TYPE_FEAT) ||
> > +         (elem_hdr->elem_type > CSCFG_FILE_ELEM_TYPE_CFG))
> > +             return -EINVAL;
> > +
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf_used,
> > +                                 struct cscfg_file_elem_str *elem_str)
> > +{
> > +     int used = *buf_used;
> > +
> > +     if ((buflen - used) < sizeof(u16))
> > +             return -EINVAL;
> > +
> > +     cscfg_extract_u16(elem_str->str_len);
> > +
> > +     if ((buflen - used) < elem_str->str_len)
> > +             return -EINVAL;
> > +
> > +     /* check for 0 termination */
> > +     if (buffer[elem_str->str_len - 1] != 0)
>
> As far as I can see @buffer is never incremented, and @elem_str->str_len is the
> length of the upcoming series of characters in the buffer.  As such either this
> will always reference the start of @buffer and it is a bug or I am missing
> something very subtle.  In the latter case please add a comment to underline
> what is happening.
>

in the buffer a string consists of u16:len, followed by u8:*len
characters - zero terminated.
The buffer[len-1] points at what should be the zero terminating
character, Must be zero terminated to allow kstrdup to work correctly.


> > +             return -EINVAL;
> > +
> > +     elem_str->str = devm_kstrdup(cscfg_device(), (char *)buffer, GFP_KERNEL);
> > +     used += elem_str->str_len;
> > +
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +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);
> > +     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);
> > +     if (!desc_arrays->feat_descs)
> > +             return -ENOMEM;
> > +     return 0;
> > +}
> > +
> > +static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int *buf_used,
> > +                                    struct cscfg_fs_load_descs *desc_arrays)
> > +{
> > +     struct cscfg_file_elem_header elem_hdr;
> > +     struct cscfg_file_elem_str elem_str;
> > +     struct cscfg_config_desc *config_desc;
> > +     int used = *buf_used, nr_preset_vals, nr_preset_bytes, i;
> > +     int err = 0;
> > +     u64 *presets;
> > +
> > +     /*
> > +      * read the header - if not config, then don't update buf_used
> > +      * pointer on return
> > +      */
> > +     err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> > +     if (err)
> > +             return err;
> > +     if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> > +             return 0;
> > +
> > +     /* we have a config - allocate the descriptor */
> > +     config_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_config_desc),
> > +                                GFP_KERNEL);
> > +     if (!config_desc)
> > +             return -ENOMEM;
> > +
> > +     /* read the name string */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     config_desc->name = elem_str.str;
> > +
> > +     /* read the description string */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     config_desc->description = elem_str.str;
> > +
> > +     /* read in some values */
> > +     if ((buflen - used) < sizeof(u64))
> > +             return -EINVAL;
> > +     cscfg_extract_u16(config_desc->nr_presets);
> > +     cscfg_extract_u32(config_desc->nr_total_params);
> > +     cscfg_extract_u16(config_desc->nr_feat_refs);
> > +
> > +     /* 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);
> > +             if (!presets)
> > +                     return -ENOMEM;
> > +
> > +             nr_preset_bytes = sizeof(u64) * nr_preset_vals;
> > +             if ((buflen - used) < nr_preset_bytes)
> > +                     return -EINVAL;
> > +
> > +             memcpy(presets, (buffer + used), nr_preset_bytes);
> > +             config_desc->presets = presets;
> > +             used += nr_preset_bytes;
> > +     }
> > +
> > +     /* 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);
> > +             if (!config_desc->feat_ref_names)
> > +                     return -ENOMEM;
> > +
> > +             for (i = 0; i < config_desc->nr_feat_refs; i++) {
> > +                     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +                     if (err)
> > +                             return err;
> > +                     config_desc->feat_ref_names[i] = elem_str.str;
> > +             }
> > +     }
> > +
> > +     desc_arrays->config_descs[0] = config_desc;
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +/* just read the config name - if there is a config at this position */
>
> Please add a description of the return behavior.  Here I have to correlate with
> the code in cscfg_file_read_buffer_first_name() that it is not an error to find
> something else than a CSCFG_FILE_ELEM_TYPE_CFG type.
>
Will do.

Thanks

Mike

> More comments coming tomorrow.
>
> Thanks,
> Mathieu
>
> > +static int cscfg_file_read_elem_config_name(const u8 *buffer, const int buflen, int *buf_used,
> > +                                         struct cscfg_file_elem_str *elem_str)
> > +{
> > +     struct cscfg_file_elem_header elem_hdr;
> > +     int used = *buf_used;
> > +     int err;
> > +
> > +     elem_str->str_len = 0;
> > +     /*
> > +      * read the header - if not config, then don't update buf_used
> > +      * pointer on return
> > +      */
> > +     err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> > +     if (err)
> > +             return err;
> > +     if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> > +             return 0;
> > +
> > +     /* read the name string */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> > +     if (err)
> > +             return err;
> > +     *buf_used = used;
> > +
> > +     return 0;
> > +}
> > +
> > +static int cscfg_file_read_elem_param(const u8 *buffer, const int buflen, int *buf_used,
> > +                                   struct cscfg_parameter_desc *param_desc)
> > +{
> > +     struct cscfg_file_elem_str elem_str;
> > +     int err = 0, used = *buf_used;
> > +
> > +     /* parameter name */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     param_desc->name = elem_str.str;
> > +
> > +     /* parameter value */
> > +     if ((buflen - used) < sizeof(u64))
> > +             return -EINVAL;
> > +     cscfg_extract_u64(param_desc->value);
> > +
> > +     *buf_used = used;
> > +     return err;
> > +}
> > +
> > +static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int *buf_used,
> > +                                     struct cscfg_fs_load_descs *desc_arrays,
> > +                                     const int feat_idx)
> > +{
> > +     struct cscfg_file_elem_header elem_hdr;
> > +     struct cscfg_file_elem_str elem_str;
> > +     struct cscfg_feature_desc *feat_desc;
> > +     struct cscfg_regval_desc *p_reg_desc;
> > +     int used = *buf_used, err, i, nr_regs_bytes;
> > +     u32 val32;
> > +
> > +     /* allocate the feature descriptor object */
> > +     feat_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_feature_desc),
> > +                              GFP_KERNEL);
> > +     if (!feat_desc)
> > +             return -ENOMEM;
> > +
> > +     /* read and check the element header */
> > +     err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> > +     if (err)
> > +             return err;
> > +
> > +     if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> > +             return -EINVAL;
> > +
> > +     /* read the feature name */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     feat_desc->name = elem_str.str;
> > +
> > +     /* read the description string */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     feat_desc->description = elem_str.str;
> > +
> > +     /*
> > +      * read in some values
> > +      * [u32 value: match_flags]
> > +      * [u16 value: nr_regs]     - number of registers.
> > +      * [u16 value: nr_params]   - number of parameters.
> > +      */
> > +     cscfg_extract_u32(feat_desc->match_flags);
> > +     cscfg_extract_u16(feat_desc->nr_regs);
> > +     cscfg_extract_u16(feat_desc->nr_params);
> > +
> > +     /* register descriptors  - 32 bit + 64 bit value */
> > +     if (feat_desc->nr_regs) {
> > +             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);
> > +             if (!feat_desc->regs_desc)
> > +                     return -ENOMEM;
> > +
> > +             for (i = 0; i < feat_desc->nr_regs; i++) {
> > +                     cscfg_extract_u32(val32);
> > +                     p_reg_desc = (struct cscfg_regval_desc *)&feat_desc->regs_desc[i];
> > +                     CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_reg_desc);
> > +                     cscfg_extract_u64(feat_desc->regs_desc[i].val64);
> > +             }
> > +     }
> > +
> > +     /* 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);
> > +             if (!feat_desc->params_desc)
> > +                     return -ENOMEM;
> > +             for (i = 0; i < feat_desc->nr_params; i++) {
> > +                     err = cscfg_file_read_elem_param(buffer, buflen, &used,
> > +                                                      &feat_desc->params_desc[i]);
> > +                     if (err)
> > +                             return err;
> > +             }
> > +     }
> > +
> > +     desc_arrays->feat_descs[feat_idx] = feat_desc;
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +/* just read the feature name - if there is a feature at this position */
> > +static int cscfg_file_read_elem_feat_name(const u8 *buffer, const int buflen, int *buf_used,
> > +                                       struct cscfg_file_elem_str *elem_str)
> > +{
> > +     struct cscfg_file_elem_header elem_hdr;
> > +     int used = *buf_used;
> > +     int err;
> > +
> > +     elem_str->str_len = 0;
> > +     /*
> > +      * read the header - if not config, then don't update buf_used
> > +      * pointer on return
> > +      */
> > +     err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> > +     if (err)
> > +             return err;
> > +     if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> > +             return -EINVAL;
> > +
> > +     /* read the feature name */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> > +     if (err)
> > +             return err;
> > +     *buf_used = used;
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Read a buffer and create the configuration and feature
> > + * descriptors to load into the cscfg system
> > + */
> > +int cscfg_file_read_buffer(const u8 *buffer, const int buflen,
> > +                        struct cscfg_fs_load_descs *desc_arrays)
> > +{
> > +     struct cscfg_file_header hdr;
> > +     int used = 0, err, i;
> > +
> > +     /* read in the file header */
> > +     err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> > +     if (err)
> > +             return err;
> > +
> > +     /* allocate the memory for the descriptor pointer arrays */
> > +     err = cscfg_file_alloc_desc_arrays(desc_arrays, hdr.nr_features);
> > +     if (err)
> > +             return err;
> > +
> > +     /* read elements */
> > +
> > +     /* first element could be a config so check */
> > +     err = cscfg_file_read_elem_config(buffer, buflen, &used, desc_arrays);
> > +     if (err)
> > +             return err;
> > +
> > +     /* now read and populate all the feature descriptors */
> > +     for (i = 0; i < hdr.nr_features; i++) {
> > +             err = cscfg_file_read_elem_feature(buffer, buflen, &used, desc_arrays, i);
> > +             if (err)
> > +                     return err;
> > +     }
> > +     return used;
> > +}
> > +
> > +int cscfg_file_read_buffer_first_name(const u8 *buffer, const int buflen,
> > +                                   const char **name)
> > +{
> > +     struct cscfg_file_header hdr;
> > +     struct cscfg_file_elem_str elem_str;
> > +     int used = 0, err = 0;
> > +
> > +     *name = NULL;
> > +
> > +     /* read in the file header */
> > +     err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> > +     if (err)
> > +             return err;
> > +
> > +     err = cscfg_file_read_elem_config_name(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +
> > +     /* no config string - get first feature name */
> > +     if (!elem_str.str_len) {
> > +             err = cscfg_file_read_elem_feat_name(buffer, buflen, &used, &elem_str);
> > +             if (err)
> > +                     return err;
> > +     }
> > +     if (elem_str.str_len)
> > +             *name = elem_str.str;
> > +     return err;
> > +}
> > diff --git a/drivers/hwtracing/coresight/coresight-config-file.h b/drivers/hwtracing/coresight/coresight-config-file.h
> > new file mode 100644
> > index 000000000000..591f4c2c4be9
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-config-file.h
> > @@ -0,0 +1,117 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> > + * Author: Mike Leach <[e-mail]>
> > + */
> > +
> > +#ifndef _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> > +#define _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> > +
> > +/*
> > + * Structures to represent configuration descriptors in a memory buffer
> > + * to serialise to and from files
> > + *
> > + * File structure - for loading a configuration + features
> > + * from configfs.
> > + *
> > + * [cscfg_file_header]       - mandatory
> > + * [CONFIG_ELEM]     - optional - only one permitted,
> > + * [FEATURE_ELEM] * [cscfg_file_header.nr_features]
> > + *                   - optional - file valid with config only.
> > + *
> > + * Invalid file if no config or features.
> > + *
> > + *
> > + * File structure for [CONFIG_ELEM]:
> > + *
> > + * [cscfg_file_elem_header] - header length value to end of feature strings.
> > + * [cscfg_file_elem_str]    - name of the configuration
> > + * [cscfg_file_elem_str]    - description of configuration
> > + * [u16 value - nr_presets]
> > + * [u32 value - nr_total_params]
> > + * [u16 value - nr_feat_refs]
> > + * [u64 values] * (nr_presets * nr_total_params)
> > + * [cscfg_file_elem_str] * nr_feat_refs
> > + *
> > + * Only one configuration per file.
> > + *
> > + * File structure for a [FEATURE_ELEM]
> > + *
> > + * [cscfg_file_elem_header] - header length is total bytes to end of param structures.
> > + * [cscfg_file_elem_str]    - feature name.
> > + * [cscfg_file_elem_str]    - feature description.
> > + * [u32 value: match_flags]
> > + * [u16 value: nr_regs]          - number of registers.
> > + * [u16 value: nr_params]   - number of parameters.
> > + * [cscfg_regval_desc struct] * nr_regs
> > + * [PARAM_ELEM] * nr_params
> > + *
> > + * File structure for [PARAM_ELEM]
> > + *
> > + * [cscfg_file_elem_str]    - parameter name.
> > + * [u64 value: param_value] - initial value.
> > + */
> > +
> > +/* major element types - configurations and features */
> > +
> > +#define CSCFG_FILE_ELEM_TYPE_FEAT    0x1
> > +#define CSCFG_FILE_ELEM_TYPE_CFG     0x2
> > +
> > +#define CSCFG_FILE_MAGIC_VERSION     0xC5CF0001
> > +
> > +#define CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_desc) \
> > +     { \
> > +     p_desc->type = (val32 >> 24) & 0xFF; \
> > +     p_desc->offset = (val32 >> 12) & 0xFFF; \
> > +     p_desc->hw_info = val32 & 0xFFF; \
> > +     }
> > +
> > +#define CSCFG_FILE_REG_DESC_INFO_TO_U32(val32, p_desc) \
> > +     { \
> > +     val32 = p_desc->hw_info & 0xFFF; \
> > +     val32 |= ((p_desc->offset & 0xFFF) << 12); \
> > +     val32 |= ((p_desc->type & 0xFF) << 24); \
> > +     }
> > +
> > +/* binary attributes in configfs need a max size - declare a value for this. */
> > +#define CSCFG_FILE_MAXSIZE   16384
> > +
> > +/* limit string sizes */
> > +#define CSCFG_FILE_STR_MAXSIZE       1024
> > +
> > +/**
> > + * file header.
> > + *
> > + * @magic_version: magic number / version for file/buffer format.
> > + * @length       : total length of all data in the buffer.
> > + * @nr_features  : total number of features in the buffer.
> > + */
> > +struct cscfg_file_header {
> > +     u32 magic_version;
> > +     u16 length;
> > +     u16 nr_features;
> > +};
> > +
> > +/**
> > + * element header
> > + *
> > + * @elem_length: total length of this element
> > + * @elem_type  : type of this element - one of CSCFG_FILE_ELEM_TYPE.. defines.
> > + */
> > +struct cscfg_file_elem_header {
> > +     u16 elem_length;
> > +     u8 elem_type;
> > +};
> > +
> > +/**
> > + * string file element.
> > + *
> > + * @str_len: length of string buffer including 0 terminator
> > + * @str    : string buffer - 0 terminated.
> > + */
> > +struct cscfg_file_elem_str {
> > +     u16 str_len;
> > +     char *str;
> > +};
> > +
> > +#endif /* _CORESIGHT_CORESIGHT_CONFIG_FILE_H */
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index 2e1670523461..9cd3c26ce023 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> > @@ -85,6 +85,21 @@ struct cscfg_regval_desc {
> >       };
> >  };
> >
> > +/**
> > + * Dynamically loaded descriptor arrays loaded via configfs.
> > + *
> > + * For builtin or module loaded configurations / features these are
> > + * statically defined at compile time. For configfs we create the arrays
> > + * dynamically so need a structure to handle this.
> > + *
> > + * @config_descs:    array of config descriptor pointers.
> > + * @feat_descs:              array of feature descriptor pointers.
> > + */
> > +struct cscfg_fs_load_descs {
> > +     struct cscfg_config_desc **config_descs;
> > +     struct cscfg_feature_desc **feat_descs;
> > +};
> > +
> >  /**
> >   * Device feature descriptor - combination of registers and parameters to
> >   * program a device to implement a specific complex function.
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> > index 9106ffab4833..6a6e33585be9 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> > @@ -66,6 +66,7 @@ struct cscfg_registered_csdev {
> >  enum cscfg_load_owner_type {
> >       CSCFG_OWNER_PRELOAD,
> >       CSCFG_OWNER_MODULE,
> > +     CSCFG_OWNER_CONFIGFS,
> >  };
> >
> >  /**
> > --
> > 2.17.1
> >



--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Mathieu Poirier May 25, 2022, 5:38 p.m. UTC | #4

Good day,

I have resumed working on this patchset.  More comments below as I went back
while looking at other patches...

On Thu, Apr 14, 2022 at 07:44:53AM +0100, Mike Leach wrote:
> Add in functionality to allow load via configfs.
> 
> define a binary file format and provide a reader for that format
> that will create and populate configuration and feature structures
> use by the driver infrastructure.
> 
> Signed-off-by: Mike Leach <[e-mail]>
> ---
>  drivers/hwtracing/coresight/Makefile          |   2 +-
>  .../coresight/coresight-config-file.c         | 432 ++++++++++++++++++
>  .../coresight/coresight-config-file.h         | 117 +++++
>  .../hwtracing/coresight/coresight-config.h    |  15 +
>  .../hwtracing/coresight/coresight-syscfg.h    |   1 +
>  5 files changed, 566 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.c
>  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.h
> 
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index b6c4a48140ec..5de2bb79f4ac 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -6,7 +6,7 @@ obj-$(CONFIG_CORESIGHT) += coresight.o
>  coresight-y := coresight-core.o  coresight-etm-perf.o coresight-platform.o \
>  		coresight-sysfs.o coresight-syscfg.o coresight-config.o \
>  		coresight-cfg-preload.o coresight-cfg-afdo.o \
> -		coresight-syscfg-configfs.o
> +		coresight-syscfg-configfs.o coresight-config-file.o
>  obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
>  coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
>  		      coresight-tmc-etr.o
> diff --git a/drivers/hwtracing/coresight/coresight-config-file.c b/drivers/hwtracing/coresight/coresight-config-file.c
> new file mode 100644
> index 000000000000..3fd001938324
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-config-file.c
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Linaro Limited, All rights reserved.

I can't remember if I commented on that before but this should likely be 2022,
and for all the other files introduced by this patchset.

> + * Author: Mike Leach <[e-mail]>
> + */
> +
> +#include "coresight-config.h"
> +#include "coresight-config-file.h"
> +#include "coresight-syscfg.h"
> +
> +#define cscfg_extract_u64(val64) {	\
> +	val64 = *(u64 *)(buffer + used); \
> +	used += sizeof(u64); \
> +	}
> +
> +#define cscfg_extract_u32(val32) { \
> +	val32 = *(u32 *)(buffer + used); \
> +	used += sizeof(u32); \
> +	}
> +
> +#define cscfg_extract_u16(val16) { \
> +	val16 = *(u16 *)(buffer + used); \
> +	used += sizeof(u16); \
> +	}
> +
> +#define cscfg_extract_u8(val8) {	\
> +	val8 = *(buffer + used); \
> +	used++; \
> +	}
> +
> +static int cscfg_file_read_hdr(const u8 *buffer, const int buflen, int *buf_used,
> +			       struct cscfg_file_header *hdr)
> +{
> +	/* file header always at the start of the buffer */
> +	int used = 0;
> +
> +	if (buflen < sizeof(struct cscfg_file_header))
> +		return -EINVAL;
> +
> +	cscfg_extract_u32(hdr->magic_version);
> +	if (hdr->magic_version != CSCFG_FILE_MAGIC_VERSION)
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(hdr->length);
> +	if (hdr->length > buflen)
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(hdr->nr_features);
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_hdr(const u8 *buffer, const int buflen, int *buf_used,
> +				    struct cscfg_file_elem_header *elem_hdr)
> +{
> +	int used = *buf_used;
> +
> +	if ((buflen - used) < (sizeof(u16) + sizeof(u8)))
> +		return -EINVAL;
> +
> +	/* read length and check enough buffer remains for this element */
> +	elem_hdr->elem_length = *(u16 *)(buffer + used);
> +	if ((buflen - used) < elem_hdr->elem_length)
> +		return -EINVAL;
> +	/* don't use extract fn as we update used _after_ the comparison */
> +	used += sizeof(u16);
> +
> +	/* read type and validate */
> +	cscfg_extract_u8(elem_hdr->elem_type);
> +	if ((elem_hdr->elem_type < CSCFG_FILE_ELEM_TYPE_FEAT) ||
> +	    (elem_hdr->elem_type > CSCFG_FILE_ELEM_TYPE_CFG))
> +		return -EINVAL;
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf_used,
> +				    struct cscfg_file_elem_str *elem_str)
> +{
> +	int used = *buf_used;
> +
> +	if ((buflen - used) < sizeof(u16))
> +		return -EINVAL;
> +
> +	cscfg_extract_u16(elem_str->str_len);
> +
> +	if ((buflen - used) < elem_str->str_len)
> +		return -EINVAL;
> +
> +	/* check for 0 termination */
> +	if (buffer[elem_str->str_len - 1] != 0)
> +		return -EINVAL;
> +
> +	elem_str->str = devm_kstrdup(cscfg_device(), (char *)buffer, GFP_KERNEL);
> +	used += elem_str->str_len;
> +
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +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);
> +	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);
> +	if (!desc_arrays->feat_descs)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int *buf_used,
> +				       struct cscfg_fs_load_descs *desc_arrays)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	struct cscfg_config_desc *config_desc;
> +	int used = *buf_used, nr_preset_vals, nr_preset_bytes, i;
> +	int err = 0;
> +	u64 *presets;
> +
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> +		return 0;
> +
> +	/* we have a config - allocate the descriptor */
> +	config_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_config_desc),
> +				   GFP_KERNEL);
> +	if (!config_desc)
> +		return -ENOMEM;
> +
> +	/* read the name string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	config_desc->name = elem_str.str;
> +
> +	/* read the description string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	config_desc->description = elem_str.str;
> +
> +	/* read in some values */
> +	if ((buflen - used) < sizeof(u64))
> +		return -EINVAL;
> +	cscfg_extract_u16(config_desc->nr_presets);
> +	cscfg_extract_u32(config_desc->nr_total_params);
> +	cscfg_extract_u16(config_desc->nr_feat_refs);
> +
> +	/* 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);
> +		if (!presets)
> +			return -ENOMEM;
> +
> +		nr_preset_bytes = sizeof(u64) * nr_preset_vals;
> +		if ((buflen - used) < nr_preset_bytes)
> +			return -EINVAL;
> +
> +		memcpy(presets, (buffer + used), nr_preset_bytes);
> +		config_desc->presets = presets;
> +		used += nr_preset_bytes;
> +	}
> +
> +	/* 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);
> +		if (!config_desc->feat_ref_names)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < config_desc->nr_feat_refs; i++) {
> +			err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +			if (err)
> +				return err;
> +			config_desc->feat_ref_names[i] = elem_str.str;
> +		}
> +	}
> +
> +	desc_arrays->config_descs[0] = config_desc;
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +/* just read the config name - if there is a config at this position */
> +static int cscfg_file_read_elem_config_name(const u8 *buffer, const int buflen, int *buf_used,
> +					    struct cscfg_file_elem_str *elem_str)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	int used = *buf_used;
> +	int err;
> +
> +	elem_str->str_len = 0;
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> +		return 0;
> +
> +	/* read the name string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> +	if (err)
> +		return err;
> +	*buf_used = used;
> +
> +	return 0;
> +}
> +
> +static int cscfg_file_read_elem_param(const u8 *buffer, const int buflen, int *buf_used,
> +				      struct cscfg_parameter_desc *param_desc)
> +{
> +	struct cscfg_file_elem_str elem_str;
> +	int err = 0, used = *buf_used;
> +
> +	/* parameter name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	param_desc->name = elem_str.str;
> +
> +	/* parameter value */
> +	if ((buflen - used) < sizeof(u64))
> +		return -EINVAL;
> +	cscfg_extract_u64(param_desc->value);
> +
> +	*buf_used = used;
> +	return err;
> +}
> +
> +static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int *buf_used,
> +					struct cscfg_fs_load_descs *desc_arrays,
> +					const int feat_idx)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	struct cscfg_feature_desc *feat_desc;
> +	struct cscfg_regval_desc *p_reg_desc;
> +	int used = *buf_used, err, i, nr_regs_bytes;
> +	u32 val32;
> +
> +	/* allocate the feature descriptor object */
> +	feat_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_feature_desc),
> +				 GFP_KERNEL);
> +	if (!feat_desc)
> +		return -ENOMEM;
> +
> +	/* read and check the element header */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> +		return -EINVAL;
> +
> +	/* read the feature name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	feat_desc->name = elem_str.str;
> +
> +	/* read the description string */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +	feat_desc->description = elem_str.str;
> +
> +	/*
> +	 * read in some values
> +	 * [u32 value: match_flags]
> +	 * [u16 value: nr_regs]	    - number of registers.
> +	 * [u16 value: nr_params]   - number of parameters.
> +	 */
> +	cscfg_extract_u32(feat_desc->match_flags);
> +	cscfg_extract_u16(feat_desc->nr_regs);
> +	cscfg_extract_u16(feat_desc->nr_params);
> +
> +	/* register descriptors  - 32 bit + 64 bit value */
> +	if (feat_desc->nr_regs) {
> +		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);
> +		if (!feat_desc->regs_desc)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < feat_desc->nr_regs; i++) {
> +			cscfg_extract_u32(val32);
> +			p_reg_desc = (struct cscfg_regval_desc *)&feat_desc->regs_desc[i];
> +			CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_reg_desc);
> +			cscfg_extract_u64(feat_desc->regs_desc[i].val64);
> +		}
> +	}
> +
> +	/* 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);
> +		if (!feat_desc->params_desc)
> +			return -ENOMEM;
> +		for (i = 0; i < feat_desc->nr_params; i++) {
> +			err = cscfg_file_read_elem_param(buffer, buflen, &used,
> +							 &feat_desc->params_desc[i]);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	desc_arrays->feat_descs[feat_idx] = feat_desc;
> +	*buf_used = used;
> +	return 0;
> +}
> +
> +/* just read the feature name - if there is a feature at this position */
> +static int cscfg_file_read_elem_feat_name(const u8 *buffer, const int buflen, int *buf_used,
> +					  struct cscfg_file_elem_str *elem_str)
> +{
> +	struct cscfg_file_elem_header elem_hdr;
> +	int used = *buf_used;
> +	int err;
> +
> +	elem_str->str_len = 0;
> +	/*
> +	 * read the header - if not config, then don't update buf_used
> +	 * pointer on return
> +	 */
> +	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> +	if (err)
> +		return err;
> +	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> +		return -EINVAL;
> +
> +	/* read the feature name */
> +	err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> +	if (err)
> +		return err;
> +	*buf_used = used;
> +
> +	return 0;
> +}
> +
> +/*
> + * Read a buffer and create the configuration and feature
> + * descriptors to load into the cscfg system
> + */
> +int cscfg_file_read_buffer(const u8 *buffer, const int buflen,
> +			   struct cscfg_fs_load_descs *desc_arrays)
> +{
> +	struct cscfg_file_header hdr;
> +	int used = 0, err, i;
> +
> +	/* read in the file header */
> +	err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> +	if (err)
> +		return err;
> +
> +	/* allocate the memory for the descriptor pointer arrays */
> +	err = cscfg_file_alloc_desc_arrays(desc_arrays, hdr.nr_features);
> +	if (err)
> +		return err;
> +
> +	/* read elements */
> +
> +	/* first element could be a config so check */
> +	err = cscfg_file_read_elem_config(buffer, buflen, &used, desc_arrays);
> +	if (err)
> +		return err;
> +
> +	/* now read and populate all the feature descriptors */
> +	for (i = 0; i < hdr.nr_features; i++) {
> +		err = cscfg_file_read_elem_feature(buffer, buflen, &used, desc_arrays, i);
> +		if (err)
> +			return err;
> +	}
> +	return used;
> +}
> +
> +int cscfg_file_read_buffer_first_name(const u8 *buffer, const int buflen,
> +				      const char **name)
> +{
> +	struct cscfg_file_header hdr;
> +	struct cscfg_file_elem_str elem_str;
> +	int used = 0, err = 0;
> +
> +	*name = NULL;
> +
> +	/* read in the file header */
> +	err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> +	if (err)
> +		return err;
> +
> +	err = cscfg_file_read_elem_config_name(buffer, buflen, &used, &elem_str);
> +	if (err)
> +		return err;
> +
> +	/* no config string - get first feature name */
> +	if (!elem_str.str_len) {
> +		err = cscfg_file_read_elem_feat_name(buffer, buflen, &used, &elem_str);
> +		if (err)
> +			return err;
> +	}
> +	if (elem_str.str_len)
> +		*name = elem_str.str;
> +	return err;
> +}
> diff --git a/drivers/hwtracing/coresight/coresight-config-file.h b/drivers/hwtracing/coresight/coresight-config-file.h
> new file mode 100644
> index 000000000000..591f4c2c4be9
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-config-file.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> + * Author: Mike Leach <[e-mail]>
> + */
> +
> +#ifndef _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> +#define _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> +
> +/*
> + * Structures to represent configuration descriptors in a memory buffer
> + * to serialise to and from files
> + *
> + * File structure - for loading a configuration + features
> + * from configfs.
> + *
> + * [cscfg_file_header]	- mandatory
> + * [CONFIG_ELEM]	- optional - only one permitted,
> + * [FEATURE_ELEM] * [cscfg_file_header.nr_features]
> + *			- optional - file valid with config only.
> + *
> + * Invalid file if no config or features.
> + *
> + *
> + * File structure for [CONFIG_ELEM]:
> + *
> + * [cscfg_file_elem_header] - header length value to end of feature strings.
> + * [cscfg_file_elem_str]    - name of the configuration
> + * [cscfg_file_elem_str]    - description of configuration
> + * [u16 value - nr_presets]
> + * [u32 value - nr_total_params]
> + * [u16 value - nr_feat_refs]
> + * [u64 values] * (nr_presets * nr_total_params)
> + * [cscfg_file_elem_str] * nr_feat_refs
> + *
> + * Only one configuration per file.
> + *
> + * File structure for a [FEATURE_ELEM]
> + *
> + * [cscfg_file_elem_header] - header length is total bytes to end of param structures.
> + * [cscfg_file_elem_str]    - feature name.
> + * [cscfg_file_elem_str]    - feature description.
> + * [u32 value: match_flags]
> + * [u16 value: nr_regs]	    - number of registers.
> + * [u16 value: nr_params]   - number of parameters.
> + * [cscfg_regval_desc struct] * nr_regs
> + * [PARAM_ELEM] * nr_params
> + *
> + * File structure for [PARAM_ELEM]
> + *
> + * [cscfg_file_elem_str]    - parameter name.
> + * [u64 value: param_value] - initial value.
> + */
> +
> +/* major element types - configurations and features */
> +
> +#define CSCFG_FILE_ELEM_TYPE_FEAT	0x1
> +#define CSCFG_FILE_ELEM_TYPE_CFG	0x2
> +
> +#define CSCFG_FILE_MAGIC_VERSION	0xC5CF0001
> +
> +#define CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_desc) \
> +	{ \
> +	p_desc->type = (val32 >> 24) & 0xFF; \
> +	p_desc->offset = (val32 >> 12) & 0xFFF; \
> +	p_desc->hw_info = val32 & 0xFFF; \
> +	}
> +
> +#define CSCFG_FILE_REG_DESC_INFO_TO_U32(val32, p_desc) \
> +	{ \
> +	val32 = p_desc->hw_info & 0xFFF; \
> +	val32 |= ((p_desc->offset & 0xFFF) << 12); \
> +	val32 |= ((p_desc->type & 0xFF) << 24); \
> +	}
> +
> +/* binary attributes in configfs need a max size - declare a value for this. */
> +#define CSCFG_FILE_MAXSIZE	16384
> +

s/16384/SZ_16K

Also, please enhance the comment to explain the 16K limit.


> +/* limit string sizes */
> +#define CSCFG_FILE_STR_MAXSIZE	1024
> +

s/1024/SZ_1K

Thanks,
Mathieu

> +/**
> + * file header.
> + *
> + * @magic_version: magic number / version for file/buffer format.
> + * @length       : total length of all data in the buffer.
> + * @nr_features  : total number of features in the buffer.
> + */
> +struct cscfg_file_header {
> +	u32 magic_version;
> +	u16 length;
> +	u16 nr_features;
> +};
> +
> +/**
> + * element header
> + *
> + * @elem_length: total length of this element
> + * @elem_type  : type of this element - one of CSCFG_FILE_ELEM_TYPE.. defines.
> + */
> +struct cscfg_file_elem_header {
> +	u16 elem_length;
> +	u8 elem_type;
> +};
> +
> +/**
> + * string file element.
> + *
> + * @str_len: length of string buffer including 0 terminator
> + * @str    : string buffer - 0 terminated.
> + */
> +struct cscfg_file_elem_str {
> +	u16 str_len;
> +	char *str;
> +};
> +
> +#endif /* _CORESIGHT_CORESIGHT_CONFIG_FILE_H */
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index 2e1670523461..9cd3c26ce023 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -85,6 +85,21 @@ struct cscfg_regval_desc {
>  	};
>  };
>  
> +/**
> + * Dynamically loaded descriptor arrays loaded via configfs.
> + *
> + * For builtin or module loaded configurations / features these are
> + * statically defined at compile time. For configfs we create the arrays
> + * dynamically so need a structure to handle this.
> + *
> + * @config_descs:	array of config descriptor pointers.
> + * @feat_descs:		array of feature descriptor pointers.
> + */
> +struct cscfg_fs_load_descs {
> +	struct cscfg_config_desc **config_descs;
> +	struct cscfg_feature_desc **feat_descs;
> +};
> +
>  /**
>   * Device feature descriptor - combination of registers and parameters to
>   * program a device to implement a specific complex function.
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index 9106ffab4833..6a6e33585be9 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -66,6 +66,7 @@ struct cscfg_registered_csdev {
>  enum cscfg_load_owner_type {
>  	CSCFG_OWNER_PRELOAD,
>  	CSCFG_OWNER_MODULE,
> +	CSCFG_OWNER_CONFIGFS,
>  };
>  
>  /**
> -- 
> 2.17.1
>

Mike Leach June 1, 2022, 8:32 a.m. UTC | #5

Hi,

On Wed, 25 May 2022 at 18:38, Mathieu Poirier
<[e-mail]> wrote:
>
> Good day,
>
> I have resumed working on this patchset.  More comments below as I went back
> while looking at other patches...
>
> On Thu, Apr 14, 2022 at 07:44:53AM +0100, Mike Leach wrote:
> > Add in functionality to allow load via configfs.
> >
> > define a binary file format and provide a reader for that format
> > that will create and populate configuration and feature structures
> > use by the driver infrastructure.
> >
> > Signed-off-by: Mike Leach <[e-mail]>
> > ---
> >  drivers/hwtracing/coresight/Makefile          |   2 +-
> >  .../coresight/coresight-config-file.c         | 432 ++++++++++++++++++
> >  .../coresight/coresight-config-file.h         | 117 +++++
> >  .../hwtracing/coresight/coresight-config.h    |  15 +
> >  .../hwtracing/coresight/coresight-syscfg.h    |   1 +
> >  5 files changed, 566 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.c
> >  create mode 100644 drivers/hwtracing/coresight/coresight-config-file.h
> >
> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> > index b6c4a48140ec..5de2bb79f4ac 100644
> > --- a/drivers/hwtracing/coresight/Makefile
> > +++ b/drivers/hwtracing/coresight/Makefile
> > @@ -6,7 +6,7 @@ obj-$(CONFIG_CORESIGHT) += coresight.o
> >  coresight-y := coresight-core.o  coresight-etm-perf.o coresight-platform.o \
> >               coresight-sysfs.o coresight-syscfg.o coresight-config.o \
> >               coresight-cfg-preload.o coresight-cfg-afdo.o \
> > -             coresight-syscfg-configfs.o
> > +             coresight-syscfg-configfs.o coresight-config-file.o
> >  obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
> >  coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
> >                     coresight-tmc-etr.o
> > diff --git a/drivers/hwtracing/coresight/coresight-config-file.c b/drivers/hwtracing/coresight/coresight-config-file.c
> > new file mode 100644
> > index 000000000000..3fd001938324
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-config-file.c
> > @@ -0,0 +1,432 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020 Linaro Limited, All rights reserved.
>
> I can't remember if I commented on that before but this should likely be 2022,
> and for all the other files introduced by this patchset.
>
> > + * Author: Mike Leach <[e-mail]>
> > + */
> > +
> > +#include "coresight-config.h"
> > +#include "coresight-config-file.h"
> > +#include "coresight-syscfg.h"
> > +
> > +#define cscfg_extract_u64(val64) {   \
> > +     val64 = *(u64 *)(buffer + used); \
> > +     used += sizeof(u64); \
> > +     }
> > +
> > +#define cscfg_extract_u32(val32) { \
> > +     val32 = *(u32 *)(buffer + used); \
> > +     used += sizeof(u32); \
> > +     }
> > +
> > +#define cscfg_extract_u16(val16) { \
> > +     val16 = *(u16 *)(buffer + used); \
> > +     used += sizeof(u16); \
> > +     }
> > +
> > +#define cscfg_extract_u8(val8) {     \
> > +     val8 = *(buffer + used); \
> > +     used++; \
> > +     }
> > +
> > +static int cscfg_file_read_hdr(const u8 *buffer, const int buflen, int *buf_used,
> > +                            struct cscfg_file_header *hdr)
> > +{
> > +     /* file header always at the start of the buffer */
> > +     int used = 0;
> > +
> > +     if (buflen < sizeof(struct cscfg_file_header))
> > +             return -EINVAL;
> > +
> > +     cscfg_extract_u32(hdr->magic_version);
> > +     if (hdr->magic_version != CSCFG_FILE_MAGIC_VERSION)
> > +             return -EINVAL;
> > +
> > +     cscfg_extract_u16(hdr->length);
> > +     if (hdr->length > buflen)
> > +             return -EINVAL;
> > +
> > +     cscfg_extract_u16(hdr->nr_features);
> > +
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +static int cscfg_file_read_elem_hdr(const u8 *buffer, const int buflen, int *buf_used,
> > +                                 struct cscfg_file_elem_header *elem_hdr)
> > +{
> > +     int used = *buf_used;
> > +
> > +     if ((buflen - used) < (sizeof(u16) + sizeof(u8)))
> > +             return -EINVAL;
> > +
> > +     /* read length and check enough buffer remains for this element */
> > +     elem_hdr->elem_length = *(u16 *)(buffer + used);
> > +     if ((buflen - used) < elem_hdr->elem_length)
> > +             return -EINVAL;
> > +     /* don't use extract fn as we update used _after_ the comparison */
> > +     used += sizeof(u16);
> > +
> > +     /* read type and validate */
> > +     cscfg_extract_u8(elem_hdr->elem_type);
> > +     if ((elem_hdr->elem_type < CSCFG_FILE_ELEM_TYPE_FEAT) ||
> > +         (elem_hdr->elem_type > CSCFG_FILE_ELEM_TYPE_CFG))
> > +             return -EINVAL;
> > +
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf_used,
> > +                                 struct cscfg_file_elem_str *elem_str)
> > +{
> > +     int used = *buf_used;
> > +
> > +     if ((buflen - used) < sizeof(u16))
> > +             return -EINVAL;
> > +
> > +     cscfg_extract_u16(elem_str->str_len);
> > +
> > +     if ((buflen - used) < elem_str->str_len)
> > +             return -EINVAL;
> > +
> > +     /* check for 0 termination */
> > +     if (buffer[elem_str->str_len - 1] != 0)
> > +             return -EINVAL;
> > +
> > +     elem_str->str = devm_kstrdup(cscfg_device(), (char *)buffer, GFP_KERNEL);
> > +     used += elem_str->str_len;
> > +
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +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);
> > +     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);
> > +     if (!desc_arrays->feat_descs)
> > +             return -ENOMEM;
> > +     return 0;
> > +}
> > +
> > +static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int *buf_used,
> > +                                    struct cscfg_fs_load_descs *desc_arrays)
> > +{
> > +     struct cscfg_file_elem_header elem_hdr;
> > +     struct cscfg_file_elem_str elem_str;
> > +     struct cscfg_config_desc *config_desc;
> > +     int used = *buf_used, nr_preset_vals, nr_preset_bytes, i;
> > +     int err = 0;
> > +     u64 *presets;
> > +
> > +     /*
> > +      * read the header - if not config, then don't update buf_used
> > +      * pointer on return
> > +      */
> > +     err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> > +     if (err)
> > +             return err;
> > +     if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> > +             return 0;
> > +
> > +     /* we have a config - allocate the descriptor */
> > +     config_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_config_desc),
> > +                                GFP_KERNEL);
> > +     if (!config_desc)
> > +             return -ENOMEM;
> > +
> > +     /* read the name string */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     config_desc->name = elem_str.str;
> > +
> > +     /* read the description string */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     config_desc->description = elem_str.str;
> > +
> > +     /* read in some values */
> > +     if ((buflen - used) < sizeof(u64))
> > +             return -EINVAL;
> > +     cscfg_extract_u16(config_desc->nr_presets);
> > +     cscfg_extract_u32(config_desc->nr_total_params);
> > +     cscfg_extract_u16(config_desc->nr_feat_refs);
> > +
> > +     /* 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);
> > +             if (!presets)
> > +                     return -ENOMEM;
> > +
> > +             nr_preset_bytes = sizeof(u64) * nr_preset_vals;
> > +             if ((buflen - used) < nr_preset_bytes)
> > +                     return -EINVAL;
> > +
> > +             memcpy(presets, (buffer + used), nr_preset_bytes);
> > +             config_desc->presets = presets;
> > +             used += nr_preset_bytes;
> > +     }
> > +
> > +     /* 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);
> > +             if (!config_desc->feat_ref_names)
> > +                     return -ENOMEM;
> > +
> > +             for (i = 0; i < config_desc->nr_feat_refs; i++) {
> > +                     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +                     if (err)
> > +                             return err;
> > +                     config_desc->feat_ref_names[i] = elem_str.str;
> > +             }
> > +     }
> > +
> > +     desc_arrays->config_descs[0] = config_desc;
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +/* just read the config name - if there is a config at this position */
> > +static int cscfg_file_read_elem_config_name(const u8 *buffer, const int buflen, int *buf_used,
> > +                                         struct cscfg_file_elem_str *elem_str)
> > +{
> > +     struct cscfg_file_elem_header elem_hdr;
> > +     int used = *buf_used;
> > +     int err;
> > +
> > +     elem_str->str_len = 0;
> > +     /*
> > +      * read the header - if not config, then don't update buf_used
> > +      * pointer on return
> > +      */
> > +     err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> > +     if (err)
> > +             return err;
> > +     if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
> > +             return 0;
> > +
> > +     /* read the name string */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> > +     if (err)
> > +             return err;
> > +     *buf_used = used;
> > +
> > +     return 0;
> > +}
> > +
> > +static int cscfg_file_read_elem_param(const u8 *buffer, const int buflen, int *buf_used,
> > +                                   struct cscfg_parameter_desc *param_desc)
> > +{
> > +     struct cscfg_file_elem_str elem_str;
> > +     int err = 0, used = *buf_used;
> > +
> > +     /* parameter name */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     param_desc->name = elem_str.str;
> > +
> > +     /* parameter value */
> > +     if ((buflen - used) < sizeof(u64))
> > +             return -EINVAL;
> > +     cscfg_extract_u64(param_desc->value);
> > +
> > +     *buf_used = used;
> > +     return err;
> > +}
> > +
> > +static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int *buf_used,
> > +                                     struct cscfg_fs_load_descs *desc_arrays,
> > +                                     const int feat_idx)
> > +{
> > +     struct cscfg_file_elem_header elem_hdr;
> > +     struct cscfg_file_elem_str elem_str;
> > +     struct cscfg_feature_desc *feat_desc;
> > +     struct cscfg_regval_desc *p_reg_desc;
> > +     int used = *buf_used, err, i, nr_regs_bytes;
> > +     u32 val32;
> > +
> > +     /* allocate the feature descriptor object */
> > +     feat_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_feature_desc),
> > +                              GFP_KERNEL);
> > +     if (!feat_desc)
> > +             return -ENOMEM;
> > +
> > +     /* read and check the element header */
> > +     err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> > +     if (err)
> > +             return err;
> > +
> > +     if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> > +             return -EINVAL;
> > +
> > +     /* read the feature name */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     feat_desc->name = elem_str.str;
> > +
> > +     /* read the description string */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +     feat_desc->description = elem_str.str;
> > +
> > +     /*
> > +      * read in some values
> > +      * [u32 value: match_flags]
> > +      * [u16 value: nr_regs]     - number of registers.
> > +      * [u16 value: nr_params]   - number of parameters.
> > +      */
> > +     cscfg_extract_u32(feat_desc->match_flags);
> > +     cscfg_extract_u16(feat_desc->nr_regs);
> > +     cscfg_extract_u16(feat_desc->nr_params);
> > +
> > +     /* register descriptors  - 32 bit + 64 bit value */
> > +     if (feat_desc->nr_regs) {
> > +             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);
> > +             if (!feat_desc->regs_desc)
> > +                     return -ENOMEM;
> > +
> > +             for (i = 0; i < feat_desc->nr_regs; i++) {
> > +                     cscfg_extract_u32(val32);
> > +                     p_reg_desc = (struct cscfg_regval_desc *)&feat_desc->regs_desc[i];
> > +                     CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_reg_desc);
> > +                     cscfg_extract_u64(feat_desc->regs_desc[i].val64);
> > +             }
> > +     }
> > +
> > +     /* 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);
> > +             if (!feat_desc->params_desc)
> > +                     return -ENOMEM;
> > +             for (i = 0; i < feat_desc->nr_params; i++) {
> > +                     err = cscfg_file_read_elem_param(buffer, buflen, &used,
> > +                                                      &feat_desc->params_desc[i]);
> > +                     if (err)
> > +                             return err;
> > +             }
> > +     }
> > +
> > +     desc_arrays->feat_descs[feat_idx] = feat_desc;
> > +     *buf_used = used;
> > +     return 0;
> > +}
> > +
> > +/* just read the feature name - if there is a feature at this position */
> > +static int cscfg_file_read_elem_feat_name(const u8 *buffer, const int buflen, int *buf_used,
> > +                                       struct cscfg_file_elem_str *elem_str)
> > +{
> > +     struct cscfg_file_elem_header elem_hdr;
> > +     int used = *buf_used;
> > +     int err;
> > +
> > +     elem_str->str_len = 0;
> > +     /*
> > +      * read the header - if not config, then don't update buf_used
> > +      * pointer on return
> > +      */
> > +     err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
> > +     if (err)
> > +             return err;
> > +     if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
> > +             return -EINVAL;
> > +
> > +     /* read the feature name */
> > +     err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
> > +     if (err)
> > +             return err;
> > +     *buf_used = used;
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Read a buffer and create the configuration and feature
> > + * descriptors to load into the cscfg system
> > + */
> > +int cscfg_file_read_buffer(const u8 *buffer, const int buflen,
> > +                        struct cscfg_fs_load_descs *desc_arrays)
> > +{
> > +     struct cscfg_file_header hdr;
> > +     int used = 0, err, i;
> > +
> > +     /* read in the file header */
> > +     err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> > +     if (err)
> > +             return err;
> > +
> > +     /* allocate the memory for the descriptor pointer arrays */
> > +     err = cscfg_file_alloc_desc_arrays(desc_arrays, hdr.nr_features);
> > +     if (err)
> > +             return err;
> > +
> > +     /* read elements */
> > +
> > +     /* first element could be a config so check */
> > +     err = cscfg_file_read_elem_config(buffer, buflen, &used, desc_arrays);
> > +     if (err)
> > +             return err;
> > +
> > +     /* now read and populate all the feature descriptors */
> > +     for (i = 0; i < hdr.nr_features; i++) {
> > +             err = cscfg_file_read_elem_feature(buffer, buflen, &used, desc_arrays, i);
> > +             if (err)
> > +                     return err;
> > +     }
> > +     return used;
> > +}
> > +
> > +int cscfg_file_read_buffer_first_name(const u8 *buffer, const int buflen,
> > +                                   const char **name)
> > +{
> > +     struct cscfg_file_header hdr;
> > +     struct cscfg_file_elem_str elem_str;
> > +     int used = 0, err = 0;
> > +
> > +     *name = NULL;
> > +
> > +     /* read in the file header */
> > +     err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
> > +     if (err)
> > +             return err;
> > +
> > +     err = cscfg_file_read_elem_config_name(buffer, buflen, &used, &elem_str);
> > +     if (err)
> > +             return err;
> > +
> > +     /* no config string - get first feature name */
> > +     if (!elem_str.str_len) {
> > +             err = cscfg_file_read_elem_feat_name(buffer, buflen, &used, &elem_str);
> > +             if (err)
> > +                     return err;
> > +     }
> > +     if (elem_str.str_len)
> > +             *name = elem_str.str;
> > +     return err;
> > +}
> > diff --git a/drivers/hwtracing/coresight/coresight-config-file.h b/drivers/hwtracing/coresight/coresight-config-file.h
> > new file mode 100644
> > index 000000000000..591f4c2c4be9
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-config-file.h
> > @@ -0,0 +1,117 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> > + * Author: Mike Leach <[e-mail]>
> > + */
> > +
> > +#ifndef _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> > +#define _CORESIGHT_CORESIGHT_CONFIG_FILE_H
> > +
> > +/*
> > + * Structures to represent configuration descriptors in a memory buffer
> > + * to serialise to and from files
> > + *
> > + * File structure - for loading a configuration + features
> > + * from configfs.
> > + *
> > + * [cscfg_file_header]       - mandatory
> > + * [CONFIG_ELEM]     - optional - only one permitted,
> > + * [FEATURE_ELEM] * [cscfg_file_header.nr_features]
> > + *                   - optional - file valid with config only.
> > + *
> > + * Invalid file if no config or features.
> > + *
> > + *
> > + * File structure for [CONFIG_ELEM]:
> > + *
> > + * [cscfg_file_elem_header] - header length value to end of feature strings.
> > + * [cscfg_file_elem_str]    - name of the configuration
> > + * [cscfg_file_elem_str]    - description of configuration
> > + * [u16 value - nr_presets]
> > + * [u32 value - nr_total_params]
> > + * [u16 value - nr_feat_refs]
> > + * [u64 values] * (nr_presets * nr_total_params)
> > + * [cscfg_file_elem_str] * nr_feat_refs
> > + *
> > + * Only one configuration per file.
> > + *
> > + * File structure for a [FEATURE_ELEM]
> > + *
> > + * [cscfg_file_elem_header] - header length is total bytes to end of param structures.
> > + * [cscfg_file_elem_str]    - feature name.
> > + * [cscfg_file_elem_str]    - feature description.
> > + * [u32 value: match_flags]
> > + * [u16 value: nr_regs]          - number of registers.
> > + * [u16 value: nr_params]   - number of parameters.
> > + * [cscfg_regval_desc struct] * nr_regs
> > + * [PARAM_ELEM] * nr_params
> > + *
> > + * File structure for [PARAM_ELEM]
> > + *
> > + * [cscfg_file_elem_str]    - parameter name.
> > + * [u64 value: param_value] - initial value.
> > + */
> > +
> > +/* major element types - configurations and features */
> > +
> > +#define CSCFG_FILE_ELEM_TYPE_FEAT    0x1
> > +#define CSCFG_FILE_ELEM_TYPE_CFG     0x2
> > +
> > +#define CSCFG_FILE_MAGIC_VERSION     0xC5CF0001
> > +
> > +#define CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_desc) \
> > +     { \
> > +     p_desc->type = (val32 >> 24) & 0xFF; \
> > +     p_desc->offset = (val32 >> 12) & 0xFFF; \
> > +     p_desc->hw_info = val32 & 0xFFF; \
> > +     }
> > +
> > +#define CSCFG_FILE_REG_DESC_INFO_TO_U32(val32, p_desc) \
> > +     { \
> > +     val32 = p_desc->hw_info & 0xFFF; \
> > +     val32 |= ((p_desc->offset & 0xFFF) << 12); \
> > +     val32 |= ((p_desc->type & 0xFF) << 24); \
> > +     }
> > +
> > +/* binary attributes in configfs need a max size - declare a value for this. */
> > +#define CSCFG_FILE_MAXSIZE   16384
> > +
>
> s/16384/SZ_16K
>
> Also, please enhance the comment to explain the 16K limit.
>
>
> > +/* limit string sizes */
> > +#define CSCFG_FILE_STR_MAXSIZE       1024
> > +
>
> s/1024/SZ_1K
>
> Thanks,
> Mathieu
>

Done all the above.

Thanks


Mike

> > +/**
> > + * file header.
> > + *
> > + * @magic_version: magic number / version for file/buffer format.
> > + * @length       : total length of all data in the buffer.
> > + * @nr_features  : total number of features in the buffer.
> > + */
> > +struct cscfg_file_header {
> > +     u32 magic_version;
> > +     u16 length;
> > +     u16 nr_features;
> > +};
> > +
> > +/**
> > + * element header
> > + *
> > + * @elem_length: total length of this element
> > + * @elem_type  : type of this element - one of CSCFG_FILE_ELEM_TYPE.. defines.
> > + */
> > +struct cscfg_file_elem_header {
> > +     u16 elem_length;
> > +     u8 elem_type;
> > +};
> > +
> > +/**
> > + * string file element.
> > + *
> > + * @str_len: length of string buffer including 0 terminator
> > + * @str    : string buffer - 0 terminated.
> > + */
> > +struct cscfg_file_elem_str {
> > +     u16 str_len;
> > +     char *str;
> > +};
> > +
> > +#endif /* _CORESIGHT_CORESIGHT_CONFIG_FILE_H */
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index 2e1670523461..9cd3c26ce023 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> > @@ -85,6 +85,21 @@ struct cscfg_regval_desc {
> >       };
> >  };
> >
> > +/**
> > + * Dynamically loaded descriptor arrays loaded via configfs.
> > + *
> > + * For builtin or module loaded configurations / features these are
> > + * statically defined at compile time. For configfs we create the arrays
> > + * dynamically so need a structure to handle this.
> > + *
> > + * @config_descs:    array of config descriptor pointers.
> > + * @feat_descs:              array of feature descriptor pointers.
> > + */
> > +struct cscfg_fs_load_descs {
> > +     struct cscfg_config_desc **config_descs;
> > +     struct cscfg_feature_desc **feat_descs;
> > +};
> > +
> >  /**
> >   * Device feature descriptor - combination of registers and parameters to
> >   * program a device to implement a specific complex function.
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> > index 9106ffab4833..6a6e33585be9 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> > @@ -66,6 +66,7 @@ struct cscfg_registered_csdev {
> >  enum cscfg_load_owner_type {
> >       CSCFG_OWNER_PRELOAD,
> >       CSCFG_OWNER_MODULE,
> > +     CSCFG_OWNER_CONFIGFS,
> >  };
> >
> >  /**
> > --
> > 2.17.1
> >



--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Patch

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index b6c4a48140ec..5de2bb79f4ac 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -6,7 +6,7 @@  obj-$(CONFIG_CORESIGHT) += coresight.o
 coresight-y := coresight-core.o  coresight-etm-perf.o coresight-platform.o \
 		coresight-sysfs.o coresight-syscfg.o coresight-config.o \
 		coresight-cfg-preload.o coresight-cfg-afdo.o \
-		coresight-syscfg-configfs.o
+		coresight-syscfg-configfs.o coresight-config-file.o
 obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
 coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
 		      coresight-tmc-etr.o
diff --git a/drivers/hwtracing/coresight/coresight-config-file.c b/drivers/hwtracing/coresight/coresight-config-file.c
new file mode 100644
index 000000000000..3fd001938324
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-config-file.c
@@ -0,0 +1,432 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Linaro Limited, All rights reserved.
+ * Author: Mike Leach <[e-mail]>
+ */
+
+#include "coresight-config.h"
+#include "coresight-config-file.h"
+#include "coresight-syscfg.h"
+
+#define cscfg_extract_u64(val64) {	\
+	val64 = *(u64 *)(buffer + used); \
+	used += sizeof(u64); \
+	}
+
+#define cscfg_extract_u32(val32) { \
+	val32 = *(u32 *)(buffer + used); \
+	used += sizeof(u32); \
+	}
+
+#define cscfg_extract_u16(val16) { \
+	val16 = *(u16 *)(buffer + used); \
+	used += sizeof(u16); \
+	}
+
+#define cscfg_extract_u8(val8) {	\
+	val8 = *(buffer + used); \
+	used++; \
+	}
+
+static int cscfg_file_read_hdr(const u8 *buffer, const int buflen, int *buf_used,
+			       struct cscfg_file_header *hdr)
+{
+	/* file header always at the start of the buffer */
+	int used = 0;
+
+	if (buflen < sizeof(struct cscfg_file_header))
+		return -EINVAL;
+
+	cscfg_extract_u32(hdr->magic_version);
+	if (hdr->magic_version != CSCFG_FILE_MAGIC_VERSION)
+		return -EINVAL;
+
+	cscfg_extract_u16(hdr->length);
+	if (hdr->length > buflen)
+		return -EINVAL;
+
+	cscfg_extract_u16(hdr->nr_features);
+
+	*buf_used = used;
+	return 0;
+}
+
+static int cscfg_file_read_elem_hdr(const u8 *buffer, const int buflen, int *buf_used,
+				    struct cscfg_file_elem_header *elem_hdr)
+{
+	int used = *buf_used;
+
+	if ((buflen - used) < (sizeof(u16) + sizeof(u8)))
+		return -EINVAL;
+
+	/* read length and check enough buffer remains for this element */
+	elem_hdr->elem_length = *(u16 *)(buffer + used);
+	if ((buflen - used) < elem_hdr->elem_length)
+		return -EINVAL;
+	/* don't use extract fn as we update used _after_ the comparison */
+	used += sizeof(u16);
+
+	/* read type and validate */
+	cscfg_extract_u8(elem_hdr->elem_type);
+	if ((elem_hdr->elem_type < CSCFG_FILE_ELEM_TYPE_FEAT) ||
+	    (elem_hdr->elem_type > CSCFG_FILE_ELEM_TYPE_CFG))
+		return -EINVAL;
+
+	*buf_used = used;
+	return 0;
+}
+
+static int cscfg_file_read_elem_str(const u8 *buffer, const int buflen, int *buf_used,
+				    struct cscfg_file_elem_str *elem_str)
+{
+	int used = *buf_used;
+
+	if ((buflen - used) < sizeof(u16))
+		return -EINVAL;
+
+	cscfg_extract_u16(elem_str->str_len);
+
+	if ((buflen - used) < elem_str->str_len)
+		return -EINVAL;
+
+	/* check for 0 termination */
+	if (buffer[elem_str->str_len - 1] != 0)
+		return -EINVAL;
+
+	elem_str->str = devm_kstrdup(cscfg_device(), (char *)buffer, GFP_KERNEL);
+	used += elem_str->str_len;
+
+	*buf_used = used;
+	return 0;
+}
+
+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);
+	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);
+	if (!desc_arrays->feat_descs)
+		return -ENOMEM;
+	return 0;
+}
+
+static int cscfg_file_read_elem_config(const u8 *buffer, const int buflen, int *buf_used,
+				       struct cscfg_fs_load_descs *desc_arrays)
+{
+	struct cscfg_file_elem_header elem_hdr;
+	struct cscfg_file_elem_str elem_str;
+	struct cscfg_config_desc *config_desc;
+	int used = *buf_used, nr_preset_vals, nr_preset_bytes, i;
+	int err = 0;
+	u64 *presets;
+
+	/*
+	 * read the header - if not config, then don't update buf_used
+	 * pointer on return
+	 */
+	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
+	if (err)
+		return err;
+	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
+		return 0;
+
+	/* we have a config - allocate the descriptor */
+	config_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_config_desc),
+				   GFP_KERNEL);
+	if (!config_desc)
+		return -ENOMEM;
+
+	/* read the name string */
+	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
+	if (err)
+		return err;
+	config_desc->name = elem_str.str;
+
+	/* read the description string */
+	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
+	if (err)
+		return err;
+	config_desc->description = elem_str.str;
+
+	/* read in some values */
+	if ((buflen - used) < sizeof(u64))
+		return -EINVAL;
+	cscfg_extract_u16(config_desc->nr_presets);
+	cscfg_extract_u32(config_desc->nr_total_params);
+	cscfg_extract_u16(config_desc->nr_feat_refs);
+
+	/* 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);
+		if (!presets)
+			return -ENOMEM;
+
+		nr_preset_bytes = sizeof(u64) * nr_preset_vals;
+		if ((buflen - used) < nr_preset_bytes)
+			return -EINVAL;
+
+		memcpy(presets, (buffer + used), nr_preset_bytes);
+		config_desc->presets = presets;
+		used += nr_preset_bytes;
+	}
+
+	/* 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);
+		if (!config_desc->feat_ref_names)
+			return -ENOMEM;
+
+		for (i = 0; i < config_desc->nr_feat_refs; i++) {
+			err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
+			if (err)
+				return err;
+			config_desc->feat_ref_names[i] = elem_str.str;
+		}
+	}
+
+	desc_arrays->config_descs[0] = config_desc;
+	*buf_used = used;
+	return 0;
+}
+
+/* just read the config name - if there is a config at this position */
+static int cscfg_file_read_elem_config_name(const u8 *buffer, const int buflen, int *buf_used,
+					    struct cscfg_file_elem_str *elem_str)
+{
+	struct cscfg_file_elem_header elem_hdr;
+	int used = *buf_used;
+	int err;
+
+	elem_str->str_len = 0;
+	/*
+	 * read the header - if not config, then don't update buf_used
+	 * pointer on return
+	 */
+	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
+	if (err)
+		return err;
+	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_CFG)
+		return 0;
+
+	/* read the name string */
+	err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
+	if (err)
+		return err;
+	*buf_used = used;
+
+	return 0;
+}
+
+static int cscfg_file_read_elem_param(const u8 *buffer, const int buflen, int *buf_used,
+				      struct cscfg_parameter_desc *param_desc)
+{
+	struct cscfg_file_elem_str elem_str;
+	int err = 0, used = *buf_used;
+
+	/* parameter name */
+	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
+	if (err)
+		return err;
+	param_desc->name = elem_str.str;
+
+	/* parameter value */
+	if ((buflen - used) < sizeof(u64))
+		return -EINVAL;
+	cscfg_extract_u64(param_desc->value);
+
+	*buf_used = used;
+	return err;
+}
+
+static int cscfg_file_read_elem_feature(const u8 *buffer, const int buflen, int *buf_used,
+					struct cscfg_fs_load_descs *desc_arrays,
+					const int feat_idx)
+{
+	struct cscfg_file_elem_header elem_hdr;
+	struct cscfg_file_elem_str elem_str;
+	struct cscfg_feature_desc *feat_desc;
+	struct cscfg_regval_desc *p_reg_desc;
+	int used = *buf_used, err, i, nr_regs_bytes;
+	u32 val32;
+
+	/* allocate the feature descriptor object */
+	feat_desc = devm_kzalloc(cscfg_device(), sizeof(struct cscfg_feature_desc),
+				 GFP_KERNEL);
+	if (!feat_desc)
+		return -ENOMEM;
+
+	/* read and check the element header */
+	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
+	if (err)
+		return err;
+
+	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
+		return -EINVAL;
+
+	/* read the feature name */
+	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
+	if (err)
+		return err;
+	feat_desc->name = elem_str.str;
+
+	/* read the description string */
+	err = cscfg_file_read_elem_str(buffer, buflen, &used, &elem_str);
+	if (err)
+		return err;
+	feat_desc->description = elem_str.str;
+
+	/*
+	 * read in some values
+	 * [u32 value: match_flags]
+	 * [u16 value: nr_regs]	    - number of registers.
+	 * [u16 value: nr_params]   - number of parameters.
+	 */
+	cscfg_extract_u32(feat_desc->match_flags);
+	cscfg_extract_u16(feat_desc->nr_regs);
+	cscfg_extract_u16(feat_desc->nr_params);
+
+	/* register descriptors  - 32 bit + 64 bit value */
+	if (feat_desc->nr_regs) {
+		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);
+		if (!feat_desc->regs_desc)
+			return -ENOMEM;
+
+		for (i = 0; i < feat_desc->nr_regs; i++) {
+			cscfg_extract_u32(val32);
+			p_reg_desc = (struct cscfg_regval_desc *)&feat_desc->regs_desc[i];
+			CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_reg_desc);
+			cscfg_extract_u64(feat_desc->regs_desc[i].val64);
+		}
+	}
+
+	/* 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);
+		if (!feat_desc->params_desc)
+			return -ENOMEM;
+		for (i = 0; i < feat_desc->nr_params; i++) {
+			err = cscfg_file_read_elem_param(buffer, buflen, &used,
+							 &feat_desc->params_desc[i]);
+			if (err)
+				return err;
+		}
+	}
+
+	desc_arrays->feat_descs[feat_idx] = feat_desc;
+	*buf_used = used;
+	return 0;
+}
+
+/* just read the feature name - if there is a feature at this position */
+static int cscfg_file_read_elem_feat_name(const u8 *buffer, const int buflen, int *buf_used,
+					  struct cscfg_file_elem_str *elem_str)
+{
+	struct cscfg_file_elem_header elem_hdr;
+	int used = *buf_used;
+	int err;
+
+	elem_str->str_len = 0;
+	/*
+	 * read the header - if not config, then don't update buf_used
+	 * pointer on return
+	 */
+	err = cscfg_file_read_elem_hdr(buffer, buflen, &used, &elem_hdr);
+	if (err)
+		return err;
+	if (elem_hdr.elem_type != CSCFG_FILE_ELEM_TYPE_FEAT)
+		return -EINVAL;
+
+	/* read the feature name */
+	err = cscfg_file_read_elem_str(buffer, buflen, &used, elem_str);
+	if (err)
+		return err;
+	*buf_used = used;
+
+	return 0;
+}
+
+/*
+ * Read a buffer and create the configuration and feature
+ * descriptors to load into the cscfg system
+ */
+int cscfg_file_read_buffer(const u8 *buffer, const int buflen,
+			   struct cscfg_fs_load_descs *desc_arrays)
+{
+	struct cscfg_file_header hdr;
+	int used = 0, err, i;
+
+	/* read in the file header */
+	err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
+	if (err)
+		return err;
+
+	/* allocate the memory for the descriptor pointer arrays */
+	err = cscfg_file_alloc_desc_arrays(desc_arrays, hdr.nr_features);
+	if (err)
+		return err;
+
+	/* read elements */
+
+	/* first element could be a config so check */
+	err = cscfg_file_read_elem_config(buffer, buflen, &used, desc_arrays);
+	if (err)
+		return err;
+
+	/* now read and populate all the feature descriptors */
+	for (i = 0; i < hdr.nr_features; i++) {
+		err = cscfg_file_read_elem_feature(buffer, buflen, &used, desc_arrays, i);
+		if (err)
+			return err;
+	}
+	return used;
+}
+
+int cscfg_file_read_buffer_first_name(const u8 *buffer, const int buflen,
+				      const char **name)
+{
+	struct cscfg_file_header hdr;
+	struct cscfg_file_elem_str elem_str;
+	int used = 0, err = 0;
+
+	*name = NULL;
+
+	/* read in the file header */
+	err = cscfg_file_read_hdr(buffer, buflen, &used, &hdr);
+	if (err)
+		return err;
+
+	err = cscfg_file_read_elem_config_name(buffer, buflen, &used, &elem_str);
+	if (err)
+		return err;
+
+	/* no config string - get first feature name */
+	if (!elem_str.str_len) {
+		err = cscfg_file_read_elem_feat_name(buffer, buflen, &used, &elem_str);
+		if (err)
+			return err;
+	}
+	if (elem_str.str_len)
+		*name = elem_str.str;
+	return err;
+}
diff --git a/drivers/hwtracing/coresight/coresight-config-file.h b/drivers/hwtracing/coresight/coresight-config-file.h
new file mode 100644
index 000000000000..591f4c2c4be9
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-config-file.h
@@ -0,0 +1,117 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2020 Linaro Limited, All rights reserved.
+ * Author: Mike Leach <[e-mail]>
+ */
+
+#ifndef _CORESIGHT_CORESIGHT_CONFIG_FILE_H
+#define _CORESIGHT_CORESIGHT_CONFIG_FILE_H
+
+/*
+ * Structures to represent configuration descriptors in a memory buffer
+ * to serialise to and from files
+ *
+ * File structure - for loading a configuration + features
+ * from configfs.
+ *
+ * [cscfg_file_header]	- mandatory
+ * [CONFIG_ELEM]	- optional - only one permitted,
+ * [FEATURE_ELEM] * [cscfg_file_header.nr_features]
+ *			- optional - file valid with config only.
+ *
+ * Invalid file if no config or features.
+ *
+ *
+ * File structure for [CONFIG_ELEM]:
+ *
+ * [cscfg_file_elem_header] - header length value to end of feature strings.
+ * [cscfg_file_elem_str]    - name of the configuration
+ * [cscfg_file_elem_str]    - description of configuration
+ * [u16 value - nr_presets]
+ * [u32 value - nr_total_params]
+ * [u16 value - nr_feat_refs]
+ * [u64 values] * (nr_presets * nr_total_params)
+ * [cscfg_file_elem_str] * nr_feat_refs
+ *
+ * Only one configuration per file.
+ *
+ * File structure for a [FEATURE_ELEM]
+ *
+ * [cscfg_file_elem_header] - header length is total bytes to end of param structures.
+ * [cscfg_file_elem_str]    - feature name.
+ * [cscfg_file_elem_str]    - feature description.
+ * [u32 value: match_flags]
+ * [u16 value: nr_regs]	    - number of registers.
+ * [u16 value: nr_params]   - number of parameters.
+ * [cscfg_regval_desc struct] * nr_regs
+ * [PARAM_ELEM] * nr_params
+ *
+ * File structure for [PARAM_ELEM]
+ *
+ * [cscfg_file_elem_str]    - parameter name.
+ * [u64 value: param_value] - initial value.
+ */
+
+/* major element types - configurations and features */
+
+#define CSCFG_FILE_ELEM_TYPE_FEAT	0x1
+#define CSCFG_FILE_ELEM_TYPE_CFG	0x2
+
+#define CSCFG_FILE_MAGIC_VERSION	0xC5CF0001
+
+#define CSCFG_FILE_U32_TO_REG_DESC_INFO(val32, p_desc) \
+	{ \
+	p_desc->type = (val32 >> 24) & 0xFF; \
+	p_desc->offset = (val32 >> 12) & 0xFFF; \
+	p_desc->hw_info = val32 & 0xFFF; \
+	}
+
+#define CSCFG_FILE_REG_DESC_INFO_TO_U32(val32, p_desc) \
+	{ \
+	val32 = p_desc->hw_info & 0xFFF; \
+	val32 |= ((p_desc->offset & 0xFFF) << 12); \
+	val32 |= ((p_desc->type & 0xFF) << 24); \
+	}
+
+/* binary attributes in configfs need a max size - declare a value for this. */
+#define CSCFG_FILE_MAXSIZE	16384
+
+/* limit string sizes */
+#define CSCFG_FILE_STR_MAXSIZE	1024
+
+/**
+ * file header.
+ *
+ * @magic_version: magic number / version for file/buffer format.
+ * @length       : total length of all data in the buffer.
+ * @nr_features  : total number of features in the buffer.
+ */
+struct cscfg_file_header {
+	u32 magic_version;
+	u16 length;
+	u16 nr_features;
+};
+
+/**
+ * element header
+ *
+ * @elem_length: total length of this element
+ * @elem_type  : type of this element - one of CSCFG_FILE_ELEM_TYPE.. defines.
+ */
+struct cscfg_file_elem_header {
+	u16 elem_length;
+	u8 elem_type;
+};
+
+/**
+ * string file element.
+ *
+ * @str_len: length of string buffer including 0 terminator
+ * @str    : string buffer - 0 terminated.
+ */
+struct cscfg_file_elem_str {
+	u16 str_len;
+	char *str;
+};
+
+#endif /* _CORESIGHT_CORESIGHT_CONFIG_FILE_H */
diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
index 2e1670523461..9cd3c26ce023 100644
--- a/drivers/hwtracing/coresight/coresight-config.h
+++ b/drivers/hwtracing/coresight/coresight-config.h
@@ -85,6 +85,21 @@  struct cscfg_regval_desc {
 	};
 };
 
+/**
+ * Dynamically loaded descriptor arrays loaded via configfs.
+ *
+ * For builtin or module loaded configurations / features these are
+ * statically defined at compile time. For configfs we create the arrays
+ * dynamically so need a structure to handle this.
+ *
+ * @config_descs:	array of config descriptor pointers.
+ * @feat_descs:		array of feature descriptor pointers.
+ */
+struct cscfg_fs_load_descs {
+	struct cscfg_config_desc **config_descs;
+	struct cscfg_feature_desc **feat_descs;
+};
+
 /**
  * Device feature descriptor - combination of registers and parameters to
  * program a device to implement a specific complex function.
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
index 9106ffab4833..6a6e33585be9 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.h
+++ b/drivers/hwtracing/coresight/coresight-syscfg.h
@@ -66,6 +66,7 @@  struct cscfg_registered_csdev {
 enum cscfg_load_owner_type {
 	CSCFG_OWNER_PRELOAD,
 	CSCFG_OWNER_MODULE,
+	CSCFG_OWNER_CONFIGFS,
 };
 
 /**