* [PATCH v3 0/3] coresight: cti: Add PM runtime call in enable_store
@ 2023-01-10 11:07 ` James Clark
0 siblings, 0 replies; 14+ messages in thread
From: James Clark @ 2023-01-10 11:07 UTC (permalink / raw)
To: coresight, quic_jinlmao, suzuki.poulose, mike.leach
Cc: James Clark, Mathieu Poirier, Leo Yan, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 0/3] coresight: cti: Add PM runtime call in enable_store
@ 2023-01-10 11:07 ` James Clark
0 siblings, 0 replies; 14+ messages in thread
From: James Clark @ 2023-01-10 11:07 UTC (permalink / raw)
To: coresight, quic_jinlmao, suzuki.poulose, mike.leach
Cc: James Clark, Mathieu Poirier, Leo Yan, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/3] coresight: cti: Prevent negative values of enable count
2023-01-10 11:07 ` James Clark
@ 2023-01-10 11:07 ` James Clark
-1 siblings, 0 replies; 14+ messages in thread
From: James Clark @ 2023-01-10 11:07 UTC (permalink / raw)
To: coresight, quic_jinlmao, suzuki.poulose, mike.leach
Cc: James Clark, Mathieu Poirier, Leo Yan, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel
Writing 0 to the enable control repeatedly results in a negative value
for enable_req_count. After this, writing 1 to the enable control
appears to not work until the count returns to positive.
Change it so that it's impossible for enable_req_count to be < 0.
Return an error to indicate that the disable request was invalid.
Fixes: 835d722ba10a ("coresight: cti: Initial CoreSight CTI Driver")
Tested-by: Jinlong Mao <quic_jinlmao@quicinc.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-cti-core.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
index d2cf4f4848e1..838872f2484d 100644
--- a/drivers/hwtracing/coresight/coresight-cti-core.c
+++ b/drivers/hwtracing/coresight/coresight-cti-core.c
@@ -151,9 +151,16 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
{
struct cti_config *config = &drvdata->config;
struct coresight_device *csdev = drvdata->csdev;
+ int ret = 0;
spin_lock(&drvdata->spinlock);
+ /* don't allow negative refcounts, return an error */
+ if (!atomic_read(&drvdata->config.enable_req_count)) {
+ ret = -EINVAL;
+ goto cti_not_disabled;
+ }
+
/* check refcount - disable on 0 */
if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
goto cti_not_disabled;
@@ -171,12 +178,12 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
coresight_disclaim_device_unlocked(csdev);
CS_LOCK(drvdata->base);
spin_unlock(&drvdata->spinlock);
- return 0;
+ return ret;
/* not disabled this call */
cti_not_disabled:
spin_unlock(&drvdata->spinlock);
- return 0;
+ return ret;
}
void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value)
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 1/3] coresight: cti: Prevent negative values of enable count
@ 2023-01-10 11:07 ` James Clark
0 siblings, 0 replies; 14+ messages in thread
From: James Clark @ 2023-01-10 11:07 UTC (permalink / raw)
To: coresight, quic_jinlmao, suzuki.poulose, mike.leach
Cc: James Clark, Mathieu Poirier, Leo Yan, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel
Writing 0 to the enable control repeatedly results in a negative value
for enable_req_count. After this, writing 1 to the enable control
appears to not work until the count returns to positive.
Change it so that it's impossible for enable_req_count to be < 0.
Return an error to indicate that the disable request was invalid.
Fixes: 835d722ba10a ("coresight: cti: Initial CoreSight CTI Driver")
Tested-by: Jinlong Mao <quic_jinlmao@quicinc.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-cti-core.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
index d2cf4f4848e1..838872f2484d 100644
--- a/drivers/hwtracing/coresight/coresight-cti-core.c
+++ b/drivers/hwtracing/coresight/coresight-cti-core.c
@@ -151,9 +151,16 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
{
struct cti_config *config = &drvdata->config;
struct coresight_device *csdev = drvdata->csdev;
+ int ret = 0;
spin_lock(&drvdata->spinlock);
+ /* don't allow negative refcounts, return an error */
+ if (!atomic_read(&drvdata->config.enable_req_count)) {
+ ret = -EINVAL;
+ goto cti_not_disabled;
+ }
+
/* check refcount - disable on 0 */
if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
goto cti_not_disabled;
@@ -171,12 +178,12 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
coresight_disclaim_device_unlocked(csdev);
CS_LOCK(drvdata->base);
spin_unlock(&drvdata->spinlock);
- return 0;
+ return ret;
/* not disabled this call */
cti_not_disabled:
spin_unlock(&drvdata->spinlock);
- return 0;
+ return ret;
}
void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value)
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/3] coresight: cti: Add PM runtime call in enable_store
2023-01-10 11:07 ` James Clark
@ 2023-01-10 11:07 ` James Clark
-1 siblings, 0 replies; 14+ messages in thread
From: James Clark @ 2023-01-10 11:07 UTC (permalink / raw)
To: coresight, quic_jinlmao, suzuki.poulose, mike.leach
Cc: James Clark, Mathieu Poirier, Leo Yan, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel
From: Mao Jinlong <quic_jinlmao@quicinc.com>
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 <quic_jinlmao@quicinc.com>
[Change to only call pm_runtime_put if a disable happened]
Tested-by: Jinlong Mao <quic_jinlmao@quicinc.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/3] coresight: cti: Add PM runtime call in enable_store
@ 2023-01-10 11:07 ` James Clark
0 siblings, 0 replies; 14+ messages in thread
From: James Clark @ 2023-01-10 11:07 UTC (permalink / raw)
To: coresight, quic_jinlmao, suzuki.poulose, mike.leach
Cc: James Clark, Mathieu Poirier, Leo Yan, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel
From: Mao Jinlong <quic_jinlmao@quicinc.com>
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 <quic_jinlmao@quicinc.com>
[Change to only call pm_runtime_put if a disable happened]
Tested-by: Jinlong Mao <quic_jinlmao@quicinc.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/3] coresight: cti: Remove atomic type from enable_req_count
2023-01-10 11:07 ` James Clark
@ 2023-01-10 11:07 ` James Clark
-1 siblings, 0 replies; 14+ messages in thread
From: James Clark @ 2023-01-10 11:07 UTC (permalink / raw)
To: coresight, quic_jinlmao, suzuki.poulose, mike.leach
Cc: James Clark, Mathieu Poirier, Leo Yan, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel
enable_req_count is only ever accessed inside the spinlock, so to avoid
confusion that there are concurrent accesses and simplify the code,
change it to an int.
One access outside of the spinlock is in enable_show() which appears to
allow partially written data to be displayed between enable_req_count,
powered and enabled so move this one inside the spin lock too.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-cti-core.c | 14 +++++++-------
drivers/hwtracing/coresight/coresight-cti-sysfs.c | 2 +-
drivers/hwtracing/coresight/coresight-cti.h | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
index 838872f2484d..277c890a1f1f 100644
--- a/drivers/hwtracing/coresight/coresight-cti-core.c
+++ b/drivers/hwtracing/coresight/coresight-cti-core.c
@@ -107,12 +107,12 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
cti_write_all_hw_regs(drvdata);
config->hw_enabled = true;
- atomic_inc(&drvdata->config.enable_req_count);
+ drvdata->config.enable_req_count++;
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return rc;
cti_state_unchanged:
- atomic_inc(&drvdata->config.enable_req_count);
+ drvdata->config.enable_req_count++;
/* cannot enable due to error */
cti_err_not_enabled:
@@ -129,7 +129,7 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata)
config->hw_powered = true;
/* no need to do anything if no enable request */
- if (!atomic_read(&drvdata->config.enable_req_count))
+ if (!drvdata->config.enable_req_count)
goto cti_hp_not_enabled;
/* try to claim the device */
@@ -156,13 +156,13 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
spin_lock(&drvdata->spinlock);
/* don't allow negative refcounts, return an error */
- if (!atomic_read(&drvdata->config.enable_req_count)) {
+ if (!drvdata->config.enable_req_count) {
ret = -EINVAL;
goto cti_not_disabled;
}
/* check refcount - disable on 0 */
- if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
+ if (--drvdata->config.enable_req_count > 0)
goto cti_not_disabled;
/* no need to do anything if disabled or cpu unpowered */
@@ -239,7 +239,7 @@ static void cti_set_default_config(struct device *dev,
/* Most regs default to 0 as zalloc'ed except...*/
config->trig_filter_enable = true;
config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0);
- atomic_set(&config->enable_req_count, 0);
+ config->enable_req_count = 0;
}
/*
@@ -696,7 +696,7 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
drvdata->config.hw_enabled = false;
/* check enable reference count to enable HW */
- if (atomic_read(&drvdata->config.enable_req_count)) {
+ if (drvdata->config.enable_req_count) {
/* check we can claim the device as we re-power */
if (coresight_claim_device(csdev))
goto cti_notify_exit;
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
index 71e7a8266bb3..e528cff9d4e2 100644
--- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
@@ -84,8 +84,8 @@ static ssize_t enable_show(struct device *dev,
bool enabled, powered;
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- enable_req = atomic_read(&drvdata->config.enable_req_count);
spin_lock(&drvdata->spinlock);
+ enable_req = drvdata->config.enable_req_count;
powered = drvdata->config.hw_powered;
enabled = drvdata->config.hw_enabled;
spin_unlock(&drvdata->spinlock);
diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
index acf7b545e6b9..8b106b13a244 100644
--- a/drivers/hwtracing/coresight/coresight-cti.h
+++ b/drivers/hwtracing/coresight/coresight-cti.h
@@ -141,7 +141,7 @@ struct cti_config {
int nr_trig_max;
/* cti enable control */
- atomic_t enable_req_count;
+ int enable_req_count;
bool hw_enabled;
bool hw_powered;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/3] coresight: cti: Remove atomic type from enable_req_count
@ 2023-01-10 11:07 ` James Clark
0 siblings, 0 replies; 14+ messages in thread
From: James Clark @ 2023-01-10 11:07 UTC (permalink / raw)
To: coresight, quic_jinlmao, suzuki.poulose, mike.leach
Cc: James Clark, Mathieu Poirier, Leo Yan, Alexander Shishkin,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel
enable_req_count is only ever accessed inside the spinlock, so to avoid
confusion that there are concurrent accesses and simplify the code,
change it to an int.
One access outside of the spinlock is in enable_show() which appears to
allow partially written data to be displayed between enable_req_count,
powered and enabled so move this one inside the spin lock too.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-cti-core.c | 14 +++++++-------
drivers/hwtracing/coresight/coresight-cti-sysfs.c | 2 +-
drivers/hwtracing/coresight/coresight-cti.h | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
index 838872f2484d..277c890a1f1f 100644
--- a/drivers/hwtracing/coresight/coresight-cti-core.c
+++ b/drivers/hwtracing/coresight/coresight-cti-core.c
@@ -107,12 +107,12 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
cti_write_all_hw_regs(drvdata);
config->hw_enabled = true;
- atomic_inc(&drvdata->config.enable_req_count);
+ drvdata->config.enable_req_count++;
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return rc;
cti_state_unchanged:
- atomic_inc(&drvdata->config.enable_req_count);
+ drvdata->config.enable_req_count++;
/* cannot enable due to error */
cti_err_not_enabled:
@@ -129,7 +129,7 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata)
config->hw_powered = true;
/* no need to do anything if no enable request */
- if (!atomic_read(&drvdata->config.enable_req_count))
+ if (!drvdata->config.enable_req_count)
goto cti_hp_not_enabled;
/* try to claim the device */
@@ -156,13 +156,13 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
spin_lock(&drvdata->spinlock);
/* don't allow negative refcounts, return an error */
- if (!atomic_read(&drvdata->config.enable_req_count)) {
+ if (!drvdata->config.enable_req_count) {
ret = -EINVAL;
goto cti_not_disabled;
}
/* check refcount - disable on 0 */
- if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
+ if (--drvdata->config.enable_req_count > 0)
goto cti_not_disabled;
/* no need to do anything if disabled or cpu unpowered */
@@ -239,7 +239,7 @@ static void cti_set_default_config(struct device *dev,
/* Most regs default to 0 as zalloc'ed except...*/
config->trig_filter_enable = true;
config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0);
- atomic_set(&config->enable_req_count, 0);
+ config->enable_req_count = 0;
}
/*
@@ -696,7 +696,7 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
drvdata->config.hw_enabled = false;
/* check enable reference count to enable HW */
- if (atomic_read(&drvdata->config.enable_req_count)) {
+ if (drvdata->config.enable_req_count) {
/* check we can claim the device as we re-power */
if (coresight_claim_device(csdev))
goto cti_notify_exit;
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
index 71e7a8266bb3..e528cff9d4e2 100644
--- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
@@ -84,8 +84,8 @@ static ssize_t enable_show(struct device *dev,
bool enabled, powered;
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- enable_req = atomic_read(&drvdata->config.enable_req_count);
spin_lock(&drvdata->spinlock);
+ enable_req = drvdata->config.enable_req_count;
powered = drvdata->config.hw_powered;
enabled = drvdata->config.hw_enabled;
spin_unlock(&drvdata->spinlock);
diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
index acf7b545e6b9..8b106b13a244 100644
--- a/drivers/hwtracing/coresight/coresight-cti.h
+++ b/drivers/hwtracing/coresight/coresight-cti.h
@@ -141,7 +141,7 @@ struct cti_config {
int nr_trig_max;
/* cti enable control */
- atomic_t enable_req_count;
+ int enable_req_count;
bool hw_enabled;
bool hw_powered;
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/3] coresight: cti: Prevent negative values of enable count
2023-01-10 11:07 ` James Clark
@ 2023-01-12 14:42 ` Mike Leach
-1 siblings, 0 replies; 14+ messages in thread
From: Mike Leach @ 2023-01-12 14:42 UTC (permalink / raw)
To: James Clark
Cc: coresight, quic_jinlmao, suzuki.poulose, Mathieu Poirier,
Leo Yan, Alexander Shishkin, Greg Kroah-Hartman,
linux-arm-kernel, linux-kernel
On Tue, 10 Jan 2023 at 11:08, James Clark <james.clark@arm.com> wrote:
>
> Writing 0 to the enable control repeatedly results in a negative value
> for enable_req_count. After this, writing 1 to the enable control
> appears to not work until the count returns to positive.
>
> Change it so that it's impossible for enable_req_count to be < 0.
> Return an error to indicate that the disable request was invalid.
>
> Fixes: 835d722ba10a ("coresight: cti: Initial CoreSight CTI Driver")
> Tested-by: Jinlong Mao <quic_jinlmao@quicinc.com>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-cti-core.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index d2cf4f4848e1..838872f2484d 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -151,9 +151,16 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
> {
> struct cti_config *config = &drvdata->config;
> struct coresight_device *csdev = drvdata->csdev;
> + int ret = 0;
>
> spin_lock(&drvdata->spinlock);
>
> + /* don't allow negative refcounts, return an error */
> + if (!atomic_read(&drvdata->config.enable_req_count)) {
> + ret = -EINVAL;
> + goto cti_not_disabled;
> + }
> +
> /* check refcount - disable on 0 */
> if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
> goto cti_not_disabled;
> @@ -171,12 +178,12 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
> coresight_disclaim_device_unlocked(csdev);
> CS_LOCK(drvdata->base);
> spin_unlock(&drvdata->spinlock);
> - return 0;
> + return ret;
>
> /* not disabled this call */
> cti_not_disabled:
> spin_unlock(&drvdata->spinlock);
> - return 0;
> + return ret;
> }
>
> void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value)
> --
> 2.25.1
>
reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/3] coresight: cti: Prevent negative values of enable count
@ 2023-01-12 14:42 ` Mike Leach
0 siblings, 0 replies; 14+ messages in thread
From: Mike Leach @ 2023-01-12 14:42 UTC (permalink / raw)
To: James Clark
Cc: coresight, quic_jinlmao, suzuki.poulose, Mathieu Poirier,
Leo Yan, Alexander Shishkin, Greg Kroah-Hartman,
linux-arm-kernel, linux-kernel
On Tue, 10 Jan 2023 at 11:08, James Clark <james.clark@arm.com> wrote:
>
> Writing 0 to the enable control repeatedly results in a negative value
> for enable_req_count. After this, writing 1 to the enable control
> appears to not work until the count returns to positive.
>
> Change it so that it's impossible for enable_req_count to be < 0.
> Return an error to indicate that the disable request was invalid.
>
> Fixes: 835d722ba10a ("coresight: cti: Initial CoreSight CTI Driver")
> Tested-by: Jinlong Mao <quic_jinlmao@quicinc.com>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-cti-core.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index d2cf4f4848e1..838872f2484d 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -151,9 +151,16 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
> {
> struct cti_config *config = &drvdata->config;
> struct coresight_device *csdev = drvdata->csdev;
> + int ret = 0;
>
> spin_lock(&drvdata->spinlock);
>
> + /* don't allow negative refcounts, return an error */
> + if (!atomic_read(&drvdata->config.enable_req_count)) {
> + ret = -EINVAL;
> + goto cti_not_disabled;
> + }
> +
> /* check refcount - disable on 0 */
> if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
> goto cti_not_disabled;
> @@ -171,12 +178,12 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
> coresight_disclaim_device_unlocked(csdev);
> CS_LOCK(drvdata->base);
> spin_unlock(&drvdata->spinlock);
> - return 0;
> + return ret;
>
> /* not disabled this call */
> cti_not_disabled:
> spin_unlock(&drvdata->spinlock);
> - return 0;
> + return ret;
> }
>
> void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value)
> --
> 2.25.1
>
reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/3] coresight: cti: Remove atomic type from enable_req_count
2023-01-10 11:07 ` James Clark
@ 2023-01-12 14:43 ` Mike Leach
-1 siblings, 0 replies; 14+ messages in thread
From: Mike Leach @ 2023-01-12 14:43 UTC (permalink / raw)
To: James Clark
Cc: coresight, quic_jinlmao, suzuki.poulose, Mathieu Poirier,
Leo Yan, Alexander Shishkin, Greg Kroah-Hartman,
linux-arm-kernel, linux-kernel
On Tue, 10 Jan 2023 at 11:08, James Clark <james.clark@arm.com> wrote:
>
> enable_req_count is only ever accessed inside the spinlock, so to avoid
> confusion that there are concurrent accesses and simplify the code,
> change it to an int.
>
> One access outside of the spinlock is in enable_show() which appears to
> allow partially written data to be displayed between enable_req_count,
> powered and enabled so move this one inside the spin lock too.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-cti-core.c | 14 +++++++-------
> drivers/hwtracing/coresight/coresight-cti-sysfs.c | 2 +-
> drivers/hwtracing/coresight/coresight-cti.h | 2 +-
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index 838872f2484d..277c890a1f1f 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -107,12 +107,12 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
> cti_write_all_hw_regs(drvdata);
>
> config->hw_enabled = true;
> - atomic_inc(&drvdata->config.enable_req_count);
> + drvdata->config.enable_req_count++;
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
>
> cti_state_unchanged:
> - atomic_inc(&drvdata->config.enable_req_count);
> + drvdata->config.enable_req_count++;
>
> /* cannot enable due to error */
> cti_err_not_enabled:
> @@ -129,7 +129,7 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata)
> config->hw_powered = true;
>
> /* no need to do anything if no enable request */
> - if (!atomic_read(&drvdata->config.enable_req_count))
> + if (!drvdata->config.enable_req_count)
> goto cti_hp_not_enabled;
>
> /* try to claim the device */
> @@ -156,13 +156,13 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
> spin_lock(&drvdata->spinlock);
>
> /* don't allow negative refcounts, return an error */
> - if (!atomic_read(&drvdata->config.enable_req_count)) {
> + if (!drvdata->config.enable_req_count) {
> ret = -EINVAL;
> goto cti_not_disabled;
> }
>
> /* check refcount - disable on 0 */
> - if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
> + if (--drvdata->config.enable_req_count > 0)
> goto cti_not_disabled;
>
> /* no need to do anything if disabled or cpu unpowered */
> @@ -239,7 +239,7 @@ static void cti_set_default_config(struct device *dev,
> /* Most regs default to 0 as zalloc'ed except...*/
> config->trig_filter_enable = true;
> config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0);
> - atomic_set(&config->enable_req_count, 0);
> + config->enable_req_count = 0;
> }
>
> /*
> @@ -696,7 +696,7 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> drvdata->config.hw_enabled = false;
>
> /* check enable reference count to enable HW */
> - if (atomic_read(&drvdata->config.enable_req_count)) {
> + if (drvdata->config.enable_req_count) {
> /* check we can claim the device as we re-power */
> if (coresight_claim_device(csdev))
> goto cti_notify_exit;
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index 71e7a8266bb3..e528cff9d4e2 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -84,8 +84,8 @@ static ssize_t enable_show(struct device *dev,
> bool enabled, powered;
> struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>
> - enable_req = atomic_read(&drvdata->config.enable_req_count);
> spin_lock(&drvdata->spinlock);
> + enable_req = drvdata->config.enable_req_count;
> powered = drvdata->config.hw_powered;
> enabled = drvdata->config.hw_enabled;
> spin_unlock(&drvdata->spinlock);
> diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
> index acf7b545e6b9..8b106b13a244 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.h
> +++ b/drivers/hwtracing/coresight/coresight-cti.h
> @@ -141,7 +141,7 @@ struct cti_config {
> int nr_trig_max;
>
> /* cti enable control */
> - atomic_t enable_req_count;
> + int enable_req_count;
> bool hw_enabled;
> bool hw_powered;
>
> --
> 2.25.1
>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/3] coresight: cti: Remove atomic type from enable_req_count
@ 2023-01-12 14:43 ` Mike Leach
0 siblings, 0 replies; 14+ messages in thread
From: Mike Leach @ 2023-01-12 14:43 UTC (permalink / raw)
To: James Clark
Cc: coresight, quic_jinlmao, suzuki.poulose, Mathieu Poirier,
Leo Yan, Alexander Shishkin, Greg Kroah-Hartman,
linux-arm-kernel, linux-kernel
On Tue, 10 Jan 2023 at 11:08, James Clark <james.clark@arm.com> wrote:
>
> enable_req_count is only ever accessed inside the spinlock, so to avoid
> confusion that there are concurrent accesses and simplify the code,
> change it to an int.
>
> One access outside of the spinlock is in enable_show() which appears to
> allow partially written data to be displayed between enable_req_count,
> powered and enabled so move this one inside the spin lock too.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-cti-core.c | 14 +++++++-------
> drivers/hwtracing/coresight/coresight-cti-sysfs.c | 2 +-
> drivers/hwtracing/coresight/coresight-cti.h | 2 +-
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index 838872f2484d..277c890a1f1f 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -107,12 +107,12 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
> cti_write_all_hw_regs(drvdata);
>
> config->hw_enabled = true;
> - atomic_inc(&drvdata->config.enable_req_count);
> + drvdata->config.enable_req_count++;
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
>
> cti_state_unchanged:
> - atomic_inc(&drvdata->config.enable_req_count);
> + drvdata->config.enable_req_count++;
>
> /* cannot enable due to error */
> cti_err_not_enabled:
> @@ -129,7 +129,7 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata)
> config->hw_powered = true;
>
> /* no need to do anything if no enable request */
> - if (!atomic_read(&drvdata->config.enable_req_count))
> + if (!drvdata->config.enable_req_count)
> goto cti_hp_not_enabled;
>
> /* try to claim the device */
> @@ -156,13 +156,13 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
> spin_lock(&drvdata->spinlock);
>
> /* don't allow negative refcounts, return an error */
> - if (!atomic_read(&drvdata->config.enable_req_count)) {
> + if (!drvdata->config.enable_req_count) {
> ret = -EINVAL;
> goto cti_not_disabled;
> }
>
> /* check refcount - disable on 0 */
> - if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
> + if (--drvdata->config.enable_req_count > 0)
> goto cti_not_disabled;
>
> /* no need to do anything if disabled or cpu unpowered */
> @@ -239,7 +239,7 @@ static void cti_set_default_config(struct device *dev,
> /* Most regs default to 0 as zalloc'ed except...*/
> config->trig_filter_enable = true;
> config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0);
> - atomic_set(&config->enable_req_count, 0);
> + config->enable_req_count = 0;
> }
>
> /*
> @@ -696,7 +696,7 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> drvdata->config.hw_enabled = false;
>
> /* check enable reference count to enable HW */
> - if (atomic_read(&drvdata->config.enable_req_count)) {
> + if (drvdata->config.enable_req_count) {
> /* check we can claim the device as we re-power */
> if (coresight_claim_device(csdev))
> goto cti_notify_exit;
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index 71e7a8266bb3..e528cff9d4e2 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -84,8 +84,8 @@ static ssize_t enable_show(struct device *dev,
> bool enabled, powered;
> struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>
> - enable_req = atomic_read(&drvdata->config.enable_req_count);
> spin_lock(&drvdata->spinlock);
> + enable_req = drvdata->config.enable_req_count;
> powered = drvdata->config.hw_powered;
> enabled = drvdata->config.hw_enabled;
> spin_unlock(&drvdata->spinlock);
> diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
> index acf7b545e6b9..8b106b13a244 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.h
> +++ b/drivers/hwtracing/coresight/coresight-cti.h
> @@ -141,7 +141,7 @@ struct cti_config {
> int nr_trig_max;
>
> /* cti enable control */
> - atomic_t enable_req_count;
> + int enable_req_count;
> bool hw_enabled;
> bool hw_powered;
>
> --
> 2.25.1
>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/3] coresight: cti: Add PM runtime call in enable_store
2023-01-10 11:07 ` James Clark
@ 2023-01-16 10:13 ` Suzuki K Poulose
-1 siblings, 0 replies; 14+ messages in thread
From: Suzuki K Poulose @ 2023-01-16 10:13 UTC (permalink / raw)
To: James Clark, coresight, quic_jinlmao, mike.leach
Cc: Mathieu Poirier, Leo Yan, Alexander Shishkin, Greg Kroah-Hartman,
linux-arm-kernel, linux-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/3] coresight: cti: Add PM runtime call in enable_store
@ 2023-01-16 10:13 ` Suzuki K Poulose
0 siblings, 0 replies; 14+ messages in thread
From: Suzuki K Poulose @ 2023-01-16 10:13 UTC (permalink / raw)
To: James Clark, coresight, quic_jinlmao, mike.leach
Cc: Mathieu Poirier, Leo Yan, Alexander Shishkin, Greg Kroah-Hartman,
linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-01-16 10:15 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-10 11:07 [PATCH v3 0/3] coresight: cti: Add PM runtime call in enable_store James Clark
2023-01-10 11:07 ` James Clark
2023-01-10 11:07 ` [PATCH v3 1/3] coresight: cti: Prevent negative values of enable count James Clark
2023-01-10 11:07 ` James Clark
2023-01-12 14:42 ` Mike Leach
2023-01-12 14:42 ` Mike Leach
2023-01-10 11:07 ` [PATCH v3 2/3] coresight: cti: Add PM runtime call in enable_store James Clark
2023-01-10 11:07 ` James Clark
2023-01-10 11:07 ` [PATCH v3 3/3] coresight: cti: Remove atomic type from enable_req_count James Clark
2023-01-10 11:07 ` James Clark
2023-01-12 14:43 ` Mike Leach
2023-01-12 14:43 ` Mike Leach
2023-01-16 10:13 ` [PATCH v3 0/3] coresight: cti: Add PM runtime call in enable_store Suzuki K Poulose
2023-01-16 10:13 ` Suzuki K Poulose
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.