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.
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/[email protected]/msg2551216.html
Signed-off-by: Tingwei Zhang <[email protected]>
Signed-off-by: Mao Jinlong <[email protected]>
Signed-off-by: Tao Zhang <[email protected]>
---
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;
+
+ return 0;
+}
+
+static void coresight_reset_all_sink(void)
+{
+ bus_for_each_dev(&coresight_bustype, NULL, NULL, coresight_reset_sink);
+}
+
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);
+
+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);
+ __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)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri, Apr 23, 2021 at 04:29:38PM +0800, 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.
>
> 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/[email protected]/msg2551216.html
>
> Signed-off-by: Tingwei Zhang <[email protected]>
> Signed-off-by: Mao Jinlong <[email protected]>
> Signed-off-by: Tao Zhang <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 72 ++++++++++++++++++++++++----
> 1 file changed, 64 insertions(+), 8 deletions(-)
You added new sysfs files with no new Documentation/ABI entries, so we
have no idea how to review this code to determine if it does what you
want it to do :(
thanks,
greg k-h
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/[email protected]/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 <[email protected]>
> Signed-off-by: Mao Jinlong <[email protected]>
> Signed-off-by: Tao Zhang <[email protected]>
> ---
> 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)
>
On 2021-04-23 21:20, Suzuki K Poulose wrote:
> 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)
>
We have a tool needs a command that could reset all active devices.
Since the tool cannot what dvices are activated, we add this new node
to sysFS for our tool could reset all active device by one command.
We hope that this patch can also provide a more convenient option
for the other users with the same needs.
>> 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/[email protected]/msg2551216.html
>
> Please post related patches as a series, possibly describing the
> overall
> problem that you are trying to solve, in the cover letter.
>
Sure, I will post related patches as a series in patch v2.
>>
>> Signed-off-by: Tingwei Zhang <[email protected]>
>> Signed-off-by: Mao Jinlong <[email protected]>
>> Signed-off-by: Tao Zhang <[email protected]>
>> ---
>> 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 ?
>
coresight_disable_patch will be called before this part of code.
The aim of this patch is to disable all active coresight devices. Reset
all
sinks and disable the sysFS "enable_sink" flag is a part of the aim.
>> +
>> + 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 ?
>
This function will only be called when users need to reset all source
and
sinks. I see that other functions will also disable the active sink, and
then it may also cause all active sinks to be disabled.
Is it possible to provide users with a reminder that it may affect some
sinks could be driven by perf by adding a description of this function?
>> 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 ?
>
I will restore this part of code in patch v2.
>> +
>> +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 ?
>
I think so.
This code "atomic_set(csdev->refcnt, 1);" will be called only
finding source coresight device. We could refer to the function
enable_source_store, it will also set refcnt to 1 before disable
source coresight device.
Best,
Tao
>> + __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)
>>
On 2021-04-23 18:02, Greg Kroah-Hartman wrote:
> On Fri, Apr 23, 2021 at 04:29:38PM +0800, 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.
>>
>> 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/[email protected]/msg2551216.html
>>
>> Signed-off-by: Tingwei Zhang <[email protected]>
>> Signed-off-by: Mao Jinlong <[email protected]>
>> Signed-off-by: Tao Zhang <[email protected]>
>> ---
>> drivers/hwtracing/coresight/coresight-core.c | 72
>> ++++++++++++++++++++++++----
>> 1 file changed, 64 insertions(+), 8 deletions(-)
>
> You added new sysfs files with no new Documentation/ABI entries, so we
> have no idea how to review this code to determine if it does what you
> want it to do :(
>
> thanks,
>
> greg k-h
I will update this part according to your suggestion in patch v2.
Best,
Tao
On 28/04/2021 10:57, [email protected] wrote:
> On 2021-04-23 21:20, Suzuki K Poulose wrote:
>> 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)
>>
> We have a tool needs a command that could reset all active devices.
> Since the tool cannot what dvices are activated, we add this new node
> to sysFS for our tool could reset all active device by one command.
> We hope that this patch can also provide a more convenient option
> for the other users with the same needs.
There is sysfs handles to do this already. See the testcase under perf,
e.g, that can walk the sysfs and figure out the source devices and the
"sinks" that can be reached from the given device.
If some information is missing, to achieve this. we could provide that.
But simply because a tool wants to do something without bothering to use
the provided ABI, is not a justification to add something to the kernel.
Suzuki