diff mbox series

[v1,2/6] bus: mhi: core: Mark device inactive soon after host issues a shutdown

Message ID 1589934631-22752-3-git-send-email-bbhatt@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series Bug fixes and bootup and shutdown improvements | expand

Commit Message

Bhaumik Bhatt May 20, 2020, 12:30 a.m. UTC 
Clients on the host may see the device in an active state for a short
period of time after the host detects a device error or power down.
Prevent any further host activity which could lead to race conditions
and timeouts seen by clients attempting to push data as they must be
notified of the host's intent sooner than later. This also allows the
host and device states to be in sync with one another and prevents
unnecessary host operations.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 33 ++++++++++++++++++++++++++-------
 drivers/bus/mhi/core/pm.c   | 31 +++++++++++++++++++------------
 2 files changed, 45 insertions(+), 19 deletions(-)

Comments

kernel test robot May 20, 2020, 5:16 a.m. UTC | #1 
Hi Bhaumik,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20200519]
[cannot apply to linus/master v5.7-rc6 v5.7-rc5 v5.7-rc4 v5.7-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Bhaumik-Bhatt/Bug-fixes-and-bootup-and-shutdown-improvements/20200520-083400
base:    fb57b1fabcb28f358901b2df90abd2b48abc1ca8
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

drivers/bus/mhi/core/main.c: In function 'mhi_intvec_threaded_handler':
>> drivers/bus/mhi/core/main.c:397:8: error: implicit declaration of function 'mhi_is_active' [-Werror=implicit-function-declaration]
397 |   if (!mhi_is_active(mhi_cntrl)) {
|        ^~~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +/mhi_is_active +397 drivers/bus/mhi/core/main.c

   371	
   372	irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
   373	{
   374		struct mhi_controller *mhi_cntrl = priv;
   375		struct device *dev = &mhi_cntrl->mhi_dev->dev;
   376		enum mhi_state state = MHI_STATE_MAX;
   377		enum mhi_pm_state pm_state = 0;
   378		enum mhi_ee_type ee = 0;
   379		bool handle_rddm = false;
   380	
   381		write_lock_irq(&mhi_cntrl->pm_lock);
   382		if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
   383			write_unlock_irq(&mhi_cntrl->pm_lock);
   384			goto exit_intvec;
   385		}
   386	
   387		state = mhi_get_mhi_state(mhi_cntrl);
   388		ee = mhi_cntrl->ee;
   389		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
   390		dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
   391			TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
   392			TO_MHI_STATE_STR(state));
   393	
   394		 /* If device supports RDDM don't bother processing SYS error */
   395		if (mhi_cntrl->rddm_image) {
   396			/* host may be performing a device power down already */
 > 397			if (!mhi_is_active(mhi_cntrl)) {
   398				write_unlock_irq(&mhi_cntrl->pm_lock);
   399				goto exit_intvec;
   400			}
   401	
   402			if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
   403				/* prevent clients from queueing any more packets */
   404				pm_state = mhi_tryset_pm_state(mhi_cntrl,
   405							       MHI_PM_SYS_ERR_DETECT);
   406				if (pm_state == MHI_PM_SYS_ERR_DETECT)
   407					handle_rddm = true;
   408			}
   409	
   410			write_unlock_irq(&mhi_cntrl->pm_lock);
   411	
   412			if (handle_rddm) {
   413				dev_err(dev, "RDDM event occurred!\n");
   414				mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
   415				wake_up_all(&mhi_cntrl->state_event);
   416			}
   417			goto exit_intvec;
   418		}
   419	
   420		if (state == MHI_STATE_SYS_ERR) {
   421			dev_dbg(dev, "System error detected\n");
   422			pm_state = mhi_tryset_pm_state(mhi_cntrl,
   423						       MHI_PM_SYS_ERR_DETECT);
   424		}
   425	
   426		write_unlock_irq(&mhi_cntrl->pm_lock);
   427	
   428		if (pm_state == MHI_PM_SYS_ERR_DETECT) {
   429			wake_up_all(&mhi_cntrl->state_event);
   430	
   431			/* For fatal errors, we let controller decide next step */
   432			if (MHI_IN_PBL(ee))
   433				mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
   434			else
   435				mhi_pm_sys_err_handler(mhi_cntrl);
   436		}
   437	
   438	exit_intvec:
   439	
   440		return IRQ_HANDLED;
   441	}
   442	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot May 21, 2020, 10:26 a.m. UTC | #2 
Hi Bhaumik,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200519]
[cannot apply to linus/master v5.7-rc6 v5.7-rc5 v5.7-rc4 v5.7-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Bhaumik-Bhatt/Bug-fixes-and-bootup-and-shutdown-improvements/20200520-083400
base:    fb57b1fabcb28f358901b2df90abd2b48abc1ca8
config: xtensa-randconfig-r036-20200520 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

In file included from include/linux/dev_printk.h:14,
from include/linux/device.h:15,
from drivers/bus/mhi/core/main.c:7:
include/linux/dma-mapping.h: In function 'dma_map_resource':
arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
|         ^~
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                    ^~~~
include/linux/dma-mapping.h:352:2: note: in expansion of macro 'if'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|  ^~
include/linux/dma-mapping.h:352:6: note: in expansion of macro 'WARN_ON_ONCE'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|      ^~~~~~~~~~~~
include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|                   ^~~~~~~~~
arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
|         ^~
include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                             ^~~~
include/linux/dma-mapping.h:352:2: note: in expansion of macro 'if'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|  ^~
include/linux/dma-mapping.h:352:6: note: in expansion of macro 'WARN_ON_ONCE'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|      ^~~~~~~~~~~~
include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|                   ^~~~~~~~~
arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
|         ^~
include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
69 |  (cond) ?              |   ^~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
|                            ^~~~~~~~~~~~~~
include/linux/dma-mapping.h:352:2: note: in expansion of macro 'if'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|  ^~
include/linux/dma-mapping.h:352:6: note: in expansion of macro 'WARN_ON_ONCE'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|      ^~~~~~~~~~~~
include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
|                   ^~~~~~~~~
drivers/bus/mhi/core/main.c: In function 'mhi_intvec_threaded_handler':
drivers/bus/mhi/core/main.c:397:8: error: implicit declaration of function 'mhi_is_active' [-Werror=implicit-function-declaration]
397 |   if (!mhi_is_active(mhi_cntrl)) {
|        ^~~~~~~~~~~~~
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                    ^~~~
>> drivers/bus/mhi/core/main.c:397:3: note: in expansion of macro 'if'
397 |   if (!mhi_is_active(mhi_cntrl)) {
|   ^~
cc1: some warnings being treated as errors

vim +/if +397 drivers/bus/mhi/core/main.c

   371	
   372	irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
   373	{
   374		struct mhi_controller *mhi_cntrl = priv;
   375		struct device *dev = &mhi_cntrl->mhi_dev->dev;
   376		enum mhi_state state = MHI_STATE_MAX;
   377		enum mhi_pm_state pm_state = 0;
   378		enum mhi_ee_type ee = 0;
   379		bool handle_rddm = false;
   380	
   381		write_lock_irq(&mhi_cntrl->pm_lock);
   382		if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
   383			write_unlock_irq(&mhi_cntrl->pm_lock);
   384			goto exit_intvec;
   385		}
   386	
   387		state = mhi_get_mhi_state(mhi_cntrl);
   388		ee = mhi_cntrl->ee;
   389		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
   390		dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
   391			TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
   392			TO_MHI_STATE_STR(state));
   393	
   394		 /* If device supports RDDM don't bother processing SYS error */
   395		if (mhi_cntrl->rddm_image) {
   396			/* host may be performing a device power down already */
 > 397			if (!mhi_is_active(mhi_cntrl)) {
   398				write_unlock_irq(&mhi_cntrl->pm_lock);
   399				goto exit_intvec;
   400			}
   401	
   402			if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
   403				/* prevent clients from queueing any more packets */
   404				pm_state = mhi_tryset_pm_state(mhi_cntrl,
   405							       MHI_PM_SYS_ERR_DETECT);
   406				if (pm_state == MHI_PM_SYS_ERR_DETECT)
   407					handle_rddm = true;
   408			}
   409	
   410			write_unlock_irq(&mhi_cntrl->pm_lock);
   411	
   412			if (handle_rddm) {
   413				dev_err(dev, "RDDM event occurred!\n");
   414				mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
   415				wake_up_all(&mhi_cntrl->state_event);
   416			}
   417			goto exit_intvec;
   418		}
   419	
   420		if (state == MHI_STATE_SYS_ERR) {
   421			dev_dbg(dev, "System error detected\n");
   422			pm_state = mhi_tryset_pm_state(mhi_cntrl,
   423						       MHI_PM_SYS_ERR_DETECT);
   424		}
   425	
   426		write_unlock_irq(&mhi_cntrl->pm_lock);
   427	
   428		if (pm_state == MHI_PM_SYS_ERR_DETECT) {
   429			wake_up_all(&mhi_cntrl->state_event);
   430	
   431			/* For fatal errors, we let controller decide next step */
   432			if (MHI_IN_PBL(ee))
   433				mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
   434			else
   435				mhi_pm_sys_err_handler(mhi_cntrl);
   436		}
   437	
   438	exit_intvec:
   439	
   440		return IRQ_HANDLED;
   441	}
   442	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index bafc12a..da32c23 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -376,6 +376,7 @@  irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 	enum mhi_state state = MHI_STATE_MAX;
 	enum mhi_pm_state pm_state = 0;
 	enum mhi_ee_type ee = 0;
+	bool handle_rddm = false;
 
 	write_lock_irq(&mhi_cntrl->pm_lock);
 	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
@@ -390,22 +391,40 @@  irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
 		TO_MHI_STATE_STR(state));
 
-	if (state == MHI_STATE_SYS_ERR) {
-		dev_dbg(dev, "System error detected\n");
-		pm_state = mhi_tryset_pm_state(mhi_cntrl,
-					       MHI_PM_SYS_ERR_DETECT);
-	}
-	write_unlock_irq(&mhi_cntrl->pm_lock);
-
 	 /* If device supports RDDM don't bother processing SYS error */
 	if (mhi_cntrl->rddm_image) {
+		/* host may be performing a device power down already */
+		if (!mhi_is_active(mhi_cntrl)) {
+			write_unlock_irq(&mhi_cntrl->pm_lock);
+			goto exit_intvec;
+		}
+
 		if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
+			/* prevent clients from queueing any more packets */
+			pm_state = mhi_tryset_pm_state(mhi_cntrl,
+						       MHI_PM_SYS_ERR_DETECT);
+			if (pm_state == MHI_PM_SYS_ERR_DETECT)
+				handle_rddm = true;
+		}
+
+		write_unlock_irq(&mhi_cntrl->pm_lock);
+
+		if (handle_rddm) {
+			dev_err(dev, "RDDM event occurred!\n");
 			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
 			wake_up_all(&mhi_cntrl->state_event);
 		}
 		goto exit_intvec;
 	}
 
+	if (state == MHI_STATE_SYS_ERR) {
+		dev_dbg(dev, "System error detected\n");
+		pm_state = mhi_tryset_pm_state(mhi_cntrl,
+					       MHI_PM_SYS_ERR_DETECT);
+	}
+
+	write_unlock_irq(&mhi_cntrl->pm_lock);
+
 	if (pm_state == MHI_PM_SYS_ERR_DETECT) {
 		wake_up_all(&mhi_cntrl->state_event);
 
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index b2b3de7..1daed86 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -465,15 +465,10 @@  static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 	write_lock_irq(&mhi_cntrl->pm_lock);
 	prev_state = mhi_cntrl->pm_state;
 	cur_state = mhi_tryset_pm_state(mhi_cntrl, transition_state);
-	if (cur_state == transition_state) {
-		mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
+	if (cur_state == MHI_PM_SYS_ERR_PROCESS)
 		mhi_cntrl->dev_state = MHI_STATE_RESET;
-	}
 	write_unlock_irq(&mhi_cntrl->pm_lock);
 
-	/* Wake up threads waiting for state transition */
-	wake_up_all(&mhi_cntrl->state_event);
-
 	if (cur_state != transition_state) {
 		dev_err(dev, "Failed to transition to state: %s from: %s\n",
 			to_mhi_pm_state_str(transition_state),
@@ -482,6 +477,11 @@  static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 		return;
 	}
 
+	mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
+
+	/* Wake up threads waiting for state transition */
+	wake_up_all(&mhi_cntrl->state_event);
+
 	/* Trigger MHI RESET so that the device will not access host memory */
 	if (MHI_REG_ACCESS_VALID(prev_state)) {
 		u32 in_reset = -1;
@@ -1048,22 +1048,29 @@  void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
 	enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 
+	mutex_lock(&mhi_cntrl->pm_mutex);
+	write_lock_irq(&mhi_cntrl->pm_lock);
+
 	/* If it's not a graceful shutdown, force MHI to linkdown state */
 	if (!graceful) {
-		mutex_lock(&mhi_cntrl->pm_mutex);
-		write_lock_irq(&mhi_cntrl->pm_lock);
 		cur_state = mhi_tryset_pm_state(mhi_cntrl,
 						MHI_PM_LD_ERR_FATAL_DETECT);
-		write_unlock_irq(&mhi_cntrl->pm_lock);
-		mutex_unlock(&mhi_cntrl->pm_mutex);
-		if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT)
+		if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) {
 			dev_dbg(dev, "Failed to move to state: %s from: %s\n",
 				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
 				to_mhi_pm_state_str(mhi_cntrl->pm_state));
-		else
+		} else {
 			next_state = DEV_ST_TRANSITION_FATAL;
+			wake_up_all(&mhi_cntrl->state_event);
+		}
 	}
 
+	/* mark device as inactive to avoid any further host processing */
+	mhi_cntrl->dev_state = MHI_STATE_RESET;
+
+	write_unlock_irq(&mhi_cntrl->pm_lock);
+	mutex_unlock(&mhi_cntrl->pm_mutex);
+
 	mhi_queue_state_transition(mhi_cntrl, next_state);
 
 	/* Wait for shutdown to complete */