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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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.
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.