Skip to contentSkip to conversation

UFS driver general fixes bundle 1

PATCH Superseded Can Guo 2 participants 9 messages Oct 28, 2019 – Oct 29, 2019 SCSI

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Can Guo

author
#0 lore raw

cover UFS driver general fixes bundle 1

This bundle includes 5 general fixes for UFS driver.

Can Guo (5):
scsi: Adjust DBD setting in mode sense for caching mode page per LLD
scsi: ufs: Set DBD setting in mode sense for caching mode page
scsi: ufs: Release clock if DMA map fails
scsi: ufs: Do not clear the DL layer timers
scsi: ufs: Do not free irq in suspend

 drivers/scsi/sd.c         |  6 +++++-
 drivers/scsi/ufs/ufshcd.c | 40 ++++++++++++++++++++++++++--------------
 drivers/scsi/ufs/unipro.h | 11 +++++++++++
 include/scsi/scsi_host.h  |  6 ++++++
 4 files changed, 48 insertions(+), 15 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#1 lore raw

PATCH 1/5 scsi: Adjust DBD setting in mode sense for caching mode page per LLD

Host sends MODE_SENSE_10 with caching mode page, to check if the device supports the cache feature.
Some LLD standards requires DBD field to be set to 1.

This patch allows LLD to define the setting of DBD if required.

Change-Id I0c8752c1888654942d6d7e6e0f6dc197033ac326
Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/sd.c        | 6 +++++-
 include/scsi/scsi_host.h | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)
drivers/scsi/sd.c +5 -1
index aab4ed8..6d8194f 100644
@@ -2629,6 +2629,7 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
 {
 	int len = 0, res;
 	struct scsi_device *sdp = sdkp->device;
+	struct Scsi_Host *host = sdp->host;
 
 	int dbd;
 	int modepage;

@@ -2660,7 +2661,10 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
 		dbd = 8;
 	} else {
 		modepage = 8;
-		dbd = 0;
+		if (host->set_dbd_for_caching)
+			dbd = 8;
+		else
+			dbd = 0;
 	}
 
 	/* cautiously ask */
include/scsi/scsi_host.h +6 -0
index 2c3f0c5..3900987 100644
@@ -650,6 +650,12 @@ struct Scsi_Host {
 	unsigned no_scsi2_lun_in_cdb:1;
 
 	/*
+	 * Set "DBD" field in mode_sense caching mode page in case it is
+	 * mandatory by LLD standard.
+	 */
+	unsigned set_dbd_for_caching:1;
+
+	/*
 	 * Optional work queue to be utilized by the transport
 	 */
 	char work_q_name[20];
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#5 lore raw

PATCH 2/5 scsi: ufs: Set DBD setting in mode sense for caching mode page

In reply to Can Guo

Host sends MODE_SENSE_10 with caching mode page, to check if the device supports the cache feature.
UFS standards requires DBD field to be set to 1.

Some card vendors are more strict and check the DBD field, hence respond with CHECK_CONDITION (Sense key set to ILLEGAL_REQUEST and ASC set to INVALID FIELD IN CDB).
As a result of the CHECK_CONDITION response, host assumes that the device doesn't support the cache feature and doesn't send SYNCHORONIZE_CACHE commands to flush the device cache. This can result in data corruption in case of sudden power down, when there is data stored in the device cache.

This patch fixes the DBD field setting in case of caching mode page.

Change-Id Ifb674dfcc497a2c9dd9b49556f86979d9a3e4acf
Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)
drivers/scsi/ufs/ufshcd.c +1 -0
index c28c144..101b4d0 100644
@@ -8329,6 +8329,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	host->max_channel = UFSHCD_MAX_CHANNEL;
 	host->unique_id = host->host_no;
 	host->max_cmd_len = UFS_CDB_SIZE;
+	host->set_dbd_for_caching = 1;
 
 	hba->max_pwr_info.is_valid = false;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#6 lore raw

PATCH 3/5 scsi: ufs: Release clock if DMA map fails

In reply to Can Guo

In queuecommand path, if DMA map fails, it bails out with clock held. In this case, release the clock to keep its usage paired.

Change-Id Ia3e3e19086c58a27ed2a794fcbbac341e584a474
Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)
drivers/scsi/ufs/ufshcd.c +1 -0
index 101b4d0..6e9236a 100644
@@ -2480,6 +2480,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	if (err) {
 		lrbp->cmd = NULL;
 		clear_bit_unlock(tag, &hba->lrb_in_use);
+		ufshcd_release(hba);
 		goto out;
 	}
 	/* Make sure descriptors are ready before ringing the doorbell */
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#7 lore raw

PATCH 4/5 scsi: ufs: Do not clear the DL layer timers

In reply to Can Guo

During power mode change, PACP_PWR_Req frame sends PAPowerModeUserData parameters (and they are considered valid by device if Flags[4] - UserDataValid bit is set in the same frame). Currently we don't set these PAPowerModeUserData parameters and hardware always sets UserDataValid bit which would clear all the DL layer timeout values of the peer device after the power mode change.

This change sets the PAPowerModeUserData[0..5] to UniPro specification recommended default values, in addition we are also setting the relevant DME_LOCAL_* timer attributes as required by UFS HCI specification.

Change-Id I1a8c02832a185dc4ce9043eee6572ddee89bdcdb
Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 20 ++++++++++++++++++++
 drivers/scsi/ufs/unipro.h | 11 +++++++++++
 2 files changed, 31 insertions(+)
drivers/scsi/ufs/ufshcd.c +20 -0
index 6e9236a..34efa8a 100644
@@ -4084,6 +4084,26 @@ static int ufshcd_change_power_mode(struct ufs_hba *hba,
 		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_HSSERIES),
 						pwr_mode->hs_rate);
 
+	ufshcd_dme_set(hba, UIC_ARG_MIB(PA_PWRMODEUSERDATA0),
+			DL_FC0ProtectionTimeOutVal_Default);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(PA_PWRMODEUSERDATA1),
+			DL_TC0ReplayTimeOutVal_Default);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(PA_PWRMODEUSERDATA2),
+			DL_AFC0ReqTimeOutVal_Default);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(PA_PWRMODEUSERDATA3),
+			DL_FC1ProtectionTimeOutVal_Default);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(PA_PWRMODEUSERDATA4),
+			DL_TC1ReplayTimeOutVal_Default);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(PA_PWRMODEUSERDATA5),
+			DL_AFC1ReqTimeOutVal_Default);
+
+	ufshcd_dme_set(hba, UIC_ARG_MIB(DME_LocalFC0ProtectionTimeOutVal),
+			DL_FC0ProtectionTimeOutVal_Default);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(DME_LocalTC0ReplayTimeOutVal),
+			DL_TC0ReplayTimeOutVal_Default);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(DME_LocalAFC0ReqTimeOutVal),
+			DL_AFC0ReqTimeOutVal_Default);
+
 	ret = ufshcd_uic_change_pwr_mode(hba, pwr_mode->pwr_rx << 4
 			| pwr_mode->pwr_tx);
drivers/scsi/ufs/unipro.h +11 -0
index f539f87..3dc4d8b 100644
@@ -161,6 +161,17 @@
 /* PHY Adapter Protocol Constants */
 #define PA_MAXDATALANES	4
 
+#define DL_FC0ProtectionTimeOutVal_Default	8191
+#define DL_TC0ReplayTimeOutVal_Default		65535
+#define DL_AFC0ReqTimeOutVal_Default		32767
+#define DL_FC1ProtectionTimeOutVal_Default	8191
+#define DL_TC1ReplayTimeOutVal_Default		65535
+#define DL_AFC1ReqTimeOutVal_Default		32767
+
+#define DME_LocalFC0ProtectionTimeOutVal	0xD041
+#define DME_LocalTC0ReplayTimeOutVal		0xD042
+#define DME_LocalAFC0ReqTimeOutVal		0xD043
+
 /* PA power modes */
 enum {
 	FAST_MODE	= 1,
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#8 lore raw

PATCH 5/5 scsi: ufs: Do not free irq in suspend

In reply to Can Guo

If PM QoS is enabled and we set request type to PM_QOS_REQ_AFFINE_IRQ then freeing up the irq makes the free_irq() print out warning with call stack. We don't really need to free up irq during suspend, disabling it during suspend and reenabling it during resume should be good enough.

Change-Id I4c4033552ae2cd849aeccd98dd517fdeb91c5cb7
Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)
drivers/scsi/ufs/ufshcd.c +4 -14
index 34efa8a..fb36a51 100644
@@ -266,26 +266,18 @@ static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
 	return tag >= 0 && tag < hba->nutrs;
 }
 
-static inline int ufshcd_enable_irq(struct ufs_hba *hba)
+static inline void ufshcd_enable_irq(struct ufs_hba *hba)
 {
-	int ret = 0;
-
 	if (!hba->is_irq_enabled) {
-		ret = request_irq(hba->irq, ufshcd_intr, IRQF_SHARED, UFSHCD,
-				hba);
-		if (ret)
-			dev_err(hba->dev, "%s: request_irq failed, ret=%d\n",
-				__func__, ret);
+		enable_irq(hba->irq);
 		hba->is_irq_enabled = true;
 	}
-
-	return ret;
 }
 
 static inline void ufshcd_disable_irq(struct ufs_hba *hba)
 {
 	if (hba->is_irq_enabled) {
-		free_irq(hba->irq, hba);
+		disable_irq(hba->irq);
 		hba->is_irq_enabled = false;
 	}
 }

@@ -7927,9 +7919,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		goto out;
 
 	/* enable the host irq as host controller would be active soon */
-	ret = ufshcd_enable_irq(hba);
-	if (ret)
-		goto disable_irq_and_vops_clks;
+	ufshcd_enable_irq(hba);
 
 	ret = ufshcd_vreg_set_hpm(hba);
 	if (ret)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#2 lore raw

Re: [PATCH v1 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD

In reply to Can Guo
Quoted 3 lines · Can Guo

Host sends MODE_SENSE_10 with caching mode page, to check if the device supports the cache feature.
Some LLD standards requires DBD field to be set to 1.

Which LLD standard are you referring to? Please mention at least one name of such a standard in the patch description.

Quoted 1 line

Change-Id: I0c8752c1888654942d6d7e6e0f6dc197033ac326

Change-IDs should be left out from upstream patches. Does the presence of this ID mean that this patch has not been verified with checkpatch? From the checkpatch source code:

# Check for unwanted Gerrit info

if ($in_commit_log && $line =~ /^\s*change-id:/i) {
	ERROR("GERRIT_CHANGE_ID",
	      "Remove Gerrit Change-Id's before submitting upstream.\n"\
		 . $herecurr);
}
Quoted 31 lines
drivers/scsi/sd.c +5 -1
index aab4ed8..6d8194f 100644
@@ -2629,6 +2629,7 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
  {
  	int len = 0, res;
  	struct scsi_device *sdp = sdkp->device;
+	struct Scsi_Host *host = sdp->host;
 
  	int dbd;
  	int modepage;

@@ -2660,7 +2661,10 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
  		dbd = 8;
  	} else {
  		modepage = 8;
-		dbd = 0;
+		if (host->set_dbd_for_caching)
+			dbd = 8;
+		else
+			dbd = 0;
  	}
 
  	/* cautiously ask */
include/scsi/scsi_host.h +0 -0
index 2c3f0c5..3900987 100644
@@ -650,6 +650,12 @@ struct Scsi_Host {
  	unsigned no_scsi2_lun_in_cdb:1;

include/scsi/scsi_host.h struct Scsi_Host { · line 661

Quoted 10 lines
include/scsi/scsi_host.h +6 -0
  	/*
+	 * Set "DBD" field in mode_sense caching mode page in case it is
+	 * mandatory by LLD standard.
+	 */
+	unsigned set_dbd_for_caching:1;
+
+	/*
  	 * Optional work queue to be utilized by the transport
  	 */
  	char work_q_name[20];

Since this patch by itself has no effect, please resubmit this patch together with the LLD patch that sets set_dbd_for_caching.

Thanks,

Bart.

#3 lore raw

Re: [PATCH v1 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD

Quoted 1 line · Bart Van Assche

Since this patch by itself has no effect, please resubmit this patch together with the LLD patch that sets set_dbd_for_caching.

Please ignore this part of my email. Patch 1/5 showed up in my inbox before the rest of this patch series.

Bart.

author
#4 lore raw

Re: [PATCH v1 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD

In reply to Bart Van Assche
Quoted 8 lines · Bart Van Assche
Quoted 3 lines · Can Guo

Host sends MODE_SENSE_10 with caching mode page, to check if the device supports the cache feature.
Some LLD standards requires DBD field to be set to 1.

Which LLD standard are you referring to? Please mention at least one name of such a standard in the patch description.

Hi Bart, Thank you for your review.

The LLD standard here is UFS. I will update the commit message and re-upload it later.

Thanks,

Can Guo

Quoted 6 lines
Quoted 1 line

Change-Id: I0c8752c1888654942d6d7e6e0f6dc197033ac326

Change-IDs should be left out from upstream patches. Does the presence of this ID mean that this patch has not been verified with checkpatch? From the checkpatch source code:

Quoted 7 lines

# Check for unwanted Gerrit info

if ($in_commit_log && $line =~ /^\s*change-id:/i) {
	ERROR("GERRIT_CHANGE_ID",
	      "Remove Gerrit Change-Id's before submitting upstream.\n"\
		 . $herecurr);
}

Sorry, forgot to remove the change-id.

Quoted 47 lines
Quoted 30 lines
drivers/scsi/sd.c +5 -1
index aab4ed8..6d8194f 100644
@@ -2629,6 +2629,7 @@ static int sd_try_rc16_first(struct scsi_device 
 *sdp)
  {
  	int len = 0, res;
  	struct scsi_device *sdp = sdkp->device;
+	struct Scsi_Host *host = sdp->host;
    	int dbd;
  	int modepage;

@@ -2660,7 +2661,10 @@ static int sd_try_rc16_first(struct scsi_device 
 *sdp)
  		dbd = 8;
  	} else {
  		modepage = 8;
-		dbd = 0;
+		if (host->set_dbd_for_caching)
+			dbd = 8;
+		else
+			dbd = 0;
  	}
    	/* cautiously ask */
include/scsi/scsi_host.h +0 -0
index 2c3f0c5..3900987 100644
@@ -650,6 +650,12 @@ struct Scsi_Host {
  	unsigned no_scsi2_lun_in_cdb:1;

include/scsi/scsi_host.h struct Scsi_Host { · line 660

Quoted 10 lines
include/scsi/scsi_host.h +6 -0
    	/*
+	 * Set "DBD" field in mode_sense caching mode page in case it is
+	 * mandatory by LLD standard.
+	 */
+	unsigned set_dbd_for_caching:1;
+
+	/*
  	 * Optional work queue to be utilized by the transport
  	 */
  	char work_q_name[20];

Since this patch by itself has no effect, please resubmit this patch together with the LLD patch that sets set_dbd_for_caching.

Thanks,

Bart.