| Message ID | 20190122181144.26645-3-mathieu.poirier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | perf: Communicate sink via event::attr:config2 | expand |
Hi Mathieu, On 01/22/2019 06:11 PM, Mathieu Poirier wrote: > Add a "sinks" directory entry so that users can see all the sinks > available in the system in a single place. Individual sink are added > as they are registered with the coresight bus. A couple of minor comments. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > .../hwtracing/coresight/coresight-etm-perf.c | 76 +++++++++++++++++++ > .../hwtracing/coresight/coresight-etm-perf.h | 6 +- > drivers/hwtracing/coresight/coresight.c | 18 +++++ > include/linux/coresight.h | 7 +- > 4 files changed, 104 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index f21eb28b6782..c68a0036532c 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -14,6 +14,7 @@ > #include <linux/perf_event.h> > #include <linux/percpu-defs.h> > #include <linux/slab.h> > +#include <linux/stringhash.h> > #include <linux/types.h> > #include <linux/workqueue.h> > > @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = { > .attrs = etm_config_formats_attr, > }; > > +static struct attribute *etm_config_sinks_attr[] = { > + NULL, > +}; > + > +static const struct attribute_group etm_pmu_sinks_group = { > + .name = "sinks", > + .attrs = etm_config_sinks_attr, > +}; > + > static const struct attribute_group *etm_pmu_attr_groups[] = { > &etm_pmu_format_group, > + &etm_pmu_sinks_group, > NULL, > }; I was thinking if this could be the "events" directory for ETM pmu. We don't have any other event codes. Even if we get in the future, we could expose them here. But from a quick try it looks like the event names cannot start with a number (e.g, 2007000.etr wouldn't parse as an event name). So we could leave this as above and switch when we get generic naming scheme. > > @@ -479,6 +490,71 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link) > return 0; > } > > +static ssize_t etm_perf_sink_name_show(struct device *dev, > + struct device_attribute *dattr, > + char *buf) > +{ > + /* See function coresight_get_sink_by_id() to know where this is used */ > + u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name)); > + > + return scnprintf(buf, PAGE_SIZE, "%x\n", hash); > +} You may need "0x%x" to avoid confusions interpreting the data. > + > +int etm_perf_add_symlink_sink(struct coresight_device *csdev) > +{ > + int ret; > + struct device *pmu_dev = etm_pmu.dev; > + struct device *pdev = csdev->dev.parent; > + struct device_attribute *dattr; If we make this a struct dev_ext_attribute(), you get a space to store the "id" in the "var" field. This can be used for 1) name_show() above we could do : struct dev_ext_attribute *eattr = container_of(attr, struct dev_ext_attribute, attr); return scnprintf(bu, PAGE_SIZE, "0x%x\n", (u32)eattr->var); 2) Matching the ID of a sink device in lookup by simply doing : (u32)csdev->dattr.var == (u32)(void *)data and can avoid computing the hash all the time. > + > + if (csdev->type != CORESIGHT_DEV_TYPE_SINK && > + csdev->type != CORESIGHT_DEV_TYPE_LINKSINK) > + return -EINVAL; > + > + if (csdev->dattr != NULL) > + return -EINVAL; > + > + if (!etm_perf_up) > + return -EPROBE_DEFER; > + > + dattr = kzalloc(sizeof(*dattr), GFP_KERNEL); nit: Could we use devm_kzalloc(pdev, ...) ? > + dattr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);. similarly here : devm_kstrdup() > + dattr->attr.mode = 0444; > + dattr->show = etm_perf_sink_name_show; > + csdev->dattr = dattr; > + > + ret = sysfs_add_file_to_group(&pmu_dev->kobj, > + &dattr->attr, "sinks"); > + > + if (!ret) > + return 0; > + > + csdev->dattr = NULL; > + kfree(dattr->attr.name); > + kfree(dattr); And we could get rid of these ^ > + > + return ret; > +} > + > +void etm_perf_del_symlink_sink(struct coresight_device *csdev) > +{ > + struct device *pmu_dev = etm_pmu.dev; > + struct device_attribute *dattr = csdev->dattr; > + > + if (csdev->type != CORESIGHT_DEV_TYPE_SINK && > + csdev->type != CORESIGHT_DEV_TYPE_LINKSINK) > + return; > + > + if (!dattr) > + return; > + > + sysfs_remove_file_from_group(&pmu_dev->kobj, > + &dattr->attr, "sinks"); > + csdev->dattr = NULL; > + kfree(dattr->attr.name); > + kfree(dattr);ext And these ^^ > +} > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 46c67a764877..a42fac83eac9 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -154,8 +154,9 @@ struct coresight_connection { > * @orphan: true if the component has connections that haven't been linked. > * @enable: 'true' if component is currently part of an active path. > * @activated: 'true' only if a _sink_ has been activated. A sink can be > - activated but not yet enabled. Enabling for a _sink_ > - happens when a source has been selected for that it. > + * activated but not yet enabled. Enabling for a _sink_ > + * appens when a source has been selected for that it. > + * @dattr: Device attribute for sink representation under PMU directory. > */ > struct coresight_device { > struct coresight_connection *conns; > @@ -168,7 +169,9 @@ struct coresight_device { > atomic_t *refcnt; > bool orphan; > bool enable; /* true only if configured as part of a path */ > + /* sink specific fields */ > bool activated; /* true only if a sink is part of a path */ > + struct device_attribute *dattr; See my comment above about switching to struct dev_ext_attribute *. Suzuki
On Wed, 30 Jan 2019 at 10:42, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > Hi Mathieu, > > > On 01/22/2019 06:11 PM, Mathieu Poirier wrote: > > Add a "sinks" directory entry so that users can see all the sinks > > available in the system in a single place. Individual sink are added > > as they are registered with the coresight bus. > > A couple of minor comments. > > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > .../hwtracing/coresight/coresight-etm-perf.c | 76 +++++++++++++++++++ > > .../hwtracing/coresight/coresight-etm-perf.h | 6 +- > > drivers/hwtracing/coresight/coresight.c | 18 +++++ > > include/linux/coresight.h | 7 +- > > 4 files changed, 104 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > > index f21eb28b6782..c68a0036532c 100644 > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > > @@ -14,6 +14,7 @@ > > #include <linux/perf_event.h> > > #include <linux/percpu-defs.h> > > #include <linux/slab.h> > > +#include <linux/stringhash.h> > > #include <linux/types.h> > > #include <linux/workqueue.h> > > > > @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = { > > .attrs = etm_config_formats_attr, > > }; > > > > +static struct attribute *etm_config_sinks_attr[] = { > > + NULL, > > +}; > > + > > +static const struct attribute_group etm_pmu_sinks_group = { > > + .name = "sinks", > > + .attrs = etm_config_sinks_attr, > > +}; > > + > > static const struct attribute_group *etm_pmu_attr_groups[] = { > > &etm_pmu_format_group, > > + &etm_pmu_sinks_group, > > NULL, > > }; > > I was thinking if this could be the "events" directory for ETM pmu. We > don't have any other event codes. Even if we get in the future, we could > expose them here. But from a quick try it looks like the event names > cannot start with a number (e.g, 2007000.etr wouldn't parse as an event > name). So we could leave this as above and switch when we get generic > naming scheme. > > > > > @@ -479,6 +490,71 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link) > > return 0; > > } > > > > +static ssize_t etm_perf_sink_name_show(struct device *dev, > > + struct device_attribute *dattr, > > + char *buf) > > +{ > > + /* See function coresight_get_sink_by_id() to know where this is used */ > > + u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name)); > > + > > + return scnprintf(buf, PAGE_SIZE, "%x\n", hash); > > +} > > You may need "0x%x" to avoid confusions interpreting the data. Yes > > > + > > +int etm_perf_add_symlink_sink(struct coresight_device *csdev) > > +{ > > + int ret; > > + struct device *pmu_dev = etm_pmu.dev; > > + struct device *pdev = csdev->dev.parent; > > + struct device_attribute *dattr; > > If we make this a struct dev_ext_attribute(), you get a space to > store the "id" in the "var" field. This can be used for Much easier - thanks for pointing this out. > > 1) name_show() above > > we could do : > struct dev_ext_attribute *eattr = container_of(attr, > struct dev_ext_attribute, attr); > > return scnprintf(bu, PAGE_SIZE, "0x%x\n", (u32)eattr->var);a Casting with a u32 will make the compiler complain because of the size difference between the types but using a cast type with the same size will do just fine. > > 2) Matching the ID of a sink device in lookup by simply doing : > (u32)csdev->dattr.var == (u32)(void *)data > > and can avoid computing the hash all the time. Same comment as above but with the right cast it will work. > > > + > > + if (csdev->type != CORESIGHT_DEV_TYPE_SINK && > > + csdev->type != CORESIGHT_DEV_TYPE_LINKSINK) > > + return -EINVAL; > > + > > + if (csdev->dattr != NULL) > > + return -EINVAL; > > + > > + if (!etm_perf_up) > > + return -EPROBE_DEFER; > > + > > + dattr = kzalloc(sizeof(*dattr), GFP_KERNEL); > > nit: Could we use devm_kzalloc(pdev, ...) ? > > > + dattr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);. > > > similarly here : devm_kstrdup() Yes > > > + dattr->attr.mode = 0444; > > + dattr->show = etm_perf_sink_name_show; > > + csdev->dattr = dattr; > > + > > + ret = sysfs_add_file_to_group(&pmu_dev->kobj, > > + &dattr->attr, "sinks"); > > + > > + if (!ret) > > + return 0; > > + > > + csdev->dattr = NULL; > > + kfree(dattr->attr.name); > > + kfree(dattr); > > And we could get rid of these ^ > > > + > > + return ret; > > +} > > + > > +void etm_perf_del_symlink_sink(struct coresight_device *csdev) > > +{ > > + struct device *pmu_dev = etm_pmu.dev; > > + struct device_attribute *dattr = csdev->dattr; > > + > > + if (csdev->type != CORESIGHT_DEV_TYPE_SINK && > > + csdev->type != CORESIGHT_DEV_TYPE_LINKSINK) > > + return; > > + > > + if (!dattr) > > + return; > > + > > + sysfs_remove_file_from_group(&pmu_dev->kobj, > > + &dattr->attr, "sinks"); > > + csdev->dattr = NULL; > > + kfree(dattr->attr.name); > > + kfree(dattr);ext > > And these ^^ > > > +} > > > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > > index 46c67a764877..a42fac83eac9 100644 > > --- a/include/linux/coresight.h > > +++ b/include/linux/coresight.h > > @@ -154,8 +154,9 @@ struct coresight_connection { > > * @orphan: true if the component has connections that haven't been linked. > > * @enable: 'true' if component is currently part of an active path. > > * @activated: 'true' only if a _sink_ has been activated. A sink can be > > - activated but not yet enabled. Enabling for a _sink_ > > - happens when a source has been selected for that it. > > + * activated but not yet enabled. Enabling for a _sink_ > > + * appens when a source has been selected for that it. > > + * @dattr: Device attribute for sink representation under PMU directory. > > */ > > struct coresight_device { > > struct coresight_connection *conns; > > @@ -168,7 +169,9 @@ struct coresight_device { > > atomic_t *refcnt; > > bool orphan; > > bool enable; /* true only if configured as part of a path */ > > + /* sink specific fields */ > > bool activated; /* true only if a sink is part of a path */ > > + struct device_attribute *dattr; > > See my comment above about switching to struct dev_ext_attribute *. > > Suzuki
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index f21eb28b6782..c68a0036532c 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -14,6 +14,7 @@ #include <linux/perf_event.h> #include <linux/percpu-defs.h> #include <linux/slab.h> +#include <linux/stringhash.h> #include <linux/types.h> #include <linux/workqueue.h> @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = { .attrs = etm_config_formats_attr, }; +static struct attribute *etm_config_sinks_attr[] = { + NULL, +}; + +static const struct attribute_group etm_pmu_sinks_group = { + .name = "sinks", + .attrs = etm_config_sinks_attr, +}; + static const struct attribute_group *etm_pmu_attr_groups[] = { &etm_pmu_format_group, + &etm_pmu_sinks_group, NULL, }; @@ -479,6 +490,71 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link) return 0; } +static ssize_t etm_perf_sink_name_show(struct device *dev, + struct device_attribute *dattr, + char *buf) +{ + /* See function coresight_get_sink_by_id() to know where this is used */ + u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name)); + + return scnprintf(buf, PAGE_SIZE, "%x\n", hash); +} + +int etm_perf_add_symlink_sink(struct coresight_device *csdev) +{ + int ret; + struct device *pmu_dev = etm_pmu.dev; + struct device *pdev = csdev->dev.parent; + struct device_attribute *dattr; + + if (csdev->type != CORESIGHT_DEV_TYPE_SINK && + csdev->type != CORESIGHT_DEV_TYPE_LINKSINK) + return -EINVAL; + + if (csdev->dattr != NULL) + return -EINVAL; + + if (!etm_perf_up) + return -EPROBE_DEFER; + + dattr = kzalloc(sizeof(*dattr), GFP_KERNEL); + dattr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL); + dattr->attr.mode = 0444; + dattr->show = etm_perf_sink_name_show; + csdev->dattr = dattr; + + ret = sysfs_add_file_to_group(&pmu_dev->kobj, + &dattr->attr, "sinks"); + + if (!ret) + return 0; + + csdev->dattr = NULL; + kfree(dattr->attr.name); + kfree(dattr); + + return ret; +} + +void etm_perf_del_symlink_sink(struct coresight_device *csdev) +{ + struct device *pmu_dev = etm_pmu.dev; + struct device_attribute *dattr = csdev->dattr; + + if (csdev->type != CORESIGHT_DEV_TYPE_SINK && + csdev->type != CORESIGHT_DEV_TYPE_LINKSINK) + return; + + if (!dattr) + return; + + sysfs_remove_file_from_group(&pmu_dev->kobj, + &dattr->attr, "sinks"); + csdev->dattr = NULL; + kfree(dattr->attr.name); + kfree(dattr); +} + static int __init etm_perf_init(void) { int ret; diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index da7d9336a15c..015213abe00a 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -59,6 +59,8 @@ struct etm_event_data { #ifdef CONFIG_CORESIGHT int etm_perf_symlink(struct coresight_device *csdev, bool link); +int etm_perf_add_symlink_sink(struct coresight_device *csdev); +void etm_perf_del_symlink_sink(struct coresight_device *csdev); static inline void *etm_perf_sink_config(struct perf_output_handle *handle) { struct etm_event_data *data = perf_get_aux(handle); @@ -70,7 +72,9 @@ static inline void *etm_perf_sink_config(struct perf_output_handle *handle) #else static inline int etm_perf_symlink(struct coresight_device *csdev, bool link) { return -EINVAL; } - +int etm_perf_add_symlink_sink(struct coresight_device *csdev) +{ return -EINVAL; } +void etm_perf_del_symlink_sink(struct coresight_device *csdev) {} static inline void *etm_perf_sink_config(struct perf_output_handle *handle) { return NULL; diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 2b0df1a0a8df..d7fa90be6f42 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -18,6 +18,7 @@ #include <linux/delay.h> #include <linux/pm_runtime.h> +#include "coresight-etm-perf.h" #include "coresight-priv.h" static DEFINE_MUTEX(coresight_mutex); @@ -1167,6 +1168,22 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) goto err_out; } + if (csdev->type == CORESIGHT_DEV_TYPE_SINK || + csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) { + ret = etm_perf_add_symlink_sink(csdev); + + if (ret) { + device_unregister(&csdev->dev); + /* + * As with the above, all resources are free'd + * explicitly via coresight_device_release() triggered + * from put_device(), which is in turn called from + * function device_unregister(). + */ + goto err_out; + } + } + mutex_lock(&coresight_mutex); coresight_fixup_device_conns(csdev); @@ -1185,6 +1202,7 @@ EXPORT_SYMBOL_GPL(coresight_register); void coresight_unregister(struct coresight_device *csdev) { + etm_perf_del_symlink_sink(csdev); /* Remove references of that device in the topology */ coresight_remove_conns(csdev); device_unregister(&csdev->dev); diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 46c67a764877..a42fac83eac9 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -154,8 +154,9 @@ struct coresight_connection { * @orphan: true if the component has connections that haven't been linked. * @enable: 'true' if component is currently part of an active path. * @activated: 'true' only if a _sink_ has been activated. A sink can be - activated but not yet enabled. Enabling for a _sink_ - happens when a source has been selected for that it. + * activated but not yet enabled. Enabling for a _sink_ + * appens when a source has been selected for that it. + * @dattr: Device attribute for sink representation under PMU directory. */ struct coresight_device { struct coresight_connection *conns; @@ -168,7 +169,9 @@ struct coresight_device { atomic_t *refcnt; bool orphan; bool enable; /* true only if configured as part of a path */ + /* sink specific fields */ bool activated; /* true only if a sink is part of a path */ + struct device_attribute *dattr; }; #define to_coresight_device(d) container_of(d, struct coresight_device, dev)
Add a "sinks" directory entry so that users can see all the sinks available in the system in a single place. Individual sink are added as they are registered with the coresight bus. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- .../hwtracing/coresight/coresight-etm-perf.c | 76 +++++++++++++++++++ .../hwtracing/coresight/coresight-etm-perf.h | 6 +- drivers/hwtracing/coresight/coresight.c | 18 +++++ include/linux/coresight.h | 7 +- 4 files changed, 104 insertions(+), 3 deletions(-) -- 2.17.1