From: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com>
To: Mike Leach <mike.leach@linaro.org>,
coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
devicetree@vger.kernel.org, linux-doc@vger.kernel.org
Cc: mathieu.poirier@linaro.org
Subject: Re: [PATCH v5 09/14] coresight: cti: Add connection information to sysfs
Date: Mon, 2 Dec 2019 09:47:57 +0000 [thread overview]
Message-ID: <ad75693c-8c6f-35fd-f9bb-0317c2b4dcd2@arm.com> (raw)
In-Reply-To: <20191119231912.12768-10-mike.leach@linaro.org>
On 19/11/2019 23:19, 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 in the connections sub-directory.
>
> 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 | 376 +++++++++++++++++-
> drivers/hwtracing/coresight/coresight-cti.c | 13 +-
> drivers/hwtracing/coresight/coresight-cti.h | 11 +-
> 3 files changed, 396 insertions(+), 4 deletions(-)
>
The patch looks good overall, some minor comments below.
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index f800402f73da..91986732506f 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -8,6 +8,67 @@
> @@ -818,7 +890,306 @@ static struct attribute *coresight_cti_channel_attrs[] = {
> NULL,
> };
>
> -/* sysfs groups */
> +/* Create the connections trigger groups and attrs dynamically */
> +/*
> + * Each connection has dynamic group triggers<N> + name, trigin/out sigs/types
> + * attributes, + each device has static nr_trigger_cons giving the number
> + * of groups. e.g. in sysfs:-
> + * /cti_<name>/triggers0
> + * /cti_<name>/triggers1
> + * /cti_<name>/nr_trigger_cons
> + * where nr_trigger_cons = 2
> + */
> +static ssize_t con_name_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct dev_ext_attribute *ext_attr =
> + container_of(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 =
> + container_of(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 =
> + container_of(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)
> +{
> + 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 =
> + container_of(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 =
> + container_of(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;
> +}
> +
> +/*
> + * Array of show function names declared above to allow selection
> + * for the connection attributes
> + */
> +static p_show_fn show_fns[CTI_CON_ATTR_MAX] = {
> + con_name_show,
> + trigin_sig_show,
> + trigout_sig_show,
> + trigin_type_show,
> + trigout_type_show,
> +};
> +
> +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;
super minor nit: You may use "eattr" instead.
> + char *name = 0;
> +
> + dev_ext_attr = kzalloc(sizeof(struct dev_ext_attribute), GFP_KERNEL);
Could we not use devm_* alloc helpers everywhere ?
> + if (dev_ext_attr) {
> + name = kstrdup(con_attr_names[attr_type], GFP_KERNEL);
> + if (name) {
> + /* fill out the underlying attribute struct */
> + 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->attr.attr;
> + return 0;
> +}
> +
> +static struct attribute_group *
> +cti_create_con_sysfs_group(struct cti_device *ctidev, int con_idx,
> + struct cti_trig_con *con)
> +{
> + 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 + CORESIGHT_CTI_STATIC_GROUPS_MAX - 1]
> + = group;
nit:
con_idx += CORESIGHT_CTI_STATIC_GROUPS_MAX - 1;
ctidev->con_groups[con_idx] = group;
> + con->attr_group = group;
> + return group;
> +}
> +
> +/* 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, con);
> + if (!attr_group)
> + return -ENOMEM;
> +
> + /* allocate NULL terminated array of attributes */
> + con->con_attrs = kcalloc(CTI_CON_ATTR_MAX + 1,
> + sizeof(struct attribute *),
> + GFP_KERNEL);
Again why not devm_* allocations ? That takes the pain of freeing the
memory away and helps prevent memory leaks.
> + 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 = con->con_attrs;
> + 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 + CORESIGHT_CTI_STATIC_GROUPS_MAX;
> + 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 < (CORESIGHT_CTI_STATIC_GROUPS_MAX - 1); i++)
> + ctidev->con_groups[i] = coresight_cti_groups[i];
> +
> + return 0;
> +}
> +
To be frank, it doesn't make sense to have this split of populating
the groups.
> +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_free_con_attr(struct attribute *con_attr)
> +{
> + struct device_attribute *dattr =
> + container_of(con_attr, struct device_attribute, attr);
> + struct dev_ext_attribute *dev_ext_attr =
> + container_of(dattr, struct dev_ext_attribute, attr);
> + kfree(con_attr->name);
> + kfree(dev_ext_attr);
> +}
> +
> +void cti_free_con_group(struct attribute_group *attr_group)
> +{
> + if (attr_group) {
> + kfree(attr_group->name);
> + kfree(attr_group);
> + }
> +}
> +
> +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_CON_ATTR_MAX; i++) {
> + if (con->con_attrs[i])
> + cti_free_con_attr(con->con_attrs[i]);
> + }
> + kfree(con->con_attrs);
> + }
> + cti_free_con_group(con->attr_group);
> +}
> +
> +void cti_destroy_cons_sysfs(struct cti_device *ctidev)
> +{
> + struct cti_trig_con *tc;
minor nit: Please keep the variable name consistent if possible, helps a
lot with the code following. i.e, tc vs con above in
cti_destroy_cons_attr_set().
> + 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,
> };
> @@ -838,7 +1209,8 @@ static const struct attribute_group coresight_cti_channels_group = {
> .name = "channels",
> };
>
> -const struct attribute_group *coresight_cti_groups[] = {
> +const struct attribute_group *
> +coresight_cti_groups[CORESIGHT_CTI_STATIC_GROUPS_MAX] = {
> &coresight_cti_group,
> &coresight_cti_mgmt_group,
> &coresight_cti_regs_group,
> diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
> index cf116463149a..c3d63cc53bdd 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.c
> +++ b/drivers/hwtracing/coresight/coresight-cti.c
> @@ -561,6 +561,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) {
> @@ -636,12 +639,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);
nit: It may be a good idea to include the actual device name (rather
than just cti_xxx). so may be :
dev_err(dev, "%s:....", cti_desc.name) ?
> + goto err_out;
> + }
Except for the devm_ alloc question, rest are fine.
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-12-02 9:48 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-19 23:18 [PATCH v5 00/14] CoreSight CTI Driver Mike Leach
2019-11-19 23:18 ` [PATCH v5 01/14] coresight: cti: Initial " Mike Leach
2019-11-21 20:21 ` Mathieu Poirier
2019-11-29 12:05 ` Mike Leach
2019-12-03 16:53 ` Mathieu Poirier
2019-11-25 19:03 ` Suzuki Kuruppassery Poulose
2019-11-29 12:06 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 02/14] coresight: cti: Add sysfs coresight mgmt reg access Mike Leach
2019-11-22 17:19 ` Mathieu Poirier
2019-11-19 23:19 ` [PATCH v5 03/14] coresight: cti: Add sysfs access to program function regs Mike Leach
2019-11-27 18:26 ` Suzuki Kuruppassery Poulose
2019-11-29 12:47 ` Mike Leach
2019-11-28 10:54 ` Suzuki Kuruppassery Poulose
2019-11-28 17:20 ` Mathieu Poirier
2019-11-28 18:00 ` Suzuki Kuruppassery Poulose
2019-11-29 12:50 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 04/14] coresight: cti: Add sysfs trigger / channel programming API Mike Leach
2019-11-22 18:40 ` Mathieu Poirier
2019-11-27 18:40 ` Suzuki Kuruppassery Poulose
2019-11-29 13:01 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 05/14] dt-bindings: arm: Adds CoreSight CTI hardware definitions Mike Leach
2019-11-20 19:06 ` Mathieu Poirier
2019-11-20 22:39 ` Mike Leach
2019-11-22 23:33 ` Rob Herring
2019-11-29 13:50 ` Mike Leach
2019-11-29 14:12 ` Suzuki Kuruppassery Poulose
2019-11-28 18:38 ` Suzuki Kuruppassery Poulose
2019-11-29 13:57 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 06/14] coresight: cti: Add device tree support for v8 arch CTI Mike Leach
2019-11-25 19:00 ` Mathieu Poirier
2019-11-29 11:33 ` Suzuki Kuruppassery Poulose
2019-12-03 10:59 ` Mike Leach
2019-12-03 11:28 ` Suzuki Kuruppassery Poulose
2019-12-03 12:25 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 07/14] coresight: cti: Add device tree support for custom CTI Mike Leach
2019-11-25 21:22 ` Mathieu Poirier
2019-11-29 14:16 ` Suzuki Kuruppassery Poulose
2019-11-29 21:11 ` Mathieu Poirier
2019-11-29 14:18 ` Suzuki Kuruppassery Poulose
2019-12-03 14:05 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 08/14] coresight: cti: Enable CTI associated with devices Mike Leach
2019-11-25 22:45 ` Mathieu Poirier
2019-12-05 16:33 ` Mike Leach
2019-11-29 18:28 ` Suzuki Kuruppassery Poulose
2019-11-29 21:25 ` Mathieu Poirier
2019-12-05 16:33 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 09/14] coresight: cti: Add connection information to sysfs Mike Leach
2019-11-27 18:09 ` Mathieu Poirier
2019-12-06 16:24 ` Mike Leach
2019-12-02 9:47 ` Suzuki Kuruppassery Poulose [this message]
2019-12-06 16:24 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 10/14] dt-bindings: qcom: Add CTI options for qcom msm8916 Mike Leach
2019-11-27 18:18 ` Mathieu Poirier
2019-11-19 23:19 ` [PATCH v5 11/14] dt-bindings: arm: Juno platform - add CTI entries to device tree Mike Leach
2019-11-27 18:25 ` Mathieu Poirier
2019-11-19 23:19 ` [PATCH v5 12/14] dt-bindings: hisilicon: Add CTI bindings for hi-6220 Mike Leach
2019-11-19 23:19 ` [PATCH v5 13/14] docs: coresight: Update documentation for CoreSight to cover CTI Mike Leach
2019-11-27 19:00 ` Mathieu Poirier
2019-12-02 10:43 ` Suzuki Kuruppassery Poulose
2019-12-06 17:39 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 14/14] docs: sysfs: coresight: Add sysfs ABI documentation for CTI Mike Leach
2019-11-27 19:08 ` Mathieu Poirier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ad75693c-8c6f-35fd-f9bb-0317c2b4dcd2@arm.com \
--to=suzuki.poulose@arm.com \
--cc=coresight@lists.linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).