[PATCH] scsi: ufs: Introduce ufshcd_abort_all()

[PATCH] scsi: ufs: Introduce ufshcd_abort_all()

[Bart Van Assche]

Move the code for aborting all SCSI commands and TMFs into a new function.
This patch makes the ufshcd_err_handler() easier to read. Except for adding
more logging, this patch does not change any functionality.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 62 +++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ee73d7036133..05feec10fad3 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6157,6 +6157,38 @@ static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba)
 	return false;
 }
 
+static bool ufshcd_abort_all(struct ufs_hba *hba)
+{
+	bool needs_reset = false;
+	unsigned int tag, ret;
+
+	/* Clear pending transfer requests */
+	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
+		ret = ufshcd_try_to_abort_task(hba, tag);
+		dev_err(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag,
+			hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1,
+			ret ? "failed" : "succeeded");
+		if (ret) {
+			needs_reset = true;
+			goto out;
+		}
+	}
+
+	/* Clear pending task management requests */
+	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
+		if (ufshcd_clear_tm_cmd(hba, tag)) {
+			needs_reset = true;
+			goto out;
+		}
+	}
+
+out:
+	/* Complete the requests that are cleared by s/w */
+	ufshcd_complete_requests(hba);
+
+	return needs_reset;
+}
+
 /**
  * ufshcd_err_handler - handle UFS errors that require s/w attention
  * @work: pointer to work structure
@@ -6168,10 +6200,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 	unsigned long flags;
 	bool needs_restore;
 	bool needs_reset;
-	bool err_xfer;
-	bool err_tm;
 	int pmc_err;
-	int tag;
 
 	hba = container_of(work, struct ufs_hba, eh_work);
 
@@ -6200,8 +6229,6 @@ static void ufshcd_err_handler(struct work_struct *work)
 again:
 	needs_restore = false;
 	needs_reset = false;
-	err_xfer = false;
-	err_tm = false;
 
 	if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
 		hba->ufshcd_state = UFSHCD_STATE_RESET;
@@ -6270,34 +6297,13 @@ static void ufshcd_err_handler(struct work_struct *work)
 	hba->silence_err_logs = true;
 	/* release lock as clear command might sleep */
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	/* Clear pending transfer requests */
-	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
-		if (ufshcd_try_to_abort_task(hba, tag)) {
-			err_xfer = true;
-			goto lock_skip_pending_xfer_clear;
-		}
-		dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag,
-			hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1);
-	}
 
-	/* Clear pending task management requests */
-	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
-		if (ufshcd_clear_tm_cmd(hba, tag)) {
-			err_tm = true;
-			goto lock_skip_pending_xfer_clear;
-		}
-	}
-
-lock_skip_pending_xfer_clear:
-	/* Complete the requests that are cleared by s/w */
-	ufshcd_complete_requests(hba);
+	needs_reset = ufshcd_abort_all(hba);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->silence_err_logs = false;
-	if (err_xfer || err_tm) {
-		needs_reset = true;
+	if (needs_reset)
 		goto do_reset;
-	}
 
 	/*
 	 * After all reqs and tasks are cleared from doorbell,

Re: [PATCH] scsi: ufs: Introduce ufshcd_abort_all()

[Adrian Hunter]

On 27/10/22 23:43, Bart Van Assche wrote:
> Move the code for aborting all SCSI commands and TMFs into a new function.
> This patch makes the ufshcd_err_handler() easier to read. Except for adding
> more logging, this patch does not change any functionality.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Comment below, otherwise:

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/ufs/core/ufshcd.c | 62 +++++++++++++++++++++------------------
>  1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ee73d7036133..05feec10fad3 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6157,6 +6157,38 @@ static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba)
>  	return false;
>  }
>  
> +static bool ufshcd_abort_all(struct ufs_hba *hba)
> +{
> +	bool needs_reset = false;
> +	unsigned int tag, ret;

'ret' is normally 'int'

Also we seem to use 'int' for 'tag' a lot and it would match the
dev_err %d

> +
> +	/* Clear pending transfer requests */
> +	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
> +		ret = ufshcd_try_to_abort_task(hba, tag);
> +		dev_err(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag,
> +			hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1,
> +			ret ? "failed" : "succeeded");
> +		if (ret) {
> +			needs_reset = true;
> +			goto out;
> +		}
> +	}
> +
> +	/* Clear pending task management requests */
> +	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
> +		if (ufshcd_clear_tm_cmd(hba, tag)) {
> +			needs_reset = true;
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	/* Complete the requests that are cleared by s/w */
> +	ufshcd_complete_requests(hba);
> +
> +	return needs_reset;
> +}
> +
>  /**
>   * ufshcd_err_handler - handle UFS errors that require s/w attention
>   * @work: pointer to work structure
> @@ -6168,10 +6200,7 @@ static void ufshcd_err_handler(struct work_struct *work)
>  	unsigned long flags;
>  	bool needs_restore;
>  	bool needs_reset;
> -	bool err_xfer;
> -	bool err_tm;
>  	int pmc_err;
> -	int tag;
>  
>  	hba = container_of(work, struct ufs_hba, eh_work);
>  
> @@ -6200,8 +6229,6 @@ static void ufshcd_err_handler(struct work_struct *work)
>  again:
>  	needs_restore = false;
>  	needs_reset = false;
> -	err_xfer = false;
> -	err_tm = false;
>  
>  	if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
>  		hba->ufshcd_state = UFSHCD_STATE_RESET;
> @@ -6270,34 +6297,13 @@ static void ufshcd_err_handler(struct work_struct *work)
>  	hba->silence_err_logs = true;
>  	/* release lock as clear command might sleep */
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> -	/* Clear pending transfer requests */
> -	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
> -		if (ufshcd_try_to_abort_task(hba, tag)) {
> -			err_xfer = true;
> -			goto lock_skip_pending_xfer_clear;
> -		}
> -		dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag,
> -			hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1);
> -	}
>  
> -	/* Clear pending task management requests */
> -	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
> -		if (ufshcd_clear_tm_cmd(hba, tag)) {
> -			err_tm = true;
> -			goto lock_skip_pending_xfer_clear;
> -		}
> -	}
> -
> -lock_skip_pending_xfer_clear:
> -	/* Complete the requests that are cleared by s/w */
> -	ufshcd_complete_requests(hba);
> +	needs_reset = ufshcd_abort_all(hba);
>  
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	hba->silence_err_logs = false;
> -	if (err_xfer || err_tm) {
> -		needs_reset = true;
> +	if (needs_reset)
>  		goto do_reset;
> -	}
>  
>  	/*
>  	 * After all reqs and tasks are cleared from doorbell,