Good morning,
On Fri, Oct 30, 2020 at 11:02:05PM +0000, Mike Leach wrote:
Hi Mathieu,
On Fri, 30 Oct 2020 at 17:02, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Thu, Oct 29, 2020 at 06:31:53PM +0000, Mike Leach wrote:
Hi Mathieu, Thanks for looking at this.
However, per your comment in the v1 set, for the next set I have moved management of features out of individual coresight device sysfs, and into a custom configfs directory. This actually makes things easter for the user - they can change a single parameter value and have it change on all device instances of the feature.
Are you pointing out the comments I made in this patch no longer apply because you have changed your design? Have I understood you correctly?
In effect - yes. The design has evolved - and I believe for the better.
In your review of v1 of this set, you expressed a preference for most of the sysfs to be moved to configfs, with some minimal functionality remaining in sysfs. At this point features were adjusted on a per device basis via sysfs - if you have 4 ETMs, there are 4 devices to adjust.
At the time of v2 I hadn't managed to find a credible alternative within configfs that worked. While configfs and sysfs are superficially similar, there are key differences that make configfs far less feature rich than sysfs. With my subsequent work on configfs I did eventually come up with a scheme that let the feature parameters be altered in configfs (as you will see in v3 of the set). This isn't without a certain amount of compromise, so we will see if it is acceptable.
The advantage of the configfs solution, is that a single location will adjust the parameters for all instances of the device.
So if we take the case of the built-in example of strobing - which is where we have existing users with non-upstream patches that hardwire the algorithm into the driver.
In set v2 - a user has a script that will adjust the values of the strobing parameters window and period for each ETM instance in their system, using a script. Running perf then picks up these values on any ETM it happens to schedule trace on. Now with v3, a user changes one location in configfs and running perf will pick up the values on any ETM it happens to schedule trace on. - effectively eliminating the need for the script.
What we lose with the configfs solution is the ability to change just a single instance of the ETM using the feature. But this isn't the way these features are used.
So I had a choice of adding the configfs solution in parallel with the sysfs solution, or withdrawing the sysfs solution to leave configfs as the only way control.
In the circumstances I felt that the best solution was to drop the sysfs support for now. I couldn't see a good reason to have two locations to change the same thing. Should anyone come up with a credible reason it needs to come back then I can re-add it, taking account of the issue you raise.
I do believe that the configfs solution is the better one, which does mean that I have spent time developing something that ultimately will not be used, and unfortunately you have spent time reviewing it.
I'm in total agreement with everything you wrote above.
With regards to time spent on code that won't be used (either writing or reviewing it), that is simply part of working upstream. It is also inherent to the process of finding an optimal solution.
I received your V3 but Suzuki sqeezed in his patchset on system instructions just before yours. As such I won't be able to start looking at it right away but I guarantee it won't take as long as it took for V2.
Mathieu
Regards
Mike
Regards
Mike
On Fri, 9 Oct 2020 at 22:34, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Thu, Aug 27, 2020 at 03:34:07PM +0100, Mike Leach wrote:
Adds a set of generic support functions that allow devices to set and save features values on the device, and enable and disable configurations.
Additional functions for other common operations including feature reset.
Signed-off-by: Mike Leach mike.leach@linaro.org
drivers/hwtracing/coresight/Makefile | 2 +- .../hwtracing/coresight/coresight-config.c | 565 ++++++++++++++++++ .../hwtracing/coresight/coresight-config.h | 16 + 3 files changed, 582 insertions(+), 1 deletion(-) create mode 100644 drivers/hwtracing/coresight/coresight-config.c
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 3605dd770664..177bc6338312 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -4,7 +4,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-sysfs.o coresight-syscfg.o coresight-config.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.c b/drivers/hwtracing/coresight/coresight-config.c new file mode 100644 index 000000000000..96f8df59df58 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-config.c @@ -0,0 +1,565 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright(C) 2020 Linaro Limited. All rights reserved.
- Author: Mike Leach mike.leach@linaro.org
- */
+#include <linux/sysfs.h> +#include "coresight-config.h" +#include "coresight-priv.h"
+static void coresight_cfg_init_reg_param(struct cs_cfg_parameter *param);
+/*
- Handlers for creating and manipulating the feature representation
- in sysfs.
- */
+/* base feature attribute info */ +struct feat_attr_info {
const char *name;
ssize_t (*show)(struct device *dev, struct device_attribute *attr,
char *buf);
ssize_t (*store)(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count);
+};
+static ssize_t cs_cfg_enable_show(struct device *dev,
struct device_attribute *attr,
char *buf)
There is no need to have "char *buf" on a line by itself. Same for cs_cfg_param_show().
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_feature *feat = (struct cs_cfg_feature *)eattr->var;
int val;
spin_lock(feat->dev_spinlock);
I would rename cs_cfg_featre::dev_spinlock to cs_cfg_feature::csdev_spinlock to remove any ambiguity.
val = feat->enabled;
spin_unlock(feat->dev_spinlock);
return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+static ssize_t cs_cfg_enable_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t size)
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_feature *feat = (struct cs_cfg_feature *)eattr->var;
int val;
spin_lock(feat->dev_spinlock);
if (!kstrtoint(buf, 0, &val))
Please call kstrtoint() outside of the lock.
feat->enabled = val;
spin_unlock(feat->dev_spinlock);
return size;
+}
+static ssize_t cs_cfg_desc_show(struct device *dev,
struct device_attribute *attr,
char *buf)
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_feature *feat = (struct cs_cfg_feature *)eattr->var;
return scnprintf(buf, PAGE_SIZE, "%s\n", feat->desc->brief);
+}
+struct feat_attr_info feature_std_attr[] = {
{
.name = "enable",
.show = cs_cfg_enable_show,
.store = cs_cfg_enable_store,
},
{
.name = "description",
.show = cs_cfg_desc_show,
},
{ 0 },
+};
+static ssize_t cs_cfg_param_show(struct device *dev,
struct device_attribute *attr,
char *buf)
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_parameter *param = (struct cs_cfg_parameter *)eattr->var;
u64 val;
spin_lock(param->feat->dev_spinlock);
val = param->current_value;
spin_unlock(param->feat->dev_spinlock);
return scnprintf(buf, PAGE_SIZE, "%lld\n", val);
+}
+static ssize_t cs_cfg_param_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t size)
+{
struct dev_ext_attribute *eattr =
container_of(attr, struct dev_ext_attribute, attr);
struct cs_cfg_parameter *param = (struct cs_cfg_parameter *)eattr->var;
u64 val;
spin_lock(param->feat->dev_spinlock);
if (!kstrtoull(buf, 0, &val)) {
Same here.
param->current_value = val;
coresight_cfg_init_reg_param(param);
}
spin_unlock(param->feat->dev_spinlock);
return size;
+}
+static struct attribute * +cs_cfg_sysfs_create_feat_attr(struct device *dev, struct cs_cfg_feature *feat,
struct feat_attr_info *info)
+{
struct dev_ext_attribute *eattr;
eattr = devm_kzalloc(dev, sizeof(struct dev_ext_attribute),
GFP_KERNEL);
if (!eattr)
return NULL;
For concistency I would have expected a space here, as it is in cs_cfg_sysfs_create_param_attr().
eattr->attr.attr.name = devm_kstrdup(dev, info->name, GFP_KERNEL);
eattr->attr.attr.mode = info->store ? 0644 : 0444;
eattr->attr.show = info->show;
eattr->attr.store = info->store;
eattr->var = feat;
return &eattr->attr.attr;
+}
+static struct attribute * +cs_cfg_sysfs_create_param_attr(struct device *dev, const char *name,
struct cs_cfg_parameter *param)
+{
struct dev_ext_attribute *eattr;
eattr = devm_kzalloc(dev, sizeof(struct dev_ext_attribute),
GFP_KERNEL);
if (!eattr)
return NULL;
eattr->attr.attr.name = devm_kstrdup(dev, name, GFP_KERNEL);
eattr->attr.attr.mode = 0644;
eattr->attr.show = cs_cfg_param_show;
eattr->attr.store = cs_cfg_param_store;
eattr->var = param;
return &eattr->attr.attr;
+}
+int coresight_cfg_sysfs_add_grp(struct cs_cfg_feature *feat) +{
int p, attr_idx = 0;
struct attribute **attrs;
const char *name;
struct feat_attr_info *info;
struct device *dev = feat->csdev->dev.parent;
/* create sysfs group in csdev */
feat->param_group = devm_kzalloc(dev, sizeof(struct attribute_group),
GFP_KERNEL);
if (!feat->param_group)
return -ENOMEM;
feat->param_group->name = devm_kasprintf(dev, GFP_KERNEL, "%s.feat",
feat->desc->name);
if (!feat->param_group->name)
return -ENOMEM;
/* attributes - nr_params + enable + desc */
attrs = devm_kcalloc(dev, feat->nr_params + 2,
sizeof(struct attribute *), GFP_KERNEL);
@attrs needs to be NULL terminated and I'm surprised it hasn't blown up in your face yet (or I've missed the NULL terminated slot).
if (!attrs)
return -ENOMEM;
/* create base attributes in group */
while (feature_std_attr[attr_idx].name) {
info = &feature_std_attr[attr_idx];
attrs[attr_idx] = cs_cfg_sysfs_create_feat_attr(dev, feat,
info);
if (!attrs[attr_idx])
return -ENOMEM;
attr_idx++;
}
/* create params in group */
for (p = 0; p < feat->nr_params; p++) {
name = feat->desc->params[p].name;
attrs[attr_idx] =
cs_cfg_sysfs_create_param_attr(dev, name,
&feat->params[p]);
attrs[attr_idx] = cs_cfg_sysfs_create_param_attr(dev, name, &feat->params[p]);
if (!attrs[attr_idx])
return -ENOMEM;
attr_idx++;
}
feat->param_group->attrs = attrs;
return sysfs_create_group(&feat->csdev->dev.kobj, feat->param_group);
+} +EXPORT_SYMBOL_GPL(coresight_cfg_sysfs_add_grp);
+/*
- standard routines to set/save/reset enabled features, and iterate
- groups of features on a device.
- */
+void coresight_cfg_list_add_feat(struct cs_cfg_feature *feat) +{
spin_lock(&feat->csdev->cfg_lock);
list_add(&feat->node, &feat->csdev->feat_list);
spin_unlock(&feat->csdev->cfg_lock);
+} +EXPORT_SYMBOL_GPL(coresight_cfg_list_add_feat);
+static void coresight_cfg_set_reg(struct cs_cfg_reg *reg, u32 flags) +{
u32 *p_val32 = (u32 *)reg->drv_store;
u32 tmp32;
if (!(flags & CS_CFG_REG_VAL_64BIT)) {
if (flags & CS_CFG_REG_VAL_MASK) {
tmp32 = *p_val32;
tmp32 &= ~reg->value.mask32;
tmp32 |= reg->value.val32 & reg->value.mask32;
*p_val32 = tmp32;
} else
*p_val32 = reg->value.val32;
} else
*((u64 *)reg->drv_store) = reg->value.val64;
+}
+static void coresight_cfg_save_reg(struct cs_cfg_reg *reg, u32 flags) +{
if (flags & CS_CFG_REG_VAL_64BIT)
reg->value.val64 = *(u64 *)(reg->drv_store);
else
reg->value.val32 = *(u32 *)(reg->drv_store);
+}
+static void coresight_cfg_init_reg(struct cs_cfg_reg *reg,
const struct cs_cfg_reg_desc *desc)
Indentation problem.
It is hard to look a the rest of this patch without more context. As such I will come back to it as I look a the other patches in this series.
Thanks, Mathieu
+{
reg->value.val64 = desc->value.val64;
+}
+static void coresight_cfg_init_reg_param(struct cs_cfg_parameter *param) +{
if (param->val64)
param->reg->value.val64 = param->current_value;
else
param->reg->value.val32 = (u32)param->current_value;
+}
+/* default set - will set values without resource checking */ +static int cs_cfg_set_on_enable(struct cs_cfg_feature *feat, const bool force_set) +{
int i;
u32 flags;
spin_lock(feat->dev_spinlock);
if (feat->enabled || force_set) {
for (i = 0; i < feat->nr_regs; i++) {
flags = feat->desc->regs[i].flags;
coresight_cfg_set_reg(&feat->regs[i], flags);
}
}
dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name,
feat->enabled || force_set ? "Set enabled" : "Skip disabled");
spin_unlock(feat->dev_spinlock);
return 0;
+}
+static void cs_cfg_save_on_disable(struct cs_cfg_feature *feat, const bool force_save) +{
int i;
u32 flags;
spin_lock(feat->dev_spinlock);
if (feat->enabled || force_save) {
for (i = 0; i < feat->nr_regs; i++) {
flags = feat->desc->regs[i].flags;
if (flags & CS_CFG_REG_VAL_SAVE)
coresight_cfg_save_reg(&feat->regs[i], flags);
}
}
dev_dbg(&feat->csdev->dev, "Feature %s: %s", feat->desc->name,
feat->enabled || force_save ? "Save disabled" : "Skip disabled");
spin_unlock(feat->dev_spinlock);
+}
+/* default reset - restore default values, disable feature */ +static void cs_cfg_reset_feat(struct cs_cfg_feature *feat) +{
const struct cs_cfg_feature_desc *feat_desc = feat->desc;
struct cs_cfg_reg_desc *reg_desc;
struct cs_cfg_parameter *param;
struct cs_cfg_reg *reg;
int i;
u32 flags;
spin_lock(feat->dev_spinlock);
feat->enabled = false;
/*
* set the default values for all parameters and regs from the
* relevant static descriptors.
*/
for (i = 0; i < feat->nr_params; i++)
feat->params[i].current_value = feat_desc->params[i].value;
for (i = 0; i < feat->nr_regs; i++) {
reg_desc = &feat_desc->regs[i];
flags = reg_desc->flags;
reg = &feat->regs[i];
/* check if reg set from a parameter otherwise desc default */
if (flags & CS_CFG_REG_VAL_PARAM) {
param = &feat->params[reg_desc->value.val32];
param->reg = reg;
param->val64 = flags & CS_CFG_REG_VAL_64BIT;
coresight_cfg_init_reg_param(param);
} else
coresight_cfg_init_reg(reg, reg_desc);
}
spin_unlock(feat->dev_spinlock);
+}
+void coresight_cfg_set_def_ops(struct cs_cfg_feature *feat) +{
feat->ops.set_on_enable = cs_cfg_set_on_enable;
feat->ops.save_on_disable = cs_cfg_save_on_disable;
feat->ops.reset = cs_cfg_reset_feat;
+} +EXPORT_SYMBOL_GPL(coresight_cfg_set_def_ops);
+int coresight_cfg_set_enabled_feats(struct coresight_device *csdev) +{
struct cs_cfg_feature *feat;
int err = 0;
spin_lock(&csdev->cfg_lock);
if (list_empty(&csdev->feat_list)) {
spin_unlock(&csdev->cfg_lock);
return 0;
}
list_for_each_entry(feat, &csdev->feat_list, node) {
dev_dbg(&csdev->dev, "Found feature:%s", feat->desc->name);
if (feat->ops.set_on_enable) {
err = feat->ops.set_on_enable(feat, false);
dev_dbg(&csdev->dev, "Feature %s: %s",
feat->desc->name, err ? "Set failed" : "OK");
if (!err)
break;
}
}
spin_unlock(&csdev->cfg_lock);
return err;
+} +EXPORT_SYMBOL_GPL(coresight_cfg_set_enabled_feats);
+void coresight_cfg_save_enabled_feats(struct coresight_device *csdev) +{
struct cs_cfg_feature *feat;
spin_lock(&csdev->cfg_lock);
if (list_empty(&csdev->feat_list)) {
spin_unlock(&csdev->cfg_lock);
return;
}
list_for_each_entry(feat, &csdev->feat_list, node) {
if (feat->ops.save_on_disable)
feat->ops.save_on_disable(feat, false);
}
spin_unlock(&csdev->cfg_lock);
+} +EXPORT_SYMBOL_GPL(coresight_cfg_save_enabled_feats);
+void coresight_cfg_reset_feats(struct coresight_device *csdev) +{
struct cs_cfg_feature *feat;
spin_lock(&csdev->cfg_lock);
if (list_empty(&csdev->feat_list)) {
spin_unlock(&csdev->cfg_lock);
return;
}
list_for_each_entry(feat, &csdev->feat_list, node) {
if (feat->ops.reset)
feat->ops.reset(feat);
}
spin_unlock(&csdev->cfg_lock);
+} +EXPORT_SYMBOL_GPL(coresight_cfg_reset_feats);
+static int coresight_cfg_update_presets(struct cs_cfg_config_dev *cfg, int preset) +{
int i, j, line_offset = 0, val_idx = 0, max_idx;
u64 val;
struct cs_cfg_feature *feat;
struct cs_cfg_parameter *param;
const char *name;
if (preset > cfg->desc->nr_presets)
return -EINVAL;
/*
* Go through the array of features, assigning preset values to
* feature parameters in the order they appear.
* There should be precisely the same number of preset values as the
* sum of number of parameters over all the features - but we will
* ensure there is no overrun.
*/
line_offset = (preset-1) * cfg->desc->nr_total_params;
max_idx = cfg->desc->nr_total_params;
for (i = 0; i < cfg->nr_feat; i++) {
feat = cfg->feats[i];
if (feat->nr_params) {
spin_lock(feat->dev_spinlock);
for (j = 0; j < feat->nr_params; j++) {
param = &feat->params[j];
name = feat->desc->params[j].name;
val = cfg->desc->presets[line_offset + val_idx++];
if (param->val64) {
dev_dbg(&cfg->csdev->dev,
"set param %s (%lld)", name, val);
param->reg->value.val64 = val;
} else {
param->reg->value.val32 = (u32)val;
dev_dbg(&cfg->csdev->dev,
"set param %s (%d)", name, (u32)val);
}
if (val_idx >= max_idx)
break;
}
spin_unlock(feat->dev_spinlock);
}
/* don't overrun the preset array line */
if (val_idx >= max_idx)
break;
}
return 0;
+}
+/*
- if we are not using a preset, then need to update the feature params
- with current values.
- */
+static int coresight_cfg_update_curr_params(struct cs_cfg_config_dev *cfg) +{
int i, j;
struct cs_cfg_feature *feat;
struct cs_cfg_parameter *param;
const char *name;
u64 val;
for (i = 0; i < cfg->nr_feat; i++) {
feat = cfg->feats[i];
if (feat->nr_params) {
spin_lock(feat->dev_spinlock);
for (j = 0; j < feat->nr_params; j++) {
param = &feat->params[j];
name = feat->desc->params[j].name;
val = param->current_value;
if (param->val64) {
dev_dbg(&cfg->csdev->dev,
"set param %s (%lld)", name, val);
param->reg->value.val64 = val;
} else {
param->reg->value.val32 = (u32)val;
dev_dbg(&cfg->csdev->dev,
"set param %s (%d)", name, (u32)val);
}
}
spin_unlock(feat->dev_spinlock);
}
}
return 0;
+}
+static int coresight_cfg_prog_config(struct cs_cfg_config_dev *cfg, bool enable) +{
int i, err = 0;
struct cs_cfg_feature *feat;
struct coresight_device *csdev;
for (i = 0; i < cfg->nr_feat; i++) {
feat = cfg->feats[i];
csdev = feat->csdev;
dev_dbg(&csdev->dev, "cfg %s; %s feature:%s", cfg->desc->name,
enable ? "enable" : "disable", feat->desc->name);
if (enable) {
if (feat->ops.set_on_enable)
err = feat->ops.set_on_enable(feat, true);
} else {
if (feat->ops.save_on_disable)
feat->ops.save_on_disable(feat, true);
}
if (err)
break;
}
return err;
+}
+/**
- Enable configuration for the device.
- Match the config id and optionally set preset values for parameters.
- @csdev: coresight device to set config on.
- @cfg_id: hash id of configuration.
- @preset: preset values to use - 0 for default.
- */
+int coresight_cfg_enable_dev_config(struct coresight_device *csdev,
unsigned long cfg_id,
int preset)
+{
struct cs_cfg_config_dev *cfg;
int err = 0;
dev_dbg(&csdev->dev, "Check for config %lx, preset %d", cfg_id, preset);
spin_lock(&csdev->cfg_lock);
list_for_each_entry(cfg, &csdev->syscfg_list, node) {
dev_dbg(&csdev->dev, "checking %s", cfg->desc->name);
if (cfg->id_hash == cfg_id) {
if (preset)
err = coresight_cfg_update_presets(cfg, preset);
else
err = coresight_cfg_update_curr_params(cfg);
if (!err)
err = coresight_cfg_prog_config(cfg, true);
if (!err)
cfg->enabled = true;
break;
}
}
spin_unlock(&csdev->cfg_lock);
return err;
+} +EXPORT_SYMBOL_GPL(coresight_cfg_enable_dev_config);
+void coresight_cfg_disable_dev_config(struct coresight_device *csdev) +{
struct cs_cfg_config_dev *cfg;
spin_lock(&csdev->cfg_lock);
list_for_each_entry(cfg, &csdev->syscfg_list, node) {
if (cfg->enabled) {
coresight_cfg_prog_config(cfg, false);
cfg->enabled = false;
}
}
spin_unlock(&csdev->cfg_lock);
+} +EXPORT_SYMBOL_GPL(coresight_cfg_disable_dev_config); diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 949a078c4692..afecab88451a 100644 --- a/drivers/hwtracing/coresight/coresight-config.h +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -270,4 +270,20 @@ struct cs_cfg_dev_feat_ops { const struct cs_cfg_feature_desc *feat_desc); };
+/* coresight config API functions */
+/* helper functions for feature manipulation */ +int coresight_cfg_sysfs_add_grp(struct cs_cfg_feature *feat); +void coresight_cfg_set_def_ops(struct cs_cfg_feature *feat); +void coresight_cfg_list_add_feat(struct cs_cfg_feature *feat);
+/* enable / disable features or configs on a device */ +int coresight_cfg_set_enabled_feats(struct coresight_device *csdev); +void coresight_cfg_save_enabled_feats(struct coresight_device *csdev); +void coresight_cfg_reset_feats(struct coresight_device *csdev); +int coresight_cfg_enable_dev_config(struct coresight_device *csdev,
unsigned long cfg_id,
int preset);
+void coresight_cfg_disable_dev_config(struct coresight_device *csdev);
#endif /* _CORESIGHT_CORESIGHT_CONFIG_H */
2.17.1
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK