All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC 0/4] UFS patches for Linux kernel v5.14
@ 2021-06-19  0:52 Bart Van Assche
  2021-06-19  0:52 ` [PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument Bart Van Assche
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Bart Van Assche @ 2021-06-19  0:52 UTC (permalink / raw)
  To: Martin K . Petersen; +Cc: linux-scsi, 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(-)


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument
  2021-06-19  0:52 [PATCH RFC 0/4] UFS patches for Linux kernel v5.14 Bart Van Assche
@ 2021-06-19  0:52 ` Bart Van Assche
  2021-06-21  8:13   ` Avri Altman
  2021-06-19  0:52 ` [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand() Bart Van Assche
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Bart Van Assche @ 2021-06-19  0:52 UTC (permalink / raw)
  To: Martin K . Petersen
  Cc: linux-scsi, Bart Van Assche, Bean Huo, Asutosh Das, Can Guo,
	James E.J. Bottomley, Stanley Chu, Avri Altman, Jaegeuk Kim

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;

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand()
  2021-06-19  0:52 [PATCH RFC 0/4] UFS patches for Linux kernel v5.14 Bart Van Assche
  2021-06-19  0:52 ` [PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument Bart Van Assche
@ 2021-06-19  0:52 ` Bart Van Assche
  2021-06-21  8:22   ` Avri Altman
  2021-06-19  0:52 ` [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state Bart Van Assche
  2021-06-19  0:52 ` [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic Bart Van Assche
  3 siblings, 1 reply; 15+ messages in thread
From: Bart Van Assche @ 2021-06-19  0:52 UTC (permalink / raw)
  To: Martin K . Petersen
  Cc: linux-scsi, Bart Van Assche, Gilad Broner, Yaniv Gardi,
	Subhash Jadavani, Dolev Raviv, James E.J. Bottomley, Can Guo,
	Stanley Chu, Bean Huo, Avri Altman, Jaegeuk Kim, Asutosh Das

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;
 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state
  2021-06-19  0:52 [PATCH RFC 0/4] UFS patches for Linux kernel v5.14 Bart Van Assche
  2021-06-19  0:52 ` [PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument Bart Van Assche
  2021-06-19  0:52 ` [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand() Bart Van Assche
@ 2021-06-19  0:52 ` Bart Van Assche
  2021-06-21  8:26   ` Avri Altman
  2021-06-23  7:42   ` Can Guo
  2021-06-19  0:52 ` [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic Bart Van Assche
  3 siblings, 2 replies; 15+ messages in thread
From: Bart Van Assche @ 2021-06-19  0:52 UTC (permalink / raw)
  To: Martin K . Petersen
  Cc: linux-scsi, Bart Van Assche, Can Guo, James E.J. Bottomley,
	Stanley Chu, Bean Huo, Avri Altman, Jaegeuk Kim, Asutosh Das,
	Adrian Hunter, Kiwoong Kim

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 */

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic
  2021-06-19  0:52 [PATCH RFC 0/4] UFS patches for Linux kernel v5.14 Bart Van Assche
                   ` (2 preceding siblings ...)
  2021-06-19  0:52 ` [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state Bart Van Assche
@ 2021-06-19  0:52 ` Bart Van Assche
  2021-06-21  8:55   ` Avri Altman
                     ` (2 more replies)
  3 siblings, 3 replies; 15+ messages in thread
From: Bart Van Assche @ 2021-06-19  0:52 UTC (permalink / raw)
  To: Martin K . Petersen
  Cc: linux-scsi, Bart Van Assche, Can Guo, James E.J. Bottomley,
	Stanley Chu, Bean Huo, Avri Altman, Jaegeuk Kim, Asutosh Das

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,

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument
  2021-06-19  0:52 ` [PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument Bart Van Assche
@ 2021-06-21  8:13   ` Avri Altman
  0 siblings, 0 replies; 15+ messages in thread
From: Avri Altman @ 2021-06-21  8:13 UTC (permalink / raw)
  To: Bart Van Assche, Martin K . Petersen
  Cc: linux-scsi, Bean Huo, Asutosh Das, Can Guo, James E.J. Bottomley,
	Stanley Chu, Jaegeuk Kim

> 
> 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;

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand()
  2021-06-19  0:52 ` [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand() Bart Van Assche
@ 2021-06-21  8:22   ` Avri Altman
  2021-06-21 17:31     ` Bart Van Assche
  0 siblings, 1 reply; 15+ messages in thread
From: Avri Altman @ 2021-06-21  8:22 UTC (permalink / raw)
  To: Bart Van Assche, Martin K . Petersen
  Cc: linux-scsi, Gilad Broner, Yaniv Gardi, Subhash Jadavani,
	Dolev Raviv, James E.J. Bottomley, Can Guo, Stanley Chu,
	Bean Huo, Jaegeuk Kim, Asutosh Das

> 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;


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state
  2021-06-19  0:52 ` [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state Bart Van Assche
@ 2021-06-21  8:26   ` Avri Altman
  2021-06-23  7:42   ` Can Guo
  1 sibling, 0 replies; 15+ messages in thread
From: Avri Altman @ 2021-06-21  8:26 UTC (permalink / raw)
  To: Bart Van Assche, Martin K . Petersen
  Cc: linux-scsi, Can Guo, James E.J. Bottomley, Stanley Chu, Bean Huo,
	Jaegeuk Kim, Asutosh Das, Adrian Hunter, Kiwoong Kim

 
> 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 */

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic
  2021-06-19  0:52 ` [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic Bart Van Assche
@ 2021-06-21  8:55   ` Avri Altman
  2021-06-21 17:38     ` Bart Van Assche
  2021-06-23  8:08   ` Can Guo
  2021-06-23 12:02   ` Can Guo
  2 siblings, 1 reply; 15+ messages in thread
From: Avri Altman @ 2021-06-21  8:55 UTC (permalink / raw)
  To: Bart Van Assche, Martin K . Petersen
  Cc: linux-scsi, Can Guo, James E.J. Bottomley, Stanley Chu, Bean Huo,
	Jaegeuk Kim, Asutosh Das

> 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,

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand()
  2021-06-21  8:22   ` Avri Altman
@ 2021-06-21 17:31     ` Bart Van Assche
  0 siblings, 0 replies; 15+ messages in thread
From: Bart Van Assche @ 2021-06-21 17:31 UTC (permalink / raw)
  To: Avri Altman, Martin K . Petersen
  Cc: linux-scsi, Gilad Broner, Yaniv Gardi, Subhash Jadavani,
	Dolev Raviv, James E.J. Bottomley, Can Guo, Stanley Chu,
	Bean Huo, Jaegeuk Kim, Asutosh Das

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic
  2021-06-21  8:55   ` Avri Altman
@ 2021-06-21 17:38     ` Bart Van Assche
  0 siblings, 0 replies; 15+ messages in thread
From: Bart Van Assche @ 2021-06-21 17:38 UTC (permalink / raw)
  To: Avri Altman, Martin K . Petersen
  Cc: linux-scsi, Can Guo, James E.J. Bottomley, Stanley Chu, Bean Huo,
	Jaegeuk Kim, Asutosh Das

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;
}

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state
  2021-06-19  0:52 ` [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state Bart Van Assche
  2021-06-21  8:26   ` Avri Altman
@ 2021-06-23  7:42   ` Can Guo
  2021-06-23 16:10     ` Bart Van Assche
  1 sibling, 1 reply; 15+ messages in thread
From: Can Guo @ 2021-06-23  7:42 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Martin K . Petersen, linux-scsi, James E.J. Bottomley,
	Stanley Chu, Bean Huo, Avri Altman, Jaegeuk Kim, Asutosh Das,
	Adrian Hunter, Kiwoong Kim

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 */

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic
  2021-06-19  0:52 ` [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic Bart Van Assche
  2021-06-21  8:55   ` Avri Altman
@ 2021-06-23  8:08   ` Can Guo
  2021-06-23 12:02   ` Can Guo
  2 siblings, 0 replies; 15+ messages in thread
From: Can Guo @ 2021-06-23  8:08 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Martin K . Petersen, linux-scsi, James E.J. Bottomley,
	Stanley Chu, Bean Huo, Avri Altman, Jaegeuk Kim, Asutosh Das

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,

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic
  2021-06-19  0:52 ` [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic Bart Van Assche
  2021-06-21  8:55   ` Avri Altman
  2021-06-23  8:08   ` Can Guo
@ 2021-06-23 12:02   ` Can Guo
  2 siblings, 0 replies; 15+ messages in thread
From: Can Guo @ 2021-06-23 12:02 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Martin K . Petersen, linux-scsi, James E.J. Bottomley,
	Stanley Chu, Bean Huo, Avri Altman, Jaegeuk Kim, Asutosh Das

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,

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state
  2021-06-23  7:42   ` Can Guo
@ 2021-06-23 16:10     ` Bart Van Assche
  0 siblings, 0 replies; 15+ messages in thread
From: Bart Van Assche @ 2021-06-23 16:10 UTC (permalink / raw)
  To: Can Guo
  Cc: Martin K . Petersen, linux-scsi, James E.J. Bottomley,
	Stanley Chu, Bean Huo, Avri Altman, Jaegeuk Kim, Asutosh Das,
	Adrian Hunter, Kiwoong Kim

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-06-23 16:10 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-19  0:52 [PATCH RFC 0/4] UFS patches for Linux kernel v5.14 Bart Van Assche
2021-06-19  0:52 ` [PATCH RFC 1/4] ufs: Rename the second ufshcd_probe_hba() argument Bart Van Assche
2021-06-21  8:13   ` Avri Altman
2021-06-19  0:52 ` [PATCH RFC 2/4] ufs: Remove a check from ufshcd_queuecommand() Bart Van Assche
2021-06-21  8:22   ` Avri Altman
2021-06-21 17:31     ` Bart Van Assche
2021-06-19  0:52 ` [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state Bart Van Assche
2021-06-21  8:26   ` Avri Altman
2021-06-23  7:42   ` Can Guo
2021-06-23 16:10     ` Bart Van Assche
2021-06-19  0:52 ` [PATCH RFC 4/4] ufs: Make host controller state change handling more systematic Bart Van Assche
2021-06-21  8:55   ` Avri Altman
2021-06-21 17:38     ` Bart Van Assche
2021-06-23  8:08   ` Can Guo
2021-06-23 12:02   ` Can Guo

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.