Re: [RFC PATCH v3 09/16] coresight: cti: Add connection information to sysfs

Hi Mike,

I understand that the connection layout will change based on the discussion. Some minor comments below.

On 05/06/2019 20:03, Mike Leach wrote:

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

Each connection has _name, _trgin_type, _trgout_type, _trgin_sig and _trgout_sig, preceded by a connection index (e.g. 0_name is the name of connection 0).

Additionally each device has a nr_cons and trigout_filtered parameter. 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 | 338 +++++++++++++++++- drivers/hwtracing/coresight/coresight-cti.c | 11 + drivers/hwtracing/coresight/coresight-cti.h | 4 + 3 files changed, 352 insertions(+), 1 deletion(-)

+/*

• *Each connection has dynamic 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)
  

• if (cfg->trig_out_filter) {

• sig_mask = 0x1;
  

• for (sig_idx = 0; sig_idx < nr_trig_max; sig_idx++) {
  

• 	if (sig_mask & cfg->trig_out_filter) {
  

• 		used += scnprintf(buf+used, b_sz-used, "%d ",
  

• 				 sig_idx);
  

• 	}
  

• 	sig_mask <<= 1;
  

• }
  

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

• } else

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

As mentioned in the other patch, please use %pb[l] or bitmap_print_to_pagebuf().

+/* static attrs - 2 + null terminator */ +#define CTI_SYSFS_CONS_ST_ATTR 3

I would prefer you leave this 2 here and then while computing the n_attrs, say:

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

n_attrs = static_attrs + nr_trig_cons * dynamic_per_trig_con_attrs + 1 (for 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;
• u32 sig_mask;
• int sig_idx, used = 0, b_sz = PAGE_SIZE;
• sig_mask = 0x1;
• for (sig_idx = 0; sig_idx < cfg->nr_trig_max; sig_idx++) {

• if (sig_mask & con->con_in->used_mask) {
  

• 	used += scnprintf(buf+used, b_sz-used, "%d ",
  

• 			  sig_idx);
  

• }
  

• sig_mask <<= 1;
  

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

+}

+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;
• u32 sig_mask;
• int sig_idx, used = 0, b_sz = PAGE_SIZE;
• sig_mask = 0x1;
• for (sig_idx = 0; sig_idx < cfg->nr_trig_max; sig_idx++) {

• if (sig_mask & con->con_out->used_mask) {
  

• 	used += scnprintf(buf+used, b_sz-used, "%d ",
  

• 			  sig_idx);
  

• }
  

• sig_mask <<= 1;
  

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

+}

+/* 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",
  

• };

• int idx = 0;

nit: Please could we s/used_count/sig_idx/ and s/idx/sig_type/ ?

• 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;
• 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);
  

• }
• used += scnprintf(buf+used, b_sz-used, "\n");
• 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;
• 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);
  

• }
• used += scnprintf(buf+used, b_sz-used, "\n");
• 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",
• "_trgin_sig",
• "_trgout_sig",
• "_trgin_type",
• "_trgout_type",

+};

+struct dev_ext_attribute * +cti_create_con_sysfs_attr(int con_idx, enum cti_conn_attr_type attr_type) +{

• 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, "%d%s", con_idx, base_names[attr_type]);
  

--->>---

• 	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];
  

-----<<---

nit: You may do : dev_ext_attr->attr = __ATTR(name, 0444, show_fns[attr_type], NULL);

• } else {
  

• 	kfree(dev_ext_attr);
  

• 	dev_ext_attr = 0;
  

• }
  

• }
• return dev_ext_attr;

+}

+int cti_create_con_attr_set(int con_idx, int *next_attr_idx,

• 	    struct cti_device *ctidev,
  

• 	    struct cti_trig_con *con)
  

+{

• struct dev_ext_attribute *dev_ext_attr;
• int attr_idx = *next_attr_idx;
• dev_ext_attr = cti_create_con_sysfs_attr(con_idx, CTI_CON_ATTR_NAME);
• if (!dev_ext_attr)

• goto create_con_attr_err;
  

• dev_ext_attr->var = con;
• ctidev->con_attrs[attr_idx++] = dev_ext_attr;
• if (con->con_in->nr_sigs > 0) {

• dev_ext_attr =
  

• 	cti_create_con_sysfs_attr(con_idx,
  

• 				  CTI_CON_ATTR_TRIGIN_SIG);
  

• if (!dev_ext_attr)
  

• 	goto create_con_attr_err;
  

• dev_ext_attr->var = con;
  

• ctidev->con_attrs[attr_idx++] = dev_ext_attr;
  

• dev_ext_attr =
  

• 	cti_create_con_sysfs_attr(con_idx,
  

• 				  CTI_CON_ATTR_TRIGIN_TYPES);
  

• if (!dev_ext_attr)
  

• 	goto create_con_attr_err;
  

• dev_ext_attr->var = con;
  

• ctidev->con_attrs[attr_idx++] = dev_ext_attr;
  

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

• dev_ext_attr =
  

• 	cti_create_con_sysfs_attr(con_idx,
  

• 				  CTI_CON_ATTR_TRIGOUT_SIG);
  

• if (!dev_ext_attr)
  

• 	goto create_con_attr_err;
  

• dev_ext_attr->var = con;
  

• ctidev->con_attrs[attr_idx++] = dev_ext_attr;
  

• dev_ext_attr =
  

• 	cti_create_con_sysfs_attr(con_idx,
  

• 				  CTI_CON_ATTR_TRIGOUT_TYPES);
  

• if (!dev_ext_attr)
  

• 	goto create_con_attr_err;
  

• dev_ext_attr->var = con;
  

• ctidev->con_attrs[attr_idx++] = dev_ext_attr;
  

• }
• *next_attr_idx = attr_idx;
• return 0;

+create_con_attr_err:

• cti_destroy_cons_sysfs(ctidev);
• return -ENOMEM;

+}

+int cti_create_cons_sysfs(struct cti_drvdata *drvdata) +{

• struct cti_device *ctidev = &drvdata->ctidev;
• int err = 0, n_attrs = 0, con_idx = 0;
• int next_attr_idx = CTI_SYSFS_CONS_ST_ATTR - 1;
• struct cti_trig_con *tc;
• n_attrs = CTI_SYSFS_CONS_ST_ATTR +

• (CTI_SYSFS_CONS_DYN_ATTR * ctidev->nr_trig_con);
  

nit: Please could we add a macro (after the change to ST_ATTR);

/* Number of attributes to represent nr_trig_con connections */ #define cti_sysfs_connection_n_attrs(nr_trig_con) \ (CTI_SYSFS_CONS_ST_ATTR + (CTI_SYSFS_CONS_DYN_ATTR * (nr_trig_con)) + 1)

and then :

n_attrs = cti_sysfs_connection_n_attrs(ctidev->nr_trig_con);

• ctidev->con_attrs =

• kcalloc(n_attrs, sizeof(struct dev_ext_attribute *),
  

• 	GFP_KERNEL);
  

• if (ctidev->con_attrs != 0) {

• ctidev->con_attrs[0] =	(struct dev_ext_attribute *)
  

• 	&dev_attr_trigout_filtered.attr;
  

• ctidev->con_attrs[1] = (struct dev_ext_attribute *)
  

• 	&dev_attr_nr_cons.attr;
  

nit:

Also if you keep the static attrs in an array then you may simply do :

for (attr_idx = 0; attr_idx < ST_ATTR; attr_idx ++) ctidev->con_attrs[attr_idx] = static_attrs[attr_idx]; and then go on to populate the dynamic ones. Thus you may skip initializing the attr_idx to ST_ATTRS - 1 and manualy copying the bits above. This might be cleaner and future proof, if we wanted to add more static attrs.

• /* add dynamic set for each connection */
  

• list_for_each_entry(tc,  &ctidev->trig_cons, node) {
  

• 	err = cti_create_con_attr_set(con_idx++,
  

• 				      &next_attr_idx,
  

• 				      ctidev,
  

• 				      tc);
  

• 	/* if create con fails it will free all prior cons */
  

• 	if (err)
  

• 		return err;
  

• }
  

• coresight_cti_conns_group.attrs =
  

• 	(struct attribute **)ctidev->con_attrs;
  

• } else

• err = -ENOMEM;
  

• return err;

+}

+void cti_destroy_cons_sysfs(struct cti_device *ctidev) +{

• int n_attrs = CTI_SYSFS_CONS_ST_ATTR +

• (CTI_SYSFS_CONS_DYN_ATTR * ctidev->nr_trig_con);
  

nit: You could reuse the macro from above.

• int dyn_idx = CTI_SYSFS_CONS_ST_ATTR - 1;
• bool eol = false;
• struct dev_ext_attribute *dev_ext_attr = 0;
• if (ctidev->con_attrs) {

• while (!eol && (dyn_idx < n_attrs)) {
  

• 	dev_ext_attr = ctidev->con_attrs[dyn_idx];
  

• 	if (dev_ext_attr) {
  

• 		kfree(dev_ext_attr->attr.attr.name);
  

• 		kfree(dev_ext_attr);
  

• 	} else 
  

• 		eol = true;
  

break ? and drop eol variable ?

Rest looks fine.

Suzuki