Re: [PATCH v3 04/12] bus: mhi: core: Move to SYS_ERROR regardless of RDDM capability

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mani,

On 2020-10-30 06:52, Manivannan Sadhasivam wrote:
On Thu, Oct 29, 2020 at 09:10:49PM -0700, Bhaumik Bhatt wrote:
In some cases, the entry of device to RDDM execution environment
can occur after a significant amount of time has elapsed and a
SYS_ERROR state change event has already arrived.

I don't quite understand this statement. Can you elaborate? This doesn't
relate to what the patch is doing.

So the mhi_intvec_threaded_handler() (BHI) MSI that fires to switch the EE to RDDM may come much later than the SYS_ERROR state change event from the
control event ring. We currently, do not move to MHI_PM_SYS_ERR_DETECT
state if RDDM is supported i.e. mhi_cntrl->rddm_image is set. However, it means that we will remain in an "active" MHI PM state for the duration of
time until RDDM EE (BHI) MSI comes in. We have seen it take 5 seconds in
some bad cases.
This can result
in scenarios where MHI controller and client drivers are unaware
of the error state of the device. Remove the check for rddm_image
when processing the SYS_ERROR state change as it is present in
mhi_pm_sys_err_handler() already and prevent further activity
until the expected RDDM execution environment change occurs or
the controller driver decides further action.

Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
---
 drivers/bus/mhi/core/main.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 2cff5dd..1f32d67 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -733,19 +733,15 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
 				break;
 			case MHI_STATE_SYS_ERR:
 			{
-				enum mhi_pm_state new_state;
-
-				/* skip SYS_ERROR handling if RDDM supported */
-				if (mhi_cntrl->ee == MHI_EE_RDDM ||
-				    mhi_cntrl->rddm_image)
-					break;
+				enum mhi_pm_state state = MHI_PM_STATE_MAX;

 				dev_dbg(dev, "System error detected\n");
 				write_lock_irq(&mhi_cntrl->pm_lock);
-				new_state = mhi_tryset_pm_state(mhi_cntrl,
+				if (mhi_cntrl->ee != MHI_EE_RDDM)

But you are still checking for RDDM EE?

Please explain why you want to skip RDDM check.

Thanks,
Mani

Yes, the point is to only remove the mhi_cntrl->rddm_image check but still retain the "has EE moved to become RDDM" check. This allows us to avoid any extra processing of moving states to MHI_PM_SYS_ERR_DETECT state if it may be unnecessary (EE already changed to RDDM). The mhi_cntrl->rddm_image is
also present in mhi_pm_sys_err_handler(mhi_cntrl) function so it is not
needed here.
+					state = mhi_tryset_pm_state(mhi_cntrl,
 							MHI_PM_SYS_ERR_DETECT);
 				write_unlock_irq(&mhi_cntrl->pm_lock);
-				if (new_state == MHI_PM_SYS_ERR_DETECT)
+				if (state == MHI_PM_SYS_ERR_DETECT)
 					mhi_pm_sys_err_handler(mhi_cntrl);
 				break;
 			}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


This is why I mention the word RDDM "capability". If controller supports RDDM is not enough to skip the move to MHI_PM_SYS_ERR_DETECT state as it is safer
to move and stop client drivers from pushing data.

Thanks,
Bhaumik
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux