Re: [RFC PATCH v4 09/15] coresight: cti: Add connection information to sysfs

On Mon, Aug 19, 2019 at 10:13:55PM +0100, Mike Leach wrote:

Dynamically adds sysfs attributes for all connections defined in the CTI.

Each connection has a triggers<N> sub-directory with name, in_signals, in_types, out_signals and out_types as read-only parameters in the directory. in_ or out_ parameters may be omitted if there are no in or out signals for the connection.

Additionally each device has a nr_cons and trigout_filtered parameter in the connections sub-directory. .

Please remove the '.'

This allows clients to explore the connection and trigger signal details without needing to refer to device tree or specification of the device.

Standardised type information is provided for certain common functions - e.g. snk_full for a trigger from a sink indicating full. Otherwise type defaults to genio.

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

.../hwtracing/coresight/coresight-cti-sysfs.c | 343 +++++++++++++++++- drivers/hwtracing/coresight/coresight-cti.c | 13 +- drivers/hwtracing/coresight/coresight-cti.h | 6 + 3 files changed, 360 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index dbbfd1de58b3..a7ae479ee2b9 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -829,7 +829,342 @@ static struct attribute *coresight_cti_channel_attrs[] = { NULL, }; -/* sysfs groups */ +/* Create the connections trigger groups and attrs dynamically */

+/*

• • Each connection has dynamic group + name, trigin/out sigs/types attrs,
• • • each device has static nr_conns + filter attr
• */

+static ssize_t trigout_filtered_show(struct device *dev,

• 		     struct device_attribute *attr,
  

• 		     char *buf)
  

+{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *cfg = &drvdata->config;
• int size = 0, nr_trig_max = cfg->nr_trig_max;
• unsigned long mask = cfg->trig_out_filter;
• if (mask)

• size = bitmap_print_to_pagebuf(true, buf, &mask, nr_trig_max);
  

• else

• size = scnprintf(buf, PAGE_SIZE, "none\n");
  

The usual sysfs euristic is to not print anything when something isn't set.

• return size;

+} +static DEVICE_ATTR_RO(trigout_filtered);

+static ssize_t nr_cons_show(struct device *dev,

• 	    struct device_attribute *attr,
  

• 	    char *buf)
  

+{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• return scnprintf(buf, PAGE_SIZE, "%d\n", drvdata->ctidev.nr_trig_con);

+} +static DEVICE_ATTR_RO(nr_cons);

+static struct attribute *coresight_cti_cons_attrs[] = {

• &dev_attr_trigout_filtered.attr,
• &dev_attr_nr_cons.attr,
• NULL,

+};

+static ssize_t con_name_show(struct device *dev,

• 	     struct device_attribute *attr,
  

• 	     char *buf)
  

+{

• struct dev_ext_attribute *ext_attr = (struct dev_ext_attribute *)attr;
• struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var;
• return scnprintf(buf, PAGE_SIZE, "%s\n", con->con_dev_name);

+}

+static ssize_t trigin_sig_show(struct device *dev,

• 	       struct device_attribute *attr,
  

• 	       char *buf)
  

+{

• struct dev_ext_attribute *ext_attr = (struct dev_ext_attribute *)attr;
• struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var;
• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *cfg = &drvdata->config;
• unsigned long mask = con->con_in->used_mask;
• return bitmap_print_to_pagebuf(true, buf, &mask, cfg->nr_trig_max);

+}

+static ssize_t trigout_sig_show(struct device *dev,

• 		struct device_attribute *attr,
  

• 		char *buf)
  

+{

• struct dev_ext_attribute *ext_attr = (struct dev_ext_attribute *)attr;
• struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var;
• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *cfg = &drvdata->config;
• unsigned long mask = con->con_out->used_mask;
• return bitmap_print_to_pagebuf(true, buf, &mask, cfg->nr_trig_max);

+}

+/* convert a sig type id to a name */ +static const char * +cti_sig_type_name(struct cti_trig_con *con, int used_count, bool in) +{

• static const char * const sig_type_names[] = {

• "genio", "intreq", "intack", "haltreq", "restartreq",
  

• "pe_edbgreq", "pe_dbgrestart", "pe_ctiirq", "pe_pmuirq",
  

• "pe_dbgtrigger", "etm_extout",  "etm_extin", "snk_full",
  

• "snk_acqcomp", "snk_flushcomp", "snk_flushin", "snk_trigin",
  

• "stm_asyncout", "stm_tout_spte", "stm_tout_sw", "stm_tout_hete",
  

• "stm_hwevent", "ela_tstart", "ela_tstop", "ela_dbgreq",
  

• };

This would likely be very confusing for someone new to the code. Please move sig_type_names[] to the top of the file and declare as follow:

static const char * const sig_type_names[] = { "genio", /* GEN_IO */ "intreq", /* GEN_INTREQ */ ...

Along with a comment that definitions follow the defines in coresight-cti-dt.h.

• int idx = 0;
• struct cti_trig_grp *grp = in ? con->con_in : con->con_out;
• if (grp->sig_types) {

• if (used_count < grp->nr_sigs)
  

• 	idx = grp->sig_types[used_count];
  

• }
• return sig_type_names[idx];

+}

+static ssize_t trigin_type_show(struct device *dev,

• 		struct device_attribute *attr,
  

• 		char *buf)
  

+{

• struct dev_ext_attribute *ext_attr = (struct dev_ext_attribute *)attr;
• struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var;

Extra newline

• int sig_idx, used = 0, b_sz = PAGE_SIZE;
• const char *name;
• for (sig_idx = 0; sig_idx < con->con_in->nr_sigs; sig_idx++) {

• name = cti_sig_type_name(con, sig_idx, true);
  

• used += scnprintf(buf+used, b_sz-used, "%s ", name);
  

A spaces is required on both side of the operator, for both '+' and '-'.

• }
• used += scnprintf(buf+used, b_sz-used, "\n");

Same comment as above

• return used;

+}

+static ssize_t trigout_type_show(struct device *dev,

• 		 struct device_attribute *attr,
  

• 		 char *buf)
  

+{

• struct dev_ext_attribute *ext_attr = (struct dev_ext_attribute *)attr;
• struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var;

Extra newline

• int sig_idx, used = 0, b_sz = PAGE_SIZE;
• const char *name;
• for (sig_idx = 0; sig_idx < con->con_out->nr_sigs; sig_idx++) {

• name = cti_sig_type_name(con, sig_idx, false);
  

• used += scnprintf(buf+used, b_sz-used, "%s ", name);
  

Same comment as above

• }
• used += scnprintf(buf+used, b_sz-used, "\n");

Same comment as above

• return used;

+}

+typedef ssize_t (*p_show_fn)(struct device *dev, struct device_attribute *attr,

• 	     char *buf);
  

+enum cti_conn_attr_type {

• CTI_CON_ATTR_NAME,
• CTI_CON_ATTR_TRIGIN_SIG,
• CTI_CON_ATTR_TRIGOUT_SIG,
• CTI_CON_ATTR_TRIGIN_TYPES,
• CTI_CON_ATTR_TRIGOUT_TYPES

+};

+static p_show_fn show_fns[] = {

• con_name_show,
• trigin_sig_show,
• trigout_sig_show,
• trigin_type_show,
• trigout_type_show,

+};

+static const char * const base_names[] = {

• "name",
• "in_signals",
• "out_signals",
• "in_types",
• "out_types",

+};

I would move all of these to the top of the file with sig_type_names[].

+static int cti_create_con_sysfs_attr(struct cti_trig_con *con,

• 		     enum cti_conn_attr_type attr_type,
  

• 		     int attr_idx)
  

+{

• struct dev_ext_attribute *dev_ext_attr = 0;
• char *name = 0;
• dev_ext_attr = kzalloc(sizeof(struct dev_ext_attribute), GFP_KERNEL);
• if (dev_ext_attr) {

• name = kzalloc(sizeof(char) * 16, GFP_KERNEL);
  

• if (name) {
  

• 	/* fill out the underlying attribute struct */
  

• 	sprintf(name, "%s", base_names[attr_type]);
  

Use kstrdup to allocate the exact size needed rather than having to settle for the biggest possible name. Doing so we can add any name without fear of a buffer overflow.

• 	dev_ext_attr->attr.attr.name = name;
  

• 	dev_ext_attr->attr.attr.mode = 0444;
  

• 	/* now the device_attribute struct */
  

• 	dev_ext_attr->attr.show = show_fns[attr_type];
  

• } else {
  

• 	kfree(dev_ext_attr);
  

• 	return -ENOMEM;
  

• }
  

• } else {

• return -ENOMEM;
  

• }
• dev_ext_attr->var = con;
• con->con_attrs[attr_idx] = dev_ext_attr;
• return 0;

+}

+/* number of static groups (5) declared below + null terminator */ +#define NR_STATIC_GROUPS 5

We can't use ARRAY_SIZE() on coresight_cti_groups[] and at the same time we need to know the size of the array. I think the best way to do this is to move the #define to the top of the file and call it CORESIGHT_CTI_GROUPS_MAX. With that we can do "coresight_cti_groups[CORESIGHT_CTI_GROUPS_MAX] = {". That way anyone who wants to add to coresight_cti_groups needs to modify the define and things don't blow up in our face.

+static struct attribute_group * +cti_create_con_sysfs_group(struct cti_device *ctidev, int con_idx) +{

• struct attribute_group *group = NULL;
• group = kzalloc(sizeof(struct attribute_group), GFP_KERNEL);
• if (!group)

• return NULL;
  

• group->name = kasprintf(GFP_KERNEL, "triggers%d", con_idx);
• if (!group->name) {

• kfree(group);
  

• return NULL;
  

• }
• ctidev->con_groups[con_idx + NR_STATIC_GROUPS] = group;
• return group;

+}

+/* dynamic attr - 5 per connection + null terminator */ +#define CTI_SYSFS_CONS_DYN_ATTR 5

This case is easier than the previous one. I would remove this and create a new CTI_CON_ATTR_TYPE_MAX in enum cti_connt_attr_type {} that can then be used in the declaration of p_show_fn show_fns[] and base_names[].

+/* create a triggers connection group and the attributes for that group */ +static int cti_create_con_attr_set(int con_idx, struct cti_device *ctidev,

• 		   struct cti_trig_con *con)
  

+{

• struct attribute_group *attr_group = NULL;
• int attr_idx = 0;
• int err = -ENOMEM;
• attr_group = cti_create_con_sysfs_group(ctidev, con_idx);
• if (!attr_group)

• return -ENOMEM;
  

• con->con_attrs = kcalloc(CTI_SYSFS_CONS_DYN_ATTR + 1,

• 		 sizeof(struct dev_ext_attribute *),
  

• 		 GFP_KERNEL);
  

• if (!con->con_attrs)

• return -ENOMEM;
  

• err = cti_create_con_sysfs_attr(con, CTI_CON_ATTR_NAME, attr_idx++);
• if (err)

• return err;
  

• if (con->con_in->nr_sigs > 0) {

• err = cti_create_con_sysfs_attr(con, CTI_CON_ATTR_TRIGIN_SIG,
  

• 				attr_idx++);
  

• if (err)
  

• 	return err;
  

• err = cti_create_con_sysfs_attr(con, CTI_CON_ATTR_TRIGIN_TYPES,
  

• 				attr_idx++);
  

• if (err)
  

• 	return err;
  

• }
• if (con->con_in->nr_sigs > 0) {

• err = cti_create_con_sysfs_attr(con, CTI_CON_ATTR_TRIGOUT_SIG,
  

• 				attr_idx++);
  

• if (err)
  

• 	return err;
  

• err = cti_create_con_sysfs_attr(con, CTI_CON_ATTR_TRIGOUT_TYPES,
  

• 				attr_idx++);
  

• if (err)
  

• 	return err;
  

• }
• attr_group->attrs = (struct attribute **)con->con_attrs;

Did you see this being done somewhere? If so please share so that I can have a look. To me cti_trig_con::con_attrs need to be of type struct attribute **. In function cti_create_con_sysfs_attr() the last line becomes:

con->con_attrs[attr_idx] = &dev_ext_attr->attr.attr;

In the shows function the dev_ext_attribute can be reached using a container_of() like I did here:

https://elixir.bootlin.com/linux/latest/source/drivers/hwtracing/coresight/c...

• return 0;

+}

+/* create the array of group pointers for the CTI sysfs groups */ +int cti_create_cons_groups(struct cti_device *ctidev) +{

• int i, nr_groups;
• /* nr groups - dynamic + static + NULL terminator */
• nr_groups = ctidev->nr_trig_con + NR_STATIC_GROUPS + 1;

Following my recommendation on CORESIGHT_CTI_GROUPS_MAX, the above becomes:

/* nr groups - dynamic + static - NULL terminator */ nr_groups = ctidev->nr_trig_con + CORESIGHT_CTI_GROUPS_MAX - 1;

Probably best to declare a new variable and use it in the loop below as well.

• ctidev->con_groups = kcalloc(nr_groups,

• 		     sizeof(struct attribute_group *),
  

• 		     GFP_KERNEL);
  

• if (!ctidev->con_groups)

• return -ENOMEM;
  

• /* populate first locations with the static set of groups */
• for (i = 0; i < NR_STATIC_GROUPS; i++)

• ctidev->con_groups[i] = coresight_cti_groups[i];
  

• return 0;

+}

+int cti_create_cons_sysfs(struct cti_drvdata *drvdata) +{

• struct cti_device *ctidev = &drvdata->ctidev;
• int err, con_idx = 0;
• struct cti_trig_con *tc = NULL;
• err = cti_create_cons_groups(ctidev);
• if (err)

• return err;
  

• /* add dynamic set for each connection */
• list_for_each_entry(tc, &ctidev->trig_cons, node) {

• err = cti_create_con_attr_set(con_idx++, ctidev, tc);
  

• if (err)
  

• 	goto cons_sysfs_err;
  

• }
• return 0;

+cons_sysfs_err:

• cti_destroy_cons_sysfs(ctidev);
• return err;

+}

+void cti_destroy_cons_attr_set(int con_idx, struct cti_device *ctidev,

• 	       struct cti_trig_con *con)
  

+{

• int i;
• if (con->con_attrs) {

• for (i = 0; i <= CTI_SYSFS_CONS_DYN_ATTR; i++) {
  

• 	if (con->con_attrs[i]) {
  

• 		kfree(con->con_attrs[i]->attr.attr.name);
  

• 		kfree(con->con_attrs[i]);
  

• 	}
  

• }
  

• kfree(con->con_attrs);
  

• }
• kfree(ctidev->con_groups[con_idx + NR_STATIC_GROUPS]);

+}

+void cti_destroy_cons_sysfs(struct cti_device *ctidev) +{

• struct cti_trig_con *tc;
• int con_idx = 0;
• list_for_each_entry(tc, &ctidev->trig_cons, node) {

• cti_destroy_cons_attr_set(con_idx++, ctidev, tc);
  

• }
• kfree(ctidev->con_groups);

+}

+/* attribute and group sysfs tables. */ static const struct attribute_group coresight_cti_group = { .attrs = coresight_cti_attrs, }; @@ -849,10 +1184,16 @@ static const struct attribute_group coresight_cti_channels_group = { .name = "channels", }; +static struct attribute_group coresight_cti_conns_group = {

• .attrs = coresight_cti_cons_attrs,
• .name = "connections",

+};

const struct attribute_group *coresight_cti_groups[] = { &coresight_cti_group, &coresight_cti_mgmt_group, &coresight_cti_regs_group, &coresight_cti_channels_group,

• &coresight_cti_conns_group, NULL,

}; diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 927f58af92a2..dce02f08ece1 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -571,6 +571,9 @@ static void cti_device_release(struct device *dev) mutex_lock(&ect_mutex);

• /* clear the dynamic sysfs associate with connections */
• cti_destroy_cons_sysfs(&drvdata->ctidev);
• /* remove from the list */ list_for_each_entry_safe(ect_item, ect_tmp, &ect_net, node) { if (ect_item == drvdata) {

@@ -647,12 +650,20 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) goto err_out; }

• /* create dynamic attributes for connections */
• ret = cti_create_cons_sysfs(drvdata);
• if (ret) {

• pr_err("%s: create dynamic sysfs entries failed\n",
  

•        cti_desc.name);
  

s/pr_err()/dev_info()

• goto err_out;
  

• }
• /* set up coresight component description */ cti_desc.pdata = pdata; cti_desc.type = CORESIGHT_DEV_TYPE_ECT; cti_desc.subtype.ect_subtype = CORESIGHT_DEV_SUBTYPE_ECT_CTI; cti_desc.ops = &cti_ops;

• cti_desc.groups = coresight_cti_groups;

• cti_desc.groups = drvdata->ctidev.con_groups; cti_desc.dev = dev; drvdata->csdev = coresight_register(&cti_desc); if (IS_ERR(drvdata->csdev)) {

diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index 90622f3be12b..95ed026a2a61 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -74,6 +74,7 @@ struct cti_trig_grp {

• @con_dev: coresight device connected to the CTI, NULL if not CS device
• @con_dev_name: name of connected device (CS or CPU)
• @node: entry node in list of connections.

• • @con_attrs: Dynamic sysfs attributes specific to this connection.
  */

struct cti_trig_con { struct cti_trig_grp *con_in; @@ -81,6 +82,7 @@ struct cti_trig_con { struct coresight_device *con_dev; char *con_dev_name; struct list_head node;

• struct dev_ext_attribute **con_attrs;

}; /** @@ -91,12 +93,14 @@ struct cti_trig_con {

•      assumed there is a single CTM per SoC, ID 0).
  

• @trig_cons: list of connections to this device.
• @cpu: CPU ID if associated with CPU, -1 otherwise.

• • @con_groups: Dynamic sysfs groups for trigger connections.
  */

struct cti_device { int nr_trig_con; u32 ctm_id; struct list_head trig_cons; int cpu;

• const struct attribute_group **con_groups;

}; /** @@ -222,6 +226,8 @@ int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op, u32 channel_idx); int cti_channel_setop(struct device *dev, enum cti_chan_set_op op, u32 channel_idx); +int cti_create_cons_sysfs(struct cti_drvdata *drvdata); +void cti_destroy_cons_sysfs(struct cti_device *ctidev); struct coresight_platform_data * coresight_cti_get_platform_data(struct device *dev); const char *coresight_cti_get_node_name(struct device *dev); -- 2.17.1