* [PATCH 1/8] coresight: Fix issue where a source device's helpers aren't disabled
2023-12-12 15:53 [PATCH 0/8] coresight: Separate sysfs and Perf usage and some other cleanups James Clark
@ 2023-12-12 15:53 ` James Clark
2023-12-12 17:44 ` Suzuki K Poulose
2023-12-12 15:53 ` [PATCH 2/8] coresight: Make language around "activated" sinks consistent James Clark
` (6 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2023-12-12 15:53 UTC (permalink / raw)
To: coresight, suzuki.poulose
Cc: James Clark, Mike Leach, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, linux-arm-kernel, linux-kernel, linux-stm32
The linked commit reverts the change that accidentally used some sysfs
enable/disable functions from Perf which broke the refcounting, but it
also removes the fact that the sysfs disable function disabled the
helpers.
Add a new wrapper function that does both which is used by both Perf and
sysfs, and label the sysfs disable function appropriately. The naming of
all of the functions will be tidied up later to avoid this happening
again.
Fixes: 287e82cf69aa ("coresight: Fix crash when Perf and sysfs modes are used concurrently")
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-core.c | 30 ++++++++++++++-----
.../hwtracing/coresight/coresight-etm-perf.c | 2 +-
drivers/hwtracing/coresight/coresight-priv.h | 2 +-
3 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index d7f0e231feb9..965bb6d4e1bf 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -441,8 +441,26 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
}
}
+/*
+ * Helper function to call source_ops(csdev)->disable and also disable the
+ * helpers.
+ *
+ * There is an imbalance between coresight_enable_path() and
+ * coresight_disable_path(). Enabling also enables the source's helpers as part
+ * of the path, but disabling always skips the first item in the path (which is
+ * the source), so sources and their helpers don't get disabled as part of that
+ * function and we need the extra step here.
+ */
+void coresight_disable_source(struct coresight_device *csdev, void *data)
+{
+ if (source_ops(csdev)->disable)
+ source_ops(csdev)->disable(csdev, data);
+ coresight_disable_helpers(csdev);
+}
+EXPORT_SYMBOL_GPL(coresight_disable_source);
+
/**
- * coresight_disable_source - Drop the reference count by 1 and disable
+ * coresight_disable_source_sysfs - Drop the reference count by 1 and disable
* the device if there are no users left.
*
* @csdev: The coresight device to disable
@@ -451,17 +469,15 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
*
* Returns true if the device has been disabled.
*/
-bool coresight_disable_source(struct coresight_device *csdev, void *data)
+static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
+ void *data)
{
if (atomic_dec_return(&csdev->refcnt) == 0) {
- if (source_ops(csdev)->disable)
- source_ops(csdev)->disable(csdev, data);
- coresight_disable_helpers(csdev);
+ coresight_disable_source(csdev, data);
csdev->enable = false;
}
return !csdev->enable;
}
-EXPORT_SYMBOL_GPL(coresight_disable_source);
/*
* coresight_disable_path_from : Disable components in the given path beyond
@@ -1204,7 +1220,7 @@ void coresight_disable(struct coresight_device *csdev)
if (ret)
goto out;
- if (!csdev->enable || !coresight_disable_source(csdev, NULL))
+ if (!csdev->enable || !coresight_disable_source_sysfs(csdev, NULL))
goto out;
switch (csdev->subtype.source_subtype) {
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index a52cfcce25d6..c0c60e6a1703 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -589,7 +589,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
return;
/* stop tracer */
- source_ops(csdev)->disable(csdev, event);
+ coresight_disable_source(csdev, event);
/* tell the core */
event->hw.state = PERF_HES_STOPPED;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 767076e07970..30c051055e54 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -233,6 +233,6 @@ void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev);
struct coresight_device *coresight_get_percpu_sink(int cpu);
int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
void *data);
-bool coresight_disable_source(struct coresight_device *csdev, void *data);
+void coresight_disable_source(struct coresight_device *csdev, void *data);
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/8] coresight: Fix issue where a source device's helpers aren't disabled
2023-12-12 15:53 ` [PATCH 1/8] coresight: Fix issue where a source device's helpers aren't disabled James Clark
@ 2023-12-12 17:44 ` Suzuki K Poulose
2023-12-13 13:54 ` James Clark
0 siblings, 1 reply; 22+ messages in thread
From: Suzuki K Poulose @ 2023-12-12 17:44 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
Hi James
On 12/12/2023 15:53, James Clark wrote:
> The linked commit reverts the change that accidentally used some sysfs
> enable/disable functions from Perf which broke the refcounting, but it
> also removes the fact that the sysfs disable function disabled the
> helpers.
>
> Add a new wrapper function that does both which is used by both Perf and
> sysfs, and label the sysfs disable function appropriately. The naming of
> all of the functions will be tidied up later to avoid this happening
> again.
>
> Fixes: 287e82cf69aa ("coresight: Fix crash when Perf and sysfs modes are used concurrently")
But we still don't "enable" the helpers from perf mode with this patch.
i.e., we use source_ops()->enable directly. So, I guess this patch
doesn't fix a bug as such. But that said, it would be good to
enable/disable helpers for sources, in perf mode.
Suzuki
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 30 ++++++++++++++-----
> .../hwtracing/coresight/coresight-etm-perf.c | 2 +-
> drivers/hwtracing/coresight/coresight-priv.h | 2 +-
> 3 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index d7f0e231feb9..965bb6d4e1bf 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -441,8 +441,26 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
> }
> }
>
> +/*
> + * Helper function to call source_ops(csdev)->disable and also disable the
> + * helpers.
> + *
> + * There is an imbalance between coresight_enable_path() and
> + * coresight_disable_path(). Enabling also enables the source's helpers as part
> + * of the path, but disabling always skips the first item in the path (which is
> + * the source), so sources and their helpers don't get disabled as part of that
> + * function and we need the extra step here.
> + */
> +void coresight_disable_source(struct coresight_device *csdev, void *data)
> +{
> + if (source_ops(csdev)->disable)
> + source_ops(csdev)->disable(csdev, data);
> + coresight_disable_helpers(csdev);
> +}
> +EXPORT_SYMBOL_GPL(coresight_disable_source);
> +
> /**
> - * coresight_disable_source - Drop the reference count by 1 and disable
> + * coresight_disable_source_sysfs - Drop the reference count by 1 and disable
> * the device if there are no users left.
> *
> * @csdev: The coresight device to disable
> @@ -451,17 +469,15 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
> *
> * Returns true if the device has been disabled.
> */
> -bool coresight_disable_source(struct coresight_device *csdev, void *data)
> +static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
> + void *data)
> {
> if (atomic_dec_return(&csdev->refcnt) == 0) {
> - if (source_ops(csdev)->disable)
> - source_ops(csdev)->disable(csdev, data);
> - coresight_disable_helpers(csdev);
> + coresight_disable_source(csdev, data);
> csdev->enable = false;
> }
> return !csdev->enable;
> }
> -EXPORT_SYMBOL_GPL(coresight_disable_source);
>
> /*
> * coresight_disable_path_from : Disable components in the given path beyond
> @@ -1204,7 +1220,7 @@ void coresight_disable(struct coresight_device *csdev)
> if (ret)
> goto out;
>
> - if (!csdev->enable || !coresight_disable_source(csdev, NULL))
> + if (!csdev->enable || !coresight_disable_source_sysfs(csdev, NULL))
> goto out;
>
> switch (csdev->subtype.source_subtype) {
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index a52cfcce25d6..c0c60e6a1703 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -589,7 +589,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
> return;
>
> /* stop tracer */
> - source_ops(csdev)->disable(csdev, event);
> + coresight_disable_source(csdev, event);
Does this result i
>
> /* tell the core */
> event->hw.state = PERF_HES_STOPPED;
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 767076e07970..30c051055e54 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -233,6 +233,6 @@ void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev);
> struct coresight_device *coresight_get_percpu_sink(int cpu);
> int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
> void *data);
> -bool coresight_disable_source(struct coresight_device *csdev, void *data);
> +void coresight_disable_source(struct coresight_device *csdev, void *data);
>
> #endif
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/8] coresight: Fix issue where a source device's helpers aren't disabled
2023-12-12 17:44 ` Suzuki K Poulose
@ 2023-12-13 13:54 ` James Clark
2023-12-13 16:28 ` Suzuki K Poulose
0 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2023-12-13 13:54 UTC (permalink / raw)
To: Suzuki K Poulose, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
On 12/12/2023 17:44, Suzuki K Poulose wrote:
> Hi James
>
> On 12/12/2023 15:53, James Clark wrote:
>> The linked commit reverts the change that accidentally used some sysfs
>> enable/disable functions from Perf which broke the refcounting, but it
>> also removes the fact that the sysfs disable function disabled the
>> helpers.
>
>
>>
>> Add a new wrapper function that does both which is used by both Perf and
>> sysfs, and label the sysfs disable function appropriately. The naming of
>> all of the functions will be tidied up later to avoid this happening
>> again.
>>
>> Fixes: 287e82cf69aa ("coresight: Fix crash when Perf and sysfs modes
>> are used concurrently")
>
> But we still don't "enable" the helpers from perf mode with this patch.
> i.e., we use source_ops()->enable directly. So, I guess this patch
> doesn't fix a bug as such. But that said, it would be good to
> enable/disable helpers for sources, in perf mode.
>
> Suzuki
We do, it happens in coresight_enable_path() which Perf uses. I added
the comment below about that.
[...]
>> +/*
>> + * Helper function to call source_ops(csdev)->disable and also
>> disable the
>> + * helpers.
>> + *
>> + * There is an imbalance between coresight_enable_path() and
>> + * coresight_disable_path(). Enabling also enables the source's
>> helpers as part
>> + * of the path, but disabling always skips the first item in the path
>> (which is
>> + * the source), so sources and their helpers don't get disabled as
>> part of that
>> + * function and we need the extra step here.
>> + */
The reason coresight_disable_path() skips the first item is because it's
used after errors where a path is only partially enabled and it unwinds,
skipping the last item, because the last item didn't enable.
It seemed a bit more intrusive to change that, and it's already working.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/8] coresight: Fix issue where a source device's helpers aren't disabled
2023-12-13 13:54 ` James Clark
@ 2023-12-13 16:28 ` Suzuki K Poulose
0 siblings, 0 replies; 22+ messages in thread
From: Suzuki K Poulose @ 2023-12-13 16:28 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
On 13/12/2023 13:54, James Clark wrote:
>
>
> On 12/12/2023 17:44, Suzuki K Poulose wrote:
>> Hi James
>>
>> On 12/12/2023 15:53, James Clark wrote:
>>> The linked commit reverts the change that accidentally used some sysfs
>>> enable/disable functions from Perf which broke the refcounting, but it
>>> also removes the fact that the sysfs disable function disabled the
>>> helpers.
>>
>>
>>>
>>> Add a new wrapper function that does both which is used by both Perf and
>>> sysfs, and label the sysfs disable function appropriately. The naming of
>>> all of the functions will be tidied up later to avoid this happening
>>> again.
>>>
>>> Fixes: 287e82cf69aa ("coresight: Fix crash when Perf and sysfs modes
>>> are used concurrently")
>>
>> But we still don't "enable" the helpers from perf mode with this patch.
>> i.e., we use source_ops()->enable directly. So, I guess this patch
>> doesn't fix a bug as such. But that said, it would be good to
>> enable/disable helpers for sources, in perf mode.
>>
>> Suzuki
>
> We do, it happens in coresight_enable_path() which Perf uses. I added
> the comment below about that.
Ah, I see. That indeed is a bit confusing. And I think all users of
coresight_enable_path() enables the source right after. So, I don't
see any point in having it separate. That said, this fix makes sense
and apologies for the confusion. We could may be cleanup the
enable_path() to enable the source too, in a separate patch.
Suzuki
>
> [...]
>
>>> +/*
>>> + * Helper function to call source_ops(csdev)->disable and also
>>> disable the
>>> + * helpers.
>>> + *
>>> + * There is an imbalance between coresight_enable_path() and
>>> + * coresight_disable_path(). Enabling also enables the source's
>>> helpers as part
>>> + * of the path, but disabling always skips the first item in the path
>>> (which is
>>> + * the source), so sources and their helpers don't get disabled as
>>> part of that
>>> + * function and we need the extra step here.
>>> + */
>
> The reason coresight_disable_path() skips the first item is because it's
> used after errors where a path is only partially enabled and it unwinds,
> skipping the last item, because the last item didn't enable.
>
> It seemed a bit more intrusive to change that, and it's already working.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/8] coresight: Make language around "activated" sinks consistent
2023-12-12 15:53 [PATCH 0/8] coresight: Separate sysfs and Perf usage and some other cleanups James Clark
2023-12-12 15:53 ` [PATCH 1/8] coresight: Fix issue where a source device's helpers aren't disabled James Clark
@ 2023-12-12 15:53 ` James Clark
2024-01-08 11:21 ` Suzuki K Poulose
2023-12-12 15:54 ` [PATCH 3/8] coresight: Remove ops callback checks James Clark
` (5 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2023-12-12 15:53 UTC (permalink / raw)
To: coresight, suzuki.poulose
Cc: James Clark, Mike Leach, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, linux-arm-kernel, linux-kernel, linux-stm32
Activated has the specific meaning of a sink that's selected for use by
the user via sysfs. But comments in some code that's shared by Perf use
the same word, so in those cases change them to just say "selected"
instead. With selected implying either via Perf or "activated" via
sysfs.
coresight_get_enabled_sink() doesn't actually get an enabled sink, it
only gets an activated one, so change that too.
And change the activated variable name to include "sysfs" so it can't
be confused as a general status.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-core.c | 51 ++++++++------------
drivers/hwtracing/coresight/coresight-priv.h | 2 -
include/linux/coresight.h | 14 +++---
3 files changed, 27 insertions(+), 40 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 965bb6d4e1bf..f79aa9ff9b64 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -500,7 +500,7 @@ static void coresight_disable_path_from(struct list_head *path,
/*
* ETF devices are tricky... They can be a link or a sink,
* depending on how they are configured. If an ETF has been
- * "activated" it will be configured as a sink, otherwise
+ * selected as a sink it will be configured as a sink, otherwise
* go ahead with the link configuration.
*/
if (type == CORESIGHT_DEV_TYPE_LINKSINK)
@@ -578,7 +578,7 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
/*
* ETF devices are tricky... They can be a link or a sink,
* depending on how they are configured. If an ETF has been
- * "activated" it will be configured as a sink, otherwise
+ * selected as a sink it will be configured as a sink, otherwise
* go ahead with the link configuration.
*/
if (type == CORESIGHT_DEV_TYPE_LINKSINK)
@@ -635,15 +635,21 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
return csdev;
}
+/**
+ * coresight_find_activated_sysfs_sink - returns the first sink activated via
+ * sysfs using connection based search starting from the source reference.
+ *
+ * @csdev: Coresight source device reference
+ */
static struct coresight_device *
-coresight_find_enabled_sink(struct coresight_device *csdev)
+coresight_find_activated_sysfs_sink(struct coresight_device *csdev)
{
int i;
struct coresight_device *sink = NULL;
if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
- csdev->activated)
+ csdev->sysfs_sink_activated)
return csdev;
/*
@@ -654,7 +660,7 @@ coresight_find_enabled_sink(struct coresight_device *csdev)
child_dev = csdev->pdata->out_conns[i]->dest_dev;
if (child_dev)
- sink = coresight_find_enabled_sink(child_dev);
+ sink = coresight_find_activated_sysfs_sink(child_dev);
if (sink)
return sink;
}
@@ -662,21 +668,6 @@ coresight_find_enabled_sink(struct coresight_device *csdev)
return NULL;
}
-/**
- * coresight_get_enabled_sink - returns the first enabled sink using
- * connection based search starting from the source reference
- *
- * @source: Coresight source device reference
- */
-struct coresight_device *
-coresight_get_enabled_sink(struct coresight_device *source)
-{
- if (!source)
- return NULL;
-
- return coresight_find_enabled_sink(source);
-}
-
static int coresight_sink_by_id(struct device *dev, const void *data)
{
struct coresight_device *csdev = to_coresight_device(dev);
@@ -810,11 +801,10 @@ static void coresight_drop_device(struct coresight_device *csdev)
* @sink: The final sink we want in this path.
* @path: The list to add devices to.
*
- * The tree of Coresight device is traversed until an activated sink is
- * found. From there the sink is added to the list along with all the
- * devices that led to that point - the end result is a list from source
- * to sink. In that list the source is the first device and the sink the
- * last one.
+ * The tree of Coresight device is traversed until the selected sink is found.
+ * From there the sink is added to the list along with all the devices that led
+ * to that point - the end result is a list from source to sink. In that list
+ * the source is the first device and the sink the last one.
*/
static int _coresight_build_path(struct coresight_device *csdev,
struct coresight_device *sink,
@@ -824,7 +814,7 @@ static int _coresight_build_path(struct coresight_device *csdev,
bool found = false;
struct coresight_node *node;
- /* An activated sink has been found. Enqueue the element */
+ /* The selected sink has been found. Enqueue the element */
if (csdev == sink)
goto out;
@@ -1145,7 +1135,7 @@ int coresight_enable(struct coresight_device *csdev)
goto out;
}
- sink = coresight_get_enabled_sink(csdev);
+ sink = coresight_find_activated_sysfs_sink(csdev);
if (!sink) {
ret = -EINVAL;
goto out;
@@ -1259,7 +1249,7 @@ static ssize_t enable_sink_show(struct device *dev,
{
struct coresight_device *csdev = to_coresight_device(dev);
- return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->activated);
+ return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->sysfs_sink_activated);
}
static ssize_t enable_sink_store(struct device *dev,
@@ -1274,10 +1264,7 @@ static ssize_t enable_sink_store(struct device *dev,
if (ret)
return ret;
- if (val)
- csdev->activated = true;
- else
- csdev->activated = false;
+ csdev->sysfs_sink_activated = !!val;
return size;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 30c051055e54..ced5be05a527 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -130,8 +130,6 @@ void coresight_disable_path(struct list_head *path);
int coresight_enable_path(struct list_head *path, enum cs_mode mode,
void *sink_data);
struct coresight_device *coresight_get_sink(struct list_head *path);
-struct coresight_device *
-coresight_get_enabled_sink(struct coresight_device *source);
struct coresight_device *coresight_get_sink_by_id(u32 id);
struct coresight_device *
coresight_find_default_sink(struct coresight_device *csdev);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index a4cb7dd6ca23..65131bfbd904 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -229,10 +229,12 @@ struct coresight_sysfs_link {
* @refcnt: keep track of what is in use.
* @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 and a path is enabled
- * from source to that sink.
+ * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
+ * by writing a 1 to the 'enable_sink' file. A sink can be
+ * activated but not yet enabled. Enabling for a _sink_ happens
+ * when a source has been selected and a path is enabled from
+ * source to that sink. A sink can also become enabled but not
+ * activated if it's used via Perf.
* @ea: Device attribute for sink representation under PMU directory.
* @def_sink: cached reference to default sink found for this device.
* @nr_links: number of sysfs links created to other components from this
@@ -252,9 +254,9 @@ struct coresight_device {
struct device dev;
atomic_t refcnt;
bool orphan;
- bool enable; /* true only if configured as part of a path */
+ bool enable;
/* sink specific fields */
- bool activated; /* true only if a sink is part of a path */
+ bool sysfs_sink_activated;
struct dev_ext_attribute *ea;
struct coresight_device *def_sink;
/* sysfs links between components */
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 2/8] coresight: Make language around "activated" sinks consistent
2023-12-12 15:53 ` [PATCH 2/8] coresight: Make language around "activated" sinks consistent James Clark
@ 2024-01-08 11:21 ` Suzuki K Poulose
2024-01-24 11:10 ` James Clark
0 siblings, 1 reply; 22+ messages in thread
From: Suzuki K Poulose @ 2024-01-08 11:21 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
Hi James,
On 12/12/2023 15:53, James Clark wrote:
> Activated has the specific meaning of a sink that's selected for use by
> the user via sysfs. But comments in some code that's shared by Perf use
> the same word, so in those cases change them to just say "selected"
> instead. With selected implying either via Perf or "activated" via
> sysfs.
>
> coresight_get_enabled_sink() doesn't actually get an enabled sink, it
> only gets an activated one, so change that too.
>
These changes look good to me. Please see a minor nit below.
> And change the activated variable name to include "sysfs" so it can't
> be confused as a general status.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 51 ++++++++------------
> drivers/hwtracing/coresight/coresight-priv.h | 2 -
> include/linux/coresight.h | 14 +++---
> 3 files changed, 27 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 965bb6d4e1bf..f79aa9ff9b64 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -500,7 +500,7 @@ static void coresight_disable_path_from(struct list_head *path,
> /*
> * ETF devices are tricky... They can be a link or a sink,
> * depending on how they are configured. If an ETF has been
> - * "activated" it will be configured as a sink, otherwise
> + * selected as a sink it will be configured as a sink, otherwise
> * go ahead with the link configuration.
> */
> if (type == CORESIGHT_DEV_TYPE_LINKSINK)
> @@ -578,7 +578,7 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
> /*
> * ETF devices are tricky... They can be a link or a sink,
> * depending on how they are configured. If an ETF has been
> - * "activated" it will be configured as a sink, otherwise
> + * selected as a sink it will be configured as a sink, otherwise
> * go ahead with the link configuration.
> */
> if (type == CORESIGHT_DEV_TYPE_LINKSINK)
> @@ -635,15 +635,21 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
> return csdev;
> }
>
> +/**
> + * coresight_find_activated_sysfs_sink - returns the first sink activated via
> + * sysfs using connection based search starting from the source reference.
> + *
> + * @csdev: Coresight source device reference
> + */
> static struct coresight_device *
> -coresight_find_enabled_sink(struct coresight_device *csdev)
> +coresight_find_activated_sysfs_sink(struct coresight_device *csdev)
> {
> int i;
> struct coresight_device *sink = NULL;
>
> if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
> csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
> - csdev->activated)
> + csdev->sysfs_sink_activated)
> return csdev;
>
> /*
> @@ -654,7 +660,7 @@ coresight_find_enabled_sink(struct coresight_device *csdev)
>
> child_dev = csdev->pdata->out_conns[i]->dest_dev;
> if (child_dev)
> - sink = coresight_find_enabled_sink(child_dev);
> + sink = coresight_find_activated_sysfs_sink(child_dev);
> if (sink)
> return sink;
> }
> @@ -662,21 +668,6 @@ coresight_find_enabled_sink(struct coresight_device *csdev)
> return NULL;
> }
>
> -/**
> - * coresight_get_enabled_sink - returns the first enabled sink using
> - * connection based search starting from the source reference
> - *
> - * @source: Coresight source device reference
> - */
> -struct coresight_device *
> -coresight_get_enabled_sink(struct coresight_device *source)
> -{
> - if (!source)
> - return NULL;
> -
> - return coresight_find_enabled_sink(source);
> -}
> -
> static int coresight_sink_by_id(struct device *dev, const void *data)
> {
> struct coresight_device *csdev = to_coresight_device(dev);
> @@ -810,11 +801,10 @@ static void coresight_drop_device(struct coresight_device *csdev)
> * @sink: The final sink we want in this path.
> * @path: The list to add devices to.
> *
> - * The tree of Coresight device is traversed until an activated sink is
> - * found. From there the sink is added to the list along with all the
> - * devices that led to that point - the end result is a list from source
> - * to sink. In that list the source is the first device and the sink the
> - * last one.
> + * The tree of Coresight device is traversed until the selected sink is found.
minor nit: s/until the selected/until *a* selected/
There could be multiple sinks activated that can be reached from the
source. But we choose the "closest" possible selected sink for a source.
So, it may be better to drop "the".
> + * From there the sink is added to the list along with all the devices that led
> + * to that point - the end result is a list from source to sink. In that list
> + * the source is the first device and the sink the last one.
> */
> static int _coresight_build_path(struct coresight_device *csdev,
> struct coresight_device *sink,
> @@ -824,7 +814,7 @@ static int _coresight_build_path(struct coresight_device *csdev,
> bool found = false;
> struct coresight_node *node;
>
> - /* An activated sink has been found. Enqueue the element */
> + /* The selected sink has been found. Enqueue the element */
Similarly here.
Rest looks fine to me.
Suzuki
> if (csdev == sink)
> goto out;
>
> @@ -1145,7 +1135,7 @@ int coresight_enable(struct coresight_device *csdev)
> goto out;
> }
>
> - sink = coresight_get_enabled_sink(csdev);
> + sink = coresight_find_activated_sysfs_sink(csdev);
> if (!sink) {
> ret = -EINVAL;
> goto out;
> @@ -1259,7 +1249,7 @@ static ssize_t enable_sink_show(struct device *dev,
> {
> struct coresight_device *csdev = to_coresight_device(dev);
>
> - return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->activated);
> + return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->sysfs_sink_activated);
> }
>
> static ssize_t enable_sink_store(struct device *dev,
> @@ -1274,10 +1264,7 @@ static ssize_t enable_sink_store(struct device *dev,
> if (ret)
> return ret;
>
> - if (val)
> - csdev->activated = true;
> - else
> - csdev->activated = false;
> + csdev->sysfs_sink_activated = !!val;
>
> return size;
>
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 30c051055e54..ced5be05a527 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -130,8 +130,6 @@ void coresight_disable_path(struct list_head *path);
> int coresight_enable_path(struct list_head *path, enum cs_mode mode,
> void *sink_data);
> struct coresight_device *coresight_get_sink(struct list_head *path);
> -struct coresight_device *
> -coresight_get_enabled_sink(struct coresight_device *source);
> struct coresight_device *coresight_get_sink_by_id(u32 id);
> struct coresight_device *
> coresight_find_default_sink(struct coresight_device *csdev);
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index a4cb7dd6ca23..65131bfbd904 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -229,10 +229,12 @@ struct coresight_sysfs_link {
> * @refcnt: keep track of what is in use.
> * @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 and a path is enabled
> - * from source to that sink.
> + * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
> + * by writing a 1 to the 'enable_sink' file. A sink can be
> + * activated but not yet enabled. Enabling for a _sink_ happens
> + * when a source has been selected and a path is enabled from
> + * source to that sink. A sink can also become enabled but not
> + * activated if it's used via Perf.
> * @ea: Device attribute for sink representation under PMU directory.
> * @def_sink: cached reference to default sink found for this device.
> * @nr_links: number of sysfs links created to other components from this
> @@ -252,9 +254,9 @@ struct coresight_device {
> struct device dev;
> atomic_t refcnt;
> bool orphan;
> - bool enable; /* true only if configured as part of a path */
> + bool enable;
> /* sink specific fields */
> - bool activated; /* true only if a sink is part of a path */
> + bool sysfs_sink_activated;
> struct dev_ext_attribute *ea;
> struct coresight_device *def_sink;
> /* sysfs links between components */
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/8] coresight: Make language around "activated" sinks consistent
2024-01-08 11:21 ` Suzuki K Poulose
@ 2024-01-24 11:10 ` James Clark
0 siblings, 0 replies; 22+ messages in thread
From: James Clark @ 2024-01-24 11:10 UTC (permalink / raw)
To: Suzuki K Poulose, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
On 08/01/2024 11:21, Suzuki K Poulose wrote:
> Hi James,
>
>
> On 12/12/2023 15:53, James Clark wrote:
>> Activated has the specific meaning of a sink that's selected for use by
>> the user via sysfs. But comments in some code that's shared by Perf use
>> the same word, so in those cases change them to just say "selected"
>> instead. With selected implying either via Perf or "activated" via
>> sysfs.
>>
>> coresight_get_enabled_sink() doesn't actually get an enabled sink, it
>> only gets an activated one, so change that too.
>>
>
> These changes look good to me. Please see a minor nit below.
>
>> And change the activated variable name to include "sysfs" so it can't
>> be confused as a general status.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> drivers/hwtracing/coresight/coresight-core.c | 51 ++++++++------------
>> drivers/hwtracing/coresight/coresight-priv.h | 2 -
>> include/linux/coresight.h | 14 +++---
>> 3 files changed, 27 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 965bb6d4e1bf..f79aa9ff9b64 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -500,7 +500,7 @@ static void coresight_disable_path_from(struct
>> list_head *path,
>> /*
>> * ETF devices are tricky... They can be a link or a sink,
>> * depending on how they are configured. If an ETF has been
>> - * "activated" it will be configured as a sink, otherwise
>> + * selected as a sink it will be configured as a sink, otherwise
>> * go ahead with the link configuration.
>> */
>> if (type == CORESIGHT_DEV_TYPE_LINKSINK)
>> @@ -578,7 +578,7 @@ int coresight_enable_path(struct list_head *path,
>> enum cs_mode mode,
>> /*
>> * ETF devices are tricky... They can be a link or a sink,
>> * depending on how they are configured. If an ETF has been
>> - * "activated" it will be configured as a sink, otherwise
>> + * selected as a sink it will be configured as a sink, otherwise
>> * go ahead with the link configuration.
>> */
>> if (type == CORESIGHT_DEV_TYPE_LINKSINK)
>> @@ -635,15 +635,21 @@ struct coresight_device
>> *coresight_get_sink(struct list_head *path)
>> return csdev;
>> }
>> +/**
>> + * coresight_find_activated_sysfs_sink - returns the first sink
>> activated via
>> + * sysfs using connection based search starting from the source
>> reference.
>> + *
>> + * @csdev: Coresight source device reference
>> + */
>> static struct coresight_device *
>> -coresight_find_enabled_sink(struct coresight_device *csdev)
>> +coresight_find_activated_sysfs_sink(struct coresight_device *csdev)
>> {
>> int i;
>> struct coresight_device *sink = NULL;
>> if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
>> csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
>> - csdev->activated)
>> + csdev->sysfs_sink_activated)
>> return csdev;
>> /*
>> @@ -654,7 +660,7 @@ coresight_find_enabled_sink(struct
>> coresight_device *csdev)
>> child_dev = csdev->pdata->out_conns[i]->dest_dev;
>> if (child_dev)
>> - sink = coresight_find_enabled_sink(child_dev);
>> + sink = coresight_find_activated_sysfs_sink(child_dev);
>> if (sink)
>> return sink;
>> }
>> @@ -662,21 +668,6 @@ coresight_find_enabled_sink(struct
>> coresight_device *csdev)
>> return NULL;
>> }
>> -/**
>> - * coresight_get_enabled_sink - returns the first enabled sink using
>> - * connection based search starting from the source reference
>> - *
>> - * @source: Coresight source device reference
>> - */
>> -struct coresight_device *
>> -coresight_get_enabled_sink(struct coresight_device *source)
>> -{
>> - if (!source)
>> - return NULL;
>> -
>> - return coresight_find_enabled_sink(source);
>> -}
>> -
>> static int coresight_sink_by_id(struct device *dev, const void *data)
>> {
>> struct coresight_device *csdev = to_coresight_device(dev);
>> @@ -810,11 +801,10 @@ static void coresight_drop_device(struct
>> coresight_device *csdev)
>> * @sink: The final sink we want in this path.
>> * @path: The list to add devices to.
>> *
>> - * The tree of Coresight device is traversed until an activated sink is
>> - * found. From there the sink is added to the list along with all the
>> - * devices that led to that point - the end result is a list from source
>> - * to sink. In that list the source is the first device and the sink the
>> - * last one.
>> + * The tree of Coresight device is traversed until the selected sink
>> is found.
>
> minor nit: s/until the selected/until *a* selected/
>
> There could be multiple sinks activated that can be reached from the
> source. But we choose the "closest" possible selected sink for a source.
> So, it may be better to drop "the".
>
>
For _coresight_build_path() this isn't strictly true because the sink is
passed in as an argument so there is only one. The only function that
behaves like that is coresight_find_activated_sysfs_sink() which already
seems to be documented properly.
To clarify it I'll just change "until the selected" to "until @sink" is
found.
>> + * From there the sink is added to the list along with all the
>> devices that led
>> + * to that point - the end result is a list from source to sink. In
>> that list
>> + * the source is the first device and the sink the last one.
>> */
>> static int _coresight_build_path(struct coresight_device *csdev,
>> struct coresight_device *sink,
>> @@ -824,7 +814,7 @@ static int _coresight_build_path(struct
>> coresight_device *csdev,
>> bool found = false;
>> struct coresight_node *node;
>> - /* An activated sink has been found. Enqueue the element */
>> + /* The selected sink has been found. Enqueue the element */
>
> Similarly here.
>
Same as above.
> Rest looks fine to me.
>
> Suzuki
>
>
>> if (csdev == sink)
>> goto out;
>> @@ -1145,7 +1135,7 @@ int coresight_enable(struct coresight_device
>> *csdev)
>> goto out;
>> }
>> - sink = coresight_get_enabled_sink(csdev);
>> + sink = coresight_find_activated_sysfs_sink(csdev);
>> if (!sink) {
>> ret = -EINVAL;
>> goto out;
>> @@ -1259,7 +1249,7 @@ static ssize_t enable_sink_show(struct device *dev,
>> {
>> struct coresight_device *csdev = to_coresight_device(dev);
>> - return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->activated);
>> + return scnprintf(buf, PAGE_SIZE, "%u\n",
>> csdev->sysfs_sink_activated);
>> }
>> static ssize_t enable_sink_store(struct device *dev,
>> @@ -1274,10 +1264,7 @@ static ssize_t enable_sink_store(struct device
>> *dev,
>> if (ret)
>> return ret;
>> - if (val)
>> - csdev->activated = true;
>> - else
>> - csdev->activated = false;
>> + csdev->sysfs_sink_activated = !!val;
>> return size;
>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h
>> b/drivers/hwtracing/coresight/coresight-priv.h
>> index 30c051055e54..ced5be05a527 100644
>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> @@ -130,8 +130,6 @@ void coresight_disable_path(struct list_head *path);
>> int coresight_enable_path(struct list_head *path, enum cs_mode mode,
>> void *sink_data);
>> struct coresight_device *coresight_get_sink(struct list_head *path);
>> -struct coresight_device *
>> -coresight_get_enabled_sink(struct coresight_device *source);
>> struct coresight_device *coresight_get_sink_by_id(u32 id);
>> struct coresight_device *
>> coresight_find_default_sink(struct coresight_device *csdev);
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index a4cb7dd6ca23..65131bfbd904 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -229,10 +229,12 @@ struct coresight_sysfs_link {
>> * @refcnt: keep track of what is in use.
>> * @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 and a path is enabled
>> - * from source to that sink.
>> + * @sysfs_sink_activated: 'true' when a sink has been selected for
>> use via sysfs
>> + * by writing a 1 to the 'enable_sink' file. A sink can be
>> + * activated but not yet enabled. Enabling for a _sink_ happens
>> + * when a source has been selected and a path is enabled from
>> + * source to that sink. A sink can also become enabled but not
>> + * activated if it's used via Perf.
>> * @ea: Device attribute for sink representation under PMU
>> directory.
>> * @def_sink: cached reference to default sink found for this
>> device.
>> * @nr_links: number of sysfs links created to other components
>> from this
>> @@ -252,9 +254,9 @@ struct coresight_device {
>> struct device dev;
>> atomic_t refcnt;
>> bool orphan;
>> - bool enable; /* true only if configured as part of a path */
>> + bool enable;
>> /* sink specific fields */
>> - bool activated; /* true only if a sink is part of a path */
>> + bool sysfs_sink_activated;
>> struct dev_ext_attribute *ea;
>> struct coresight_device *def_sink;
>> /* sysfs links between components */
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/8] coresight: Remove ops callback checks
2023-12-12 15:53 [PATCH 0/8] coresight: Separate sysfs and Perf usage and some other cleanups James Clark
2023-12-12 15:53 ` [PATCH 1/8] coresight: Fix issue where a source device's helpers aren't disabled James Clark
2023-12-12 15:53 ` [PATCH 2/8] coresight: Make language around "activated" sinks consistent James Clark
@ 2023-12-12 15:54 ` James Clark
2023-12-12 15:54 ` [PATCH 4/8] coresight: Move mode to struct coresight_device James Clark
` (4 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: James Clark @ 2023-12-12 15:54 UTC (permalink / raw)
To: coresight, suzuki.poulose
Cc: James Clark, Mike Leach, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, linux-arm-kernel, linux-kernel, linux-stm32
The check for the existence of callbacks before using them implies that
this happens and is supported. There are no devices without
enable/disable callbacks, and it wouldn't be possible to add a new
working device without adding them either, so just remove them.
Furthermore, there are more callbacks than just enable and disable that
are already used unguarded in other places.
The comment about new session compatibility doesn't seem to match up to
the line of code that it's on so remove it. I think it's alluding to the
fact that sinks will check if they were already enabled via sysfs or
Perf and fail the enable. But there are more detailed comments at those
places, and this one isn't very useful.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-core.c | 51 +++++---------------
1 file changed, 12 insertions(+), 39 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index f79aa9ff9b64..ab226441e5f4 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -279,16 +279,8 @@ EXPORT_SYMBOL_GPL(coresight_add_helper);
static int coresight_enable_sink(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
- int ret;
-
- /*
- * We need to make sure the "new" session is compatible with the
- * existing "mode" of operation.
- */
- if (!sink_ops(csdev)->enable)
- return -EINVAL;
+ int ret = sink_ops(csdev)->enable(csdev, mode, data);
- ret = sink_ops(csdev)->enable(csdev, mode, data);
if (ret)
return ret;
@@ -299,12 +291,7 @@ static int coresight_enable_sink(struct coresight_device *csdev,
static void coresight_disable_sink(struct coresight_device *csdev)
{
- int ret;
-
- if (!sink_ops(csdev)->disable)
- return;
-
- ret = sink_ops(csdev)->disable(csdev);
+ int ret = sink_ops(csdev)->disable(csdev);
if (ret)
return;
csdev->enable = false;
@@ -330,11 +317,9 @@ static int coresight_enable_link(struct coresight_device *csdev,
if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT && IS_ERR(outconn))
return PTR_ERR(outconn);
- if (link_ops(csdev)->enable) {
- ret = link_ops(csdev)->enable(csdev, inconn, outconn);
- if (!ret)
- csdev->enable = true;
- }
+ ret = link_ops(csdev)->enable(csdev, inconn, outconn);
+ if (!ret)
+ csdev->enable = true;
return ret;
}
@@ -354,9 +339,7 @@ static void coresight_disable_link(struct coresight_device *csdev,
outconn = coresight_find_out_connection(csdev, child);
link_subtype = csdev->subtype.link_subtype;
- if (link_ops(csdev)->disable) {
- link_ops(csdev)->disable(csdev, inconn, outconn);
- }
+ link_ops(csdev)->disable(csdev, inconn, outconn);
if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
for (i = 0; i < csdev->pdata->nr_inconns; i++)
@@ -382,11 +365,9 @@ int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
int ret;
if (!csdev->enable) {
- if (source_ops(csdev)->enable) {
- ret = source_ops(csdev)->enable(csdev, data, mode);
- if (ret)
- return ret;
- }
+ ret = source_ops(csdev)->enable(csdev, data, mode);
+ if (ret)
+ return ret;
csdev->enable = true;
}
@@ -404,11 +385,8 @@ static bool coresight_is_helper(struct coresight_device *csdev)
static int coresight_enable_helper(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
- int ret;
+ int ret = helper_ops(csdev)->enable(csdev, mode, data);
- if (!helper_ops(csdev)->enable)
- return 0;
- ret = helper_ops(csdev)->enable(csdev, mode, data);
if (ret)
return ret;
@@ -418,12 +396,8 @@ static int coresight_enable_helper(struct coresight_device *csdev,
static void coresight_disable_helper(struct coresight_device *csdev)
{
- int ret;
-
- if (!helper_ops(csdev)->disable)
- return;
+ int ret = helper_ops(csdev)->disable(csdev, NULL);
- ret = helper_ops(csdev)->disable(csdev, NULL);
if (ret)
return;
csdev->enable = false;
@@ -453,8 +427,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
*/
void coresight_disable_source(struct coresight_device *csdev, void *data)
{
- if (source_ops(csdev)->disable)
- source_ops(csdev)->disable(csdev, data);
+ source_ops(csdev)->disable(csdev, data);
coresight_disable_helpers(csdev);
}
EXPORT_SYMBOL_GPL(coresight_disable_source);
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 4/8] coresight: Move mode to struct coresight_device
2023-12-12 15:53 [PATCH 0/8] coresight: Separate sysfs and Perf usage and some other cleanups James Clark
` (2 preceding siblings ...)
2023-12-12 15:54 ` [PATCH 3/8] coresight: Remove ops callback checks James Clark
@ 2023-12-12 15:54 ` James Clark
2024-01-08 11:32 ` Suzuki K Poulose
2023-12-12 15:54 ` [PATCH 5/8] coresight: Remove the 'enable' field James Clark
` (3 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2023-12-12 15:54 UTC (permalink / raw)
To: coresight, suzuki.poulose
Cc: James Clark, Mike Leach, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, linux-arm-kernel, linux-kernel, linux-stm32
Most devices use mode, so move the mode definition out of the individual
devices and up to the Coresight device. This will allow the core code to
also know the mode which will be useful in a later commit.
This also fixes the inconsistency of the documentation of the mode field
on the individual device types. For example ETB10 had "this ETB is being
used".
Two devices didn't require an atomic mode type, so these usages have
been converted to atomic_get() and atomic_set() only to make it compile,
but the documentation of the field in struct coresight_device explains
this type of usage.
In the future, manipulation of the mode could be completely moved out of
the individual devices and into the core code because it's almost all
duplicate code, and this change is a step towards that.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-etb10.c | 18 ++++++-------
drivers/hwtracing/coresight/coresight-etm.h | 2 --
.../coresight/coresight-etm3x-core.c | 13 +++++-----
.../coresight/coresight-etm3x-sysfs.c | 4 +--
drivers/hwtracing/coresight/coresight-stm.c | 20 +++++++-------
.../hwtracing/coresight/coresight-tmc-core.c | 2 +-
.../hwtracing/coresight/coresight-tmc-etf.c | 26 +++++++++----------
.../hwtracing/coresight/coresight-tmc-etr.c | 20 +++++++-------
drivers/hwtracing/coresight/coresight-tmc.h | 2 --
drivers/hwtracing/coresight/ultrasoc-smb.c | 13 +++++-----
drivers/hwtracing/coresight/ultrasoc-smb.h | 2 --
include/linux/coresight.h | 6 +++++
12 files changed, 62 insertions(+), 66 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index fa80039e0821..08e5bba862db 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -76,7 +76,6 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "etb");
* @pid: Process ID of the process being monitored by the session
* that is using this component.
* @buf: area of memory where ETB buffer content gets sent.
- * @mode: this ETB is being used.
* @buffer_depth: size of @buf.
* @trigger_cntr: amount of words to store after a trigger.
*/
@@ -89,7 +88,6 @@ struct etb_drvdata {
local_t reading;
pid_t pid;
u8 *buf;
- u32 mode;
u32 buffer_depth;
u32 trigger_cntr;
};
@@ -150,17 +148,17 @@ static int etb_enable_sysfs(struct coresight_device *csdev)
spin_lock_irqsave(&drvdata->spinlock, flags);
/* Don't messup with perf sessions. */
- if (drvdata->mode == CS_MODE_PERF) {
+ if (local_read(&csdev->mode) == CS_MODE_PERF) {
ret = -EBUSY;
goto out;
}
- if (drvdata->mode == CS_MODE_DISABLED) {
+ if (local_read(&csdev->mode) == CS_MODE_DISABLED) {
ret = etb_enable_hw(drvdata);
if (ret)
goto out;
- drvdata->mode = CS_MODE_SYSFS;
+ local_set(&csdev->mode, CS_MODE_SYSFS);
}
atomic_inc(&csdev->refcnt);
@@ -181,7 +179,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
spin_lock_irqsave(&drvdata->spinlock, flags);
/* No need to continue if the component is already in used by sysFS. */
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
ret = -EBUSY;
goto out;
}
@@ -216,7 +214,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
if (!ret) {
/* Associate with monitored process. */
drvdata->pid = pid;
- drvdata->mode = CS_MODE_PERF;
+ local_set(&drvdata->csdev->mode, CS_MODE_PERF);
atomic_inc(&csdev->refcnt);
}
@@ -362,11 +360,11 @@ static int etb_disable(struct coresight_device *csdev)
}
/* Complain if we (somehow) got out of sync */
- WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
+ WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
etb_disable_hw(drvdata);
/* Dissociate from monitored process. */
drvdata->pid = -1;
- drvdata->mode = CS_MODE_DISABLED;
+ local_set(&csdev->mode, CS_MODE_DISABLED);
spin_unlock_irqrestore(&drvdata->spinlock, flags);
dev_dbg(&csdev->dev, "ETB disabled\n");
@@ -589,7 +587,7 @@ static void etb_dump(struct etb_drvdata *drvdata)
unsigned long flags;
spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
__etb_disable_hw(drvdata);
etb_dump_hw(drvdata);
__etb_enable_hw(drvdata);
diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h
index 9a0d08b092ae..e02c3ea972c9 100644
--- a/drivers/hwtracing/coresight/coresight-etm.h
+++ b/drivers/hwtracing/coresight/coresight-etm.h
@@ -215,7 +215,6 @@ struct etm_config {
* @port_size: port size as reported by ETMCR bit 4-6 and 21.
* @arch: ETM/PTM version number.
* @use_cpu14: true if management registers need to be accessed via CP14.
- * @mode: this tracer's mode, i.e sysFS, Perf or disabled.
* @sticky_enable: true if ETM base configuration has been done.
* @boot_enable:true if we should start tracing at boot time.
* @os_unlock: true if access to management registers is allowed.
@@ -238,7 +237,6 @@ struct etm_drvdata {
int port_size;
u8 arch;
bool use_cp14;
- local_t mode;
bool sticky_enable;
bool boot_enable;
bool os_unlock;
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index 116a91d90ac2..29f62dfac673 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -559,7 +559,7 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event,
u32 val;
struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- val = local_cmpxchg(&drvdata->mode, CS_MODE_DISABLED, mode);
+ val = local_cmpxchg(&drvdata->csdev->mode, CS_MODE_DISABLED, mode);
/* Someone is already using the tracer */
if (val)
@@ -578,7 +578,7 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event,
/* The tracer didn't start */
if (ret)
- local_set(&drvdata->mode, CS_MODE_DISABLED);
+ local_set(&drvdata->csdev->mode, CS_MODE_DISABLED);
return ret;
}
@@ -672,14 +672,13 @@ static void etm_disable(struct coresight_device *csdev,
struct perf_event *event)
{
enum cs_mode mode;
- struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
/*
* For as long as the tracer isn't disabled another entity can't
* change its status. As such we can read the status here without
* fearing it will change under us.
*/
- mode = local_read(&drvdata->mode);
+ mode = local_read(&csdev->mode);
switch (mode) {
case CS_MODE_DISABLED:
@@ -696,7 +695,7 @@ static void etm_disable(struct coresight_device *csdev,
}
if (mode)
- local_set(&drvdata->mode, CS_MODE_DISABLED);
+ local_set(&csdev->mode, CS_MODE_DISABLED);
}
static const struct coresight_ops_source etm_source_ops = {
@@ -730,7 +729,7 @@ static int etm_starting_cpu(unsigned int cpu)
etmdrvdata[cpu]->os_unlock = true;
}
- if (local_read(&etmdrvdata[cpu]->mode))
+ if (local_read(&etmdrvdata[cpu]->csdev->mode))
etm_enable_hw(etmdrvdata[cpu]);
spin_unlock(&etmdrvdata[cpu]->spinlock);
return 0;
@@ -742,7 +741,7 @@ static int etm_dying_cpu(unsigned int cpu)
return 0;
spin_lock(&etmdrvdata[cpu]->spinlock);
- if (local_read(&etmdrvdata[cpu]->mode))
+ if (local_read(&etmdrvdata[cpu]->csdev->mode))
etm_disable_hw(etmdrvdata[cpu]);
spin_unlock(&etmdrvdata[cpu]->spinlock);
return 0;
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
index 2f271b7fb048..6c8429c980b1 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
@@ -722,7 +722,7 @@ static ssize_t cntr_val_show(struct device *dev,
struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
struct etm_config *config = &drvdata->config;
- if (!local_read(&drvdata->mode)) {
+ if (!local_read(&drvdata->csdev->mode)) {
spin_lock(&drvdata->spinlock);
for (i = 0; i < drvdata->nr_cntr; i++)
ret += sprintf(buf, "counter %d: %x\n",
@@ -941,7 +941,7 @@ static ssize_t seq_curr_state_show(struct device *dev,
struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
struct etm_config *config = &drvdata->config;
- if (!local_read(&drvdata->mode)) {
+ if (!local_read(&drvdata->csdev->mode)) {
val = config->seq_curr_state;
goto out;
}
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index a1c27c901ad1..190ba569b05a 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -119,7 +119,6 @@ DEFINE_CORESIGHT_DEVLIST(stm_devs, "stm");
* @spinlock: only one at a time pls.
* @chs: the channels accociated to this STM.
* @stm: structure associated to the generic STM interface.
- * @mode: this tracer's mode (enum cs_mode), i.e sysFS, or disabled.
* @traceid: value of the current ID for this component.
* @write_bytes: Maximus bytes this STM can write at a time.
* @stmsper: settings for register STMSPER.
@@ -136,7 +135,6 @@ struct stm_drvdata {
spinlock_t spinlock;
struct channel_space chs;
struct stm_data stm;
- local_t mode;
u8 traceid;
u32 write_bytes;
u32 stmsper;
@@ -201,7 +199,7 @@ static int stm_enable(struct coresight_device *csdev, struct perf_event *event,
if (mode != CS_MODE_SYSFS)
return -EINVAL;
- val = local_cmpxchg(&drvdata->mode, CS_MODE_DISABLED, mode);
+ val = local_cmpxchg(&csdev->mode, CS_MODE_DISABLED, mode);
/* Someone is already using the tracer */
if (val)
@@ -266,7 +264,7 @@ static void stm_disable(struct coresight_device *csdev,
* change its status. As such we can read the status here without
* fearing it will change under us.
*/
- if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
+ if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
spin_lock(&drvdata->spinlock);
stm_disable_hw(drvdata);
spin_unlock(&drvdata->spinlock);
@@ -276,7 +274,7 @@ static void stm_disable(struct coresight_device *csdev,
pm_runtime_put(csdev->dev.parent);
- local_set(&drvdata->mode, CS_MODE_DISABLED);
+ local_set(&csdev->mode, CS_MODE_DISABLED);
dev_dbg(&csdev->dev, "STM tracing disabled\n");
}
}
@@ -373,7 +371,7 @@ static long stm_generic_set_options(struct stm_data *stm_data,
{
struct stm_drvdata *drvdata = container_of(stm_data,
struct stm_drvdata, stm);
- if (!(drvdata && local_read(&drvdata->mode)))
+ if (!(drvdata && local_read(&drvdata->csdev->mode)))
return -EINVAL;
if (channel >= drvdata->numsp)
@@ -408,7 +406,7 @@ static ssize_t notrace stm_generic_packet(struct stm_data *stm_data,
struct stm_drvdata, stm);
unsigned int stm_flags;
- if (!(drvdata && local_read(&drvdata->mode)))
+ if (!(drvdata && local_read(&drvdata->csdev->mode)))
return -EACCES;
if (channel >= drvdata->numsp)
@@ -515,7 +513,7 @@ static ssize_t port_select_show(struct device *dev,
struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
unsigned long val;
- if (!local_read(&drvdata->mode)) {
+ if (!local_read(&drvdata->csdev->mode)) {
val = drvdata->stmspscr;
} else {
spin_lock(&drvdata->spinlock);
@@ -541,7 +539,7 @@ static ssize_t port_select_store(struct device *dev,
spin_lock(&drvdata->spinlock);
drvdata->stmspscr = val;
- if (local_read(&drvdata->mode)) {
+ if (local_read(&drvdata->csdev->mode)) {
CS_UNLOCK(drvdata->base);
/* Process as per ARM's TRM recommendation */
stmsper = readl_relaxed(drvdata->base + STMSPER);
@@ -562,7 +560,7 @@ static ssize_t port_enable_show(struct device *dev,
struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
unsigned long val;
- if (!local_read(&drvdata->mode)) {
+ if (!local_read(&drvdata->csdev->mode)) {
val = drvdata->stmsper;
} else {
spin_lock(&drvdata->spinlock);
@@ -588,7 +586,7 @@ static ssize_t port_enable_store(struct device *dev,
spin_lock(&drvdata->spinlock);
drvdata->stmsper = val;
- if (local_read(&drvdata->mode)) {
+ if (local_read(&drvdata->csdev->mode)) {
CS_UNLOCK(drvdata->base);
writel_relaxed(drvdata->stmsper, drvdata->base + STMSPER);
CS_LOCK(drvdata->base);
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 7ec5365e2b64..e5d47f61f9f3 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -558,7 +558,7 @@ static void tmc_shutdown(struct amba_device *adev)
spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->mode == CS_MODE_DISABLED)
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_DISABLED)
goto out;
if (drvdata->config_type == TMC_CONFIG_TYPE_ETR)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 7406b65e2cdd..2a7e516052a2 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -89,7 +89,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
* When operating in sysFS mode the content of the buffer needs to be
* read before the TMC is disabled.
*/
- if (drvdata->mode == CS_MODE_SYSFS)
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
tmc_etb_dump_hw(drvdata);
tmc_disable_hw(drvdata);
@@ -205,7 +205,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
* sink is already enabled no memory is needed and the HW need not be
* touched.
*/
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
atomic_inc(&csdev->refcnt);
goto out;
}
@@ -228,7 +228,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
ret = tmc_etb_enable_hw(drvdata);
if (!ret) {
- drvdata->mode = CS_MODE_SYSFS;
+ local_set(&csdev->mode, CS_MODE_SYSFS);
atomic_inc(&csdev->refcnt);
} else {
/* Free up the buffer if we failed to enable */
@@ -262,7 +262,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
* No need to continue if the ETB/ETF is already operated
* from sysFS.
*/
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
ret = -EBUSY;
break;
}
@@ -292,7 +292,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
if (!ret) {
/* Associate with monitored process. */
drvdata->pid = pid;
- drvdata->mode = CS_MODE_PERF;
+ local_set(&csdev->mode, CS_MODE_PERF);
atomic_inc(&csdev->refcnt);
}
} while (0);
@@ -344,11 +344,11 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
}
/* Complain if we (somehow) got out of sync */
- WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
+ WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
tmc_etb_disable_hw(drvdata);
/* Dissociate from monitored process. */
drvdata->pid = -1;
- drvdata->mode = CS_MODE_DISABLED;
+ local_set(&csdev->mode, CS_MODE_DISABLED);
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -374,7 +374,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
if (atomic_read(&csdev->refcnt) == 0) {
ret = tmc_etf_enable_hw(drvdata);
if (!ret) {
- drvdata->mode = CS_MODE_SYSFS;
+ local_set(&csdev->mode, CS_MODE_SYSFS);
first_enable = true;
}
}
@@ -403,7 +403,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
if (atomic_dec_return(&csdev->refcnt) == 0) {
tmc_etf_disable_hw(drvdata);
- drvdata->mode = CS_MODE_DISABLED;
+ local_set(&csdev->mode, CS_MODE_DISABLED);
last_disable = true;
}
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -483,7 +483,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
return 0;
/* This shouldn't happen */
- if (WARN_ON_ONCE(drvdata->mode != CS_MODE_PERF))
+ if (WARN_ON_ONCE(local_read(&csdev->mode) != CS_MODE_PERF))
return 0;
spin_lock_irqsave(&drvdata->spinlock, flags);
@@ -629,7 +629,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
}
/* Don't interfere if operated from Perf */
- if (drvdata->mode == CS_MODE_PERF) {
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_PERF) {
ret = -EINVAL;
goto out;
}
@@ -641,7 +641,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
}
/* Disable the TMC if need be */
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
/* There is no point in reading a TMC in HW FIFO mode */
mode = readl_relaxed(drvdata->base + TMC_MODE);
if (mode != TMC_MODE_CIRCULAR_BUFFER) {
@@ -673,7 +673,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
spin_lock_irqsave(&drvdata->spinlock, flags);
/* Re-enable the TMC if need be */
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
/* There is no point in reading a TMC in HW FIFO mode */
mode = readl_relaxed(drvdata->base + TMC_MODE);
if (mode != TMC_MODE_CIRCULAR_BUFFER) {
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index af02ba5d5f15..3dc989d4fcab 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1143,7 +1143,7 @@ static void __tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
* When operating in sysFS mode the content of the buffer needs to be
* read before the TMC is disabled.
*/
- if (drvdata->mode == CS_MODE_SYSFS)
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
tmc_etr_sync_sysfs_buf(drvdata);
tmc_disable_hw(drvdata);
@@ -1189,7 +1189,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
spin_lock_irqsave(&drvdata->spinlock, flags);
}
- if (drvdata->reading || drvdata->mode == CS_MODE_PERF) {
+ if (drvdata->reading || local_read(&csdev->mode) == CS_MODE_PERF) {
ret = -EBUSY;
goto out;
}
@@ -1230,14 +1230,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
* sink is already enabled no memory is needed and the HW need not be
* touched, even if the buffer size has changed.
*/
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
atomic_inc(&csdev->refcnt);
goto out;
}
ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
if (!ret) {
- drvdata->mode = CS_MODE_SYSFS;
+ local_set(&csdev->mode, CS_MODE_SYSFS);
atomic_inc(&csdev->refcnt);
}
@@ -1652,7 +1652,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
spin_lock_irqsave(&drvdata->spinlock, flags);
/* Don't use this sink if it is already claimed by sysFS */
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
rc = -EBUSY;
goto unlock_out;
}
@@ -1684,7 +1684,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
if (!rc) {
/* Associate with monitored process. */
drvdata->pid = pid;
- drvdata->mode = CS_MODE_PERF;
+ local_set(&csdev->mode, CS_MODE_PERF);
drvdata->perf_buf = etr_perf->etr_buf;
atomic_inc(&csdev->refcnt);
}
@@ -1725,11 +1725,11 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
}
/* Complain if we (somehow) got out of sync */
- WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
+ WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
tmc_etr_disable_hw(drvdata);
/* Dissociate from monitored process. */
drvdata->pid = -1;
- drvdata->mode = CS_MODE_DISABLED;
+ local_set(&csdev->mode, CS_MODE_DISABLED);
/* Reset perf specific data */
drvdata->perf_buf = NULL;
@@ -1777,7 +1777,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
}
/* Disable the TMC if we are trying to read from a running session. */
- if (drvdata->mode == CS_MODE_SYSFS)
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
__tmc_etr_disable_hw(drvdata);
drvdata->reading = true;
@@ -1799,7 +1799,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
spin_lock_irqsave(&drvdata->spinlock, flags);
/* RE-enable the TMC if need be */
- if (drvdata->mode == CS_MODE_SYSFS) {
+ if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
/*
* The trace run will continue with the same allocated trace
* buffer. Since the tracer is still enabled drvdata::buf can't
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 8dcb426ac3e7..cef979c897e6 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -178,7 +178,6 @@ struct etr_buf {
* @size: trace buffer size for this TMC (common for all modes).
* @max_burst_size: The maximum burst size that can be initiated by
* TMC-ETR on AXI bus.
- * @mode: how this TMC is being used.
* @config_type: TMC variant, must be of type @tmc_config_type.
* @memwidth: width of the memory interface databus, in bytes.
* @trigger_cntr: amount of words to store after a trigger.
@@ -203,7 +202,6 @@ struct tmc_drvdata {
u32 len;
u32 size;
u32 max_burst_size;
- u32 mode;
enum tmc_config_type config_type;
enum tmc_mem_intf_width memwidth;
u32 trigger_cntr;
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 10e886455b8b..1a8c64645b92 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -207,11 +207,11 @@ static void smb_enable_sysfs(struct coresight_device *csdev)
{
struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
- if (drvdata->mode != CS_MODE_DISABLED)
+ if (local_read(&csdev->mode) != CS_MODE_DISABLED)
return;
smb_enable_hw(drvdata);
- drvdata->mode = CS_MODE_SYSFS;
+ local_set(&csdev->mode, CS_MODE_SYSFS);
}
static int smb_enable_perf(struct coresight_device *csdev, void *data)
@@ -234,7 +234,7 @@ static int smb_enable_perf(struct coresight_device *csdev, void *data)
if (drvdata->pid == -1) {
smb_enable_hw(drvdata);
drvdata->pid = pid;
- drvdata->mode = CS_MODE_PERF;
+ local_set(&csdev->mode, CS_MODE_PERF);
}
return 0;
@@ -253,7 +253,8 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
return -EBUSY;
/* Do nothing, the SMB is already enabled as other mode */
- if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode)
+ if (local_read(&csdev->mode) != CS_MODE_DISABLED &&
+ local_read(&csdev->mode) != mode)
return -EBUSY;
switch (mode) {
@@ -289,13 +290,13 @@ static int smb_disable(struct coresight_device *csdev)
return -EBUSY;
/* Complain if we (somehow) got out of sync */
- WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
+ WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
smb_disable_hw(drvdata);
/* Dissociate from the target process. */
drvdata->pid = -1;
- drvdata->mode = CS_MODE_DISABLED;
+ local_set(&csdev->mode, CS_MODE_DISABLED);
dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
return 0;
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
index 82a44c14a882..a91d39cfccb8 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.h
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
@@ -109,7 +109,6 @@ struct smb_data_buffer {
* @reading: Synchronise user space access to SMB buffer.
* @pid: Process ID of the process being monitored by the
* session that is using this component.
- * @mode: How this SMB is being used, perf mode or sysfs mode.
*/
struct smb_drv_data {
void __iomem *base;
@@ -119,7 +118,6 @@ struct smb_drv_data {
spinlock_t spinlock;
bool reading;
pid_t pid;
- enum cs_mode mode;
};
#endif
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 65131bfbd904..ba817f563ff7 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -226,6 +226,11 @@ struct coresight_sysfs_link {
* by @coresight_ops.
* @access: Device i/o access abstraction for this device.
* @dev: The device entity associated to this component.
+ * @mode: This tracer's mode, i.e sysFS, Perf or disabled. This is
+ * actually an 'enum cs_mode', but is stored in an atomic type.
+ * This is always accessed through local_read() and local_set(),
+ * but wherever it's done from within the Coresight device's lock,
+ * a non-atomic read would also work.
* @refcnt: keep track of what is in use.
* @orphan: true if the component has connections that haven't been linked.
* @enable: 'true' if component is currently part of an active path.
@@ -252,6 +257,7 @@ struct coresight_device {
const struct coresight_ops *ops;
struct csdev_access access;
struct device dev;
+ local_t mode;
atomic_t refcnt;
bool orphan;
bool enable;
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/8] coresight: Move mode to struct coresight_device
2023-12-12 15:54 ` [PATCH 4/8] coresight: Move mode to struct coresight_device James Clark
@ 2024-01-08 11:32 ` Suzuki K Poulose
0 siblings, 0 replies; 22+ messages in thread
From: Suzuki K Poulose @ 2024-01-08 11:32 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
On 12/12/2023 15:54, James Clark wrote:
> Most devices use mode, so move the mode definition out of the individual
> devices and up to the Coresight device. This will allow the core code to
> also know the mode which will be useful in a later commit.
>
> This also fixes the inconsistency of the documentation of the mode field
> on the individual device types. For example ETB10 had "this ETB is being
> used".
>
> Two devices didn't require an atomic mode type, so these usages have
> been converted to atomic_get() and atomic_set() only to make it compile,
> but the documentation of the field in struct coresight_device explains
> this type of usage.
>
> In the future, manipulation of the mode could be completely moved out of
> the individual devices and into the core code because it's almost all
> duplicate code, and this change is a step towards that.
The change looks good to me. I am wondering if we should abstract it out ?
i.e.,
csdev_get_mode(struct coresight_device *csdev)
csdev_set_mode(struct coresight_device *csdev, mode)
csdev_{cmpxchg or set_if}_mode(csdev, old_mode, new_mode)
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-etb10.c | 18 ++++++-------
> drivers/hwtracing/coresight/coresight-etm.h | 2 --
> .../coresight/coresight-etm3x-core.c | 13 +++++-----
> .../coresight/coresight-etm3x-sysfs.c | 4 +--
> drivers/hwtracing/coresight/coresight-stm.c | 20 +++++++-------
> .../hwtracing/coresight/coresight-tmc-core.c | 2 +-
> .../hwtracing/coresight/coresight-tmc-etf.c | 26 +++++++++----------
> .../hwtracing/coresight/coresight-tmc-etr.c | 20 +++++++-------
> drivers/hwtracing/coresight/coresight-tmc.h | 2 --
> drivers/hwtracing/coresight/ultrasoc-smb.c | 13 +++++-----
> drivers/hwtracing/coresight/ultrasoc-smb.h | 2 --
> include/linux/coresight.h | 6 +++++
> 12 files changed, 62 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> index fa80039e0821..08e5bba862db 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -76,7 +76,6 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "etb");
> * @pid: Process ID of the process being monitored by the session
> * that is using this component.
> * @buf: area of memory where ETB buffer content gets sent.
> - * @mode: this ETB is being used.
> * @buffer_depth: size of @buf.
> * @trigger_cntr: amount of words to store after a trigger.
> */
> @@ -89,7 +88,6 @@ struct etb_drvdata {
> local_t reading;
> pid_t pid;
> u8 *buf;
> - u32 mode;
> u32 buffer_depth;
> u32 trigger_cntr;
> };
> @@ -150,17 +148,17 @@ static int etb_enable_sysfs(struct coresight_device *csdev)
> spin_lock_irqsave(&drvdata->spinlock, flags);
>
> /* Don't messup with perf sessions. */
> - if (drvdata->mode == CS_MODE_PERF) {
> + if (local_read(&csdev->mode) == CS_MODE_PERF) {
> ret = -EBUSY;
> goto out;
> }
>
> - if (drvdata->mode == CS_MODE_DISABLED) {
> + if (local_read(&csdev->mode) == CS_MODE_DISABLED) {
> ret = etb_enable_hw(drvdata);
> if (ret)
> goto out;
>
> - drvdata->mode = CS_MODE_SYSFS;
> + local_set(&csdev->mode, CS_MODE_SYSFS);
> }
>
> atomic_inc(&csdev->refcnt);
> @@ -181,7 +179,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
> spin_lock_irqsave(&drvdata->spinlock, flags);
>
> /* No need to continue if the component is already in used by sysFS. */
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
> ret = -EBUSY;
> goto out;
> }
> @@ -216,7 +214,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
> if (!ret) {
> /* Associate with monitored process. */
> drvdata->pid = pid;
> - drvdata->mode = CS_MODE_PERF;
> + local_set(&drvdata->csdev->mode, CS_MODE_PERF);
> atomic_inc(&csdev->refcnt);
> }
>
> @@ -362,11 +360,11 @@ static int etb_disable(struct coresight_device *csdev)
> }
>
> /* Complain if we (somehow) got out of sync */
> - WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> + WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
> etb_disable_hw(drvdata);
> /* Dissociate from monitored process. */
> drvdata->pid = -1;
> - drvdata->mode = CS_MODE_DISABLED;
> + local_set(&csdev->mode, CS_MODE_DISABLED);
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> dev_dbg(&csdev->dev, "ETB disabled\n");
> @@ -589,7 +587,7 @@ static void etb_dump(struct etb_drvdata *drvdata)
> unsigned long flags;
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
> __etb_disable_hw(drvdata);
> etb_dump_hw(drvdata);
> __etb_enable_hw(drvdata);
> diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h
> index 9a0d08b092ae..e02c3ea972c9 100644
> --- a/drivers/hwtracing/coresight/coresight-etm.h
> +++ b/drivers/hwtracing/coresight/coresight-etm.h
> @@ -215,7 +215,6 @@ struct etm_config {
> * @port_size: port size as reported by ETMCR bit 4-6 and 21.
> * @arch: ETM/PTM version number.
> * @use_cpu14: true if management registers need to be accessed via CP14.
> - * @mode: this tracer's mode, i.e sysFS, Perf or disabled.
> * @sticky_enable: true if ETM base configuration has been done.
> * @boot_enable:true if we should start tracing at boot time.
> * @os_unlock: true if access to management registers is allowed.
> @@ -238,7 +237,6 @@ struct etm_drvdata {
> int port_size;
> u8 arch;
> bool use_cp14;
> - local_t mode;
> bool sticky_enable;
> bool boot_enable;
> bool os_unlock;
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> index 116a91d90ac2..29f62dfac673 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> @@ -559,7 +559,7 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event,
> u32 val;
> struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> - val = local_cmpxchg(&drvdata->mode, CS_MODE_DISABLED, mode);
> + val = local_cmpxchg(&drvdata->csdev->mode, CS_MODE_DISABLED, mode);
>
> /* Someone is already using the tracer */
> if (val)
> @@ -578,7 +578,7 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event,
>
> /* The tracer didn't start */
> if (ret)
> - local_set(&drvdata->mode, CS_MODE_DISABLED);
> + local_set(&drvdata->csdev->mode, CS_MODE_DISABLED);
>
> return ret;
> }
> @@ -672,14 +672,13 @@ static void etm_disable(struct coresight_device *csdev,
> struct perf_event *event)
> {
> enum cs_mode mode;
> - struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> /*
> * For as long as the tracer isn't disabled another entity can't
> * change its status. As such we can read the status here without
> * fearing it will change under us.
> */
> - mode = local_read(&drvdata->mode);
> + mode = local_read(&csdev->mode);
>
> switch (mode) {
> case CS_MODE_DISABLED:
> @@ -696,7 +695,7 @@ static void etm_disable(struct coresight_device *csdev,
> }
>
> if (mode)
> - local_set(&drvdata->mode, CS_MODE_DISABLED);
> + local_set(&csdev->mode, CS_MODE_DISABLED);
> }
>
> static const struct coresight_ops_source etm_source_ops = {
> @@ -730,7 +729,7 @@ static int etm_starting_cpu(unsigned int cpu)
> etmdrvdata[cpu]->os_unlock = true;
> }
>
> - if (local_read(&etmdrvdata[cpu]->mode))
> + if (local_read(&etmdrvdata[cpu]->csdev->mode))
> etm_enable_hw(etmdrvdata[cpu]);
> spin_unlock(&etmdrvdata[cpu]->spinlock);
> return 0;
> @@ -742,7 +741,7 @@ static int etm_dying_cpu(unsigned int cpu)
> return 0;
>
> spin_lock(&etmdrvdata[cpu]->spinlock);
> - if (local_read(&etmdrvdata[cpu]->mode))
> + if (local_read(&etmdrvdata[cpu]->csdev->mode))
> etm_disable_hw(etmdrvdata[cpu]);
> spin_unlock(&etmdrvdata[cpu]->spinlock);
> return 0;
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> index 2f271b7fb048..6c8429c980b1 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> @@ -722,7 +722,7 @@ static ssize_t cntr_val_show(struct device *dev,
> struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> struct etm_config *config = &drvdata->config;
>
> - if (!local_read(&drvdata->mode)) {
> + if (!local_read(&drvdata->csdev->mode)) {
> spin_lock(&drvdata->spinlock);
> for (i = 0; i < drvdata->nr_cntr; i++)
> ret += sprintf(buf, "counter %d: %x\n",
> @@ -941,7 +941,7 @@ static ssize_t seq_curr_state_show(struct device *dev,
> struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> struct etm_config *config = &drvdata->config;
>
> - if (!local_read(&drvdata->mode)) {
> + if (!local_read(&drvdata->csdev->mode)) {
> val = config->seq_curr_state;
> goto out;
> }
> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> index a1c27c901ad1..190ba569b05a 100644
> --- a/drivers/hwtracing/coresight/coresight-stm.c
> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> @@ -119,7 +119,6 @@ DEFINE_CORESIGHT_DEVLIST(stm_devs, "stm");
> * @spinlock: only one at a time pls.
> * @chs: the channels accociated to this STM.
> * @stm: structure associated to the generic STM interface.
> - * @mode: this tracer's mode (enum cs_mode), i.e sysFS, or disabled.
> * @traceid: value of the current ID for this component.
> * @write_bytes: Maximus bytes this STM can write at a time.
> * @stmsper: settings for register STMSPER.
> @@ -136,7 +135,6 @@ struct stm_drvdata {
> spinlock_t spinlock;
> struct channel_space chs;
> struct stm_data stm;
> - local_t mode;
> u8 traceid;
> u32 write_bytes;
> u32 stmsper;
> @@ -201,7 +199,7 @@ static int stm_enable(struct coresight_device *csdev, struct perf_event *event,
> if (mode != CS_MODE_SYSFS)
> return -EINVAL;
>
> - val = local_cmpxchg(&drvdata->mode, CS_MODE_DISABLED, mode);
> + val = local_cmpxchg(&csdev->mode, CS_MODE_DISABLED, mode);
>
> /* Someone is already using the tracer */
> if (val)
> @@ -266,7 +264,7 @@ static void stm_disable(struct coresight_device *csdev,
> * change its status. As such we can read the status here without
> * fearing it will change under us.
> */
> - if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
> + if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
> spin_lock(&drvdata->spinlock);
> stm_disable_hw(drvdata);
> spin_unlock(&drvdata->spinlock);
> @@ -276,7 +274,7 @@ static void stm_disable(struct coresight_device *csdev,
>
> pm_runtime_put(csdev->dev.parent);
>
> - local_set(&drvdata->mode, CS_MODE_DISABLED);
> + local_set(&csdev->mode, CS_MODE_DISABLED);
> dev_dbg(&csdev->dev, "STM tracing disabled\n");
> }
> }
> @@ -373,7 +371,7 @@ static long stm_generic_set_options(struct stm_data *stm_data,
> {
> struct stm_drvdata *drvdata = container_of(stm_data,
> struct stm_drvdata, stm);
> - if (!(drvdata && local_read(&drvdata->mode)))
> + if (!(drvdata && local_read(&drvdata->csdev->mode)))
> return -EINVAL;
>
> if (channel >= drvdata->numsp)
> @@ -408,7 +406,7 @@ static ssize_t notrace stm_generic_packet(struct stm_data *stm_data,
> struct stm_drvdata, stm);
> unsigned int stm_flags;
>
> - if (!(drvdata && local_read(&drvdata->mode)))
> + if (!(drvdata && local_read(&drvdata->csdev->mode)))
> return -EACCES;
>
> if (channel >= drvdata->numsp)
> @@ -515,7 +513,7 @@ static ssize_t port_select_show(struct device *dev,
> struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> unsigned long val;
>
> - if (!local_read(&drvdata->mode)) {
> + if (!local_read(&drvdata->csdev->mode)) {
> val = drvdata->stmspscr;
> } else {
> spin_lock(&drvdata->spinlock);
> @@ -541,7 +539,7 @@ static ssize_t port_select_store(struct device *dev,
> spin_lock(&drvdata->spinlock);
> drvdata->stmspscr = val;
>
> - if (local_read(&drvdata->mode)) {
> + if (local_read(&drvdata->csdev->mode)) {
> CS_UNLOCK(drvdata->base);
> /* Process as per ARM's TRM recommendation */
> stmsper = readl_relaxed(drvdata->base + STMSPER);
> @@ -562,7 +560,7 @@ static ssize_t port_enable_show(struct device *dev,
> struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> unsigned long val;
>
> - if (!local_read(&drvdata->mode)) {
> + if (!local_read(&drvdata->csdev->mode)) {
> val = drvdata->stmsper;
> } else {
> spin_lock(&drvdata->spinlock);
> @@ -588,7 +586,7 @@ static ssize_t port_enable_store(struct device *dev,
> spin_lock(&drvdata->spinlock);
> drvdata->stmsper = val;
>
> - if (local_read(&drvdata->mode)) {
> + if (local_read(&drvdata->csdev->mode)) {
> CS_UNLOCK(drvdata->base);
> writel_relaxed(drvdata->stmsper, drvdata->base + STMSPER);
> CS_LOCK(drvdata->base);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 7ec5365e2b64..e5d47f61f9f3 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -558,7 +558,7 @@ static void tmc_shutdown(struct amba_device *adev)
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
>
> - if (drvdata->mode == CS_MODE_DISABLED)
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_DISABLED)
> goto out;
>
> if (drvdata->config_type == TMC_CONFIG_TYPE_ETR)
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 7406b65e2cdd..2a7e516052a2 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -89,7 +89,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
> * When operating in sysFS mode the content of the buffer needs to be
> * read before the TMC is disabled.
> */
> - if (drvdata->mode == CS_MODE_SYSFS)
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
> tmc_etb_dump_hw(drvdata);
> tmc_disable_hw(drvdata);
>
> @@ -205,7 +205,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
> * sink is already enabled no memory is needed and the HW need not be
> * touched.
> */
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
> atomic_inc(&csdev->refcnt);
> goto out;
> }
> @@ -228,7 +228,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
>
> ret = tmc_etb_enable_hw(drvdata);
> if (!ret) {
> - drvdata->mode = CS_MODE_SYSFS;
> + local_set(&csdev->mode, CS_MODE_SYSFS);
> atomic_inc(&csdev->refcnt);
> } else {
> /* Free up the buffer if we failed to enable */
> @@ -262,7 +262,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
> * No need to continue if the ETB/ETF is already operated
> * from sysFS.
> */
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
> ret = -EBUSY;
> break;
> }
> @@ -292,7 +292,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
> if (!ret) {
> /* Associate with monitored process. */
> drvdata->pid = pid;
> - drvdata->mode = CS_MODE_PERF;
> + local_set(&csdev->mode, CS_MODE_PERF);
> atomic_inc(&csdev->refcnt);
> }
> } while (0);
> @@ -344,11 +344,11 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
> }
>
> /* Complain if we (somehow) got out of sync */
> - WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> + WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
> tmc_etb_disable_hw(drvdata);
> /* Dissociate from monitored process. */
> drvdata->pid = -1;
> - drvdata->mode = CS_MODE_DISABLED;
> + local_set(&csdev->mode, CS_MODE_DISABLED);
>
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> @@ -374,7 +374,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
> if (atomic_read(&csdev->refcnt) == 0) {
> ret = tmc_etf_enable_hw(drvdata);
> if (!ret) {
> - drvdata->mode = CS_MODE_SYSFS;
> + local_set(&csdev->mode, CS_MODE_SYSFS);
> first_enable = true;
> }
> }
> @@ -403,7 +403,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
>
> if (atomic_dec_return(&csdev->refcnt) == 0) {
> tmc_etf_disable_hw(drvdata);
> - drvdata->mode = CS_MODE_DISABLED;
> + local_set(&csdev->mode, CS_MODE_DISABLED);
> last_disable = true;
> }
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> @@ -483,7 +483,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
> return 0;
>
> /* This shouldn't happen */
> - if (WARN_ON_ONCE(drvdata->mode != CS_MODE_PERF))
> + if (WARN_ON_ONCE(local_read(&csdev->mode) != CS_MODE_PERF))
> return 0;
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> @@ -629,7 +629,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
> }
>
> /* Don't interfere if operated from Perf */
> - if (drvdata->mode == CS_MODE_PERF) {
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_PERF) {
> ret = -EINVAL;
> goto out;
> }
> @@ -641,7 +641,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
> }
>
> /* Disable the TMC if need be */
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
> /* There is no point in reading a TMC in HW FIFO mode */
> mode = readl_relaxed(drvdata->base + TMC_MODE);
> if (mode != TMC_MODE_CIRCULAR_BUFFER) {
> @@ -673,7 +673,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
> spin_lock_irqsave(&drvdata->spinlock, flags);
>
> /* Re-enable the TMC if need be */
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
> /* There is no point in reading a TMC in HW FIFO mode */
> mode = readl_relaxed(drvdata->base + TMC_MODE);
> if (mode != TMC_MODE_CIRCULAR_BUFFER) {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index af02ba5d5f15..3dc989d4fcab 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1143,7 +1143,7 @@ static void __tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
> * When operating in sysFS mode the content of the buffer needs to be
> * read before the TMC is disabled.
> */
> - if (drvdata->mode == CS_MODE_SYSFS)
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
> tmc_etr_sync_sysfs_buf(drvdata);
>
> tmc_disable_hw(drvdata);
> @@ -1189,7 +1189,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
> spin_lock_irqsave(&drvdata->spinlock, flags);
> }
>
> - if (drvdata->reading || drvdata->mode == CS_MODE_PERF) {
> + if (drvdata->reading || local_read(&csdev->mode) == CS_MODE_PERF) {
> ret = -EBUSY;
> goto out;
> }
> @@ -1230,14 +1230,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> * sink is already enabled no memory is needed and the HW need not be
> * touched, even if the buffer size has changed.
> */
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
> atomic_inc(&csdev->refcnt);
> goto out;
> }
>
> ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
> if (!ret) {
> - drvdata->mode = CS_MODE_SYSFS;
> + local_set(&csdev->mode, CS_MODE_SYSFS);
> atomic_inc(&csdev->refcnt);
> }
>
> @@ -1652,7 +1652,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> /* Don't use this sink if it is already claimed by sysFS */
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
> rc = -EBUSY;
> goto unlock_out;
> }
> @@ -1684,7 +1684,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
> if (!rc) {
> /* Associate with monitored process. */
> drvdata->pid = pid;
> - drvdata->mode = CS_MODE_PERF;
> + local_set(&csdev->mode, CS_MODE_PERF);
> drvdata->perf_buf = etr_perf->etr_buf;
> atomic_inc(&csdev->refcnt);
> }
> @@ -1725,11 +1725,11 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
> }
>
> /* Complain if we (somehow) got out of sync */
> - WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> + WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
> tmc_etr_disable_hw(drvdata);
> /* Dissociate from monitored process. */
> drvdata->pid = -1;
> - drvdata->mode = CS_MODE_DISABLED;
> + local_set(&csdev->mode, CS_MODE_DISABLED);
> /* Reset perf specific data */
> drvdata->perf_buf = NULL;
>
> @@ -1777,7 +1777,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
> }
>
> /* Disable the TMC if we are trying to read from a running session. */
> - if (drvdata->mode == CS_MODE_SYSFS)
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
> __tmc_etr_disable_hw(drvdata);
>
> drvdata->reading = true;
> @@ -1799,7 +1799,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
> spin_lock_irqsave(&drvdata->spinlock, flags);
>
> /* RE-enable the TMC if need be */
> - if (drvdata->mode == CS_MODE_SYSFS) {
> + if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
> /*
> * The trace run will continue with the same allocated trace
> * buffer. Since the tracer is still enabled drvdata::buf can't
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 8dcb426ac3e7..cef979c897e6 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -178,7 +178,6 @@ struct etr_buf {
> * @size: trace buffer size for this TMC (common for all modes).
> * @max_burst_size: The maximum burst size that can be initiated by
> * TMC-ETR on AXI bus.
> - * @mode: how this TMC is being used.
> * @config_type: TMC variant, must be of type @tmc_config_type.
> * @memwidth: width of the memory interface databus, in bytes.
> * @trigger_cntr: amount of words to store after a trigger.
> @@ -203,7 +202,6 @@ struct tmc_drvdata {
> u32 len;
> u32 size;
> u32 max_burst_size;
> - u32 mode;
> enum tmc_config_type config_type;
> enum tmc_mem_intf_width memwidth;
> u32 trigger_cntr;
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 10e886455b8b..1a8c64645b92 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -207,11 +207,11 @@ static void smb_enable_sysfs(struct coresight_device *csdev)
> {
> struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> - if (drvdata->mode != CS_MODE_DISABLED)
> + if (local_read(&csdev->mode) != CS_MODE_DISABLED)
> return;
>
> smb_enable_hw(drvdata);
> - drvdata->mode = CS_MODE_SYSFS;
> + local_set(&csdev->mode, CS_MODE_SYSFS);
> }
>
> static int smb_enable_perf(struct coresight_device *csdev, void *data)
> @@ -234,7 +234,7 @@ static int smb_enable_perf(struct coresight_device *csdev, void *data)
> if (drvdata->pid == -1) {
> smb_enable_hw(drvdata);
> drvdata->pid = pid;
> - drvdata->mode = CS_MODE_PERF;
> + local_set(&csdev->mode, CS_MODE_PERF);
> }
>
> return 0;
> @@ -253,7 +253,8 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
> return -EBUSY;
>
> /* Do nothing, the SMB is already enabled as other mode */
> - if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode)
> + if (local_read(&csdev->mode) != CS_MODE_DISABLED &&
> + local_read(&csdev->mode) != mode)
> return -EBUSY;
>
> switch (mode) {
> @@ -289,13 +290,13 @@ static int smb_disable(struct coresight_device *csdev)
> return -EBUSY;
>
> /* Complain if we (somehow) got out of sync */
> - WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> + WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
>
> smb_disable_hw(drvdata);
>
> /* Dissociate from the target process. */
> drvdata->pid = -1;
> - drvdata->mode = CS_MODE_DISABLED;
> + local_set(&csdev->mode, CS_MODE_DISABLED);
> dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
>
> return 0;
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
> index 82a44c14a882..a91d39cfccb8 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.h
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
> @@ -109,7 +109,6 @@ struct smb_data_buffer {
> * @reading: Synchronise user space access to SMB buffer.
> * @pid: Process ID of the process being monitored by the
> * session that is using this component.
> - * @mode: How this SMB is being used, perf mode or sysfs mode.
> */
> struct smb_drv_data {
> void __iomem *base;
> @@ -119,7 +118,6 @@ struct smb_drv_data {
> spinlock_t spinlock;
> bool reading;
> pid_t pid;
> - enum cs_mode mode;
> };
>
> #endif
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 65131bfbd904..ba817f563ff7 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -226,6 +226,11 @@ struct coresight_sysfs_link {
> * by @coresight_ops.
> * @access: Device i/o access abstraction for this device.
> * @dev: The device entity associated to this component.
> + * @mode: This tracer's mode, i.e sysFS, Perf or disabled. This is
> + * actually an 'enum cs_mode', but is stored in an atomic type.
> + * This is always accessed through local_read() and local_set(),
> + * but wherever it's done from within the Coresight device's lock,
> + * a non-atomic read would also work.
By abstracting it, we could enforce this change. Eitherways, the change
as such is fine by me.
Suzuki
> * @refcnt: keep track of what is in use.
> * @orphan: true if the component has connections that haven't been linked.
> * @enable: 'true' if component is currently part of an active path.
> @@ -252,6 +257,7 @@ struct coresight_device {
> const struct coresight_ops *ops;
> struct csdev_access access;
> struct device dev;
> + local_t mode;
> atomic_t refcnt;
> bool orphan;
> bool enable;
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/8] coresight: Remove the 'enable' field.
2023-12-12 15:53 [PATCH 0/8] coresight: Separate sysfs and Perf usage and some other cleanups James Clark
` (3 preceding siblings ...)
2023-12-12 15:54 ` [PATCH 4/8] coresight: Move mode to struct coresight_device James Clark
@ 2023-12-12 15:54 ` James Clark
2024-01-08 14:42 ` Suzuki K Poulose
2023-12-12 15:54 ` [PATCH 6/8] coresight: Move all sysfs code to sysfs file James Clark
` (2 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2023-12-12 15:54 UTC (permalink / raw)
To: coresight, suzuki.poulose
Cc: James Clark, Mike Leach, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, linux-arm-kernel, linux-kernel, linux-stm32
'enable', which probably should have been 'enabled', is only ever read
in the core code in relation to controlling sources, and specifically
only sources in sysfs mode. Confusingly it's not labelled as such and
relying on it can be a source of bugs like the one fixed by
commit 078dbba3f0c9 ("coresight: Fix crash when Perf and sysfs modes are
used concurrently").
Most importantly, it can only be used when the coresight_mutex is held
which is only done when enabling and disabling paths in sysfs mode, and
not Perf mode. So to prevent its usage spreading and leaking out to
other devices, remove it.
It's use is equivalent to checking if the mode is currently sysfs, as
due to the coresight_mutex lock, mode == CS_MODE_SYSFS can only become
true or untrue when that lock is held, and when mode == CS_MODE_SYSFS
the device is both enabled and in sysfs mode.
The one place it was used outside of the core code is in TPDA, but that
pattern is more appropriately represented using refcounts inside the
device's own spinlock.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-core.c | 86 +++++++-------------
drivers/hwtracing/coresight/coresight-tpda.c | 12 ++-
include/linux/coresight.h | 2 -
3 files changed, 38 insertions(+), 62 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index ab226441e5f4..1d0bd1586590 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -279,29 +279,18 @@ EXPORT_SYMBOL_GPL(coresight_add_helper);
static int coresight_enable_sink(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
- int ret = sink_ops(csdev)->enable(csdev, mode, data);
-
- if (ret)
- return ret;
-
- csdev->enable = true;
-
- return 0;
+ return sink_ops(csdev)->enable(csdev, mode, data);
}
static void coresight_disable_sink(struct coresight_device *csdev)
{
- int ret = sink_ops(csdev)->disable(csdev);
- if (ret)
- return;
- csdev->enable = false;
+ sink_ops(csdev)->disable(csdev);
}
static int coresight_enable_link(struct coresight_device *csdev,
struct coresight_device *parent,
struct coresight_device *child)
{
- int ret = 0;
int link_subtype;
struct coresight_connection *inconn, *outconn;
@@ -317,19 +306,13 @@ static int coresight_enable_link(struct coresight_device *csdev,
if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT && IS_ERR(outconn))
return PTR_ERR(outconn);
- ret = link_ops(csdev)->enable(csdev, inconn, outconn);
- if (!ret)
- csdev->enable = true;
-
- return ret;
+ return link_ops(csdev)->enable(csdev, inconn, outconn);
}
static void coresight_disable_link(struct coresight_device *csdev,
struct coresight_device *parent,
struct coresight_device *child)
{
- int i;
- int link_subtype;
struct coresight_connection *inconn, *outconn;
if (!parent || !child)
@@ -337,26 +320,8 @@ static void coresight_disable_link(struct coresight_device *csdev,
inconn = coresight_find_out_connection(parent, csdev);
outconn = coresight_find_out_connection(csdev, child);
- link_subtype = csdev->subtype.link_subtype;
link_ops(csdev)->disable(csdev, inconn, outconn);
-
- if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
- for (i = 0; i < csdev->pdata->nr_inconns; i++)
- if (atomic_read(&csdev->pdata->in_conns[i]->dest_refcnt) !=
- 0)
- return;
- } else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT) {
- for (i = 0; i < csdev->pdata->nr_outconns; i++)
- if (atomic_read(&csdev->pdata->out_conns[i]->src_refcnt) !=
- 0)
- return;
- } else {
- if (atomic_read(&csdev->refcnt) != 0)
- return;
- }
-
- csdev->enable = false;
}
int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
@@ -364,11 +329,16 @@ int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
{
int ret;
- if (!csdev->enable) {
+ /*
+ * Comparison with CS_MODE_SYSFS works without taking any device
+ * specific spinlock because the truthyness of that comparison can only
+ * change with coresight_mutex held, which we already have here.
+ */
+ lockdep_assert_held(&coresight_mutex);
+ if (local_read(&csdev->mode) != CS_MODE_SYSFS) {
ret = source_ops(csdev)->enable(csdev, data, mode);
if (ret)
return ret;
- csdev->enable = true;
}
atomic_inc(&csdev->refcnt);
@@ -385,22 +355,12 @@ static bool coresight_is_helper(struct coresight_device *csdev)
static int coresight_enable_helper(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
- int ret = helper_ops(csdev)->enable(csdev, mode, data);
-
- if (ret)
- return ret;
-
- csdev->enable = true;
- return 0;
+ return helper_ops(csdev)->enable(csdev, mode, data);
}
static void coresight_disable_helper(struct coresight_device *csdev)
{
- int ret = helper_ops(csdev)->disable(csdev, NULL);
-
- if (ret)
- return;
- csdev->enable = false;
+ helper_ops(csdev)->disable(csdev, NULL);
}
static void coresight_disable_helpers(struct coresight_device *csdev)
@@ -445,11 +405,15 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
void *data)
{
+ lockdep_assert_held(&coresight_mutex);
+ if (local_read(&csdev->mode) != CS_MODE_SYSFS)
+ return false;
+
if (atomic_dec_return(&csdev->refcnt) == 0) {
coresight_disable_source(csdev, data);
- csdev->enable = false;
+ return true;
}
- return !csdev->enable;
+ return false;
}
/*
@@ -1097,7 +1061,13 @@ int coresight_enable(struct coresight_device *csdev)
if (ret)
goto out;
- if (csdev->enable) {
+ /*
+ * mode == SYSFS implies that it's already enabled. Don't look at the
+ * refcount to determine this because we don't claim the source until
+ * coresight_enable_source() so can still race with Perf mode which
+ * doesn't hold coresight_mutex.
+ */
+ if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
/*
* There could be multiple applications driving the software
* source. So keep the refcount for each such user when the
@@ -1183,7 +1153,7 @@ void coresight_disable(struct coresight_device *csdev)
if (ret)
goto out;
- if (!csdev->enable || !coresight_disable_source_sysfs(csdev, NULL))
+ if (!coresight_disable_source_sysfs(csdev, NULL))
goto out;
switch (csdev->subtype.source_subtype) {
@@ -1249,7 +1219,9 @@ static ssize_t enable_source_show(struct device *dev,
{
struct coresight_device *csdev = to_coresight_device(dev);
- return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->enable);
+ guard(mutex)(&coresight_mutex);
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
+ local_read(&csdev->mode) == CS_MODE_SYSFS);
}
static ssize_t enable_source_store(struct device *dev,
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 5f82737c37bb..65c70995ab00 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -148,7 +148,11 @@ static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
CS_UNLOCK(drvdata->base);
- if (!drvdata->csdev->enable)
+ /*
+ * Only do pre-port enable for first port that calls enable when the
+ * device's main refcount is still 0
+ */
+ if (!atomic_read(&drvdata->csdev->refcnt))
tpda_enable_pre_port(drvdata);
ret = tpda_enable_port(drvdata, port);
@@ -169,6 +173,7 @@ static int tpda_enable(struct coresight_device *csdev,
ret = __tpda_enable(drvdata, in->dest_port);
if (!ret) {
atomic_inc(&in->dest_refcnt);
+ atomic_inc(&csdev->refcnt);
dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
}
}
@@ -197,9 +202,10 @@ static void tpda_disable(struct coresight_device *csdev,
struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
spin_lock(&drvdata->spinlock);
- if (atomic_dec_return(&in->dest_refcnt) == 0)
+ if (atomic_dec_return(&in->dest_refcnt) == 0) {
__tpda_disable(drvdata, in->dest_port);
-
+ atomic_dec(&csdev->refcnt);
+ }
spin_unlock(&drvdata->spinlock);
dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index ba817f563ff7..46e6667f72ce 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -233,7 +233,6 @@ struct coresight_sysfs_link {
* a non-atomic read would also work.
* @refcnt: keep track of what is in use.
* @orphan: true if the component has connections that haven't been linked.
- * @enable: 'true' if component is currently part of an active path.
* @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
* by writing a 1 to the 'enable_sink' file. A sink can be
* activated but not yet enabled. Enabling for a _sink_ happens
@@ -260,7 +259,6 @@ struct coresight_device {
local_t mode;
atomic_t refcnt;
bool orphan;
- bool enable;
/* sink specific fields */
bool sysfs_sink_activated;
struct dev_ext_attribute *ea;
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 5/8] coresight: Remove the 'enable' field.
2023-12-12 15:54 ` [PATCH 5/8] coresight: Remove the 'enable' field James Clark
@ 2024-01-08 14:42 ` Suzuki K Poulose
2024-01-19 9:59 ` James Clark
0 siblings, 1 reply; 22+ messages in thread
From: Suzuki K Poulose @ 2024-01-08 14:42 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32, Tao Zhang,
Mao Jinlong
Hi James
+Cc: Tao Zhang <quic_taozha@quicinc.com>
+Cc: Mao Jinlong <quic_jinlmao@quicinc.com>
On 12/12/2023 15:54, James Clark wrote:
> 'enable', which probably should have been 'enabled', is only ever read
> in the core code in relation to controlling sources, and specifically
> only sources in sysfs mode. Confusingly it's not labelled as such and
> relying on it can be a source of bugs like the one fixed by
> commit 078dbba3f0c9 ("coresight: Fix crash when Perf and sysfs modes are
> used concurrently").
>
> Most importantly, it can only be used when the coresight_mutex is held
> which is only done when enabling and disabling paths in sysfs mode, and
> not Perf mode.
I think we may be able to relax this a bit for the syfs. The sole reason
for holding the mutex is for the "build_path" (and get_enabled_sink)
where we need to make sure that no devices are removed/added. We hold
necessary refcount on the device and the module (via
coresight_grab_device()). After which, we should be able to release the
mutex and perform the rest without it in coresight_enable()
> So to prevent its usage spreading and leaking out to
> other devices, remove it.
>
> It's use is equivalent to checking if the mode is currently sysfs, as
> due to the coresight_mutex lock, mode == CS_MODE_SYSFS can only become
> true or untrue when that lock is held, and when mode == CS_MODE_SYSFS
> the device is both enabled and in sysfs mode.
All of the above makes sense and looks good to me.
>
> The one place it was used outside of the core code is in TPDA, but that
> pattern is more appropriately represented using refcounts inside the
> device's own spinlock.
But, I think we can clean up this code a bit more better. See below.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 86 +++++++-------------
> drivers/hwtracing/coresight/coresight-tpda.c | 12 ++-
> include/linux/coresight.h | 2 -
> 3 files changed, 38 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index ab226441e5f4..1d0bd1586590 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -279,29 +279,18 @@ EXPORT_SYMBOL_GPL(coresight_add_helper);
> static int coresight_enable_sink(struct coresight_device *csdev,
> enum cs_mode mode, void *data)
> {
> - int ret = sink_ops(csdev)->enable(csdev, mode, data);
> -
> - if (ret)
> - return ret;
> -
> - csdev->enable = true;
> -
> - return 0;
> + return sink_ops(csdev)->enable(csdev, mode, data);
> }
>
> static void coresight_disable_sink(struct coresight_device *csdev)
> {
> - int ret = sink_ops(csdev)->disable(csdev);
> - if (ret)
> - return;
> - csdev->enable = false;
> + sink_ops(csdev)->disable(csdev);
> }
>
> static int coresight_enable_link(struct coresight_device *csdev,
> struct coresight_device *parent,
> struct coresight_device *child)
> {
> - int ret = 0;
> int link_subtype;
> struct coresight_connection *inconn, *outconn;
>
> @@ -317,19 +306,13 @@ static int coresight_enable_link(struct coresight_device *csdev,
> if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT && IS_ERR(outconn))
> return PTR_ERR(outconn);
>
> - ret = link_ops(csdev)->enable(csdev, inconn, outconn);
> - if (!ret)
> - csdev->enable = true;
> -
> - return ret;
> + return link_ops(csdev)->enable(csdev, inconn, outconn);
> }
>
> static void coresight_disable_link(struct coresight_device *csdev,
> struct coresight_device *parent,
> struct coresight_device *child)
> {
> - int i;
> - int link_subtype;
> struct coresight_connection *inconn, *outconn;
>
> if (!parent || !child)
> @@ -337,26 +320,8 @@ static void coresight_disable_link(struct coresight_device *csdev,
>
> inconn = coresight_find_out_connection(parent, csdev);
> outconn = coresight_find_out_connection(csdev, child);
> - link_subtype = csdev->subtype.link_subtype;
>
> link_ops(csdev)->disable(csdev, inconn, outconn);
> -
> - if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
> - for (i = 0; i < csdev->pdata->nr_inconns; i++)
> - if (atomic_read(&csdev->pdata->in_conns[i]->dest_refcnt) !=
> - 0)
> - return;
> - } else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT) {
> - for (i = 0; i < csdev->pdata->nr_outconns; i++)
> - if (atomic_read(&csdev->pdata->out_conns[i]->src_refcnt) !=
> - 0)
> - return;
> - } else {
> - if (atomic_read(&csdev->refcnt) != 0)
> - return;
> - }
> -
> - csdev->enable = false;
> }
>
> int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
> @@ -364,11 +329,16 @@ int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
> {
> int ret;
>
> - if (!csdev->enable) {
> + /*
> + * Comparison with CS_MODE_SYSFS works without taking any device
> + * specific spinlock because the truthyness of that comparison can only
> + * change with coresight_mutex held, which we already have here.
> + */
> + lockdep_assert_held(&coresight_mutex);
> + if (local_read(&csdev->mode) != CS_MODE_SYSFS) {
> ret = source_ops(csdev)->enable(csdev, data, mode);
> if (ret)
> return ret;
> - csdev->enable = true;
> }
>
> atomic_inc(&csdev->refcnt);
> @@ -385,22 +355,12 @@ static bool coresight_is_helper(struct coresight_device *csdev)
> static int coresight_enable_helper(struct coresight_device *csdev,
> enum cs_mode mode, void *data)
> {
> - int ret = helper_ops(csdev)->enable(csdev, mode, data);
> -
> - if (ret)
> - return ret;
> -
> - csdev->enable = true;
> - return 0;
> + return helper_ops(csdev)->enable(csdev, mode, data);
> }
>
> static void coresight_disable_helper(struct coresight_device *csdev)
> {
> - int ret = helper_ops(csdev)->disable(csdev, NULL);
> -
> - if (ret)
> - return;
> - csdev->enable = false;
> + helper_ops(csdev)->disable(csdev, NULL);
> }
>
> static void coresight_disable_helpers(struct coresight_device *csdev)
> @@ -445,11 +405,15 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
> static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
> void *data)
> {
> + lockdep_assert_held(&coresight_mutex);
> + if (local_read(&csdev->mode) != CS_MODE_SYSFS)
> + return false;
> +
> if (atomic_dec_return(&csdev->refcnt) == 0) {
> coresight_disable_source(csdev, data);
> - csdev->enable = false;
> + return true;
> }
> - return !csdev->enable;
> + return false;
> }
>
> /*
> @@ -1097,7 +1061,13 @@ int coresight_enable(struct coresight_device *csdev)
> if (ret)
> goto out;
>
> - if (csdev->enable) {
> + /*
> + * mode == SYSFS implies that it's already enabled. Don't look at the
> + * refcount to determine this because we don't claim the source until
> + * coresight_enable_source() so can still race with Perf mode which
> + * doesn't hold coresight_mutex.
> + */
> + if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
> /*
> * There could be multiple applications driving the software
> * source. So keep the refcount for each such user when the
> @@ -1183,7 +1153,7 @@ void coresight_disable(struct coresight_device *csdev)
> if (ret)
> goto out;
>
> - if (!csdev->enable || !coresight_disable_source_sysfs(csdev, NULL))
> + if (!coresight_disable_source_sysfs(csdev, NULL))
> goto out;
>
> switch (csdev->subtype.source_subtype) {
> @@ -1249,7 +1219,9 @@ static ssize_t enable_source_show(struct device *dev,
> {
> struct coresight_device *csdev = to_coresight_device(dev);
>
> - return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->enable);
> + guard(mutex)(&coresight_mutex);
> + return scnprintf(buf, PAGE_SIZE, "%u\n",
> + local_read(&csdev->mode) == CS_MODE_SYSFS);
> }
>
> static ssize_t enable_source_store(struct device *dev,
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 5f82737c37bb..65c70995ab00 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -148,7 +148,11 @@ static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
>
> CS_UNLOCK(drvdata->base);
>
> - if (!drvdata->csdev->enable)
> + /*
> + * Only do pre-port enable for first port that calls enable when the
> + * device's main refcount is still 0
> + */
> + if (!atomic_read(&drvdata->csdev->refcnt))
> tpda_enable_pre_port(drvdata);
Relying on the "csdev->enable" to do pre-port configuration looks like
a complete hack to me. This could have been performed once and for all
during the probe time, at say, tpda_init_default_data(). That value is
(drvdata->atid) never updated and need not be re-written unless the
value is lost during a power idle.
Mao, Tao, are you able to confirm this ? If that is the case, we don't
need this csdev->refcnt.
>
> ret = tpda_enable_port(drvdata, port);
> @@ -169,6 +173,7 @@ static int tpda_enable(struct coresight_device *csdev,
> ret = __tpda_enable(drvdata, in->dest_port);
> if (!ret) {
> atomic_inc(&in->dest_refcnt);
> + atomic_inc(&csdev->refcnt);
> dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
> }
> }
> @@ -197,9 +202,10 @@ static void tpda_disable(struct coresight_device *csdev,
> struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> spin_lock(&drvdata->spinlock);
> - if (atomic_dec_return(&in->dest_refcnt) == 0)
> + if (atomic_dec_return(&in->dest_refcnt) == 0) {
> __tpda_disable(drvdata, in->dest_port);
> -
> + atomic_dec(&csdev->refcnt);
If we need this, then: This should be performed outside the if () isn't it ?
e.g.,
Operation: in_conn->refcnt csdev->refcnt
port_enable : port0 0 - > 1 0 - > 1
port_enable : port0 1 - > 2 1 - > 2
port_disable: port0 2 - > 1 2 (no change)
port_disable: port0 1 - > 0 2 - > 1
As you can see the csdev->refcnt skipped a dec.
Suzuki
> + }
> spin_unlock(&drvdata->spinlock);
>
> dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port);
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index ba817f563ff7..46e6667f72ce 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -233,7 +233,6 @@ struct coresight_sysfs_link {
> * a non-atomic read would also work.
> * @refcnt: keep track of what is in use.
> * @orphan: true if the component has connections that haven't been linked.
> - * @enable: 'true' if component is currently part of an active path.
> * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
> * by writing a 1 to the 'enable_sink' file. A sink can be
> * activated but not yet enabled. Enabling for a _sink_ happens
> @@ -260,7 +259,6 @@ struct coresight_device {
> local_t mode;
> atomic_t refcnt;
> bool orphan;
> - bool enable;
> /* sink specific fields */
> bool sysfs_sink_activated;
> struct dev_ext_attribute *ea;
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 5/8] coresight: Remove the 'enable' field.
2024-01-08 14:42 ` Suzuki K Poulose
@ 2024-01-19 9:59 ` James Clark
2024-01-19 10:07 ` Suzuki K Poulose
0 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2024-01-19 9:59 UTC (permalink / raw)
To: Suzuki K Poulose, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32, Tao Zhang,
Mao Jinlong
On 08/01/2024 14:42, Suzuki K Poulose wrote:
> Hi James
>
> +Cc: Tao Zhang <quic_taozha@quicinc.com>
> +Cc: Mao Jinlong <quic_jinlmao@quicinc.com>
>
> On 12/12/2023 15:54, James Clark wrote:
>> 'enable', which probably should have been 'enabled', is only ever read
>> in the core code in relation to controlling sources, and specifically
>> only sources in sysfs mode. Confusingly it's not labelled as such and
>> relying on it can be a source of bugs like the one fixed by
>> commit 078dbba3f0c9 ("coresight: Fix crash when Perf and sysfs modes are
>> used concurrently").
>>
>> Most importantly, it can only be used when the coresight_mutex is held
>> which is only done when enabling and disabling paths in sysfs mode, and
>> not Perf mode.
>
>
> I think we may be able to relax this a bit for the syfs. The sole reason
> for holding the mutex is for the "build_path" (and get_enabled_sink)
> where we need to make sure that no devices are removed/added. We hold
> necessary refcount on the device and the module (via
> coresight_grab_device()). After which, we should be able to release the
> mutex and perform the rest without it in coresight_enable()
>
After looking at the per-sink trace ID maps a bit more, I'm not sure if
it will be worth the mental effort and risk to relax the sysfs locking
right now.
We also currently have other things like writing to the global
tracer_path which are outside of build_path/get_enabled_sink. But for
the per-sink maps change we'll also have some tracking for sysfs mode
about which sink map was used for each source and sink. And this state
will be accessed across multiple sources, and after building the path,
so it makes sense to leave the locking as-is for now IMO.
I also can't see a realistic gain from doing it, most sysfs use cases
would be done from a single threaded script. Maybe in the future we
could do the change to move the per-device locks into struct
coresight_device, and then the core code can use them for things that
need to be locked, but don't need the full coresight_mutex. And then
that would also work for the per-sink case. But at the moment each
device has its own lock so that's difficult.
>> So to prevent its usage spreading and leaking out to
>> other devices, remove it.
>>
>> It's use is equivalent to checking if the mode is currently sysfs, as
>> due to the coresight_mutex lock, mode == CS_MODE_SYSFS can only become
>> true or untrue when that lock is held, and when mode == CS_MODE_SYSFS
>> the device is both enabled and in sysfs mode.
>
> All of the above makes sense and looks good to me.
>
>>
>> The one place it was used outside of the core code is in TPDA, but that
>> pattern is more appropriately represented using refcounts inside the
>> device's own spinlock.
>
> But, I think we can clean up this code a bit more better. See below.
>
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> drivers/hwtracing/coresight/coresight-core.c | 86 +++++++-------------
>> drivers/hwtracing/coresight/coresight-tpda.c | 12 ++-
>> include/linux/coresight.h | 2 -
>> 3 files changed, 38 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index ab226441e5f4..1d0bd1586590 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -279,29 +279,18 @@ EXPORT_SYMBOL_GPL(coresight_add_helper);
>> static int coresight_enable_sink(struct coresight_device *csdev,
>> enum cs_mode mode, void *data)
>> {
>> - int ret = sink_ops(csdev)->enable(csdev, mode, data);
>> -
>> - if (ret)
>> - return ret;
>> -
>> - csdev->enable = true;
>> -
>> - return 0;
>> + return sink_ops(csdev)->enable(csdev, mode, data);
>> }
>> static void coresight_disable_sink(struct coresight_device *csdev)
>> {
>> - int ret = sink_ops(csdev)->disable(csdev);
>> - if (ret)
>> - return;
>> - csdev->enable = false;
>> + sink_ops(csdev)->disable(csdev);
>> }
>> static int coresight_enable_link(struct coresight_device *csdev,
>> struct coresight_device *parent,
>> struct coresight_device *child)
>> {
>> - int ret = 0;
>> int link_subtype;
>> struct coresight_connection *inconn, *outconn;
>> @@ -317,19 +306,13 @@ static int coresight_enable_link(struct
>> coresight_device *csdev,
>> if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT &&
>> IS_ERR(outconn))
>> return PTR_ERR(outconn);
>> - ret = link_ops(csdev)->enable(csdev, inconn, outconn);
>> - if (!ret)
>> - csdev->enable = true;
>> -
>> - return ret;
>> + return link_ops(csdev)->enable(csdev, inconn, outconn);
>> }
>> static void coresight_disable_link(struct coresight_device *csdev,
>> struct coresight_device *parent,
>> struct coresight_device *child)
>> {
>> - int i;
>> - int link_subtype;
>> struct coresight_connection *inconn, *outconn;
>> if (!parent || !child)
>> @@ -337,26 +320,8 @@ static void coresight_disable_link(struct
>> coresight_device *csdev,
>> inconn = coresight_find_out_connection(parent, csdev);
>> outconn = coresight_find_out_connection(csdev, child);
>> - link_subtype = csdev->subtype.link_subtype;
>> link_ops(csdev)->disable(csdev, inconn, outconn);
>> -
>> - if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
>> - for (i = 0; i < csdev->pdata->nr_inconns; i++)
>> - if (atomic_read(&csdev->pdata->in_conns[i]->dest_refcnt) !=
>> - 0)
>> - return;
>> - } else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT) {
>> - for (i = 0; i < csdev->pdata->nr_outconns; i++)
>> - if (atomic_read(&csdev->pdata->out_conns[i]->src_refcnt) !=
>> - 0)
>> - return;
>> - } else {
>> - if (atomic_read(&csdev->refcnt) != 0)
>> - return;
>> - }
>> -
>> - csdev->enable = false;
>> }
>> int coresight_enable_source(struct coresight_device *csdev, enum
>> cs_mode mode,
>> @@ -364,11 +329,16 @@ int coresight_enable_source(struct
>> coresight_device *csdev, enum cs_mode mode,
>> {
>> int ret;
>> - if (!csdev->enable) {
>> + /*
>> + * Comparison with CS_MODE_SYSFS works without taking any device
>> + * specific spinlock because the truthyness of that comparison
>> can only
>> + * change with coresight_mutex held, which we already have here.
>> + */
>> + lockdep_assert_held(&coresight_mutex);
>> + if (local_read(&csdev->mode) != CS_MODE_SYSFS) {
>> ret = source_ops(csdev)->enable(csdev, data, mode);
>> if (ret)
>> return ret;
>> - csdev->enable = true;
>> }
>> atomic_inc(&csdev->refcnt);
>> @@ -385,22 +355,12 @@ static bool coresight_is_helper(struct
>> coresight_device *csdev)
>> static int coresight_enable_helper(struct coresight_device *csdev,
>> enum cs_mode mode, void *data)
>> {
>> - int ret = helper_ops(csdev)->enable(csdev, mode, data);
>> -
>> - if (ret)
>> - return ret;
>> -
>> - csdev->enable = true;
>> - return 0;
>> + return helper_ops(csdev)->enable(csdev, mode, data);
>> }
>> static void coresight_disable_helper(struct coresight_device *csdev)
>> {
>> - int ret = helper_ops(csdev)->disable(csdev, NULL);
>> -
>> - if (ret)
>> - return;
>> - csdev->enable = false;
>> + helper_ops(csdev)->disable(csdev, NULL);
>> }
>> static void coresight_disable_helpers(struct coresight_device *csdev)
>> @@ -445,11 +405,15 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
>> static bool coresight_disable_source_sysfs(struct coresight_device
>> *csdev,
>> void *data)
>> {
>> + lockdep_assert_held(&coresight_mutex);
>> + if (local_read(&csdev->mode) != CS_MODE_SYSFS)
>> + return false;
>> +
>> if (atomic_dec_return(&csdev->refcnt) == 0) {
>> coresight_disable_source(csdev, data);
>> - csdev->enable = false;
>> + return true;
>> }
>> - return !csdev->enable;
>> + return false;
>> }
>> /*
>> @@ -1097,7 +1061,13 @@ int coresight_enable(struct coresight_device
>> *csdev)
>> if (ret)
>> goto out;
>> - if (csdev->enable) {
>> + /*
>> + * mode == SYSFS implies that it's already enabled. Don't look at
>> the
>> + * refcount to determine this because we don't claim the source
>> until
>> + * coresight_enable_source() so can still race with Perf mode which
>> + * doesn't hold coresight_mutex.
>> + */
>> + if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
>> /*
>> * There could be multiple applications driving the software
>> * source. So keep the refcount for each such user when the
>> @@ -1183,7 +1153,7 @@ void coresight_disable(struct coresight_device
>> *csdev)
>> if (ret)
>> goto out;
>> - if (!csdev->enable || !coresight_disable_source_sysfs(csdev,
>> NULL))
>> + if (!coresight_disable_source_sysfs(csdev, NULL))
>> goto out;
>> switch (csdev->subtype.source_subtype) {
>> @@ -1249,7 +1219,9 @@ static ssize_t enable_source_show(struct device
>> *dev,
>> {
>> struct coresight_device *csdev = to_coresight_device(dev);
>> - return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->enable);
>> + guard(mutex)(&coresight_mutex);
>> + return scnprintf(buf, PAGE_SIZE, "%u\n",
>> + local_read(&csdev->mode) == CS_MODE_SYSFS);
>> }
>> static ssize_t enable_source_store(struct device *dev,
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c
>> b/drivers/hwtracing/coresight/coresight-tpda.c
>> index 5f82737c37bb..65c70995ab00 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -148,7 +148,11 @@ static int __tpda_enable(struct tpda_drvdata
>> *drvdata, int port)
>> CS_UNLOCK(drvdata->base);
>> - if (!drvdata->csdev->enable)
>> + /*
>> + * Only do pre-port enable for first port that calls enable when the
>> + * device's main refcount is still 0
>> + */
>> + if (!atomic_read(&drvdata->csdev->refcnt))
>> tpda_enable_pre_port(drvdata);
>
> Relying on the "csdev->enable" to do pre-port configuration looks like
> a complete hack to me. This could have been performed once and for all
> during the probe time, at say, tpda_init_default_data(). That value is
> (drvdata->atid) never updated and need not be re-written unless the
> value is lost during a power idle.
>
> Mao, Tao, are you able to confirm this ? If that is the case, we don't
> need this csdev->refcnt.
>
"unless the value is lost during a power idle."
That one might be a reason to do it. I suppose it makes sense to write
all the registers on enable, but not to keep re-writing the trace ID
again for every port that's used.
>> ret = tpda_enable_port(drvdata, port);
>> @@ -169,6 +173,7 @@ static int tpda_enable(struct coresight_device
>> *csdev,
>> ret = __tpda_enable(drvdata, in->dest_port);
>> if (!ret) {
>> atomic_inc(&in->dest_refcnt);
>> + atomic_inc(&csdev->refcnt);
>> dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n",
>> in->dest_port);
>> }
>> }
>> @@ -197,9 +202,10 @@ static void tpda_disable(struct coresight_device
>> *csdev,
>> struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> spin_lock(&drvdata->spinlock);
>> - if (atomic_dec_return(&in->dest_refcnt) == 0)
>> + if (atomic_dec_return(&in->dest_refcnt) == 0) {
>> __tpda_disable(drvdata, in->dest_port);
>> -
>> + atomic_dec(&csdev->refcnt);
>
> If we need this, then: This should be performed outside the if () isn't
> it ?
>
> e.g.,
>
> Operation: in_conn->refcnt csdev->refcnt
> port_enable : port0 0 - > 1 0 - > 1
> port_enable : port0 1 - > 2 1 - > 2
I didn't get 1 -> 2 here, because the in->dest_refcnt is only
incremented once if it was already 0 (although it is once for each port):
if (atomic_read(&in->dest_refcnt) == 0) {
ret = __tpda_enable(drvdata, in->dest_port);
if (!ret) {
atomic_inc(&in->dest_refcnt);
That basically makes csdev->refcnt count how many ports were used, but
each port's refcount can only be used once. And then when each port is
disabled csdev->refcnt returns to zero on the last port.
> port_disable: port0 2 - > 1 2 (no change)
> port_disable: port0 1 - > 0 2 - > 1
>
> As you can see the csdev->refcnt skipped a dec.
>
> Suzuki
>
>
>> + }
>> spin_unlock(&drvdata->spinlock);
>> dev_dbg(drvdata->dev, "TPDA inport %d disabled\n",
>> in->dest_port);
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index ba817f563ff7..46e6667f72ce 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -233,7 +233,6 @@ struct coresight_sysfs_link {
>> * a non-atomic read would also work.
>> * @refcnt: keep track of what is in use.
>> * @orphan: true if the component has connections that haven't
>> been linked.
>> - * @enable: 'true' if component is currently part of an active path.
>> * @sysfs_sink_activated: 'true' when a sink has been selected for
>> use via sysfs
>> * by writing a 1 to the 'enable_sink' file. A sink can be
>> * activated but not yet enabled. Enabling for a _sink_ happens
>> @@ -260,7 +259,6 @@ struct coresight_device {
>> local_t mode;
>> atomic_t refcnt;
>> bool orphan;
>> - bool enable;
>> /* sink specific fields */
>> bool sysfs_sink_activated;
>> struct dev_ext_attribute *ea;
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 5/8] coresight: Remove the 'enable' field.
2024-01-19 9:59 ` James Clark
@ 2024-01-19 10:07 ` Suzuki K Poulose
0 siblings, 0 replies; 22+ messages in thread
From: Suzuki K Poulose @ 2024-01-19 10:07 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32, Tao Zhang,
Mao Jinlong
On 19/01/2024 09:59, James Clark wrote:
>
>
> On 08/01/2024 14:42, Suzuki K Poulose wrote:
>> Hi James
>>
>> +Cc: Tao Zhang <quic_taozha@quicinc.com>
>> +Cc: Mao Jinlong <quic_jinlmao@quicinc.com>
>>
>> On 12/12/2023 15:54, James Clark wrote:
>>> 'enable', which probably should have been 'enabled', is only ever read
>>> in the core code in relation to controlling sources, and specifically
>>> only sources in sysfs mode. Confusingly it's not labelled as such and
>>> relying on it can be a source of bugs like the one fixed by
>>> commit 078dbba3f0c9 ("coresight: Fix crash when Perf and sysfs modes are
>>> used concurrently").
>>>
>>> Most importantly, it can only be used when the coresight_mutex is held
>>> which is only done when enabling and disabling paths in sysfs mode, and
>>> not Perf mode.
>>
>>
>> I think we may be able to relax this a bit for the syfs. The sole reason
>> for holding the mutex is for the "build_path" (and get_enabled_sink)
>> where we need to make sure that no devices are removed/added. We hold
>> necessary refcount on the device and the module (via
>> coresight_grab_device()). After which, we should be able to release the
>> mutex and perform the rest without it in coresight_enable()
>>
>
> After looking at the per-sink trace ID maps a bit more, I'm not sure if
> it will be worth the mental effort and risk to relax the sysfs locking
> right now.
>
> We also currently have other things like writing to the global
> tracer_path which are outside of build_path/get_enabled_sink. But for
> the per-sink maps change we'll also have some tracking for sysfs mode
> about which sink map was used for each source and sink. And this state
> will be accessed across multiple sources, and after building the path,
> so it makes sense to leave the locking as-is for now IMO.
>
> I also can't see a realistic gain from doing it, most sysfs use cases
> would be done from a single threaded script. Maybe in the future we
> could do the change to move the per-device locks into struct
> coresight_device, and then the core code can use them for things that
> need to be locked, but don't need the full coresight_mutex. And then
> that would also work for the per-sink case. But at the moment each
> device has its own lock so that's difficult.
Ok, we could come back to this after the per-sink trace id pool work.
My observation was about the inconsistency between the perf vs sysfs
mode as you mentioned in the above code.
Suzuki
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 6/8] coresight: Move all sysfs code to sysfs file
2023-12-12 15:53 [PATCH 0/8] coresight: Separate sysfs and Perf usage and some other cleanups James Clark
` (4 preceding siblings ...)
2023-12-12 15:54 ` [PATCH 5/8] coresight: Remove the 'enable' field James Clark
@ 2023-12-12 15:54 ` James Clark
2024-01-09 10:22 ` Suzuki K Poulose
2023-12-12 15:54 ` [PATCH 7/8] coresight: Remove atomic type from refcnt James Clark
2023-12-12 15:54 ` [PATCH 8/8] coresight: Remove unused stubs James Clark
7 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2023-12-12 15:54 UTC (permalink / raw)
To: coresight, suzuki.poulose
Cc: James Clark, Mike Leach, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, linux-arm-kernel, linux-kernel, linux-stm32
At the moment the core file contains both sysfs functionality and
core functionality, while the Perf mode is in a separate file in
coresight-etm-perf.c
Many of the functions have ambiguous names like
coresight_enable_source() which actually only work in relation to the
sysfs mode. To avoid further confusion, move everything that isn't core
functionality into the sysfs file and append _sysfs to the ambiguous
functions.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-core.c | 394 +-----------------
.../coresight/coresight-etm3x-core.c | 4 +-
.../coresight/coresight-etm4x-core.c | 4 +-
drivers/hwtracing/coresight/coresight-priv.h | 5 +-
drivers/hwtracing/coresight/coresight-stm.c | 4 +-
drivers/hwtracing/coresight/coresight-sysfs.c | 390 +++++++++++++++++
include/linux/coresight.h | 8 +-
7 files changed, 407 insertions(+), 402 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 1d0bd1586590..eb72cdd1a273 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -9,7 +9,6 @@
#include <linux/types.h>
#include <linux/device.h>
#include <linux/io.h>
-#include <linux/idr.h>
#include <linux/err.h>
#include <linux/export.h>
#include <linux/slab.h>
@@ -25,15 +24,12 @@
#include "coresight-priv.h"
#include "coresight-syscfg.h"
-static DEFINE_MUTEX(coresight_mutex);
-static DEFINE_PER_CPU(struct coresight_device *, csdev_sink);
-
/*
- * Use IDR to map the hash of the source's device name
- * to the pointer of path for the source. The idr is for
- * the sources which aren't associated with CPU.
+ * Mutex used to lock all sysfs enable and disable actions and loading and
+ * unloading devices by the Coresight core.
*/
-static DEFINE_IDR(path_idr);
+DEFINE_MUTEX(coresight_mutex);
+static DEFINE_PER_CPU(struct coresight_device *, csdev_sink);
/**
* struct coresight_node - elements of a path, from source to sink
@@ -45,12 +41,6 @@ struct coresight_node {
struct list_head link;
};
-/*
- * When operating Coresight drivers from the sysFS interface, only a single
- * path can exist from a tracer (associated to a CPU) to a sink.
- */
-static DEFINE_PER_CPU(struct list_head *, tracer_path);
-
/*
* When losing synchronisation a new barrier packet needs to be inserted at the
* beginning of the data collected in a buffer. That way the decoder knows that
@@ -61,34 +51,6 @@ EXPORT_SYMBOL_GPL(coresight_barrier_pkt);
static const struct cti_assoc_op *cti_assoc_ops;
-ssize_t coresight_simple_show_pair(struct device *_dev,
- struct device_attribute *attr, char *buf)
-{
- struct coresight_device *csdev = container_of(_dev, struct coresight_device, dev);
- struct cs_pair_attribute *cs_attr = container_of(attr, struct cs_pair_attribute, attr);
- u64 val;
-
- pm_runtime_get_sync(_dev->parent);
- val = csdev_access_relaxed_read_pair(&csdev->access, cs_attr->lo_off, cs_attr->hi_off);
- pm_runtime_put_sync(_dev->parent);
- return sysfs_emit(buf, "0x%llx\n", val);
-}
-EXPORT_SYMBOL_GPL(coresight_simple_show_pair);
-
-ssize_t coresight_simple_show32(struct device *_dev,
- struct device_attribute *attr, char *buf)
-{
- struct coresight_device *csdev = container_of(_dev, struct coresight_device, dev);
- struct cs_off_attribute *cs_attr = container_of(attr, struct cs_off_attribute, attr);
- u64 val;
-
- pm_runtime_get_sync(_dev->parent);
- val = csdev_access_relaxed_read32(&csdev->access, cs_attr->off);
- pm_runtime_put_sync(_dev->parent);
- return sysfs_emit(buf, "0x%llx\n", val);
-}
-EXPORT_SYMBOL_GPL(coresight_simple_show32);
-
void coresight_set_cti_ops(const struct cti_assoc_op *cti_op)
{
cti_assoc_ops = cti_op;
@@ -324,29 +286,6 @@ static void coresight_disable_link(struct coresight_device *csdev,
link_ops(csdev)->disable(csdev, inconn, outconn);
}
-int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
- void *data)
-{
- int ret;
-
- /*
- * Comparison with CS_MODE_SYSFS works without taking any device
- * specific spinlock because the truthyness of that comparison can only
- * change with coresight_mutex held, which we already have here.
- */
- lockdep_assert_held(&coresight_mutex);
- if (local_read(&csdev->mode) != CS_MODE_SYSFS) {
- ret = source_ops(csdev)->enable(csdev, data, mode);
- if (ret)
- return ret;
- }
-
- atomic_inc(&csdev->refcnt);
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(coresight_enable_source);
-
static bool coresight_is_helper(struct coresight_device *csdev)
{
return csdev->type == CORESIGHT_DEV_TYPE_HELPER;
@@ -392,30 +331,6 @@ void coresight_disable_source(struct coresight_device *csdev, void *data)
}
EXPORT_SYMBOL_GPL(coresight_disable_source);
-/**
- * coresight_disable_source_sysfs - Drop the reference count by 1 and disable
- * the device if there are no users left.
- *
- * @csdev: The coresight device to disable
- * @data: Opaque data to pass on to the disable function of the source device.
- * For example in perf mode this is a pointer to the struct perf_event.
- *
- * Returns true if the device has been disabled.
- */
-static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
- void *data)
-{
- lockdep_assert_held(&coresight_mutex);
- if (local_read(&csdev->mode) != CS_MODE_SYSFS)
- return false;
-
- if (atomic_dec_return(&csdev->refcnt) == 0) {
- coresight_disable_source(csdev, data);
- return true;
- }
- return false;
-}
-
/*
* coresight_disable_path_from : Disable components in the given path beyond
* @nd in the list. If @nd is NULL, all the components, except the SOURCE are
@@ -572,39 +487,6 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
return csdev;
}
-/**
- * coresight_find_activated_sysfs_sink - returns the first sink activated via
- * sysfs using connection based search starting from the source reference.
- *
- * @csdev: Coresight source device reference
- */
-static struct coresight_device *
-coresight_find_activated_sysfs_sink(struct coresight_device *csdev)
-{
- int i;
- struct coresight_device *sink = NULL;
-
- if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
- csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
- csdev->sysfs_sink_activated)
- return csdev;
-
- /*
- * Recursively explore each port found on this element.
- */
- for (i = 0; i < csdev->pdata->nr_outconns; i++) {
- struct coresight_device *child_dev;
-
- child_dev = csdev->pdata->out_conns[i]->dest_dev;
- if (child_dev)
- sink = coresight_find_activated_sysfs_sink(child_dev);
- if (sink)
- return sink;
- }
-
- return NULL;
-}
-
static int coresight_sink_by_id(struct device *dev, const void *data)
{
struct coresight_device *csdev = to_coresight_device(dev);
@@ -1015,274 +897,6 @@ static void coresight_clear_default_sink(struct coresight_device *csdev)
}
}
-/** coresight_validate_source - make sure a source has the right credentials
- * @csdev: the device structure for a source.
- * @function: the function this was called from.
- *
- * Assumes the coresight_mutex is held.
- */
-static int coresight_validate_source(struct coresight_device *csdev,
- const char *function)
-{
- u32 type, subtype;
-
- type = csdev->type;
- subtype = csdev->subtype.source_subtype;
-
- if (type != CORESIGHT_DEV_TYPE_SOURCE) {
- dev_err(&csdev->dev, "wrong device type in %s\n", function);
- return -EINVAL;
- }
-
- if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
- subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
- subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
- subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
- dev_err(&csdev->dev, "wrong device subtype in %s\n", function);
- return -EINVAL;
- }
-
- return 0;
-}
-
-int coresight_enable(struct coresight_device *csdev)
-{
- int cpu, ret = 0;
- struct coresight_device *sink;
- struct list_head *path;
- enum coresight_dev_subtype_source subtype;
- u32 hash;
-
- subtype = csdev->subtype.source_subtype;
-
- mutex_lock(&coresight_mutex);
-
- ret = coresight_validate_source(csdev, __func__);
- if (ret)
- goto out;
-
- /*
- * mode == SYSFS implies that it's already enabled. Don't look at the
- * refcount to determine this because we don't claim the source until
- * coresight_enable_source() so can still race with Perf mode which
- * doesn't hold coresight_mutex.
- */
- if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
- /*
- * There could be multiple applications driving the software
- * source. So keep the refcount for each such user when the
- * source is already enabled.
- */
- if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE)
- atomic_inc(&csdev->refcnt);
- goto out;
- }
-
- sink = coresight_find_activated_sysfs_sink(csdev);
- if (!sink) {
- ret = -EINVAL;
- goto out;
- }
-
- path = coresight_build_path(csdev, sink);
- if (IS_ERR(path)) {
- pr_err("building path(s) failed\n");
- ret = PTR_ERR(path);
- goto out;
- }
-
- ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
- if (ret)
- goto err_path;
-
- ret = coresight_enable_source(csdev, CS_MODE_SYSFS, NULL);
- if (ret)
- goto err_source;
-
- switch (subtype) {
- case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
- /*
- * When working from sysFS it is important to keep track
- * of the paths that were created so that they can be
- * undone in 'coresight_disable()'. Since there can only
- * be a single session per tracer (when working from sysFS)
- * a per-cpu variable will do just fine.
- */
- cpu = source_ops(csdev)->cpu_id(csdev);
- per_cpu(tracer_path, cpu) = path;
- break;
- case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
- case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
- case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
- /*
- * Use the hash of source's device name as ID
- * and map the ID to the pointer of the path.
- */
- hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
- ret = idr_alloc_u32(&path_idr, path, &hash, hash, GFP_KERNEL);
- if (ret)
- goto err_source;
- break;
- default:
- /* We can't be here */
- break;
- }
-
-out:
- mutex_unlock(&coresight_mutex);
- return ret;
-
-err_source:
- coresight_disable_path(path);
-
-err_path:
- coresight_release_path(path);
- goto out;
-}
-EXPORT_SYMBOL_GPL(coresight_enable);
-
-void coresight_disable(struct coresight_device *csdev)
-{
- int cpu, ret;
- struct list_head *path = NULL;
- u32 hash;
-
- mutex_lock(&coresight_mutex);
-
- ret = coresight_validate_source(csdev, __func__);
- if (ret)
- goto out;
-
- if (!coresight_disable_source_sysfs(csdev, NULL))
- goto out;
-
- switch (csdev->subtype.source_subtype) {
- case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
- cpu = source_ops(csdev)->cpu_id(csdev);
- path = per_cpu(tracer_path, cpu);
- per_cpu(tracer_path, cpu) = NULL;
- break;
- case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
- case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
- case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
- hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
- /* Find the path by the hash. */
- path = idr_find(&path_idr, hash);
- if (path == NULL) {
- pr_err("Path is not found for %s\n", dev_name(&csdev->dev));
- goto out;
- }
- idr_remove(&path_idr, hash);
- break;
- default:
- /* We can't be here */
- break;
- }
-
- coresight_disable_path(path);
- coresight_release_path(path);
-
-out:
- mutex_unlock(&coresight_mutex);
-}
-EXPORT_SYMBOL_GPL(coresight_disable);
-
-static ssize_t enable_sink_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct coresight_device *csdev = to_coresight_device(dev);
-
- return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->sysfs_sink_activated);
-}
-
-static ssize_t enable_sink_store(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t size)
-{
- int ret;
- unsigned long val;
- struct coresight_device *csdev = to_coresight_device(dev);
-
- ret = kstrtoul(buf, 10, &val);
- if (ret)
- return ret;
-
- csdev->sysfs_sink_activated = !!val;
-
- return size;
-
-}
-static DEVICE_ATTR_RW(enable_sink);
-
-static ssize_t enable_source_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct coresight_device *csdev = to_coresight_device(dev);
-
- guard(mutex)(&coresight_mutex);
- return scnprintf(buf, PAGE_SIZE, "%u\n",
- local_read(&csdev->mode) == CS_MODE_SYSFS);
-}
-
-static ssize_t enable_source_store(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t size)
-{
- int ret = 0;
- unsigned long val;
- struct coresight_device *csdev = to_coresight_device(dev);
-
- ret = kstrtoul(buf, 10, &val);
- if (ret)
- return ret;
-
- if (val) {
- ret = coresight_enable(csdev);
- if (ret)
- return ret;
- } else {
- coresight_disable(csdev);
- }
-
- return size;
-}
-static DEVICE_ATTR_RW(enable_source);
-
-static struct attribute *coresight_sink_attrs[] = {
- &dev_attr_enable_sink.attr,
- NULL,
-};
-ATTRIBUTE_GROUPS(coresight_sink);
-
-static struct attribute *coresight_source_attrs[] = {
- &dev_attr_enable_source.attr,
- NULL,
-};
-ATTRIBUTE_GROUPS(coresight_source);
-
-static struct device_type coresight_dev_type[] = {
- {
- .name = "sink",
- .groups = coresight_sink_groups,
- },
- {
- .name = "link",
- },
- {
- .name = "linksink",
- .groups = coresight_sink_groups,
- },
- {
- .name = "source",
- .groups = coresight_source_groups,
- },
- {
- .name = "helper",
- }
-};
-/* Ensure the enum matches the names and groups */
-static_assert(ARRAY_SIZE(coresight_dev_type) == CORESIGHT_DEV_TYPE_MAX);
-
static void coresight_device_release(struct device *dev)
{
struct coresight_device *csdev = to_coresight_device(dev);
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index 29f62dfac673..a8be115636f0 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -714,7 +714,7 @@ static int etm_online_cpu(unsigned int cpu)
return 0;
if (etmdrvdata[cpu]->boot_enable && !etmdrvdata[cpu]->sticky_enable)
- coresight_enable(etmdrvdata[cpu]->csdev);
+ coresight_enable_sysfs(etmdrvdata[cpu]->csdev);
return 0;
}
@@ -924,7 +924,7 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)
dev_info(&drvdata->csdev->dev,
"%s initialized\n", (char *)coresight_get_uci_data(id));
if (boot_enable) {
- coresight_enable(drvdata->csdev);
+ coresight_enable_sysfs(drvdata->csdev);
drvdata->boot_enable = true;
}
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index ce1995a2827f..cbb4ac8be133 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1650,7 +1650,7 @@ static int etm4_online_cpu(unsigned int cpu)
return etm4_probe_cpu(cpu);
if (etmdrvdata[cpu]->boot_enable && !etmdrvdata[cpu]->sticky_enable)
- coresight_enable(etmdrvdata[cpu]->csdev);
+ coresight_enable_sysfs(etmdrvdata[cpu]->csdev);
return 0;
}
@@ -2098,7 +2098,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
drvdata->cpu, type_name, major, minor);
if (boot_enable) {
- coresight_enable(drvdata->csdev);
+ coresight_enable_sysfs(drvdata->csdev);
drvdata->boot_enable = true;
}
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index ced5be05a527..eb365236f9a9 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -12,6 +12,9 @@
#include <linux/coresight.h>
#include <linux/pm_runtime.h>
+extern struct mutex coresight_mutex;
+extern struct device_type coresight_dev_type[];
+
/*
* Coresight management registers (0xf00-0xfcc)
* 0xfa0 - 0xfa4: Management registers in PFTv1.0
@@ -229,8 +232,6 @@ void coresight_add_helper(struct coresight_device *csdev,
void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev);
struct coresight_device *coresight_get_percpu_sink(int cpu);
-int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
- void *data);
void coresight_disable_source(struct coresight_device *csdev, void *data);
#endif
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index 190ba569b05a..af9b21224246 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -332,7 +332,7 @@ static int stm_generic_link(struct stm_data *stm_data,
if (!drvdata || !drvdata->csdev)
return -EINVAL;
- return coresight_enable(drvdata->csdev);
+ return coresight_enable_sysfs(drvdata->csdev);
}
static void stm_generic_unlink(struct stm_data *stm_data,
@@ -343,7 +343,7 @@ static void stm_generic_unlink(struct stm_data *stm_data,
if (!drvdata || !drvdata->csdev)
return;
- coresight_disable(drvdata->csdev);
+ coresight_disable_sysfs(drvdata->csdev);
}
static phys_addr_t
diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
index dd78e9fcfc4d..92cdf8139f23 100644
--- a/drivers/hwtracing/coresight/coresight-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-sysfs.c
@@ -5,10 +5,400 @@
*/
#include <linux/device.h>
+#include <linux/idr.h>
#include <linux/kernel.h>
#include "coresight-priv.h"
+/*
+ * Use IDR to map the hash of the source's device name
+ * to the pointer of path for the source. The idr is for
+ * the sources which aren't associated with CPU.
+ */
+static DEFINE_IDR(path_idr);
+
+/*
+ * When operating Coresight drivers from the sysFS interface, only a single
+ * path can exist from a tracer (associated to a CPU) to a sink.
+ */
+static DEFINE_PER_CPU(struct list_head *, tracer_path);
+
+ssize_t coresight_simple_show_pair(struct device *_dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct coresight_device *csdev = container_of(_dev, struct coresight_device, dev);
+ struct cs_pair_attribute *cs_attr = container_of(attr, struct cs_pair_attribute, attr);
+ u64 val;
+
+ pm_runtime_get_sync(_dev->parent);
+ val = csdev_access_relaxed_read_pair(&csdev->access, cs_attr->lo_off, cs_attr->hi_off);
+ pm_runtime_put_sync(_dev->parent);
+ return sysfs_emit(buf, "0x%llx\n", val);
+}
+EXPORT_SYMBOL_GPL(coresight_simple_show_pair);
+
+ssize_t coresight_simple_show32(struct device *_dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct coresight_device *csdev = container_of(_dev, struct coresight_device, dev);
+ struct cs_off_attribute *cs_attr = container_of(attr, struct cs_off_attribute, attr);
+ u64 val;
+
+ pm_runtime_get_sync(_dev->parent);
+ val = csdev_access_relaxed_read32(&csdev->access, cs_attr->off);
+ pm_runtime_put_sync(_dev->parent);
+ return sysfs_emit(buf, "0x%llx\n", val);
+}
+EXPORT_SYMBOL_GPL(coresight_simple_show32);
+
+static int coresight_enable_source_sysfs(struct coresight_device *csdev,
+ enum cs_mode mode, void *data)
+{
+ int ret;
+
+ /*
+ * Comparison with CS_MODE_SYSFS works without taking any device
+ * specific spinlock because the truthyness of that comparison can only
+ * change with coresight_mutex held, which we already have here.
+ */
+ lockdep_assert_held(&coresight_mutex);
+ if (local_read(&csdev->mode) != CS_MODE_SYSFS) {
+ ret = source_ops(csdev)->enable(csdev, data, mode);
+ if (ret)
+ return ret;
+ }
+
+ atomic_inc(&csdev->refcnt);
+
+ return 0;
+}
+
+/**
+ * coresight_disable_source_sysfs - Drop the reference count by 1 and disable
+ * the device if there are no users left.
+ *
+ * @csdev: The coresight device to disable
+ * @data: Opaque data to pass on to the disable function of the source device.
+ * For example in perf mode this is a pointer to the struct perf_event.
+ *
+ * Returns true if the device has been disabled.
+ */
+static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
+ void *data)
+{
+ lockdep_assert_held(&coresight_mutex);
+ if (local_read(&csdev->mode) != CS_MODE_SYSFS)
+ return false;
+
+ if (atomic_dec_return(&csdev->refcnt) == 0) {
+ coresight_disable_source(csdev, data);
+ return true;
+ }
+ return false;
+}
+
+/**
+ * coresight_find_activated_sysfs_sink - returns the first sink activated via
+ * sysfs using connection based search starting from the source reference.
+ *
+ * @csdev: Coresight source device reference
+ */
+static struct coresight_device *
+coresight_find_activated_sysfs_sink(struct coresight_device *csdev)
+{
+ int i;
+ struct coresight_device *sink = NULL;
+
+ if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
+ csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
+ csdev->sysfs_sink_activated)
+ return csdev;
+
+ /*
+ * Recursively explore each port found on this element.
+ */
+ for (i = 0; i < csdev->pdata->nr_outconns; i++) {
+ struct coresight_device *child_dev;
+
+ child_dev = csdev->pdata->out_conns[i]->dest_dev;
+ if (child_dev)
+ sink = coresight_find_activated_sysfs_sink(child_dev);
+ if (sink)
+ return sink;
+ }
+
+ return NULL;
+}
+
+/** coresight_validate_source - make sure a source has the right credentials to
+ * be used via sysfs.
+ * @csdev: the device structure for a source.
+ * @function: the function this was called from.
+ *
+ * Assumes the coresight_mutex is held.
+ */
+static int coresight_validate_source_sysfs(struct coresight_device *csdev,
+ const char *function)
+{
+ u32 type, subtype;
+
+ type = csdev->type;
+ subtype = csdev->subtype.source_subtype;
+
+ if (type != CORESIGHT_DEV_TYPE_SOURCE) {
+ dev_err(&csdev->dev, "wrong device type in %s\n", function);
+ return -EINVAL;
+ }
+
+ if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC &&
+ subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE &&
+ subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM &&
+ subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) {
+ dev_err(&csdev->dev, "wrong device subtype in %s\n", function);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+int coresight_enable_sysfs(struct coresight_device *csdev)
+{
+ int cpu, ret = 0;
+ struct coresight_device *sink;
+ struct list_head *path;
+ enum coresight_dev_subtype_source subtype;
+ u32 hash;
+
+ subtype = csdev->subtype.source_subtype;
+
+ mutex_lock(&coresight_mutex);
+
+ ret = coresight_validate_source_sysfs(csdev, __func__);
+ if (ret)
+ goto out;
+
+ /*
+ * mode == SYSFS implies that it's already enabled. Don't look at the
+ * refcount to determine this because we don't claim the source until
+ * coresight_enable_source() so can still race with Perf mode which
+ * doesn't hold coresight_mutex.
+ */
+ if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
+ /*
+ * There could be multiple applications driving the software
+ * source. So keep the refcount for each such user when the
+ * source is already enabled.
+ */
+ if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE)
+ atomic_inc(&csdev->refcnt);
+ goto out;
+ }
+
+ sink = coresight_find_activated_sysfs_sink(csdev);
+ if (!sink) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ path = coresight_build_path(csdev, sink);
+ if (IS_ERR(path)) {
+ pr_err("building path(s) failed\n");
+ ret = PTR_ERR(path);
+ goto out;
+ }
+
+ ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
+ if (ret)
+ goto err_path;
+
+ ret = coresight_enable_source_sysfs(csdev, CS_MODE_SYSFS, NULL);
+ if (ret)
+ goto err_source;
+
+ switch (subtype) {
+ case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
+ /*
+ * When working from sysFS it is important to keep track
+ * of the paths that were created so that they can be
+ * undone in 'coresight_disable()'. Since there can only
+ * be a single session per tracer (when working from sysFS)
+ * a per-cpu variable will do just fine.
+ */
+ cpu = source_ops(csdev)->cpu_id(csdev);
+ per_cpu(tracer_path, cpu) = path;
+ break;
+ case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
+ case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
+ case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
+ /*
+ * Use the hash of source's device name as ID
+ * and map the ID to the pointer of the path.
+ */
+ hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
+ ret = idr_alloc_u32(&path_idr, path, &hash, hash, GFP_KERNEL);
+ if (ret)
+ goto err_source;
+ break;
+ default:
+ /* We can't be here */
+ break;
+ }
+
+out:
+ mutex_unlock(&coresight_mutex);
+ return ret;
+
+err_source:
+ coresight_disable_path(path);
+
+err_path:
+ coresight_release_path(path);
+ goto out;
+}
+EXPORT_SYMBOL_GPL(coresight_enable_sysfs);
+
+void coresight_disable_sysfs(struct coresight_device *csdev)
+{
+ int cpu, ret;
+ struct list_head *path = NULL;
+ u32 hash;
+
+ mutex_lock(&coresight_mutex);
+
+ ret = coresight_validate_source_sysfs(csdev, __func__);
+ if (ret)
+ goto out;
+
+ if (!coresight_disable_source_sysfs(csdev, NULL))
+ goto out;
+
+ switch (csdev->subtype.source_subtype) {
+ case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
+ cpu = source_ops(csdev)->cpu_id(csdev);
+ path = per_cpu(tracer_path, cpu);
+ per_cpu(tracer_path, cpu) = NULL;
+ break;
+ case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
+ case CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM:
+ case CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
+ hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev)));
+ /* Find the path by the hash. */
+ path = idr_find(&path_idr, hash);
+ if (path == NULL) {
+ pr_err("Path is not found for %s\n", dev_name(&csdev->dev));
+ goto out;
+ }
+ idr_remove(&path_idr, hash);
+ break;
+ default:
+ /* We can't be here */
+ break;
+ }
+
+ coresight_disable_path(path);
+ coresight_release_path(path);
+
+out:
+ mutex_unlock(&coresight_mutex);
+}
+EXPORT_SYMBOL_GPL(coresight_disable_sysfs);
+
+static ssize_t enable_sink_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct coresight_device *csdev = to_coresight_device(dev);
+
+ return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->sysfs_sink_activated);
+}
+
+static ssize_t enable_sink_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ int ret;
+ unsigned long val;
+ struct coresight_device *csdev = to_coresight_device(dev);
+
+ ret = kstrtoul(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ csdev->sysfs_sink_activated = !!val;
+
+ return size;
+
+}
+static DEVICE_ATTR_RW(enable_sink);
+
+static ssize_t enable_source_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct coresight_device *csdev = to_coresight_device(dev);
+
+ guard(mutex)(&coresight_mutex);
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
+ local_read(&csdev->mode) == CS_MODE_SYSFS);
+}
+
+static ssize_t enable_source_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ int ret = 0;
+ unsigned long val;
+ struct coresight_device *csdev = to_coresight_device(dev);
+
+ ret = kstrtoul(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val) {
+ ret = coresight_enable_sysfs(csdev);
+ if (ret)
+ return ret;
+ } else {
+ coresight_disable_sysfs(csdev);
+ }
+
+ return size;
+}
+static DEVICE_ATTR_RW(enable_source);
+
+static struct attribute *coresight_sink_attrs[] = {
+ &dev_attr_enable_sink.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(coresight_sink);
+
+static struct attribute *coresight_source_attrs[] = {
+ &dev_attr_enable_source.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(coresight_source);
+
+struct device_type coresight_dev_type[] = {
+ {
+ .name = "sink",
+ .groups = coresight_sink_groups,
+ },
+ {
+ .name = "link",
+ },
+ {
+ .name = "linksink",
+ .groups = coresight_sink_groups,
+ },
+ {
+ .name = "source",
+ .groups = coresight_source_groups,
+ },
+ {
+ .name = "helper",
+ }
+};
+/* Ensure the enum matches the names and groups */
+static_assert(ARRAY_SIZE(coresight_dev_type) == CORESIGHT_DEV_TYPE_MAX);
+
/*
* Connections group - links attribute.
* Count of created links between coresight components in the group.
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 46e6667f72ce..e8dd0df98881 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -578,8 +578,8 @@ static inline bool coresight_is_percpu_sink(struct coresight_device *csdev)
extern struct coresight_device *
coresight_register(struct coresight_desc *desc);
extern void coresight_unregister(struct coresight_device *csdev);
-extern int coresight_enable(struct coresight_device *csdev);
-extern void coresight_disable(struct coresight_device *csdev);
+extern int coresight_enable_sysfs(struct coresight_device *csdev);
+extern void coresight_disable_sysfs(struct coresight_device *csdev);
extern int coresight_timeout(struct csdev_access *csa, u32 offset,
int position, int value);
@@ -609,8 +609,8 @@ static inline struct coresight_device *
coresight_register(struct coresight_desc *desc) { return NULL; }
static inline void coresight_unregister(struct coresight_device *csdev) {}
static inline int
-coresight_enable(struct coresight_device *csdev) { return -ENOSYS; }
-static inline void coresight_disable(struct coresight_device *csdev) {}
+coresight_enable_sysfs(struct coresight_device *csdev) { return -ENOSYS; }
+static inline void coresight_disable_sysfs(struct coresight_device *csdev) {}
static inline int coresight_timeout(struct csdev_access *csa, u32 offset,
int position, int value)
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 6/8] coresight: Move all sysfs code to sysfs file
2023-12-12 15:54 ` [PATCH 6/8] coresight: Move all sysfs code to sysfs file James Clark
@ 2024-01-09 10:22 ` Suzuki K Poulose
0 siblings, 0 replies; 22+ messages in thread
From: Suzuki K Poulose @ 2024-01-09 10:22 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
On 12/12/2023 15:54, James Clark wrote:
> At the moment the core file contains both sysfs functionality and
> core functionality, while the Perf mode is in a separate file in
> coresight-etm-perf.c
>
> Many of the functions have ambiguous names like
> coresight_enable_source() which actually only work in relation to the
> sysfs mode. To avoid further confusion, move everything that isn't core
> functionality into the sysfs file and append _sysfs to the ambiguous
> functions.
>
> Signed-off-by: James Clark <james.clark@arm.com>
The changes look good to me. One minor comment below.
...
> +struct device_type coresight_dev_type[] = {
> + {
> + .name = "sink",
> + .groups = coresight_sink_groups,
> + },
> + {
> + .name = "link",
> + },
> + {
> + .name = "linksink",
> + .groups = coresight_sink_groups,
> + },
> + {
> + .name = "source",
> + .groups = coresight_source_groups,
> + },
> + {
> + .name = "helper",
> + }
> +};
> +/* Ensure the enum matches the names and groups */
> +static_assert(ARRAY_SIZE(coresight_dev_type) == CORESIGHT_DEV_TYPE_MAX);
As a general cleanup, while you are at it, could we please replace this
with explicit member initialisers as a follow up patch ?
i.e.,
struct device_typ coresight_dev_type[CORESIGHT_DEV_TYPE_MAX] = {
[CORESIGHT_DEV_TYPE_SINK] = {
.name = "sink",
.groups = coresight_sink_groups,
},
[CORESIGHT_DEV_TYPE_LINK] =
...
}
Thanks
Suzuki
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 7/8] coresight: Remove atomic type from refcnt
2023-12-12 15:53 [PATCH 0/8] coresight: Separate sysfs and Perf usage and some other cleanups James Clark
` (5 preceding siblings ...)
2023-12-12 15:54 ` [PATCH 6/8] coresight: Move all sysfs code to sysfs file James Clark
@ 2023-12-12 15:54 ` James Clark
2023-12-12 15:54 ` [PATCH 8/8] coresight: Remove unused stubs James Clark
7 siblings, 0 replies; 22+ messages in thread
From: James Clark @ 2023-12-12 15:54 UTC (permalink / raw)
To: coresight, suzuki.poulose
Cc: James Clark, Mike Leach, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, linux-arm-kernel, linux-kernel, linux-stm32
Refcnt is only ever accessed from either inside the coresight_mutex, or
the device's spinlock, making the atomic type and atomic_dec_return()
calls confusing and unnecessary. The only point of synchronisation
outside of these two types of locks is already done with a compare and
swap on 'mode', which a comment has been added for.
There was one instance of refcnt being used outside of a lock in TPIU,
but that can easily be fixed by making it the same as all the other
devices and adding a spinlock. Potentially in the future all the
refcounting and locking can be moved up into the core code, and all the
mostly duplicate code from the individual devices can be removed.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-etb10.c | 11 +++++-----
drivers/hwtracing/coresight/coresight-sysfs.c | 7 ++++---
.../hwtracing/coresight/coresight-tmc-etf.c | 20 ++++++++++---------
.../hwtracing/coresight/coresight-tmc-etr.c | 13 ++++++------
drivers/hwtracing/coresight/coresight-tpda.c | 7 ++++---
drivers/hwtracing/coresight/coresight-tpiu.c | 14 +++++++++++--
drivers/hwtracing/coresight/ultrasoc-smb.c | 9 +++++----
include/linux/coresight.h | 13 +++++++++---
8 files changed, 59 insertions(+), 35 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 08e5bba862db..5f2bb95955b7 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -161,7 +161,7 @@ static int etb_enable_sysfs(struct coresight_device *csdev)
local_set(&csdev->mode, CS_MODE_SYSFS);
}
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return ret;
@@ -197,7 +197,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
* use for this session.
*/
if (drvdata->pid == pid) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto out;
}
@@ -215,7 +215,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
/* Associate with monitored process. */
drvdata->pid = pid;
local_set(&drvdata->csdev->mode, CS_MODE_PERF);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
}
out:
@@ -354,7 +354,8 @@ static int etb_disable(struct coresight_device *csdev)
spin_lock_irqsave(&drvdata->spinlock, flags);
- if (atomic_dec_return(&csdev->refcnt)) {
+ csdev->refcnt--;
+ if (csdev->refcnt) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return -EBUSY;
}
@@ -445,7 +446,7 @@ static unsigned long etb_update_buffer(struct coresight_device *csdev,
spin_lock_irqsave(&drvdata->spinlock, flags);
/* Don't do anything if another tracer is using this sink */
- if (atomic_read(&csdev->refcnt) != 1)
+ if (csdev->refcnt != 1)
goto out;
__etb_disable_hw(drvdata);
diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
index 92cdf8139f23..5992f2c2200a 100644
--- a/drivers/hwtracing/coresight/coresight-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-sysfs.c
@@ -68,7 +68,7 @@ static int coresight_enable_source_sysfs(struct coresight_device *csdev,
return ret;
}
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
return 0;
}
@@ -90,7 +90,8 @@ static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
if (local_read(&csdev->mode) != CS_MODE_SYSFS)
return false;
- if (atomic_dec_return(&csdev->refcnt) == 0) {
+ csdev->refcnt--;
+ if (csdev->refcnt == 0) {
coresight_disable_source(csdev, data);
return true;
}
@@ -190,7 +191,7 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
* source is already enabled.
*/
if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE)
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto out;
}
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 2a7e516052a2..f3281c958a57 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -206,7 +206,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
* touched.
*/
if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto out;
}
@@ -229,7 +229,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
ret = tmc_etb_enable_hw(drvdata);
if (!ret) {
local_set(&csdev->mode, CS_MODE_SYSFS);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
} else {
/* Free up the buffer if we failed to enable */
used = false;
@@ -284,7 +284,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
* use for this session.
*/
if (drvdata->pid == pid) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
break;
}
@@ -293,7 +293,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
/* Associate with monitored process. */
drvdata->pid = pid;
local_set(&csdev->mode, CS_MODE_PERF);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
}
} while (0);
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -338,7 +338,8 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
return -EBUSY;
}
- if (atomic_dec_return(&csdev->refcnt)) {
+ csdev->refcnt--;
+ if (csdev->refcnt) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return -EBUSY;
}
@@ -371,7 +372,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
return -EBUSY;
}
- if (atomic_read(&csdev->refcnt) == 0) {
+ if (csdev->refcnt == 0) {
ret = tmc_etf_enable_hw(drvdata);
if (!ret) {
local_set(&csdev->mode, CS_MODE_SYSFS);
@@ -379,7 +380,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
}
}
if (!ret)
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
spin_unlock_irqrestore(&drvdata->spinlock, flags);
if (first_enable)
@@ -401,7 +402,8 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
return;
}
- if (atomic_dec_return(&csdev->refcnt) == 0) {
+ csdev->refcnt--;
+ if (csdev->refcnt == 0) {
tmc_etf_disable_hw(drvdata);
local_set(&csdev->mode, CS_MODE_DISABLED);
last_disable = true;
@@ -489,7 +491,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
spin_lock_irqsave(&drvdata->spinlock, flags);
/* Don't do anything if another tracer is using this sink */
- if (atomic_read(&csdev->refcnt) != 1)
+ if (csdev->refcnt != 1)
goto out;
CS_UNLOCK(drvdata->base);
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 3dc989d4fcab..88a0fc375b4d 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1231,14 +1231,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
* touched, even if the buffer size has changed.
*/
if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto out;
}
ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
if (!ret) {
local_set(&csdev->mode, CS_MODE_SYSFS);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
}
out:
@@ -1564,7 +1564,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
spin_lock_irqsave(&drvdata->spinlock, flags);
/* Don't do anything if another tracer is using this sink */
- if (atomic_read(&csdev->refcnt) != 1) {
+ if (csdev->refcnt != 1) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
goto out;
}
@@ -1676,7 +1676,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
* use for this session.
*/
if (drvdata->pid == pid) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto unlock_out;
}
@@ -1686,7 +1686,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
drvdata->pid = pid;
local_set(&csdev->mode, CS_MODE_PERF);
drvdata->perf_buf = etr_perf->etr_buf;
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
}
unlock_out:
@@ -1719,7 +1719,8 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
return -EBUSY;
}
- if (atomic_dec_return(&csdev->refcnt)) {
+ csdev->refcnt--;
+ if (csdev->refcnt) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return -EBUSY;
}
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 65c70995ab00..135a89654c4c 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -152,7 +152,8 @@ static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
* Only do pre-port enable for first port that calls enable when the
* device's main refcount is still 0
*/
- if (!atomic_read(&drvdata->csdev->refcnt))
+ lockdep_assert_held(&drvdata->spinlock);
+ if (!drvdata->csdev->refcnt)
tpda_enable_pre_port(drvdata);
ret = tpda_enable_port(drvdata, port);
@@ -173,7 +174,7 @@ static int tpda_enable(struct coresight_device *csdev,
ret = __tpda_enable(drvdata, in->dest_port);
if (!ret) {
atomic_inc(&in->dest_refcnt);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
}
}
@@ -204,7 +205,7 @@ static void tpda_disable(struct coresight_device *csdev,
spin_lock(&drvdata->spinlock);
if (atomic_dec_return(&in->dest_refcnt) == 0) {
__tpda_disable(drvdata, in->dest_port);
- atomic_dec(&csdev->refcnt);
+ csdev->refcnt--;
}
spin_unlock(&drvdata->spinlock);
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index 59eac93fd6bb..c23a6b9b41fe 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -58,6 +58,7 @@ struct tpiu_drvdata {
void __iomem *base;
struct clk *atclk;
struct coresight_device *csdev;
+ spinlock_t spinlock;
};
static void tpiu_enable_hw(struct csdev_access *csa)
@@ -72,8 +73,11 @@ static void tpiu_enable_hw(struct csdev_access *csa)
static int tpiu_enable(struct coresight_device *csdev, enum cs_mode mode,
void *__unused)
{
+ struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+ guard(spinlock)(&drvdata->spinlock);
tpiu_enable_hw(&csdev->access);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
dev_dbg(&csdev->dev, "TPIU enabled\n");
return 0;
}
@@ -96,7 +100,11 @@ static void tpiu_disable_hw(struct csdev_access *csa)
static int tpiu_disable(struct coresight_device *csdev)
{
- if (atomic_dec_return(&csdev->refcnt))
+ struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+ guard(spinlock)(&drvdata->spinlock);
+ csdev->refcnt--;
+ if (csdev->refcnt)
return -EBUSY;
tpiu_disable_hw(&csdev->access);
@@ -132,6 +140,8 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)
if (!drvdata)
return -ENOMEM;
+ spin_lock_init(&drvdata->spinlock);
+
drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */
if (!IS_ERR(drvdata->atclk)) {
ret = clk_prepare_enable(drvdata->atclk);
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 1a8c64645b92..1146584a1745 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -103,7 +103,7 @@ static int smb_open(struct inode *inode, struct file *file)
if (drvdata->reading)
return -EBUSY;
- if (atomic_read(&drvdata->csdev->refcnt))
+ if (drvdata->csdev->refcnt)
return -EBUSY;
smb_update_data_size(drvdata);
@@ -271,7 +271,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
if (ret)
return ret;
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
return ret;
@@ -286,7 +286,8 @@ static int smb_disable(struct coresight_device *csdev)
if (drvdata->reading)
return -EBUSY;
- if (atomic_dec_return(&csdev->refcnt))
+ csdev->refcnt--;
+ if (csdev->refcnt)
return -EBUSY;
/* Complain if we (somehow) got out of sync */
@@ -381,7 +382,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
guard(spinlock)(&drvdata->spinlock);
/* Don't do anything if another tracer is using this sink. */
- if (atomic_read(&csdev->refcnt) != 1)
+ if (csdev->refcnt != 1)
return 0;
smb_disable_hw(drvdata);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index e8dd0df98881..4400d554a16b 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -230,8 +230,15 @@ struct coresight_sysfs_link {
* actually an 'enum cs_mode', but is stored in an atomic type.
* This is always accessed through local_read() and local_set(),
* but wherever it's done from within the Coresight device's lock,
- * a non-atomic read would also work.
- * @refcnt: keep track of what is in use.
+ * a non-atomic read would also work. This is the main point of
+ * synchronisation between code happening inside the sysfs mode's
+ * coresight_mutex and outside when running in Perf mode. A compare
+ * and exchange swap is done to atomically claim one mode or the
+ * other.
+ * @refcnt: keep track of what is in use. Only access this outside of the
+ * device's spinlock when the coresight_mutex held and mode ==
+ * CS_MODE_SYSFS. Otherwise it must be accessed from inside the
+ * spinlock.
* @orphan: true if the component has connections that haven't been linked.
* @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
* by writing a 1 to the 'enable_sink' file. A sink can be
@@ -257,7 +264,7 @@ struct coresight_device {
struct csdev_access access;
struct device dev;
local_t mode;
- atomic_t refcnt;
+ int refcnt;
bool orphan;
/* sink specific fields */
bool sysfs_sink_activated;
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 8/8] coresight: Remove unused stubs
2023-12-12 15:53 [PATCH 0/8] coresight: Separate sysfs and Perf usage and some other cleanups James Clark
` (6 preceding siblings ...)
2023-12-12 15:54 ` [PATCH 7/8] coresight: Remove atomic type from refcnt James Clark
@ 2023-12-12 15:54 ` James Clark
2024-01-09 10:38 ` Suzuki K Poulose
7 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2023-12-12 15:54 UTC (permalink / raw)
To: coresight, suzuki.poulose
Cc: James Clark, Mike Leach, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, linux-arm-kernel, linux-kernel, linux-stm32
These are a bit annoying to keep up to date when the function signatures
change. But if CONFIG_CORESIGHT isn't enabled, then they're not used
anyway so just delete them.
Signed-off-by: James Clark <james.clark@arm.com>
---
include/linux/coresight.h | 79 ---------------------------------------
1 file changed, 79 deletions(-)
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 4400d554a16b..c5be46d7f85c 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -391,8 +391,6 @@ struct coresight_ops {
const struct coresight_ops_helper *helper_ops;
};
-#if IS_ENABLED(CONFIG_CORESIGHT)
-
static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
u32 offset)
{
@@ -611,83 +609,6 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
u64 val, u32 offset);
void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
-#else
-static inline struct coresight_device *
-coresight_register(struct coresight_desc *desc) { return NULL; }
-static inline void coresight_unregister(struct coresight_device *csdev) {}
-static inline int
-coresight_enable_sysfs(struct coresight_device *csdev) { return -ENOSYS; }
-static inline void coresight_disable_sysfs(struct coresight_device *csdev) {}
-
-static inline int coresight_timeout(struct csdev_access *csa, u32 offset,
- int position, int value)
-{
- return 1;
-}
-
-static inline int coresight_claim_device_unlocked(struct coresight_device *csdev)
-{
- return -EINVAL;
-}
-
-static inline int coresight_claim_device(struct coresight_device *csdev)
-{
- return -EINVAL;
-}
-
-static inline void coresight_disclaim_device(struct coresight_device *csdev) {}
-static inline void coresight_disclaim_device_unlocked(struct coresight_device *csdev) {}
-
-static inline bool coresight_loses_context_with_cpu(struct device *dev)
-{
- return false;
-}
-
-static inline u32 coresight_relaxed_read32(struct coresight_device *csdev, u32 offset)
-{
- WARN_ON_ONCE(1);
- return 0;
-}
-
-static inline u32 coresight_read32(struct coresight_device *csdev, u32 offset)
-{
- WARN_ON_ONCE(1);
- return 0;
-}
-
-static inline void coresight_write32(struct coresight_device *csdev, u32 val, u32 offset)
-{
-}
-
-static inline void coresight_relaxed_write32(struct coresight_device *csdev,
- u32 val, u32 offset)
-{
-}
-
-static inline u64 coresight_relaxed_read64(struct coresight_device *csdev,
- u32 offset)
-{
- WARN_ON_ONCE(1);
- return 0;
-}
-
-static inline u64 coresight_read64(struct coresight_device *csdev, u32 offset)
-{
- WARN_ON_ONCE(1);
- return 0;
-}
-
-static inline void coresight_relaxed_write64(struct coresight_device *csdev,
- u64 val, u32 offset)
-{
-}
-
-static inline void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset)
-{
-}
-
-#endif /* IS_ENABLED(CONFIG_CORESIGHT) */
-
extern int coresight_get_cpu(struct device *dev);
struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 8/8] coresight: Remove unused stubs
2023-12-12 15:54 ` [PATCH 8/8] coresight: Remove unused stubs James Clark
@ 2024-01-09 10:38 ` Suzuki K Poulose
2024-01-09 16:48 ` James Clark
0 siblings, 1 reply; 22+ messages in thread
From: Suzuki K Poulose @ 2024-01-09 10:38 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
Hi James
On 12/12/2023 15:54, James Clark wrote:
> These are a bit annoying to keep up to date when the function signatures
> change. But if CONFIG_CORESIGHT isn't enabled, then they're not used
> anyway so just delete them.
>
Have you tried building an arm32 kernel with this change in ? Looks like
arch/arm/kernel/hw_breakpoint.c includes linux/coresight.h and a build
with CONFIG_CORSIGHT=n might break the build ? So is
drivers/accel/habanalabs/common/habanalabs.h. Now, I am not sure if they
really need it (even if they do, we may be able to remove the dependency
on the header file.
Suzuki
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> include/linux/coresight.h | 79 ---------------------------------------
> 1 file changed, 79 deletions(-)
>
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 4400d554a16b..c5be46d7f85c 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -391,8 +391,6 @@ struct coresight_ops {
> const struct coresight_ops_helper *helper_ops;
> };
>
> -#if IS_ENABLED(CONFIG_CORESIGHT)
> -
> static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
> u32 offset)
> {
> @@ -611,83 +609,6 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
> u64 val, u32 offset);
> void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
>
> -#else
> -static inline struct coresight_device *
> -coresight_register(struct coresight_desc *desc) { return NULL; }
> -static inline void coresight_unregister(struct coresight_device *csdev) {}
> -static inline int
> -coresight_enable_sysfs(struct coresight_device *csdev) { return -ENOSYS; }
> -static inline void coresight_disable_sysfs(struct coresight_device *csdev) {}
> -
> -static inline int coresight_timeout(struct csdev_access *csa, u32 offset,
> - int position, int value)
> -{
> - return 1;
> -}
> -
> -static inline int coresight_claim_device_unlocked(struct coresight_device *csdev)
> -{
> - return -EINVAL;
> -}
> -
> -static inline int coresight_claim_device(struct coresight_device *csdev)
> -{
> - return -EINVAL;
> -}
> -
> -static inline void coresight_disclaim_device(struct coresight_device *csdev) {}
> -static inline void coresight_disclaim_device_unlocked(struct coresight_device *csdev) {}
> -
> -static inline bool coresight_loses_context_with_cpu(struct device *dev)
> -{
> - return false;
> -}
> -
> -static inline u32 coresight_relaxed_read32(struct coresight_device *csdev, u32 offset)
> -{
> - WARN_ON_ONCE(1);
> - return 0;
> -}
> -
> -static inline u32 coresight_read32(struct coresight_device *csdev, u32 offset)
> -{
> - WARN_ON_ONCE(1);
> - return 0;
> -}
> -
> -static inline void coresight_write32(struct coresight_device *csdev, u32 val, u32 offset)
> -{
> -}
> -
> -static inline void coresight_relaxed_write32(struct coresight_device *csdev,
> - u32 val, u32 offset)
> -{
> -}
> -
> -static inline u64 coresight_relaxed_read64(struct coresight_device *csdev,
> - u32 offset)
> -{
> - WARN_ON_ONCE(1);
> - return 0;
> -}
> -
> -static inline u64 coresight_read64(struct coresight_device *csdev, u32 offset)
> -{
> - WARN_ON_ONCE(1);
> - return 0;
> -}
> -
> -static inline void coresight_relaxed_write64(struct coresight_device *csdev,
> - u64 val, u32 offset)
> -{
> -}
> -
> -static inline void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset)
> -{
> -}
> -
> -#endif /* IS_ENABLED(CONFIG_CORESIGHT) */
> -
> extern int coresight_get_cpu(struct device *dev);
>
> struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 8/8] coresight: Remove unused stubs
2024-01-09 10:38 ` Suzuki K Poulose
@ 2024-01-09 16:48 ` James Clark
2024-01-10 14:00 ` Suzuki K Poulose
0 siblings, 1 reply; 22+ messages in thread
From: James Clark @ 2024-01-09 16:48 UTC (permalink / raw)
To: Suzuki K Poulose, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
On 09/01/2024 10:38, Suzuki K Poulose wrote:
> Hi James
>
> On 12/12/2023 15:54, James Clark wrote:
>> These are a bit annoying to keep up to date when the function signatures
>> change. But if CONFIG_CORESIGHT isn't enabled, then they're not used
>> anyway so just delete them.
>>
>
> Have you tried building an arm32 kernel with this change in ? Looks like
> arch/arm/kernel/hw_breakpoint.c includes linux/coresight.h and a build
> with CONFIG_CORSIGHT=n might break the build ? So is
arm32 and CONFIG_CORESIGHT=n works because hw_breakpoint.c doesn't use
any of those symbols, only #defines that were outside the #if
IS_ENABLED(CONFIG_CORESIGHT), specifically CORESIGHT_UNLOCK.
> drivers/accel/habanalabs/common/habanalabs.h. Now, I am not sure if they
habanalabs is interesting, it depends on X86_64, but CONFIG_CORESIGHT
depends on ARM || ARM64, so I think we can assume it's also only looking
for #defines and inlines, and not actual code.
Either way I can't find any build config that actually ever built this,
meaning it's always been dead code. I would have expected some build
robot to have flagged an error by now as I've seen that on other
coresight patches.
> really need it (even if they do, we may be able to remove the dependency
> on the header file.
>
They do really need it, also for the CORESIGHT_UNLOCK definition, but
not any functions.
> Suzuki
>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> include/linux/coresight.h | 79 ---------------------------------------
>> 1 file changed, 79 deletions(-)
>>
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index 4400d554a16b..c5be46d7f85c 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -391,8 +391,6 @@ struct coresight_ops {
>> const struct coresight_ops_helper *helper_ops;
>> };
>> -#if IS_ENABLED(CONFIG_CORESIGHT)
>> -
>> static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
>> u32 offset)
>> {
>> @@ -611,83 +609,6 @@ void coresight_relaxed_write64(struct
>> coresight_device *csdev,
>> u64 val, u32 offset);
>> void coresight_write64(struct coresight_device *csdev, u64 val, u32
>> offset);
>> -#else
>> -static inline struct coresight_device *
>> -coresight_register(struct coresight_desc *desc) { return NULL; }
>> -static inline void coresight_unregister(struct coresight_device
>> *csdev) {}
>> -static inline int
>> -coresight_enable_sysfs(struct coresight_device *csdev) { return
>> -ENOSYS; }
>> -static inline void coresight_disable_sysfs(struct coresight_device
>> *csdev) {}
>> -
>> -static inline int coresight_timeout(struct csdev_access *csa, u32
>> offset,
>> - int position, int value)
>> -{
>> - return 1;
>> -}
>> -
>> -static inline int coresight_claim_device_unlocked(struct
>> coresight_device *csdev)
>> -{
>> - return -EINVAL;
>> -}
>> -
>> -static inline int coresight_claim_device(struct coresight_device *csdev)
>> -{
>> - return -EINVAL;
>> -}
>> -
>> -static inline void coresight_disclaim_device(struct coresight_device
>> *csdev) {}
>> -static inline void coresight_disclaim_device_unlocked(struct
>> coresight_device *csdev) {}
>> -
>> -static inline bool coresight_loses_context_with_cpu(struct device *dev)
>> -{
>> - return false;
>> -}
>> -
>> -static inline u32 coresight_relaxed_read32(struct coresight_device
>> *csdev, u32 offset)
>> -{
>> - WARN_ON_ONCE(1);
>> - return 0;
>> -}
>> -
>> -static inline u32 coresight_read32(struct coresight_device *csdev,
>> u32 offset)
>> -{
>> - WARN_ON_ONCE(1);
>> - return 0;
>> -}
>> -
>> -static inline void coresight_write32(struct coresight_device *csdev,
>> u32 val, u32 offset)
>> -{
>> -}
>> -
>> -static inline void coresight_relaxed_write32(struct coresight_device
>> *csdev,
>> - u32 val, u32 offset)
>> -{
>> -}
>> -
>> -static inline u64 coresight_relaxed_read64(struct coresight_device
>> *csdev,
>> - u32 offset)
>> -{
>> - WARN_ON_ONCE(1);
>> - return 0;
>> -}
>> -
>> -static inline u64 coresight_read64(struct coresight_device *csdev,
>> u32 offset)
>> -{
>> - WARN_ON_ONCE(1);
>> - return 0;
>> -}
>> -
>> -static inline void coresight_relaxed_write64(struct coresight_device
>> *csdev,
>> - u64 val, u32 offset)
>> -{
>> -}
>> -
>> -static inline void coresight_write64(struct coresight_device *csdev,
>> u64 val, u32 offset)
>> -{
>> -}
>> -
>> -#endif /* IS_ENABLED(CONFIG_CORESIGHT) */
>> -
>> extern int coresight_get_cpu(struct device *dev);
>> struct coresight_platform_data *coresight_get_platform_data(struct
>> device *dev);
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 8/8] coresight: Remove unused stubs
2024-01-09 16:48 ` James Clark
@ 2024-01-10 14:00 ` Suzuki K Poulose
0 siblings, 0 replies; 22+ messages in thread
From: Suzuki K Poulose @ 2024-01-10 14:00 UTC (permalink / raw)
To: James Clark, coresight
Cc: Mike Leach, Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
linux-arm-kernel, linux-kernel, linux-stm32
On 09/01/2024 16:48, James Clark wrote:
>
>
> On 09/01/2024 10:38, Suzuki K Poulose wrote:
>> Hi James
>>
>> On 12/12/2023 15:54, James Clark wrote:
>>> These are a bit annoying to keep up to date when the function signatures
>>> change. But if CONFIG_CORESIGHT isn't enabled, then they're not used
>>> anyway so just delete them.
>>>
>>
>> Have you tried building an arm32 kernel with this change in ? Looks like
>> arch/arm/kernel/hw_breakpoint.c includes linux/coresight.h and a build
>> with CONFIG_CORSIGHT=n might break the build ? So is
>
> arm32 and CONFIG_CORESIGHT=n works because hw_breakpoint.c doesn't use
> any of those symbols, only #defines that were outside the #if
> IS_ENABLED(CONFIG_CORESIGHT), specifically CORESIGHT_UNLOCK.
>
>> drivers/accel/habanalabs/common/habanalabs.h. Now, I am not sure if they
>
> habanalabs is interesting, it depends on X86_64, but CONFIG_CORESIGHT
> depends on ARM || ARM64, so I think we can assume it's also only looking
> for #defines and inlines, and not actual code.
>
> Either way I can't find any build config that actually ever built this,
> meaning it's always been dead code. I would have expected some build
> robot to have flagged an error by now as I've seen that on other
> coresight patches.
>
>> really need it (even if they do, we may be able to remove the dependency
>> on the header file.
>>
>
> They do really need it, also for the CORESIGHT_UNLOCK definition, but
> not any functions.
Thanks for checking this.
Suzuki
^ permalink raw reply [flat|nested] 22+ messages in thread