| Message ID | 20221224141700.20891-1-quic_jinlmao@quicinc.com |
|---|---|
| State | Accepted |
| Commit | eff674a9b86a6ffdd10c3af3863545acf7f1ce4f |
| Headers | show |
| Series | coresight: cti: Add PM runtime call in enable_store | expand |
On 04/01/2023 13:11, James Clark wrote: > > > On 24/12/2022 14:17, Mao Jinlong wrote: >> 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. >> >> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com> >> --- >> drivers/hwtracing/coresight/coresight-cti-sysfs.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c >> index 6d59c815ecf5..b1ed424ae043 100644 >> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c >> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c >> @@ -108,10 +108,17 @@ 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); >> + pm_runtime_put(dev->parent); > > Hi Jinlong, > > This new pm_runtime_put() causes this when writing 0 to enable: > > [ 483.253814] coresight-cti 23020000.cti: Runtime PM usage count > underflow! > > Maybe we can modify cti_disable_hw() to return a value to indicate that > the disable actually happened, and only then call pm_runtime_put(). > > I suppose you could also check in the store function if it was already > enabled first, but then I don't know what kind of locking that would > need? cti_disable_hw() already seems to have a couple of locks, so maybe > the return value solution is easiest. > We've also just seen another issue where multiple calls to cti_disable_hw() can cause enable_req_count to go negative. I'm going to work on a few fixes (including yours) to make sure that it's complete and post it shortly. James
On 1/5/2023 9:55 PM, James Clark wrote: > > On 04/01/2023 13:11, James Clark wrote: >> >> On 24/12/2022 14:17, Mao Jinlong wrote: >>> 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. >>> >>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com> >>> --- >>> drivers/hwtracing/coresight/coresight-cti-sysfs.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c >>> index 6d59c815ecf5..b1ed424ae043 100644 >>> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c >>> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c >>> @@ -108,10 +108,17 @@ 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); >>> + pm_runtime_put(dev->parent); >> Hi Jinlong, >> >> This new pm_runtime_put() causes this when writing 0 to enable: >> >> [ 483.253814] coresight-cti 23020000.cti: Runtime PM usage count >> underflow! >> >> Maybe we can modify cti_disable_hw() to return a value to indicate that >> the disable actually happened, and only then call pm_runtime_put(). >> >> I suppose you could also check in the store function if it was already >> enabled first, but then I don't know what kind of locking that would >> need? cti_disable_hw() already seems to have a couple of locks, so maybe >> the return value solution is easiest. >> > We've also just seen another issue where multiple calls to > cti_disable_hw() can cause enable_req_count to go negative. I'm going to > work on a few fixes (including yours) to make sure that it's complete > and post it shortly. Ok, Thank you, James. > > James
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 6d59c815ecf5..b1ed424ae043 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -108,10 +108,17 @@ 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); + pm_runtime_put(dev->parent); + } if (ret) return ret; return size;
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. Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com> --- drivers/hwtracing/coresight/coresight-cti-sysfs.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)