[PATCH 0/5] UFS patches for kernel v5.16

[PATCH 0/5] UFS patches for kernel v5.16

[Bart Van Assche]

Hi Martin,

This patch series contains the UFS driver kernel patches I would like to see
included kernel v5.16. Please consider these patches for kernel v5.16.

Thank you,

Bart.

Bart Van Assche (5):
  scsi: ufs: Revert "Retry aborted SCSI commands instead of completing
    these successfully"
  scsi: ufs: Improve source code comments
  scsi: ufs: Improve static type checking
  scsi: ufs: Log error handler activity
  scsi: ufs: Add a sysfs attribute for triggering the UFS EH

 Documentation/ABI/testing/sysfs-driver-ufs |  10 ++
 drivers/scsi/ufs/ufshcd.c                  | 105 +++++++++++++++------
 drivers/scsi/ufs/ufshci.h                  |   9 +-
 3 files changed, 94 insertions(+), 30 deletions(-)

[PATCH 1/5] scsi: ufs: Revert "Retry aborted SCSI commands instead of completing these successfully"

[Bart Van Assche]

Commit 73dc3c4ac703 ("scsi: ufs: Retry aborted SCSI commands instead of
completing these successfully") is not necessary. If a SCSI command is
aborted successfully the UFS controller has not modified the command
status and the command status still has the value assigned by
ufshcd_prepare_req_desc_hdr(), namely OCS_INVALID_COMMAND_STATUS. The
function ufshcd_transfer_rsp_status() requeues commands that have an
invalid command status. Hence revert commit 73dc3c4ac703.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d91a405fd181..a89fe61cb8cf 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5266,12 +5266,10 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 /**
  * __ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
- * @completed_reqs: bitmask that indicates which requests to complete
- * @retry_requests: whether to ask the SCSI core to retry completed requests
+ * @completed_reqs: requests to complete
  */
 static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
-					unsigned long completed_reqs,
-					bool retry_requests)
+					unsigned long completed_reqs)
 {
 	struct ufshcd_lrb *lrbp;
 	struct scsi_cmnd *cmd;
@@ -5287,8 +5285,7 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 			if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
 				ufshcd_update_monitor(hba, lrbp);
 			ufshcd_add_command_trace(hba, index, UFS_CMD_COMP);
-			result = retry_requests ? DID_BUS_BUSY << 16 :
-				ufshcd_transfer_rsp_status(hba, lrbp);
+			result = ufshcd_transfer_rsp_status(hba, lrbp);
 			scsi_dma_unmap(cmd);
 			cmd->result = result;
 			/* Mark completed command as NULL in LRB */
@@ -5314,14 +5311,12 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 /**
  * ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
- * @retry_requests: whether or not to ask to retry requests
  *
  * Returns
  *  IRQ_HANDLED - If interrupt is valid
  *  IRQ_NONE    - If invalid interrupt
  */
-static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba,
-					     bool retry_requests)
+static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 {
 	unsigned long completed_reqs, flags;
 	u32 tr_doorbell;
@@ -5350,8 +5345,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba,
 	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
 
 	if (completed_reqs) {
-		__ufshcd_transfer_req_compl(hba, completed_reqs,
-					    retry_requests);
+		__ufshcd_transfer_req_compl(hba, completed_reqs);
 		return IRQ_HANDLED;
 	} else {
 		return IRQ_NONE;
@@ -5850,13 +5844,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 /* Complete requests that have door-bell cleared */
 static void ufshcd_complete_requests(struct ufs_hba *hba)
 {
-	ufshcd_transfer_req_compl(hba, /*retry_requests=*/false);
-	ufshcd_tmc_handler(hba);
-}
-
-static void ufshcd_retry_aborted_requests(struct ufs_hba *hba)
-{
-	ufshcd_transfer_req_compl(hba, /*retry_requests=*/true);
+	ufshcd_transfer_req_compl(hba);
 	ufshcd_tmc_handler(hba);
 }
 
@@ -6183,7 +6171,8 @@ static void ufshcd_err_handler(struct Scsi_Host *host)
 	}
 
 lock_skip_pending_xfer_clear:
-	ufshcd_retry_aborted_requests(hba);
+	/* Complete the requests that are cleared by s/w */
+	ufshcd_complete_requests(hba);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->silence_err_logs = false;
@@ -6493,7 +6482,7 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 		retval |= ufshcd_tmc_handler(hba);
 
 	if (intr_status & UTP_TRANSFER_REQ_COMPL)
-		retval |= ufshcd_transfer_req_compl(hba, /*retry_requests=*/false);
+		retval |= ufshcd_transfer_req_compl(hba);
 
 	return retval;
 }
@@ -6912,7 +6901,7 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
 			err = ufshcd_clear_cmd(hba, pos);
 			if (err)
 				break;
-			__ufshcd_transfer_req_compl(hba, pos, /*retry_requests=*/true);
+			__ufshcd_transfer_req_compl(hba, pos);
 		}
 	}
 
@@ -7074,7 +7063,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		dev_err(hba->dev,
 		"%s: cmd was completed, but without a notifying intr, tag = %d",
 		__func__, tag);
-		__ufshcd_transfer_req_compl(hba, 1UL << tag, /*retry_requests=*/false);
+		__ufshcd_transfer_req_compl(hba, 1UL << tag);
 		goto release;
 	}
 
@@ -7142,7 +7131,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	ufshpb_reset_host(hba);
 	ufshcd_hba_stop(hba);
 	hba->silence_err_logs = true;
-	ufshcd_retry_aborted_requests(hba);
+	ufshcd_complete_requests(hba);
 	hba->silence_err_logs = false;
 
 	/* scale up clocks to max frequency before full reinitialization */

[PATCH 2/5] scsi: ufs: Improve source code comments

[Bart Van Assche]

Make the descriptions above data structures that come from the UFS
specification match the terminology from that specification. This makes
it easier to find these data structures while reading the UFS
specification.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshci.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index de95be5d11d4..9a754fab8908 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -425,7 +425,7 @@ struct ufshcd_sg_entry {
 };
 
 /**
- * struct utp_transfer_cmd_desc - UFS Command Descriptor structure
+ * struct utp_transfer_cmd_desc - UTP Command Descriptor (UCD)
  * @command_upiu: Command UPIU Frame address
  * @response_upiu: Response UPIU Frame address
  * @prd_table: Physical Region Descriptor
@@ -451,7 +451,7 @@ struct request_desc_header {
 };
 
 /**
- * struct utp_transfer_req_desc - UTRD structure
+ * struct utp_transfer_req_desc - UTP Transfer Request Descriptor (UTRD)
  * @header: UTRD header DW-0 to DW-3
  * @command_desc_base_addr_lo: UCD base address low DW-4
  * @command_desc_base_addr_hi: UCD base address high DW-5

[PATCH 3/5] scsi: ufs: Improve static type checking

[Bart Van Assche]

Introduce an enumeration type for the overall command status to allow the
compiler to perform more static type checking.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 9 +++++----
 drivers/scsi/ufs/ufshci.h | 5 ++++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a89fe61cb8cf..cb55ba3cb3e6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -713,7 +713,7 @@ static inline bool ufshcd_is_device_present(struct ufs_hba *hba)
  * This function is used to get the OCS field from UTRD
  * Returns the OCS field in the UTRD
  */
-static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
+static enum utp_ocs ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
 {
 	return le32_to_cpu(lrbp->utr_descriptor_ptr->header.dword_2) & MASK_OCS;
 }
@@ -5108,7 +5108,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 {
 	int result = 0;
 	int scsi_status;
-	int ocs;
+	enum utp_ocs ocs;
 
 	/* overall command status of utrd */
 	ocs = ufshcd_get_tr_ocs(lrbp);
@@ -6645,7 +6645,8 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
 		u8 tm_function, u8 *tm_response)
 {
 	struct utp_task_req_desc treq = { { 0 }, };
-	int ocs_value, err;
+	enum utp_ocs ocs_value;
+	int err;
 
 	/* Configure task request descriptor */
 	treq.header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
@@ -6823,7 +6824,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 	int err;
 	enum dev_cmd_type cmd_type = DEV_CMD_TYPE_QUERY;
 	struct utp_task_req_desc treq = { { 0 }, };
-	int ocs_value;
+	enum utp_ocs ocs_value;
 	u8 tm_f = be32_to_cpu(req_upiu->header.dword_1) >> 16 & MASK_TM_FUNC;
 
 	switch (msgcode) {
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index 9a754fab8908..f66cf9e477cb 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -389,7 +389,7 @@ enum {
 };
 
 /* Overall command status values */
-enum {
+enum utp_ocs {
 	OCS_SUCCESS			= 0x0,
 	OCS_INVALID_CMD_TABLE_ATTR	= 0x1,
 	OCS_INVALID_PRDT_ATTR		= 0x2,
@@ -402,6 +402,9 @@ enum {
 	OCS_INVALID_CRYPTO_CONFIG	= 0x9,
 	OCS_GENERAL_CRYPTO_ERROR	= 0xA,
 	OCS_INVALID_COMMAND_STATUS	= 0x0F,
+};
+
+enum {
 	MASK_OCS			= 0x0F,
 };
 

[PATCH 4/5] scsi: ufs: Log error handler activity

[Bart Van Assche]

Kernel logs are hard to comprehend without information about what the
UFS error handler is doing. Hence this patch that logs information
about error handler activity.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index cb55ba3cb3e6..ecfe1f124f8a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -134,6 +134,14 @@ enum {
 	UFSHCD_CAN_QUEUE	= 32,
 };
 
+static const char *const ufshcd_state_name[] = {
+	[UFSHCD_STATE_RESET]			= "reset",
+	[UFSHCD_STATE_OPERATIONAL]		= "operational",
+	[UFSHCD_STATE_ERROR]			= "error",
+	[UFSHCD_STATE_EH_SCHEDULED_FATAL]	= "eh_will_reset",
+	[UFSHCD_STATE_EH_SCHEDULED_NON_FATAL]	= "eh_wont_reset",
+};
+
 /* UFSHCD error handling flags */
 enum {
 	UFSHCD_EH_IN_PROGRESS = (1 << 0),
@@ -6065,6 +6073,13 @@ static void ufshcd_err_handler(struct Scsi_Host *host)
 	int pmc_err;
 	int tag;
 
+	dev_info(hba->dev,
+		 "%s started; HBA state %s; powered %d; shutting down %d; saved_err = %d; saved_uic_err = %d; force_reset = %d%s\n",
+		 __func__, ufshcd_state_name[hba->ufshcd_state],
+		 hba->is_powered, hba->shutting_down, hba->saved_err,
+		 hba->saved_uic_err, hba->force_reset,
+		 ufshcd_is_link_broken(hba) ? "; link is broken" : "");
+
 	down(&hba->host_sem);
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->host->host_eh_scheduled = 0;
@@ -6160,6 +6175,8 @@ static void ufshcd_err_handler(struct Scsi_Host *host)
 			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 */
@@ -6240,6 +6257,9 @@ static void ufshcd_err_handler(struct Scsi_Host *host)
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	ufshcd_err_handling_unprepare(hba);
 	up(&hba->host_sem);
+
+	dev_info(hba->dev, "%s finished; HBA state %s\n", __func__,
+		 ufshcd_state_name[hba->ufshcd_state]);
 }
 
 /**
@@ -6554,6 +6574,10 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
 	err = ufshcd_wait_for_register(hba,
 			REG_UTP_TASK_REQ_DOOR_BELL,
 			mask, 0, 1000, 1000);
+
+	dev_err(hba->dev, "Clearing task management function with tag %d %s\n",
+		tag, err ? "succeeded" : "failed");
+
 out:
 	return err;
 }

Re: [PATCH 4/5] scsi: ufs: Log error handler activity

[Adrian Hunter]

On 13/10/2021 00:54, Bart Van Assche wrote:
> Kernel logs are hard to comprehend without information about what the
> UFS error handler is doing. Hence this patch that logs information
> about error handler activity.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index cb55ba3cb3e6..ecfe1f124f8a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -134,6 +134,14 @@ enum {
>  	UFSHCD_CAN_QUEUE	= 32,
>  };
>  
> +static const char *const ufshcd_state_name[] = {
> +	[UFSHCD_STATE_RESET]			= "reset",
> +	[UFSHCD_STATE_OPERATIONAL]		= "operational",
> +	[UFSHCD_STATE_ERROR]			= "error",
> +	[UFSHCD_STATE_EH_SCHEDULED_FATAL]	= "eh_will_reset",
> +	[UFSHCD_STATE_EH_SCHEDULED_NON_FATAL]	= "eh_wont_reset",

Currently, the error handler can do a reset for 
UFSHCD_STATE_EH_SCHEDULED_NON_FATAL, so that description is
misleading.

There is code like:

	if (hba->force_reset || ufshcd_is_link_broken(hba) ||
	    ufshcd_is_saved_err_fatal(hba) ||
	    ((hba->saved_err & UIC_ERROR) &&
	     (hba->saved_uic_err & (UFSHCD_UIC_DL_NAC_RECEIVED_ERROR |
				    UFSHCD_UIC_DL_TCx_REPLAY_ERROR)))) {
		needs_reset = true;
		goto do_reset;
	}

where UFSHCD_UIC_DL_NAC_RECEIVED_ERROR and UFSHCD_UIC_DL_TCx_REPLAY_ERROR
are non-fatal errors.  I think the spec. says they should not need a reset
but the driver does anyway.

> +};
> +
>  /* UFSHCD error handling flags */
>  enum {
>  	UFSHCD_EH_IN_PROGRESS = (1 << 0),
> @@ -6065,6 +6073,13 @@ static void ufshcd_err_handler(struct Scsi_Host *host)
>  	int pmc_err;
>  	int tag;
>  
> +	dev_info(hba->dev,
> +		 "%s started; HBA state %s; powered %d; shutting down %d; saved_err = %d; saved_uic_err = %d; force_reset = %d%s\n",
> +		 __func__, ufshcd_state_name[hba->ufshcd_state],
> +		 hba->is_powered, hba->shutting_down, hba->saved_err,
> +		 hba->saved_uic_err, hba->force_reset,
> +		 ufshcd_is_link_broken(hba) ? "; link is broken" : "");
> +
>  	down(&hba->host_sem);
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	hba->host->host_eh_scheduled = 0;
> @@ -6160,6 +6175,8 @@ static void ufshcd_err_handler(struct Scsi_Host *host)
>  			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 */
> @@ -6240,6 +6257,9 @@ static void ufshcd_err_handler(struct Scsi_Host *host)
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
>  	ufshcd_err_handling_unprepare(hba);
>  	up(&hba->host_sem);
> +
> +	dev_info(hba->dev, "%s finished; HBA state %s\n", __func__,
> +		 ufshcd_state_name[hba->ufshcd_state]);
>  }
>  
>  /**
> @@ -6554,6 +6574,10 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
>  	err = ufshcd_wait_for_register(hba,
>  			REG_UTP_TASK_REQ_DOOR_BELL,
>  			mask, 0, 1000, 1000);
> +
> +	dev_err(hba->dev, "Clearing task management function with tag %d %s\n",
> +		tag, err ? "succeeded" : "failed");
> +
>  out:
>  	return err;
>  }
> 

[PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Bart Van Assche]

Make it possible to test the impact of the UFS error handler on software
that submits SCSI commands to the UFS driver.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 10 ++++++
 drivers/scsi/ufs/ufshcd.c                  | 37 ++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index ec3a7149ced5..2a46f91d3f1b 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1534,3 +1534,13 @@ Contact:	Avri Altman <avri.altman@wdc.com>
 Description:	In host control mode the host is the originator of map requests.
 		To avoid flooding the device with map requests, use a simple throttling
 		mechanism that limits the number of inflight map requests.
+
+What:		/sys/class/scsi_host/*/trigger_eh
+Date:		October 2021
+Contact:	Bart Van Assche <bvanassche@acm.org>
+Description:	Writing into this sysfs attribute triggers the UFS error
+		handler. This is useful for testing how the UFS error handler
+		affects SCSI command processing. The supported values are as
+		follows: "1" triggers the error handler without resetting the
+		host controller and "2" starts the error handler and makes it
+		reset the host interface.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ecfe1f124f8a..30ff93979840 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8144,6 +8144,42 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	}
 }
 
+static ssize_t trigger_eh_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct Scsi_Host *host = class_to_shost(dev);
+	struct ufs_hba *hba = shost_priv(host);
+
+	/*
+	 * Using locking would be a better solution. However, this is a debug
+	 * attribute so ufshcd_eh_in_progress() should be good enough.
+	 */
+	if (ufshcd_eh_in_progress(hba))
+		return -EBUSY;
+
+	if (sysfs_streq(buf, "1")) {
+		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
+		hba->saved_err |= UIC_ERROR;
+	} else if (sysfs_streq(buf, "2")) {
+		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
+		hba->saved_err |= UIC_ERROR;
+	} else {
+		return -EINVAL;
+	}
+
+	scsi_schedule_eh(hba->host);
+
+	return count;
+}
+
+static DEVICE_ATTR_WO(trigger_eh);
+
+static struct device_attribute *ufshcd_shost_attrs[] = {
+	&dev_attr_trigger_eh,
+	NULL
+};
+
 static const struct attribute_group *ufshcd_driver_groups[] = {
 	&ufs_sysfs_unit_descriptor_group,
 	&ufs_sysfs_lun_attributes_group,
@@ -8183,6 +8219,7 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
 	.max_host_blocked	= 1,
 	.track_queue_depth	= 1,
+	.shost_attrs		= ufshcd_shost_attrs,
 	.sdev_groups		= ufshcd_driver_groups,
 	.dma_boundary		= PAGE_SIZE - 1,
 	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,

Re: [PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Greg Kroah-Hartman]

On Tue, Oct 12, 2021 at 02:54:33PM -0700, Bart Van Assche wrote:
> Make it possible to test the impact of the UFS error handler on software
> that submits SCSI commands to the UFS driver.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  Documentation/ABI/testing/sysfs-driver-ufs | 10 ++++++
>  drivers/scsi/ufs/ufshcd.c                  | 37 ++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
> index ec3a7149ced5..2a46f91d3f1b 100644
> --- a/Documentation/ABI/testing/sysfs-driver-ufs
> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> @@ -1534,3 +1534,13 @@ Contact:	Avri Altman <avri.altman@wdc.com>
>  Description:	In host control mode the host is the originator of map requests.
>  		To avoid flooding the device with map requests, use a simple throttling
>  		mechanism that limits the number of inflight map requests.
> +
> +What:		/sys/class/scsi_host/*/trigger_eh
> +Date:		October 2021
> +Contact:	Bart Van Assche <bvanassche@acm.org>
> +Description:	Writing into this sysfs attribute triggers the UFS error
> +		handler. This is useful for testing how the UFS error handler
> +		affects SCSI command processing. The supported values are as
> +		follows: "1" triggers the error handler without resetting the
> +		host controller and "2" starts the error handler and makes it
> +		reset the host interface.
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ecfe1f124f8a..30ff93979840 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8144,6 +8144,42 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  	}
>  }
>  
> +static ssize_t trigger_eh_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct Scsi_Host *host = class_to_shost(dev);
> +	struct ufs_hba *hba = shost_priv(host);
> +
> +	/*
> +	 * Using locking would be a better solution. However, this is a debug
> +	 * attribute so ufshcd_eh_in_progress() should be good enough.
> +	 */
> +	if (ufshcd_eh_in_progress(hba))
> +		return -EBUSY;
> +
> +	if (sysfs_streq(buf, "1")) {
> +		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
> +		hba->saved_err |= UIC_ERROR;
> +	} else if (sysfs_streq(buf, "2")) {
> +		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
> +		hba->saved_err |= UIC_ERROR;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	scsi_schedule_eh(hba->host);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_WO(trigger_eh);
> +
> +static struct device_attribute *ufshcd_shost_attrs[] = {
> +	&dev_attr_trigger_eh,
> +	NULL
> +};
> +
>  static const struct attribute_group *ufshcd_driver_groups[] = {
>  	&ufs_sysfs_unit_descriptor_group,
>  	&ufs_sysfs_lun_attributes_group,
> @@ -8183,6 +8219,7 @@ static struct scsi_host_template ufshcd_driver_template = {
>  	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
>  	.max_host_blocked	= 1,
>  	.track_queue_depth	= 1,
> +	.shost_attrs		= ufshcd_shost_attrs,

Why can't this get added to the sdev_groups list?

thanks,

greg k-h

Re: [PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Bart Van Assche]

On 10/13/21 12:19 AM, Greg Kroah-Hartman wrote:
>>   static const struct attribute_group *ufshcd_driver_groups[] = {
>>   	&ufs_sysfs_unit_descriptor_group,
>>   	&ufs_sysfs_lun_attributes_group,
>> @@ -8183,6 +8219,7 @@ static struct scsi_host_template ufshcd_driver_template = {
>>   	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
>>   	.max_host_blocked	= 1,
>>   	.track_queue_depth	= 1,
>> +	.shost_attrs		= ufshcd_shost_attrs,
> 
> Why can't this get added to the sdev_groups list?

The UFS error handler resets the host controller. There is one SCSI host 
per UFS host controller. Hence the choice to associate the sysfs 
attribute with the SCSI host.

There is one SCSI sdev per SCSI LUN. Although I'm not sure this can 
happen for UFS, it is possible that zero SCSI devices (sdevs) are 
associated with a SCSI host.

Bart.

Re: [PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Adrian Hunter]

On 13/10/2021 00:54, Bart Van Assche wrote:
> Make it possible to test the impact of the UFS error handler on software
> that submits SCSI commands to the UFS driver.

Are you sure this isn't better suited to debugfs?

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  Documentation/ABI/testing/sysfs-driver-ufs | 10 ++++++
>  drivers/scsi/ufs/ufshcd.c                  | 37 ++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
> index ec3a7149ced5..2a46f91d3f1b 100644
> --- a/Documentation/ABI/testing/sysfs-driver-ufs
> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> @@ -1534,3 +1534,13 @@ Contact:	Avri Altman <avri.altman@wdc.com>
>  Description:	In host control mode the host is the originator of map requests.
>  		To avoid flooding the device with map requests, use a simple throttling
>  		mechanism that limits the number of inflight map requests.
> +
> +What:		/sys/class/scsi_host/*/trigger_eh
> +Date:		October 2021
> +Contact:	Bart Van Assche <bvanassche@acm.org>
> +Description:	Writing into this sysfs attribute triggers the UFS error
> +		handler. This is useful for testing how the UFS error handler
> +		affects SCSI command processing. The supported values are as
> +		follows: "1" triggers the error handler without resetting the
> +		host controller and "2" starts the error handler and makes it
> +		reset the host interface.
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ecfe1f124f8a..30ff93979840 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8144,6 +8144,42 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  	}
>  }
>  
> +static ssize_t trigger_eh_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct Scsi_Host *host = class_to_shost(dev);
> +	struct ufs_hba *hba = shost_priv(host);
> +
> +	/*
> +	 * Using locking would be a better solution. However, this is a debug
> +	 * attribute so ufshcd_eh_in_progress() should be good enough.
> +	 */
> +	if (ufshcd_eh_in_progress(hba))
> +		return -EBUSY;

Does it matter if ufshcd_eh_in_progress()?

> +
> +	if (sysfs_streq(buf, "1")) {
> +		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;

Shouldn't overwrite UFSHCD_STATE_ERROR

> +		hba->saved_err |= UIC_ERROR;

ufshcd_err_handler() still behaves differently depending on
hba->saved_uic_err

> +	} else if (sysfs_streq(buf, "2")) {
> +		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
> +		hba->saved_err |= UIC_ERROR;

In addition, a fatal error must be set to get fatal error behaviour from
ufshcd_err_handler.

> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	scsi_schedule_eh(hba->host);

Probably should be:

	queue_work(hba->eh_wq, &hba->eh_work);

However, it might be simpler to replace everything with:

	spin_lock(hba->host->host_lock);
	hba->saved_err |= <something>;
	hba->saved_uic_err |= <something else>;
	ufshcd_schedule_eh_work(hba);
	spin_unlock(hba->host->host_lock);

Perhaps letting the user specify values to determine <something>
and <something else>

> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_WO(trigger_eh);
> +
> +static struct device_attribute *ufshcd_shost_attrs[] = {
> +	&dev_attr_trigger_eh,
> +	NULL
> +};
> +
>  static const struct attribute_group *ufshcd_driver_groups[] = {
>  	&ufs_sysfs_unit_descriptor_group,
>  	&ufs_sysfs_lun_attributes_group,
> @@ -8183,6 +8219,7 @@ static struct scsi_host_template ufshcd_driver_template = {
>  	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
>  	.max_host_blocked	= 1,
>  	.track_queue_depth	= 1,
> +	.shost_attrs		= ufshcd_shost_attrs,
>  	.sdev_groups		= ufshcd_driver_groups,
>  	.dma_boundary		= PAGE_SIZE - 1,
>  	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
> 

RE: [PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Avri Altman]

> 
> On 13/10/2021 00:54, Bart Van Assche wrote:
> > Make it possible to test the impact of the UFS error handler on
> > software that submits SCSI commands to the UFS driver.
> 
> Are you sure this isn't better suited to debugfs?
Was just thinking the very same thing.

Thanks,
Avri

> 
> >
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >  Documentation/ABI/testing/sysfs-driver-ufs | 10 ++++++
> >  drivers/scsi/ufs/ufshcd.c                  | 37 ++++++++++++++++++++++
> >  2 files changed, 47 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> > b/Documentation/ABI/testing/sysfs-driver-ufs
> > index ec3a7149ced5..2a46f91d3f1b 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-ufs
> > +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> > @@ -1534,3 +1534,13 @@ Contact:       Avri Altman
> <avri.altman@wdc.com>
> >  Description: In host control mode the host is the originator of map
> requests.
> >               To avoid flooding the device with map requests, use a simple
> throttling
> >               mechanism that limits the number of inflight map requests.
> > +
> > +What:                /sys/class/scsi_host/*/trigger_eh
> > +Date:                October 2021
> > +Contact:     Bart Van Assche <bvanassche@acm.org>
> > +Description: Writing into this sysfs attribute triggers the UFS error
> > +             handler. This is useful for testing how the UFS error handler
> > +             affects SCSI command processing. The supported values are as
> > +             follows: "1" triggers the error handler without resetting the
> > +             host controller and "2" starts the error handler and makes it
> > +             reset the host interface.
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index ecfe1f124f8a..30ff93979840 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8144,6 +8144,42 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
> >       }
> >  }
> >
> > +static ssize_t trigger_eh_store(struct device *dev,
> > +                             struct device_attribute *attr,
> > +                             const char *buf, size_t count) {
> > +     struct Scsi_Host *host = class_to_shost(dev);
> > +     struct ufs_hba *hba = shost_priv(host);
> > +
> > +     /*
> > +      * Using locking would be a better solution. However, this is a debug
> > +      * attribute so ufshcd_eh_in_progress() should be good enough.
> > +      */
> > +     if (ufshcd_eh_in_progress(hba))
> > +             return -EBUSY;
> 
> Does it matter if ufshcd_eh_in_progress()?
> 
> > +
> > +     if (sysfs_streq(buf, "1")) {
> > +             hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
> 
> Shouldn't overwrite UFSHCD_STATE_ERROR
> 
> > +             hba->saved_err |= UIC_ERROR;
> 
> ufshcd_err_handler() still behaves differently depending on
> hba->saved_uic_err
> 
> > +     } else if (sysfs_streq(buf, "2")) {
> > +             hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
> > +             hba->saved_err |= UIC_ERROR;
> 
> In addition, a fatal error must be set to get fatal error behaviour from
> ufshcd_err_handler.
> 
> > +     } else {
> > +             return -EINVAL;
> > +     }
> > +
> > +     scsi_schedule_eh(hba->host);
> 
> Probably should be:
> 
>         queue_work(hba->eh_wq, &hba->eh_work);
> 
> However, it might be simpler to replace everything with:
> 
>         spin_lock(hba->host->host_lock);
>         hba->saved_err |= <something>;
>         hba->saved_uic_err |= <something else>;
>         ufshcd_schedule_eh_work(hba);
>         spin_unlock(hba->host->host_lock);
> 
> Perhaps letting the user specify values to determine <something> and
> <something else>
> 
> > +
> > +     return count;
> > +}
> > +
> > +static DEVICE_ATTR_WO(trigger_eh);
> > +
> > +static struct device_attribute *ufshcd_shost_attrs[] = {
> > +     &dev_attr_trigger_eh,
> > +     NULL
> > +};
> > +
> >  static const struct attribute_group *ufshcd_driver_groups[] = {
> >       &ufs_sysfs_unit_descriptor_group,
> >       &ufs_sysfs_lun_attributes_group, @@ -8183,6 +8219,7 @@ static
> > struct scsi_host_template ufshcd_driver_template = {
> >       .max_segment_size       = PRDT_DATA_BYTE_COUNT_MAX,
> >       .max_host_blocked       = 1,
> >       .track_queue_depth      = 1,
> > +     .shost_attrs            = ufshcd_shost_attrs,
> >       .sdev_groups            = ufshcd_driver_groups,
> >       .dma_boundary           = PAGE_SIZE - 1,
> >       .rpm_autosuspend_delay  = RPM_AUTOSUSPEND_DELAY_MS,
> >

Re: [PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Bart Van Assche]

On 10/13/21 1:09 AM, Adrian Hunter wrote:
> On 13/10/2021 00:54, Bart Van Assche wrote:
>> Make it possible to test the impact of the UFS error handler on software
>> that submits SCSI commands to the UFS driver.
> 
> Are you sure this isn't better suited to debugfs?

I will convert this attribute into a debugfs attribute.

>> +	if (ufshcd_eh_in_progress(hba))
>> +		return -EBUSY;
> 
> Does it matter if ufshcd_eh_in_progress()?

The UFS error handler modifies hba->ufshcd_state and assumes that no 
other code modifies hba->ufshcd_state while the error handler is in 
progress. Hence this check.

>> +	} else {
>> +		return -EINVAL;
>> +	}
>> +
>> +	scsi_schedule_eh(hba->host);
> 
> Probably should be:
> 
> 	queue_work(hba->eh_wq, &hba->eh_work);

No. This patch is intended for Martin Petersen's 5.16/scsi-queue branch. 
The patch "Revert "scsi: ufs: Synchronize SCSI and UFS error handling"" 
has been queued on the 5.15/scsi-fixes branch only.

> However, it might be simpler to replace everything with:
> 
> 	spin_lock(hba->host->host_lock);
> 	hba->saved_err |= <something>;
> 	hba->saved_uic_err |= <something else>;
> 	ufshcd_schedule_eh_work(hba);
> 	spin_unlock(hba->host->host_lock);
> 
> Perhaps letting the user specify values to determine <something>
> and <something else>

I will look into this.

Thanks,

Bart.

Re: [PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Adrian Hunter]

On 13/10/2021 19:56, Bart Van Assche wrote:
> On 10/13/21 1:09 AM, Adrian Hunter wrote:
>> On 13/10/2021 00:54, Bart Van Assche wrote:
>>> Make it possible to test the impact of the UFS error handler on software
>>> that submits SCSI commands to the UFS driver.
>>
>> Are you sure this isn't better suited to debugfs?
> 
> I will convert this attribute into a debugfs attribute.
> 
>>> +    if (ufshcd_eh_in_progress(hba))
>>> +        return -EBUSY;
>>
>> Does it matter if ufshcd_eh_in_progress()?
> 
> The UFS error handler modifies hba->ufshcd_state and assumes that no other code modifies hba->ufshcd_state while the error handler is in progress. Hence this check.

No the error handler takes care not to overwrite ufshcd_state changes
made by the interrupt handler, by always checking if the state is
UFSHCD_STATE_RESET before changing it to UFSHCD_STATE_OPERATIONAL.

Setting saved_err/saved_uic_err and calling ufshcd_schedule_eh_work()
all under spinlock, would be just like an error interrupt, which can
happen while the error handler is running.

> 
>>> +    } else {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    scsi_schedule_eh(hba->host);
>>
>> Probably should be:
>>
>>     queue_work(hba->eh_wq, &hba->eh_work);
> 
> No. This patch is intended for Martin Petersen's 5.16/scsi-queue branch. The patch "Revert "scsi: ufs: Synchronize SCSI and UFS error handling"" has been queued on the 5.15/scsi-fixes branch only.
> 
>> However, it might be simpler to replace everything with:
>>
>>     spin_lock(hba->host->host_lock);
>>     hba->saved_err |= <something>;
>>     hba->saved_uic_err |= <something else>;
>>     ufshcd_schedule_eh_work(hba);
>>     spin_unlock(hba->host->host_lock);
>>
>> Perhaps letting the user specify values to determine <something>
>> and <something else>
> 
> I will look into this.
> 
> Thanks,
> 
> Bart.