From: Vinod Koul <vkoul@kernel.org>
To: Bard Liao <yung-chuan.liao@linux.intel.com>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
gregkh@linuxfoundation.org, srinivas.kandagatla@linaro.org,
rander.wang@linux.intel.com,
pierre-louis.bossart@linux.intel.com, sanyog.r.kale@intel.com,
bard.liao@intel.com
Subject: Re: [PATCH] soundwire: cadence: add paranoid check on self-clearing bits
Date: Mon, 2 Aug 2021 09:20:20 +0530 [thread overview]
Message-ID: <YQdrfNA08UITBjvB@matsya> (raw)
In-Reply-To: <20210714051349.13064-1-yung-chuan.liao@linux.intel.com>
On 14-07-21, 13:13, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> The Cadence IP exposes a small number of self-clearing bits in
> the MCP_CONTROL and MCP_CONFIG_UPDATE registers.
>
> We currently do not check that those bits are indeed cleared,
> e.g. during resume operations. That could lead to resuming peripheral
> devices too early.
>
> In addition, if we happen to read these registers, update one of the
> fields and write the register back, we may be writing stale data that
> might have been cleared in hardware. These sort of race conditions
> could lead to e.g. doing a hw_reset twice or stopping a clock that
> just restarted. There is no clear way of avoiding these potential race
> conditions other than making sure that these registers fields are
> cleared before any read-modify-write sequence. If we detect this sort
> of errors, we only log them since there is no clear recovery
> possible. The only way out is likely to restart the IP with a
> suspend/resume cycle.
>
> Note that the checks are performed before updating the registers, as
> well as after the Intel 'sync go' sequence in multi-link mode. That
> should cover both the start and end of suspend/resume hardware
> configurations. The Multi-Master mode gates the configuration updates
> until the 'sync go' signal is asserted, so we only check on init and
> after the end of the 'sync go' sequence.
>
> The duration of the usleep_range() was defined by the GSYNC frequency
> used in multi-master mode. With a 4kHz frequency, any configuration
> change might be deferred by up to 250us. Extending the range to
> 1000-1500us should guarantee that the configuration change is
> completed without any significant impact on the overall resume
> time.
There were some checkpatch warns, but I think code will looks worse if
we split lines up, so applied now
--
~Vinod
WARNING: multiple messages have this Message-ID (diff)
From: Vinod Koul <vkoul@kernel.org>
To: Bard Liao <yung-chuan.liao@linux.intel.com>
Cc: alsa-devel@alsa-project.org, gregkh@linuxfoundation.org,
pierre-louis.bossart@linux.intel.com,
linux-kernel@vger.kernel.org, sanyog.r.kale@intel.com,
rander.wang@linux.intel.com, bard.liao@intel.com
Subject: Re: [PATCH] soundwire: cadence: add paranoid check on self-clearing bits
Date: Mon, 2 Aug 2021 09:20:20 +0530 [thread overview]
Message-ID: <YQdrfNA08UITBjvB@matsya> (raw)
In-Reply-To: <20210714051349.13064-1-yung-chuan.liao@linux.intel.com>
On 14-07-21, 13:13, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> The Cadence IP exposes a small number of self-clearing bits in
> the MCP_CONTROL and MCP_CONFIG_UPDATE registers.
>
> We currently do not check that those bits are indeed cleared,
> e.g. during resume operations. That could lead to resuming peripheral
> devices too early.
>
> In addition, if we happen to read these registers, update one of the
> fields and write the register back, we may be writing stale data that
> might have been cleared in hardware. These sort of race conditions
> could lead to e.g. doing a hw_reset twice or stopping a clock that
> just restarted. There is no clear way of avoiding these potential race
> conditions other than making sure that these registers fields are
> cleared before any read-modify-write sequence. If we detect this sort
> of errors, we only log them since there is no clear recovery
> possible. The only way out is likely to restart the IP with a
> suspend/resume cycle.
>
> Note that the checks are performed before updating the registers, as
> well as after the Intel 'sync go' sequence in multi-link mode. That
> should cover both the start and end of suspend/resume hardware
> configurations. The Multi-Master mode gates the configuration updates
> until the 'sync go' signal is asserted, so we only check on init and
> after the end of the 'sync go' sequence.
>
> The duration of the usleep_range() was defined by the GSYNC frequency
> used in multi-master mode. With a 4kHz frequency, any configuration
> change might be deferred by up to 250us. Extending the range to
> 1000-1500us should guarantee that the configuration change is
> completed without any significant impact on the overall resume
> time.
There were some checkpatch warns, but I think code will looks worse if
we split lines up, so applied now
--
~Vinod
next prev parent reply other threads:[~2021-08-02 3:50 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-14 5:13 [PATCH] soundwire: cadence: add paranoid check on self-clearing bits Bard Liao
2021-07-14 5:13 ` Bard Liao
2021-08-02 3:50 ` Vinod Koul [this message]
2021-08-02 3:50 ` Vinod Koul
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=YQdrfNA08UITBjvB@matsya \
--to=vkoul@kernel.org \
--cc=alsa-devel@alsa-project.org \
--cc=bard.liao@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=rander.wang@linux.intel.com \
--cc=sanyog.r.kale@intel.com \
--cc=srinivas.kandagatla@linaro.org \
--cc=yung-chuan.liao@linux.intel.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: link
Be 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.