2023-01-10 11:39:41

by James Clark

[permalink] [raw]
Subject: [PATCH v3 0/3] coresight: cti: Add PM runtime call in enable_store

Changes since v2:

* Reword first commit message and add fixes tag
* Pickup Jinlong's tested-by tags

----

This should be a slight improvement on Jinlong's previous version.
Now it's not possible to trigger the error message from
pm_runtime_put() by calling disable twice.

It's also similar to the original pre-breaking change version where
pm_runtime_put() was only called if the device was actually disabled,
but with one difference: Previously pm_runtime_put() was only called
once for the last disable call, but because of the reference counting
in pm_runtime, it should have been called once for each enable call.
This meant that the clock would have never been disabled if there were
ever multiple enable calls. This is now fixed.

The third commit is a refactor and doesn't need to be backported. I
removed one of the atomic types because it didn't appear to be
required. Maybe it was added for a reason which I'm not aware of, if
so it should be pretty easy to drop that change.

James Clark (2):
coresight: cti: Prevent negative values of enable count
coresight: cti: Remove atomic type from enable_req_count

Mao Jinlong (1):
coresight: cti: Add PM runtime call in enable_store

.../hwtracing/coresight/coresight-cti-core.c | 23 ++++++++++++-------
.../hwtracing/coresight/coresight-cti-sysfs.c | 15 +++++++++---
drivers/hwtracing/coresight/coresight-cti.h | 2 +-
3 files changed, 28 insertions(+), 12 deletions(-)


base-commit: 88603b6dc419445847923fcb7fe5080067a30f98
--
2.25.1


2023-01-10 11:45:02

by James Clark

[permalink] [raw]
Subject: [PATCH v3 2/3] coresight: cti: Add PM runtime call in enable_store

From: Mao Jinlong <[email protected]>

In commit 6746eae4bbad ("coresight: cti: Fix hang in cti_disable_hw()")
PM runtime calls are removed from cti_enable_hw/cti_disable_hw. When
enabling CTI by writing enable sysfs node, clock for accessing CTI
register won't be enabled. Device will crash due to register access
issue. Add PM runtime call in enable_store to fix this issue.

Fixes: 6746eae4bbad ("coresight: cti: Fix hang in cti_disable_hw()")
Signed-off-by: Mao Jinlong <[email protected]>
[Change to only call pm_runtime_put if a disable happened]
Tested-by: Jinlong Mao <[email protected]>
Signed-off-by: James Clark <[email protected]>
---
drivers/hwtracing/coresight/coresight-cti-sysfs.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
index 6d59c815ecf5..71e7a8266bb3 100644
--- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
@@ -108,10 +108,19 @@ static ssize_t enable_store(struct device *dev,
if (ret)
return ret;

- if (val)
+ if (val) {
+ ret = pm_runtime_resume_and_get(dev->parent);
+ if (ret)
+ return ret;
ret = cti_enable(drvdata->csdev);
- else
+ if (ret)
+ pm_runtime_put(dev->parent);
+ } else {
ret = cti_disable(drvdata->csdev);
+ if (!ret)
+ pm_runtime_put(dev->parent);
+ }
+
if (ret)
return ret;
return size;
--
2.25.1

2023-01-16 11:13:46

by Suzuki K Poulose

[permalink] [raw]
Subject: Re: [PATCH v3 0/3] coresight: cti: Add PM runtime call in enable_store

On 10/01/2023 11:07, James Clark wrote:
> Changes since v2:
>
> * Reword first commit message and add fixes tag
> * Pickup Jinlong's tested-by tags
>
> ----
>
> This should be a slight improvement on Jinlong's previous version.
> Now it's not possible to trigger the error message from
> pm_runtime_put() by calling disable twice.
>
> It's also similar to the original pre-breaking change version where
> pm_runtime_put() was only called if the device was actually disabled,
> but with one difference: Previously pm_runtime_put() was only called
> once for the last disable call, but because of the reference counting
> in pm_runtime, it should have been called once for each enable call.
> This meant that the clock would have never been disabled if there were
> ever multiple enable calls. This is now fixed.
>
> The third commit is a refactor and doesn't need to be backported. I
> removed one of the atomic types because it didn't appear to be
> required. Maybe it was added for a reason which I'm not aware of, if
> so it should be pretty easy to drop that change.
>
> James Clark (2):
> coresight: cti: Prevent negative values of enable count
> coresight: cti: Remove atomic type from enable_req_count
>
> Mao Jinlong (1):
> coresight: cti: Add PM runtime call in enable_store
>
> .../hwtracing/coresight/coresight-cti-core.c | 23 ++++++++++++-------
> .../hwtracing/coresight/coresight-cti-sysfs.c | 15 +++++++++---
> drivers/hwtracing/coresight/coresight-cti.h | 2 +-
> 3 files changed, 28 insertions(+), 12 deletions(-)
>
>
> base-commit: 88603b6dc419445847923fcb7fe5080067a30f98

Queued to coresight next


https://git.kernel.org/coresight/c/479043b77833

Suzuki