[PATCH RFC 0/4] UFS patches for Linux kernel v5.14

[PATCH RFC 0/4] UFS patches for Linux kernel v5.14

[Bart Van Assche]

Hi Martin,

The four patches in this series are what I came up with while reviewing
recent UFS patches. These patches are intended for kernel v5.14. These
patches have been posted as an "RFC" because these have been compile tested
only.

Thanks,

Bart.

Bart Van Assche (4):
  ufs: Rename the second ufshcd_probe_hba() argument
  ufs: Remove a check from ufshcd_queuecommand()
  ufs: Improve static type checking for the host controller state
  ufs: Make host controller state change handling more systematic

 drivers/scsi/ufs/ufshcd.c | 96 +++++++++++++++++----------------------
 drivers/scsi/ufs/ufshcd.h | 25 +++++++++-
 2 files changed, 65 insertions(+), 56 deletions(-)

[PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument

[Bart Van Assche]

Rename the second argument of ufshcd_probe_hba() such that the name of
that argument reflects its purpose instead of how the function is called.
See also commit 1b9e21412f72 ("scsi: ufs: Split ufshcd_probe_hba() based
on its called flow").

Cc: Bean Huo <beanhuo@micron.com>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 25fe18a36cd2..c230d2a6a55c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7964,13 +7964,13 @@ static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_probe_hba - probe hba to detect device and initialize
+ * ufshcd_probe_hba - probe hba to detect device and initialize it
  * @hba: per-adapter instance
- * @async: asynchronous execution or not
+ * @init_dev_params: whether or not to call ufshcd_device_params_init().
  *
  * Execute link-startup and verify device initialization
  */
-static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
+static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 {
 	int ret;
 	unsigned long flags;
@@ -8002,7 +8002,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
 	 * Initialize UFS device parameters used by driver, these
 	 * parameters are associated with UFS descriptors.
 	 */
-	if (async) {
+	if (init_dev_params) {
 		ret = ufshcd_device_params_init(hba);
 		if (ret)
 			goto out;

[PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand()

[Bart Van Assche]

scsi_add_host() allocates shost->can_queue tags. ufshcd_init() sets
shost->can_queue to hba->nutrs. In other words, we know that tag values
will be in the range [0, hba->nutrs). Remove a check that verifies what
we already know. This check was introduced by commit 14497328b6a6 ("scsi:
ufs: verify command tag validity").

Cc: Gilad Broner <gbroner@codeaurora.org>
Cc: Yaniv Gardi <ygardi@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Dolev Raviv <draviv@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c230d2a6a55c..71c720d940a3 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2701,21 +2701,11 @@ static void ufshcd_init_lrb(struct ufs_hba *hba, struct ufshcd_lrb *lrb, int i)
  */
 static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
+	struct ufs_hba *hba = shost_priv(host);
+	int tag = cmd->request->tag;
 	struct ufshcd_lrb *lrbp;
-	struct ufs_hba *hba;
-	int tag;
 	int err = 0;
 
-	hba = shost_priv(host);
-
-	tag = cmd->request->tag;
-	if (!ufshcd_valid_tag(hba, tag)) {
-		dev_err(hba->dev,
-			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
-			__func__, tag, cmd, cmd->request);
-		BUG();
-	}
-
 	if (!down_read_trylock(&hba->clk_scaling_lock))
 		return SCSI_MLQUEUE_HOST_BUSY;
 

[PATCH RFC 3/4] ufs: Improve static type checking for the host controller state

[Bart Van Assche]

Assign a name to the enumeration type for UFS host controller states and
remove the default clause from switch statements on this enumeration type
to make the compiler warn about unhandled enumeration labels.

Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 15 ---------------
 drivers/scsi/ufs/ufshcd.h | 25 +++++++++++++++++++++++--
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 71c720d940a3..c213daec20f7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -128,15 +128,6 @@ enum {
 	UFSHCD_CAN_QUEUE	= 32,
 };
 
-/* UFSHCD states */
-enum {
-	UFSHCD_STATE_RESET,
-	UFSHCD_STATE_ERROR,
-	UFSHCD_STATE_OPERATIONAL,
-	UFSHCD_STATE_EH_SCHEDULED_FATAL,
-	UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,
-};
-
 /* UFSHCD error handling flags */
 enum {
 	UFSHCD_EH_IN_PROGRESS = (1 << 0),
@@ -2738,12 +2729,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 		set_host_byte(cmd, DID_ERROR);
 		cmd->scsi_done(cmd);
 		goto out;
-	default:
-		dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
-				__func__, hba->ufshcd_state);
-		set_host_byte(cmd, DID_BAD_TARGET);
-		cmd->scsi_done(cmd);
-		goto out;
 	}
 
 	hba->req_abort_count = 0;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c98d540ac044..f2796ea25598 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -476,6 +476,27 @@ struct ufs_stats {
 	struct ufs_event_hist event[UFS_EVT_CNT];
 };
 
+/**
+ * enum ufshcd_state - UFS host controller state
+ * @UFSHCD_STATE_RESET: Link is not operational. Postpone SCSI command
+ *	processing.
+ * @UFSHCD_STATE_OPERATIONAL: The host controller is operational and can process
+ *	SCSI commands.
+ * @UFSHCD_STATE_EH_SCHEDULED_NON_FATAL: The error handler has been scheduled.
+ *	SCSI commands may be submitted to the controller.
+ * @UFSHCD_STATE_EH_SCHEDULED_FATAL: The error handler has been scheduled. Fail
+ *	newly submitted SCSI commands with error code DID_BAD_TARGET.
+ * @UFSHCD_STATE_ERROR: An unrecoverable error occurred, e.g. link recovery
+ *	failed. Fail all SCSI commands with error code DID_ERROR.
+ */
+enum ufshcd_state {
+	UFSHCD_STATE_RESET,
+	UFSHCD_STATE_OPERATIONAL,
+	UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,
+	UFSHCD_STATE_EH_SCHEDULED_FATAL,
+	UFSHCD_STATE_ERROR,
+};
+
 enum ufshcd_quirks {
 	/* Interrupt aggregation support is broken */
 	UFSHCD_QUIRK_BROKEN_INTR_AGGR			= 1 << 0,
@@ -687,7 +708,7 @@ struct ufs_hba_monitor {
  * @tmf_tag_set: TMF tag set.
  * @tmf_queue: Used to allocate TMF tags.
  * @pwr_done: completion for power mode change
- * @ufshcd_state: UFSHCD states
+ * @ufshcd_state: UFSHCD state
  * @eh_flags: Error handling flags
  * @intr_mask: Interrupt Mask Bits
  * @ee_ctrl_mask: Exception event control mask
@@ -785,7 +806,7 @@ struct ufs_hba {
 	struct mutex uic_cmd_mutex;
 	struct completion *uic_async_done;
 
-	u32 ufshcd_state;
+	enum ufshcd_state ufshcd_state;
 	u32 eh_flags;
 	u32 intr_mask;
 	u16 ee_ctrl_mask; /* Exception event mask */

[PATCH RFC 4/4] ufs: Make host controller state change handling more systematic

[Bart Van Assche]

Introduce a function that reports whether or not a controller state change
is allowed instead of duplicating these checks in every context that
changes the host controller state. This patch includes a behavior change:
ufshcd_link_recovery() no longer can change the host controller state from
UFSHCD_STATE_ERROR into UFSHCD_STATE_RESET.

Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 59 ++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c213daec20f7..a10c8ec28468 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4070,16 +4070,32 @@ static int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 	return ret;
 }
 
+static bool ufshcd_set_state(struct ufs_hba *hba, enum ufshcd_state new_state)
+{
+	lockdep_assert_held(hba->host->host_lock);
+
+	if (old_state != UFSHCD_STATE_ERROR || new_state == UFSHCD_STATE_ERROR)
+		hba->ufshcd_state = new_state;
+		return true;
+	}
+	return false;
+}
+
 int ufshcd_link_recovery(struct ufs_hba *hba)
 {
 	int ret;
 	unsigned long flags;
+	bool proceed;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	hba->ufshcd_state = UFSHCD_STATE_RESET;
-	ufshcd_set_eh_in_progress(hba);
+	proceed = ufshcd_set_state(hba, UFSHCD_STATE_RESET);
+	if (proceed)
+		ufshcd_set_eh_in_progress(hba);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
+	if (!proceed)
+		return -EPERM;
+
 	/* Reset the attached device */
 	ufshcd_device_reset(hba);
 
@@ -4087,7 +4103,7 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (ret)
-		hba->ufshcd_state = UFSHCD_STATE_ERROR;
+		ufshcd_set_state(hba, UFSHCD_STATE_ERROR);
 	ufshcd_clear_eh_in_progress(hba);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
@@ -5856,15 +5872,17 @@ static inline bool ufshcd_is_saved_err_fatal(struct ufs_hba *hba)
 /* host lock must be held before calling this func */
 static inline void ufshcd_schedule_eh_work(struct ufs_hba *hba)
 {
-	/* handle fatal errors only when link is not in error state */
-	if (hba->ufshcd_state != UFSHCD_STATE_ERROR) {
-		if (hba->force_reset || ufshcd_is_link_broken(hba) ||
-		    ufshcd_is_saved_err_fatal(hba))
-			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
-		else
-			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
+	bool proceed;
+
+	if (hba->force_reset || ufshcd_is_link_broken(hba) ||
+	    ufshcd_is_saved_err_fatal(hba))
+		proceed = ufshcd_set_state(hba,
+					   UFSHCD_STATE_EH_SCHEDULED_FATAL);
+	else
+		proceed = ufshcd_set_state(hba,
+					   UFSHCD_STATE_EH_SCHEDULED_NON_FATAL);
+	if (proceed)
 		queue_work(hba->eh_wq, &hba->eh_work);
-	}
 }
 
 static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow)
@@ -6017,8 +6035,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 	down(&hba->host_sem);
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (ufshcd_err_handling_should_stop(hba)) {
-		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
-			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
+		ufshcd_set_state(hba, UFSHCD_STATE_OPERATIONAL);
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
 		up(&hba->host_sem);
 		return;
@@ -6029,8 +6046,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 	/* Complete requests that have door-bell cleared by h/w */
 	ufshcd_complete_requests(hba);
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
-		hba->ufshcd_state = UFSHCD_STATE_RESET;
+	ufshcd_set_state(hba, UFSHCD_STATE_RESET);
 	/*
 	 * A full reset and restore might have happened after preparation
 	 * is finished, double check whether we should stop.
@@ -6163,8 +6179,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 
 skip_err_handling:
 	if (!needs_reset) {
-		if (hba->ufshcd_state == UFSHCD_STATE_RESET)
-			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
+		ufshcd_set_state(hba, UFSHCD_STATE_OPERATIONAL);
 		if (hba->saved_err || hba->saved_uic_err)
 			dev_err_ratelimited(hba->dev, "%s: exit: saved_err 0x%x saved_uic_err 0x%x",
 			    __func__, hba->saved_err, hba->saved_uic_err);
@@ -7135,7 +7150,7 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
 	 */
 	scsi_report_bus_reset(hba->host, 0);
 	if (err) {
-		hba->ufshcd_state = UFSHCD_STATE_ERROR;
+		ufshcd_set_state(hba, UFSHCD_STATE_ERROR);
 		hba->saved_err |= saved_err;
 		hba->saved_uic_err |= saved_uic_err;
 	}
@@ -7951,7 +7966,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 	unsigned long flags;
 	ktime_t start = ktime_get();
 
-	hba->ufshcd_state = UFSHCD_STATE_RESET;
+	ufshcd_set_state(hba, UFSHCD_STATE_RESET);
 
 	ret = ufshcd_link_startup(hba);
 	if (ret)
@@ -8024,10 +8039,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 
 out:
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	if (ret)
-		hba->ufshcd_state = UFSHCD_STATE_ERROR;
-	else if (hba->ufshcd_state == UFSHCD_STATE_RESET)
-		hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
+	ufshcd_set_state(hba, ret ? UFSHCD_STATE_ERROR :
+			 UFSHCD_STATE_OPERATIONAL);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	trace_ufshcd_init(dev_name(hba->dev), ret,

Re: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4158 bytes --]

Hi Bart,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on next-20210618]
[cannot apply to scsi/for-next bvanassche/for-next v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bart-Van-Assche/UFS-patches-for-Linux-kernel-v5-14/20210619-085409
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: powerpc-randconfig-r035-20210618 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d1baf2895467735ab14f4b3415fce204c0cc8e7f)
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
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/fcc87d783f58d9f9ee2c0ccd19c61a57c5941bfb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Bart-Van-Assche/UFS-patches-for-Linux-kernel-v5-14/20210619-085409
        git checkout fcc87d783f58d9f9ee2c0ccd19c61a57c5941bfb
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

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

All error/warnings (new ones prefixed by >>):

   In file included from drivers/scsi/ufs/ufshcd.c:12:
   In file included from include/linux/async.h:12:
   In file included from include/linux/list.h:9:
   In file included from include/linux/kernel.h:12:
   In file included from include/linux/bitops.h:32:
   In file included from arch/powerpc/include/asm/bitops.h:62:
   arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined]
   #define __lwsync()      __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
           ^
   <built-in>:309:9: note: previous definition is here
   #define __lwsync __builtin_ppc_lwsync
           ^
>> drivers/scsi/ufs/ufshcd.c:4077:6: error: use of undeclared identifier 'old_state'
           if (old_state != UFSHCD_STATE_ERROR || new_state == UFSHCD_STATE_ERROR)
               ^
>> drivers/scsi/ufs/ufshcd.c:4079:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
                   return true;
                   ^
   drivers/scsi/ufs/ufshcd.c:4077:2: note: previous statement is here
           if (old_state != UFSHCD_STATE_ERROR || new_state == UFSHCD_STATE_ERROR)
           ^
>> drivers/scsi/ufs/ufshcd.c:4081:2: error: expected identifier or '('
           return false;
           ^
>> drivers/scsi/ufs/ufshcd.c:4082:1: error: extraneous closing brace ('}')
   }
   ^
   drivers/scsi/ufs/ufshcd.c:9361:44: warning: shift count >= width of type [-Wshift-count-overflow]
                   if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
                                                            ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   3 warnings and 3 errors generated.


vim +/old_state +4077 drivers/scsi/ufs/ufshcd.c

  4072	
  4073	static bool ufshcd_set_state(struct ufs_hba *hba, enum ufshcd_state new_state)
  4074	{
  4075		lockdep_assert_held(hba->host->host_lock);
  4076	
> 4077		if (old_state != UFSHCD_STATE_ERROR || new_state == UFSHCD_STATE_ERROR)
  4078			hba->ufshcd_state = new_state;
> 4079			return true;
  4080		}
> 4081		return false;
> 4082	}
  4083	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35987 bytes --]

RE: [PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument

[Avri Altman]

> 
> Rename the second argument of ufshcd_probe_hba() such that the name of
> that argument reflects its purpose instead of how the function is called.
> See also commit 1b9e21412f72 ("scsi: ufs: Split ufshcd_probe_hba() based
> on its called flow").
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>


> ---
>  drivers/scsi/ufs/ufshcd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 25fe18a36cd2..c230d2a6a55c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7964,13 +7964,13 @@ static int ufshcd_clear_ua_wluns(struct ufs_hba
> *hba)
>  }
> 
>  /**
> - * ufshcd_probe_hba - probe hba to detect device and initialize
> + * ufshcd_probe_hba - probe hba to detect device and initialize it
>   * @hba: per-adapter instance
> - * @async: asynchronous execution or not
> + * @init_dev_params: whether or not to call ufshcd_device_params_init().
>   *
>   * Execute link-startup and verify device initialization
>   */
> -static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
> +static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
>  {
>         int ret;
>         unsigned long flags;
> @@ -8002,7 +8002,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool async)
>          * Initialize UFS device parameters used by driver, these
>          * parameters are associated with UFS descriptors.
>          */
> -       if (async) {
> +       if (init_dev_params) {
>                 ret = ufshcd_device_params_init(hba);
>                 if (ret)
>                         goto out;

RE: [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand()

[Avri Altman]

> scsi_add_host() allocates shost->can_queue tags. ufshcd_init() sets
> shost->can_queue to hba->nutrs. In other words, we know that tag values
> will be in the range [0, hba->nutrs). Remove a check that verifies what
> we already know. This check was introduced by commit 14497328b6a6
> ("scsi:
> ufs: verify command tag validity").
> 
> Cc: Gilad Broner <gbroner@codeaurora.org>
> Cc: Yaniv Gardi <ygardi@codeaurora.org>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Dolev Raviv <draviv@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c230d2a6a55c..71c720d940a3 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2701,21 +2701,11 @@ static void ufshcd_init_lrb(struct ufs_hba *hba,
> struct ufshcd_lrb *lrb, int i)
>   */
>  static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd
> *cmd)
>  {
> +       struct ufs_hba *hba = shost_priv(host);
> +       int tag = cmd->request->tag;
>         struct ufshcd_lrb *lrbp;
> -       struct ufs_hba *hba;
> -       int tag;
>         int err = 0;
> 
> -       hba = shost_priv(host);
> -
> -       tag = cmd->request->tag;
> -       if (!ufshcd_valid_tag(hba, tag)) {
> -               dev_err(hba->dev,
> -                       "%s: invalid command tag %d: cmd=0x%p, cmd-
> >request=0x%p",
> -                       __func__, tag, cmd, cmd->request);
> -               BUG();
> -       }
While at it, maybe remove ufshcd_valid_tag altogether?

Thanks,
Avri
> -
>         if (!down_read_trylock(&hba->clk_scaling_lock))
>                 return SCSI_MLQUEUE_HOST_BUSY;

RE: [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state

[Avri Altman]

 
> Assign a name to the enumeration type for UFS host controller states and
> remove the default clause from switch statements on this enumeration type
> to make the compiler warn about unhandled enumeration labels.
> 
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>


> ---
>  drivers/scsi/ufs/ufshcd.c | 15 ---------------
>  drivers/scsi/ufs/ufshcd.h | 25 +++++++++++++++++++++++--
>  2 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 71c720d940a3..c213daec20f7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -128,15 +128,6 @@ enum {
>         UFSHCD_CAN_QUEUE        = 32,
>  };
> 
> -/* UFSHCD states */
> -enum {
> -       UFSHCD_STATE_RESET,
> -       UFSHCD_STATE_ERROR,
> -       UFSHCD_STATE_OPERATIONAL,
> -       UFSHCD_STATE_EH_SCHEDULED_FATAL,
> -       UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,
> -};
> -
>  /* UFSHCD error handling flags */
>  enum {
>         UFSHCD_EH_IN_PROGRESS = (1 << 0),
> @@ -2738,12 +2729,6 @@ static int ufshcd_queuecommand(struct
> Scsi_Host *host, struct scsi_cmnd *cmd)
>                 set_host_byte(cmd, DID_ERROR);
>                 cmd->scsi_done(cmd);
>                 goto out;
> -       default:
> -               dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
> -                               __func__, hba->ufshcd_state);
> -               set_host_byte(cmd, DID_BAD_TARGET);
> -               cmd->scsi_done(cmd);
> -               goto out;
>         }
> 
>         hba->req_abort_count = 0;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c98d540ac044..f2796ea25598 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -476,6 +476,27 @@ struct ufs_stats {
>         struct ufs_event_hist event[UFS_EVT_CNT];
>  };
> 
> +/**
> + * enum ufshcd_state - UFS host controller state
> + * @UFSHCD_STATE_RESET: Link is not operational. Postpone SCSI
> command
> + *     processing.
> + * @UFSHCD_STATE_OPERATIONAL: The host controller is operational and
> can process
> + *     SCSI commands.
> + * @UFSHCD_STATE_EH_SCHEDULED_NON_FATAL: The error handler has
> been scheduled.
> + *     SCSI commands may be submitted to the controller.
> + * @UFSHCD_STATE_EH_SCHEDULED_FATAL: The error handler has been
> scheduled. Fail
> + *     newly submitted SCSI commands with error code DID_BAD_TARGET.
> + * @UFSHCD_STATE_ERROR: An unrecoverable error occurred, e.g. link
> recovery
> + *     failed. Fail all SCSI commands with error code DID_ERROR.
> + */
> +enum ufshcd_state {
> +       UFSHCD_STATE_RESET,
> +       UFSHCD_STATE_OPERATIONAL,
> +       UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,
> +       UFSHCD_STATE_EH_SCHEDULED_FATAL,
> +       UFSHCD_STATE_ERROR,
> +};
> +
>  enum ufshcd_quirks {
>         /* Interrupt aggregation support is broken */
>         UFSHCD_QUIRK_BROKEN_INTR_AGGR                   = 1 << 0,
> @@ -687,7 +708,7 @@ struct ufs_hba_monitor {
>   * @tmf_tag_set: TMF tag set.
>   * @tmf_queue: Used to allocate TMF tags.
>   * @pwr_done: completion for power mode change
> - * @ufshcd_state: UFSHCD states
> + * @ufshcd_state: UFSHCD state
>   * @eh_flags: Error handling flags
>   * @intr_mask: Interrupt Mask Bits
>   * @ee_ctrl_mask: Exception event control mask
> @@ -785,7 +806,7 @@ struct ufs_hba {
>         struct mutex uic_cmd_mutex;
>         struct completion *uic_async_done;
> 
> -       u32 ufshcd_state;
> +       enum ufshcd_state ufshcd_state;
>         u32 eh_flags;
>         u32 intr_mask;
>         u16 ee_ctrl_mask; /* Exception event mask */

RE: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic

[Avri Altman]

> Introduce a function that reports whether or not a controller state change
> is allowed instead of duplicating these checks in every context that
> changes the host controller state. This patch includes a behavior change:
> ufshcd_link_recovery() no longer can change the host controller state from
> UFSHCD_STATE_ERROR into UFSHCD_STATE_RESET.
> 
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 59 ++++++++++++++++++++++++---------------
>  1 file changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c213daec20f7..a10c8ec28468 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4070,16 +4070,32 @@ static int ufshcd_uic_change_pwr_mode(struct
> ufs_hba *hba, u8 mode)
>         return ret;
>  }
> 
> +static bool ufshcd_set_state(struct ufs_hba *hba, enum ufshcd_state
> new_state)
> +{
> +       lockdep_assert_held(hba->host->host_lock);
> +
> +       if (old_state != UFSHCD_STATE_ERROR || new_state ==
> UFSHCD_STATE_ERROR)
old_state ?

Thanks,
Avri

> +               hba->ufshcd_state = new_state;
> +               return true;
> +       }
> +       return false;
> +}
> +
>  int ufshcd_link_recovery(struct ufs_hba *hba)
>  {
>         int ret;
>         unsigned long flags;
> +       bool proceed;
> 
>         spin_lock_irqsave(hba->host->host_lock, flags);
> -       hba->ufshcd_state = UFSHCD_STATE_RESET;
> -       ufshcd_set_eh_in_progress(hba);
> +       proceed = ufshcd_set_state(hba, UFSHCD_STATE_RESET);
> +       if (proceed)
> +               ufshcd_set_eh_in_progress(hba);
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
> +       if (!proceed)
> +               return -EPERM;
> +
>         /* Reset the attached device */
>         ufshcd_device_reset(hba);
> 
> @@ -4087,7 +4103,7 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
> 
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         if (ret)
> -               hba->ufshcd_state = UFSHCD_STATE_ERROR;
> +               ufshcd_set_state(hba, UFSHCD_STATE_ERROR);
>         ufshcd_clear_eh_in_progress(hba);
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
> @@ -5856,15 +5872,17 @@ static inline bool
> ufshcd_is_saved_err_fatal(struct ufs_hba *hba)
>  /* host lock must be held before calling this func */
>  static inline void ufshcd_schedule_eh_work(struct ufs_hba *hba)
>  {
> -       /* handle fatal errors only when link is not in error state */
> -       if (hba->ufshcd_state != UFSHCD_STATE_ERROR) {
> -               if (hba->force_reset || ufshcd_is_link_broken(hba) ||
> -                   ufshcd_is_saved_err_fatal(hba))
> -                       hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
> -               else
> -                       hba->ufshcd_state =
> UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
> +       bool proceed;
> +
> +       if (hba->force_reset || ufshcd_is_link_broken(hba) ||
> +           ufshcd_is_saved_err_fatal(hba))
> +               proceed = ufshcd_set_state(hba,
> +                                          UFSHCD_STATE_EH_SCHEDULED_FATAL);
> +       else
> +               proceed = ufshcd_set_state(hba,
> +                                          UFSHCD_STATE_EH_SCHEDULED_NON_FATAL);
> +       if (proceed)
>                 queue_work(hba->eh_wq, &hba->eh_work);
> -       }
>  }
> 
>  static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow)
> @@ -6017,8 +6035,7 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>         down(&hba->host_sem);
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         if (ufshcd_err_handling_should_stop(hba)) {
> -               if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> -                       hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +               ufshcd_set_state(hba, UFSHCD_STATE_OPERATIONAL);
>                 spin_unlock_irqrestore(hba->host->host_lock, flags);
>                 up(&hba->host_sem);
>                 return;
> @@ -6029,8 +6046,7 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>         /* Complete requests that have door-bell cleared by h/w */
>         ufshcd_complete_requests(hba);
>         spin_lock_irqsave(hba->host->host_lock, flags);
> -       if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> -               hba->ufshcd_state = UFSHCD_STATE_RESET;
> +       ufshcd_set_state(hba, UFSHCD_STATE_RESET);
>         /*
>          * A full reset and restore might have happened after preparation
>          * is finished, double check whether we should stop.
> @@ -6163,8 +6179,7 @@ static void ufshcd_err_handler(struct work_struct
> *work)
> 
>  skip_err_handling:
>         if (!needs_reset) {
> -               if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> -                       hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +               ufshcd_set_state(hba, UFSHCD_STATE_OPERATIONAL);
>                 if (hba->saved_err || hba->saved_uic_err)
>                         dev_err_ratelimited(hba->dev, "%s: exit: saved_err 0x%x
> saved_uic_err 0x%x",
>                             __func__, hba->saved_err, hba->saved_uic_err);
> @@ -7135,7 +7150,7 @@ static int ufshcd_reset_and_restore(struct ufs_hba
> *hba)
>          */
>         scsi_report_bus_reset(hba->host, 0);
>         if (err) {
> -               hba->ufshcd_state = UFSHCD_STATE_ERROR;
> +               ufshcd_set_state(hba, UFSHCD_STATE_ERROR);
>                 hba->saved_err |= saved_err;
>                 hba->saved_uic_err |= saved_uic_err;
>         }
> @@ -7951,7 +7966,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool init_dev_params)
>         unsigned long flags;
>         ktime_t start = ktime_get();
> 
> -       hba->ufshcd_state = UFSHCD_STATE_RESET;
> +       ufshcd_set_state(hba, UFSHCD_STATE_RESET);
> 
>         ret = ufshcd_link_startup(hba);
>         if (ret)
> @@ -8024,10 +8039,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool init_dev_params)
> 
>  out:
>         spin_lock_irqsave(hba->host->host_lock, flags);
> -       if (ret)
> -               hba->ufshcd_state = UFSHCD_STATE_ERROR;
> -       else if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> -               hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +       ufshcd_set_state(hba, ret ? UFSHCD_STATE_ERROR :
> +                        UFSHCD_STATE_OPERATIONAL);
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
>         trace_ufshcd_init(dev_name(hba->dev), ret,

Re: [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand()

[Bart Van Assche]

On 6/21/21 1:22 AM, Avri Altman wrote:
> While at it, maybe remove ufshcd_valid_tag altogether?

I Will do that.

Thanks,

Bart.

Re: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic

[Bart Van Assche]

On 6/21/21 1:55 AM, Avri Altman wrote:
>> +static bool ufshcd_set_state(struct ufs_hba *hba, enum ufshcd_state
>> new_state)
>> +{
>> +       lockdep_assert_held(hba->host->host_lock);
>> +
>> +       if (old_state != UFSHCD_STATE_ERROR || new_state ==
>> UFSHCD_STATE_ERROR)
>
> old_state ?

Thanks for having taken a look. This function should look as follows:

static bool ufshcd_set_state(struct ufs_hba *hba, enum ufshcd_state new_state)
{
	lockdep_assert_held(hba->host->host_lock);

	if (hba->ufshcd_state != UFSHCD_STATE_ERROR ||
	    new_state == UFSHCD_STATE_ERROR) {
		hba->ufshcd_state = new_state;
		return true;
	}
	return false;
}

Re: [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state

[Can Guo]

Hi Bart,

On 2021-06-19 08:52, Bart Van Assche wrote:
> Assign a name to the enumeration type for UFS host controller states 
> and
> remove the default clause from switch statements on this enumeration 
> type
> to make the compiler warn about unhandled enumeration labels.
> 
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 15 ---------------
>  drivers/scsi/ufs/ufshcd.h | 25 +++++++++++++++++++++++--
>  2 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 71c720d940a3..c213daec20f7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -128,15 +128,6 @@ enum {
>  	UFSHCD_CAN_QUEUE	= 32,
>  };
> 
> -/* UFSHCD states */
> -enum {
> -	UFSHCD_STATE_RESET,
> -	UFSHCD_STATE_ERROR,
> -	UFSHCD_STATE_OPERATIONAL,
> -	UFSHCD_STATE_EH_SCHEDULED_FATAL,
> -	UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,
> -};
> -
>  /* UFSHCD error handling flags */
>  enum {
>  	UFSHCD_EH_IN_PROGRESS = (1 << 0),
> @@ -2738,12 +2729,6 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
>  		set_host_byte(cmd, DID_ERROR);
>  		cmd->scsi_done(cmd);
>  		goto out;
> -	default:
> -		dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
> -				__func__, hba->ufshcd_state);
> -		set_host_byte(cmd, DID_BAD_TARGET);
> -		cmd->scsi_done(cmd);
> -		goto out;
>  	}
> 
>  	hba->req_abort_count = 0;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c98d540ac044..f2796ea25598 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -476,6 +476,27 @@ struct ufs_stats {
>  	struct ufs_event_hist event[UFS_EVT_CNT];
>  };
> 
> +/**
> + * enum ufshcd_state - UFS host controller state
> + * @UFSHCD_STATE_RESET: Link is not operational. Postpone SCSI command
> + *	processing.
> + * @UFSHCD_STATE_OPERATIONAL: The host controller is operational and
> can process
> + *	SCSI commands.
> + * @UFSHCD_STATE_EH_SCHEDULED_NON_FATAL: The error handler has been 
> scheduled.
> + *	SCSI commands may be submitted to the controller.
> + * @UFSHCD_STATE_EH_SCHEDULED_FATAL: The error handler has been 
> scheduled. Fail
> + *	newly submitted SCSI commands with error code DID_BAD_TARGET.
> + * @UFSHCD_STATE_ERROR: An unrecoverable error occurred, e.g. link 
> recovery
> + *	failed. Fail all SCSI commands with error code DID_ERROR.
> + */
> +enum ufshcd_state {
> +	UFSHCD_STATE_RESET,
> +	UFSHCD_STATE_OPERATIONAL,
> +	UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,
> +	UFSHCD_STATE_EH_SCHEDULED_FATAL,
> +	UFSHCD_STATE_ERROR,
> +};
> +

Hi Bart,

FYI, in my error handling update change series, I have one change
(https://lore.kernel.org/patchwork/patch/1450656/) which moves the
enumeration from ufshcd.c to ufshcd.h, which shall conflict with
this one. What shall we do?

Thanks,

Can Guo.

>  enum ufshcd_quirks {
>  	/* Interrupt aggregation support is broken */
>  	UFSHCD_QUIRK_BROKEN_INTR_AGGR			= 1 << 0,
> @@ -687,7 +708,7 @@ struct ufs_hba_monitor {
>   * @tmf_tag_set: TMF tag set.
>   * @tmf_queue: Used to allocate TMF tags.
>   * @pwr_done: completion for power mode change
> - * @ufshcd_state: UFSHCD states
> + * @ufshcd_state: UFSHCD state
>   * @eh_flags: Error handling flags
>   * @intr_mask: Interrupt Mask Bits
>   * @ee_ctrl_mask: Exception event control mask
> @@ -785,7 +806,7 @@ struct ufs_hba {
>  	struct mutex uic_cmd_mutex;
>  	struct completion *uic_async_done;
> 
> -	u32 ufshcd_state;
> +	enum ufshcd_state ufshcd_state;
>  	u32 eh_flags;
>  	u32 intr_mask;
>  	u16 ee_ctrl_mask; /* Exception event mask */

Re: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic

[Can Guo]

Hi Bart,

On 2021-06-19 08:52, Bart Van Assche wrote:
> Introduce a function that reports whether or not a controller state 
> change
> is allowed instead of duplicating these checks in every context that
> changes the host controller state. This patch includes a behavior 
> change:
> ufshcd_link_recovery() no longer can change the host controller state 
> from
> UFSHCD_STATE_ERROR into UFSHCD_STATE_RESET.
> 
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 59 ++++++++++++++++++++++++---------------
>  1 file changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c213daec20f7..a10c8ec28468 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4070,16 +4070,32 @@ static int ufshcd_uic_change_pwr_mode(struct
> ufs_hba *hba, u8 mode)
>  	return ret;
>  }
> 
> +static bool ufshcd_set_state(struct ufs_hba *hba, enum ufshcd_state 
> new_state)
> +{
> +	lockdep_assert_held(hba->host->host_lock);
> +
> +	if (old_state != UFSHCD_STATE_ERROR || new_state == 
> UFSHCD_STATE_ERROR)
> +		hba->ufshcd_state = new_state;
> +		return true;
> +	}
> +	return false;
> +}
> +
>  int ufshcd_link_recovery(struct ufs_hba *hba)
>  {
>  	int ret;
>  	unsigned long flags;
> +	bool proceed;
> 
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> -	hba->ufshcd_state = UFSHCD_STATE_RESET;
> -	ufshcd_set_eh_in_progress(hba);
> +	proceed = ufshcd_set_state(hba, UFSHCD_STATE_RESET);
> +	if (proceed)
> +		ufshcd_set_eh_in_progress(hba);
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
> +	if (!proceed)
> +		return -EPERM;
> +
>  	/* Reset the attached device */
>  	ufshcd_device_reset(hba);
> 
> @@ -4087,7 +4103,7 @@ int ufshcd_link_recovery(struct ufs_hba *hba)

I want to remove this function since it is only used by ufs-mediatek.c
through ufshcd_vops_resume(). Althrough it does not race with error
handling as of now, by removing it we can reduce one more caller of
full reset. I will ask Stanley help comment on this.

> 
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	if (ret)
> -		hba->ufshcd_state = UFSHCD_STATE_ERROR;
> +		ufshcd_set_state(hba, UFSHCD_STATE_ERROR);
>  	ufshcd_clear_eh_in_progress(hba);
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
> @@ -5856,15 +5872,17 @@ static inline bool
> ufshcd_is_saved_err_fatal(struct ufs_hba *hba)
>  /* host lock must be held before calling this func */
>  static inline void ufshcd_schedule_eh_work(struct ufs_hba *hba)
>  {
> -	/* handle fatal errors only when link is not in error state */
> -	if (hba->ufshcd_state != UFSHCD_STATE_ERROR) {
> -		if (hba->force_reset || ufshcd_is_link_broken(hba) ||
> -		    ufshcd_is_saved_err_fatal(hba))
> -			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
> -		else
> -			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
> +	bool proceed;
> +
> +	if (hba->force_reset || ufshcd_is_link_broken(hba) ||
> +	    ufshcd_is_saved_err_fatal(hba))
> +		proceed = ufshcd_set_state(hba,
> +					   UFSHCD_STATE_EH_SCHEDULED_FATAL);
> +	else
> +		proceed = ufshcd_set_state(hba,
> +					   UFSHCD_STATE_EH_SCHEDULED_NON_FATAL);
> +	if (proceed)
>  		queue_work(hba->eh_wq, &hba->eh_work);
> -	}
>  }
> 
>  static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow)
> @@ -6017,8 +6035,7 @@ static void ufshcd_err_handler(struct work_struct 
> *work)
>  	down(&hba->host_sem);
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	if (ufshcd_err_handling_should_stop(hba)) {
> -		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> -			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +		ufshcd_set_state(hba, UFSHCD_STATE_OPERATIONAL);
>  		spin_unlock_irqrestore(hba->host->host_lock, flags);
>  		up(&hba->host_sem);
>  		return;
> @@ -6029,8 +6046,7 @@ static void ufshcd_err_handler(struct work_struct 
> *work)
>  	/* Complete requests that have door-bell cleared by h/w */
>  	ufshcd_complete_requests(hba);
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> -	if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> -		hba->ufshcd_state = UFSHCD_STATE_RESET;
> +	ufshcd_set_state(hba, UFSHCD_STATE_RESET);
>  	/*
>  	 * A full reset and restore might have happened after preparation
>  	 * is finished, double check whether we should stop.
> @@ -6163,8 +6179,7 @@ static void ufshcd_err_handler(struct work_struct 
> *work)
> 
>  skip_err_handling:
>  	if (!needs_reset) {
> -		if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> -			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +		ufshcd_set_state(hba, UFSHCD_STATE_OPERATIONAL);
>  		if (hba->saved_err || hba->saved_uic_err)
>  			dev_err_ratelimited(hba->dev, "%s: exit: saved_err 0x%x 
> saved_uic_err 0x%x",
>  			    __func__, hba->saved_err, hba->saved_uic_err);
> @@ -7135,7 +7150,7 @@ static int ufshcd_reset_and_restore(struct 
> ufs_hba *hba)
>  	 */
>  	scsi_report_bus_reset(hba->host, 0);
>  	if (err) {
> -		hba->ufshcd_state = UFSHCD_STATE_ERROR;
> +		ufshcd_set_state(hba, UFSHCD_STATE_ERROR);
>  		hba->saved_err |= saved_err;
>  		hba->saved_uic_err |= saved_uic_err;
>  	}
> @@ -7951,7 +7966,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool init_dev_params)
>  	unsigned long flags;
>  	ktime_t start = ktime_get();
> 
> -	hba->ufshcd_state = UFSHCD_STATE_RESET;
> +	ufshcd_set_state(hba, UFSHCD_STATE_RESET);

1. We don't hold host lock here, but ufshcd_set_state() is expecting it.

2. Here we need to override the state to RESET anyways even state is 
ERROR,
because in ufshcd_reset_and_restore(), ufshcd_probe_hba() can be called 
5
times (retries == 5, see below code snippet). Otherwise, say on the 2nd
retry of ufshcd_probe_hba(), even if ufshcd_probe_hba() pass through 
without
an error (ret == 0), the ufshcd_set_state() (down below) cannot set the
state back to OPERATIONAL.

7042 	do {
7043 		/* Reset the attached device */
7044 		ufshcd_vops_device_reset(hba);
7045
7046 		err = ufshcd_host_reset_and_restore(hba);
7047 	} while (err && --retries);

Thanks,

Can Guo.

> 
>  	ret = ufshcd_link_startup(hba);
>  	if (ret)
> @@ -8024,10 +8039,8 @@ static int ufshcd_probe_hba(struct ufs_hba
> *hba, bool init_dev_params)
> 
>  out:
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> -	if (ret)
> -		hba->ufshcd_state = UFSHCD_STATE_ERROR;
> -	else if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> -		hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +	ufshcd_set_state(hba, ret ? UFSHCD_STATE_ERROR :
> +			 UFSHCD_STATE_OPERATIONAL);
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
>  	trace_ufshcd_init(dev_name(hba->dev), ret,

Re: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic

[Can Guo]

Hi Bart,

On 2021-06-19 08:52, Bart Van Assche wrote:
> Introduce a function that reports whether or not a controller state 
> change
> is allowed instead of duplicating these checks in every context that
> changes the host controller state. This patch includes a behavior 
> change:
> ufshcd_link_recovery() no longer can change the host controller state 
> from
> UFSHCD_STATE_ERROR into UFSHCD_STATE_RESET.
> 
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 59 ++++++++++++++++++++++++---------------
>  1 file changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c213daec20f7..a10c8ec28468 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4070,16 +4070,32 @@ static int ufshcd_uic_change_pwr_mode(struct
> ufs_hba *hba, u8 mode)
>  	return ret;
>  }
> 
> +static bool ufshcd_set_state(struct ufs_hba *hba, enum ufshcd_state 
> new_state)
> +{
> +	lockdep_assert_held(hba->host->host_lock);
> +
> +	if (old_state != UFSHCD_STATE_ERROR || new_state == 
> UFSHCD_STATE_ERROR)
> +		hba->ufshcd_state = new_state;
> +		return true;
> +	}
> +	return false;
> +}
> +
>  int ufshcd_link_recovery(struct ufs_hba *hba)
>  {
>  	int ret;
>  	unsigned long flags;
> +	bool proceed;
> 
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> -	hba->ufshcd_state = UFSHCD_STATE_RESET;
> -	ufshcd_set_eh_in_progress(hba);
> +	proceed = ufshcd_set_state(hba, UFSHCD_STATE_RESET);
> +	if (proceed)
> +		ufshcd_set_eh_in_progress(hba);
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
> +	if (!proceed)
> +		return -EPERM;
> +
>  	/* Reset the attached device */
>  	ufshcd_device_reset(hba);
> 
> @@ -4087,7 +4103,7 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
> 
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	if (ret)
> -		hba->ufshcd_state = UFSHCD_STATE_ERROR;
> +		ufshcd_set_state(hba, UFSHCD_STATE_ERROR);
>  	ufshcd_clear_eh_in_progress(hba);
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
> @@ -5856,15 +5872,17 @@ static inline bool
> ufshcd_is_saved_err_fatal(struct ufs_hba *hba)
>  /* host lock must be held before calling this func */
>  static inline void ufshcd_schedule_eh_work(struct ufs_hba *hba)
>  {
> -	/* handle fatal errors only when link is not in error state */
> -	if (hba->ufshcd_state != UFSHCD_STATE_ERROR) {
> -		if (hba->force_reset || ufshcd_is_link_broken(hba) ||
> -		    ufshcd_is_saved_err_fatal(hba))
> -			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
> -		else
> -			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
> +	bool proceed;
> +
> +	if (hba->force_reset || ufshcd_is_link_broken(hba) ||
> +	    ufshcd_is_saved_err_fatal(hba))
> +		proceed = ufshcd_set_state(hba,
> +					   UFSHCD_STATE_EH_SCHEDULED_FATAL);
> +	else
> +		proceed = ufshcd_set_state(hba,
> +					   UFSHCD_STATE_EH_SCHEDULED_NON_FATAL);
> +	if (proceed)
>  		queue_work(hba->eh_wq, &hba->eh_work);
> -	}
>  }
> 
>  static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow)
> @@ -6017,8 +6035,7 @@ static void ufshcd_err_handler(struct work_struct 
> *work)
>  	down(&hba->host_sem);
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	if (ufshcd_err_handling_should_stop(hba)) {
> -		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> -			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +		ufshcd_set_state(hba, UFSHCD_STATE_OPERATIONAL);
>  		spin_unlock_irqrestore(hba->host->host_lock, flags);
>  		up(&hba->host_sem);
>  		return;
> @@ -6029,8 +6046,7 @@ static void ufshcd_err_handler(struct work_struct 
> *work)
>  	/* Complete requests that have door-bell cleared by h/w */
>  	ufshcd_complete_requests(hba);
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> -	if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> -		hba->ufshcd_state = UFSHCD_STATE_RESET;
> +	ufshcd_set_state(hba, UFSHCD_STATE_RESET);
>  	/*
>  	 * A full reset and restore might have happened after preparation
>  	 * is finished, double check whether we should stop.
> @@ -6163,8 +6179,7 @@ static void ufshcd_err_handler(struct work_struct 
> *work)
> 
>  skip_err_handling:
>  	if (!needs_reset) {
> -		if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> -			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;

State transition to OPERATIONAL should only be from RESET, this
is to make sure that we don't miss any new error (fatal or non-fatal)
occurs while error handling is running.

> +		ufshcd_set_state(hba, UFSHCD_STATE_OPERATIONAL);
>  		if (hba->saved_err || hba->saved_uic_err)
>  			dev_err_ratelimited(hba->dev, "%s: exit: saved_err 0x%x 
> saved_uic_err 0x%x",
>  			    __func__, hba->saved_err, hba->saved_uic_err);
> @@ -7135,7 +7150,7 @@ static int ufshcd_reset_and_restore(struct 
> ufs_hba *hba)
>  	 */
>  	scsi_report_bus_reset(hba->host, 0);
>  	if (err) {
> -		hba->ufshcd_state = UFSHCD_STATE_ERROR;
> +		ufshcd_set_state(hba, UFSHCD_STATE_ERROR);
>  		hba->saved_err |= saved_err;
>  		hba->saved_uic_err |= saved_uic_err;
>  	}
> @@ -7951,7 +7966,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool init_dev_params)
>  	unsigned long flags;
>  	ktime_t start = ktime_get();
> 
> -	hba->ufshcd_state = UFSHCD_STATE_RESET;
> +	ufshcd_set_state(hba, UFSHCD_STATE_RESET);
> 
>  	ret = ufshcd_link_startup(hba);
>  	if (ret)
> @@ -8024,10 +8039,8 @@ static int ufshcd_probe_hba(struct ufs_hba
> *hba, bool init_dev_params)
> 
>  out:
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> -	if (ret)
> -		hba->ufshcd_state = UFSHCD_STATE_ERROR;
> -	else if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> -		hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;

State transition to OPERATIONAL should only be from RESET, this
is to make sure that we don't miss any new error (fatal or non-fatal)
occurs during re-probe.

Thanks,

Can Guo.

> +	ufshcd_set_state(hba, ret ? UFSHCD_STATE_ERROR :
> +			 UFSHCD_STATE_OPERATIONAL);
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
>  	trace_ufshcd_init(dev_name(hba->dev), ret,

Re: [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state

[Bart Van Assche]

On 6/23/21 12:42 AM, Can Guo wrote:
> FYI, in my error handling update change series, I have one change
> (https://lore.kernel.org/patchwork/patch/1450656/) which moves the
> enumeration from ufshcd.c to ufshcd.h, which shall conflict with
> this one. What shall we do?

Hi Can,

Thanks for asking. I will rebase my patch series on top of yours.

Bart.