From: Jinlong Mao <quic_jinlmao@quicinc.com> To: James Clark <james.clark@arm.com> Cc: <coresight@lists.linaro.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, Tingwei <quic_tingweiz@quicinc.com>, "Yuanfang Zhang" <quic_yuanfang@quicinc.com>, Tao Zhang <quic_taozha@quicinc.com>, "Hao Zhang" <quic_hazha@quicinc.com>, <linux-arm-msm@vger.kernel.org>, "Mathieu Poirier" <mathieu.poirier@linaro.org>, Mike Leach <mike.leach@linaro.org>, Alexander Shishkin <alexander.shishkin@linux.intel.com> Subject: Re: [PATCH] coresight: cti: Add PM runtime call in enable_store Date: Thu, 5 Jan 2023 21:59:27 +0800 [thread overview] Message-ID: <04155aa3-c153-1d95-94da-b482acea048f@quicinc.com> (raw) In-Reply-To: <990b948f-11b3-2463-2d0c-be9d30d10328@arm.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Jinlong Mao <quic_jinlmao@quicinc.com> To: James Clark <james.clark@arm.com> Cc: <coresight@lists.linaro.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, Tingwei <quic_tingweiz@quicinc.com>, "Yuanfang Zhang" <quic_yuanfang@quicinc.com>, Tao Zhang <quic_taozha@quicinc.com>, "Hao Zhang" <quic_hazha@quicinc.com>, <linux-arm-msm@vger.kernel.org>, "Mathieu Poirier" <mathieu.poirier@linaro.org>, Mike Leach <mike.leach@linaro.org>, Alexander Shishkin <alexander.shishkin@linux.intel.com> Subject: Re: [PATCH] coresight: cti: Add PM runtime call in enable_store Date: Thu, 5 Jan 2023 21:59:27 +0800 [thread overview] Message-ID: <04155aa3-c153-1d95-94da-b482acea048f@quicinc.com> (raw) In-Reply-To: <990b948f-11b3-2463-2d0c-be9d30d10328@arm.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-01-05 14:01 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-24 14:17 [PATCH] coresight: cti: Add PM runtime call in enable_store Mao Jinlong 2022-12-24 14:17 ` Mao Jinlong 2023-01-04 13:11 ` James Clark 2023-01-04 13:11 ` James Clark 2023-01-05 13:55 ` James Clark 2023-01-05 13:55 ` James Clark 2023-01-05 13:59 ` Jinlong Mao [this message] 2023-01-05 13:59 ` Jinlong Mao
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=04155aa3-c153-1d95-94da-b482acea048f@quicinc.com \ --to=quic_jinlmao@quicinc.com \ --cc=alexander.shishkin@linux.intel.com \ --cc=coresight@lists.linaro.org \ --cc=james.clark@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mathieu.poirier@linaro.org \ --cc=mike.leach@linaro.org \ --cc=quic_hazha@quicinc.com \ --cc=quic_taozha@quicinc.com \ --cc=quic_tingweiz@quicinc.com \ --cc=quic_yuanfang@quicinc.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.