| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 17 Nov 2020 03:46:35 +0000
From: Carl Yin(殷张成) <carl.yin@...ctel.com>
To: Bhaumik Bhatt <bbhatt@...eaurora.org>
CC: "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
"hemantk@...eaurora.org" <hemantk@...eaurora.org>,
"jhugo@...eaurora.org" <jhugo@...eaurora.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>
Subject: RE: [PATCH v1 2/2] bus: mhi: core: Process execution environment
changes serially
Hi bbhat:
On November 17, 2020 10:58 AM, bbhatt wrote:
> In current design, whenever the BHI interrupt is fired, the execution
> environment is updated. This can cause race conditions and impede any ongoing
> power up/down processing. For example, if a power down is in progress and the
> host has updated the execution environment to a local "disabled" state, any BHI
> interrupt firing later could replace it with the value from the BHI EE register.
> Another example would be that the device can enter mission mode while device
> creation for SBL is still going on, leading to multiple attempts at opening the same
> channel.
>
> Ensure that EE changes are handled only from appropriate places and occur one
> after another and handle only PBL or RDDM EE changes as critical events directly
> from the interrupt handler. This also makes sure that we use the correct
> execution environment to notify the controller driver when the device resets to
> one of the PBL execution environments.
>
> Signed-off-by: Bhaumik Bhatt <bbhatt@...eaurora.org>
> ---
> drivers/bus/mhi/core/main.c | 14 ++++++++------
> drivers/bus/mhi/core/pm.c | 6 ++++--
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index
> 4eb93d8..cd712f2 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -377,7 +377,7 @@ irqreturn_t mhi_intvec_threaded_handler(int
> irq_number, void *priv)
> struct device *dev = &mhi_cntrl->mhi_dev->dev;
> enum mhi_state state = MHI_STATE_MAX;
> enum mhi_pm_state pm_state = 0;
> - enum mhi_ee_type ee = 0;
> + enum mhi_ee_type ee = MHI_EE_MAX;
>
> write_lock_irq(&mhi_cntrl->pm_lock);
> if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) { @@ -386,8 +386,7
> @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
> }
>
> state = mhi_get_mhi_state(mhi_cntrl);
> - ee = mhi_cntrl->ee;
> - mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> + ee = mhi_get_exec_env(mhi_cntrl);
> dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
> TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
> TO_MHI_STATE_STR(state));
> @@ -405,8 +404,9 @@ irqreturn_t mhi_intvec_threaded_handler(int
> irq_number, void *priv)
> if (!mhi_is_active(mhi_cntrl))
> goto exit_intvec;
>
> - if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
> + if (ee == MHI_EE_RDDM && mhi_cntrl->ee != MHI_EE_RDDM) {
> mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
> + mhi_cntrl->ee = ee;
> wake_up_all(&mhi_cntrl->state_event);
> }
> goto exit_intvec;
> @@ -416,10 +416,12 @@ irqreturn_t mhi_intvec_threaded_handler(int
> irq_number, void *priv)
> wake_up_all(&mhi_cntrl->state_event);
>
> /* For fatal errors, we let controller decide next step */
> - if (MHI_IN_PBL(ee))
> + if (MHI_IN_PBL(ee)) {
> mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
> - else
> + mhi_cntrl->ee = ee;
> + } else {
> mhi_pm_sys_err_handler(mhi_cntrl);
> + }
> }
>
[carl.yin] I guess this patch can fix another bug (maybe is not bug)
For hw event with brst mode enabled. If M0 event come before AMSS event, as next log:
mhi_pm_m0_transition() will call mhi_ring_cmd_db(), but ring->wp not correctly set
until the later mhi_pm_mission_mode_transition() set ring->wp.
[ 1550.901882] mhi 0000:03:00.0: Requested to power ON
[ 1550.902065] mhi 0000:03:00.0: Power on setup success
[ 1550.902256] mhi 0000:03:00.0: Handling state transition: PBL
[ 1551.751325] pci 0000:01:00.0: PCI bridge to [bus 02]
[ 1556.113314] mhi 0000:03:00.0: Device in READY State
[ 1556.113319] mhi 0000:03:00.0: Initializing MHI registers
[ 1559.391644] mhi 0000:03:00.0: local ee:AMSS device ee:PASS THRU dev_state:READY
[ 1559.410969] mhi 0000:03:00.0: State change event to state: M0
[ 1559.414807] mhi 0000:03:00.0: Received EE event: AMSS
[ 1559.414860] mhi 0000:03:00.0: Handling state transition: MISSION_MODE
[ 1559.414863] mhi 0000:03:00.0: Processing Mission Mode transition
> exit_intvec:
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index
> aa156b9..c1b18d6 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -377,20 +377,22 @@ static int mhi_pm_mission_mode_transition(struct
> mhi_controller *mhi_cntrl) {
> struct mhi_event *mhi_event;
> struct device *dev = &mhi_cntrl->mhi_dev->dev;
> + enum mhi_ee_type ee = MHI_EE_MAX;
> int i, ret;
>
> dev_dbg(dev, "Processing Mission Mode transition\n");
>
> write_lock_irq(&mhi_cntrl->pm_lock);
> if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
> - mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> + ee = mhi_get_exec_env(mhi_cntrl);
>
> - if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee)) {
> + if (!MHI_IN_MISSION_MODE(ee)) {
> mhi_cntrl->pm_state = MHI_PM_LD_ERR_FATAL_DETECT;
> write_unlock_irq(&mhi_cntrl->pm_lock);
> wake_up_all(&mhi_cntrl->state_event);
> return -EIO;
> }
> + mhi_cntrl->ee = ee;
> write_unlock_irq(&mhi_cntrl->pm_lock);
>
> wake_up_all(&mhi_cntrl->state_event);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project
Powered by blists - more mailing lists