Re: [PATCH v1] coresight: add node to reset all coresight devices

On 23/04/2021 09:29, Tao Zhang wrote:

Add new reset_source_sink node to be able to disable all active coresight devices. In this way, we no longer need to manually disable all active coresight devices one by one. After enabling multiple coresight paths, users can reset their status more conveniently by this node.

What is the use case here ? Why would you trigger a reset for all the sources/sink without gracefully completing any on-going sessions (including the perf ones, which are driven by the kernel perf layer)

This patch base on coresight-next repo http://git.linaro.org/kernel/coresight.git/log/?h=next

And this patch depends on the following patch https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2551216.html

Please post related patches as a series, possibly describing the overall problem that you are trying to solve, in the cover letter.

Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Signed-off-by: Mao Jinlong jinlmao@codeaurora.org Signed-off-by: Tao Zhang taozha@codeaurora.org

---

drivers/hwtracing/coresight/coresight-core.c | 72 ++++++++++++++++++++++++---- 1 file changed, 64 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 7dfadb6..0001b6c 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -107,6 +107,23 @@ static int coresight_source_is_unique(struct coresight_device *csdev) csdev, coresight_id_match); } +static int coresight_reset_sink(struct device *dev, void *data) +{

• struct coresight_device *csdev = to_coresight_device(dev);
• if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||

•     csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
  

•     csdev->activated)
  

• csdev->activated = false;
  

Why is this needed, when the disabl_path() should have taken care of this ?

• return 0;

+}

+static void coresight_reset_all_sink(void) +{

• bus_for_each_dev(&coresight_bustype, NULL, NULL, coresight_reset_sink);

+}

How can you disable all the active sinks when some of the sinks could be driven by perf ?

static int coresight_find_link_inport(struct coresight_device *csdev, struct coresight_device *parent) { @@ -1137,7 +1154,7 @@ int coresight_enable(struct coresight_device *csdev) } EXPORT_SYMBOL_GPL(coresight_enable); -void coresight_disable(struct coresight_device *csdev) +static void __coresight_disable(struct coresight_device *csdev) { int ret; struct list_head *path = NULL; @@ -1145,14 +1162,12 @@ void coresight_disable(struct coresight_device *csdev) struct coresight_path *cspath_next = NULL; struct coresight_device *src_csdev = NULL;

• mutex_lock(&coresight_mutex);
• ret = coresight_validate_source(csdev, __func__); if (ret)

• goto out;
  

• return;
  

if (!csdev->enable || !coresight_disable_source(csdev))

• goto out;
  

• return;
  

list_for_each_entry_safe(cspath, cspath_next, &cs_active_paths, link) { src_csdev = coresight_get_source(cspath->path); @@ -1165,12 +1180,16 @@ void coresight_disable(struct coresight_device *csdev) } } if (path == NULL)

• goto out;
  

• return;
  

coresight_disable_path(path); coresight_release_path(path); +} -out: +void coresight_disable(struct coresight_device *csdev) +{

• mutex_lock(&coresight_mutex);
• __coresight_disable(csdev); mutex_unlock(&coresight_mutex); } EXPORT_SYMBOL_GPL(coresight_disable);

@@ -1467,7 +1486,43 @@ int coresight_timeout(void __iomem *addr, u32 offset, int position, int value) return -EAGAIN; } -EXPORT_SYMBOL_GPL(coresight_timeout);

Why ?

+static ssize_t reset_source_sink_store(struct bus_type *bus,

• 		       const char *buf, size_t size)
  

+{

• int ret = 0;
• unsigned long val;
• struct coresight_path *cspath = NULL;
• struct coresight_path *cspath_next = NULL;
• struct coresight_device *csdev;
• ret = kstrtoul(buf, 10, &val);
• if (ret)

• return ret;
  

• mutex_lock(&coresight_mutex);
• list_for_each_entry_safe(cspath, cspath_next, &cs_active_paths, link) {

• csdev = coresight_get_source(cspath->path);
  

• if (!csdev)
  

• 	continue;
  

• atomic_set(csdev->refcnt, 1);
  

Is this safe ?

• __coresight_disable(csdev);
  

• }
• /* Reset all activated sinks */
• coresight_reset_all_sink();
• mutex_unlock(&coresight_mutex);
• return size;

+} +static BUS_ATTR_WO(reset_source_sink);

+static struct attribute *coresight_reset_source_sink_attrs[] = {

• &bus_attr_reset_source_sink.attr,
• NULL,

+}; +ATTRIBUTE_GROUPS(coresight_reset_source_sink); /*

• coresight_release_platform_data: Release references to the devices connected

@@ -1680,6 +1735,7 @@ EXPORT_SYMBOL_GPL(coresight_alloc_device_name); struct bus_type coresight_bustype = { .name = "coresight",

• .bus_groups = coresight_reset_source_sink_groups, };

static int __init coresight_init(void)