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

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

[Can Guo]

UFS JEDEC standards require DBD field to be set to 1 in mode sense command.
This patch allows LLD to define the setting of DBD if required.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/scsi_lib.c    | 2 ++
 include/scsi/scsi_device.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5447738..3812e90 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2108,6 +2108,8 @@ void scsi_exit_queue(void)
 
 	memset(data, 0, sizeof(*data));
 	memset(&cmd[0], 0, 12);
+
+	dbd = sdev->set_dbd_for_ms ? 8 : dbd;
 	cmd[1] = dbd & 0x18;	/* allows DBD and LLBA bits */
 	cmd[2] = modepage;
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3ed836d..f8312a3 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -172,6 +172,7 @@ struct scsi_device {
 				     * because we did a bus reset. */
 	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
+	unsigned set_dbd_for_ms:1; /* Set "DBD" field in mode sense */
 	unsigned no_report_opcodes:1;	/* no REPORT SUPPORTED OPERATION CODES */
 	unsigned no_write_same:1;	/* no WRITE SAME command */
 	unsigned use_16_for_rw:1; /* Use read/write(16) over read/write(10) */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Martin K. Petersen]

Can,

> UFS JEDEC standards require DBD field to be set to 1 in mode sense
> command.  This patch allows LLD to define the setting of DBD if
> required.

These changes look OK to me, btw. Please make sure each patch has a
changelog which reflects how the review feedback was addressed.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v4 2/5] scsi: ufs: Use DBD setting in mode sense

[Can Guo]

UFS standards requires DBD field to be set to 1 in mode sense(10).

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).
When host send mode sense for page caching, as a result of the
CHECK_CONDITION response, host assumes that the device doesn't support
the cache feature and doesn't send SYNCHRONIZE_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 as required in UFS standards.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c28c144..5484177 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4596,6 +4596,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
 	/* Mode sense(6) is not supported by UFS, so use Mode sense(10) */
 	sdev->use_10_for_ms = 1;
 
+	/* DBD field should be set to 1 in mode sense(10) */
+	sdev->set_dbd_for_ms = 1;
+
 	/* allow SCSI layer to restart the device in case of errors */
 	sdev->allow_restart = 1;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [EXT] [PATCH v4 2/5] scsi: ufs: Use DBD setting in mode sense

[Bean Huo (beanhuo)]

.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>

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

[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.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5484177..9e44506 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -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 */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [EXT] [PATCH v4 3/5] scsi: ufs: Release clock if DMA map fails

[Bean Huo (beanhuo)]

> 
> In queuecommand path, if DMA map fails, it bails out with clock held.
> In this case, release the clock to keep its usage paired.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>

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

[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.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 20 ++++++++++++++++++++
 drivers/scsi/ufs/unipro.h | 11 +++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9e44506..086d359 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -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);
 
diff --git a/drivers/scsi/ufs/unipro.h b/drivers/scsi/ufs/unipro.h
index f539f87..3dc4d8b 100644
--- a/drivers/scsi/ufs/unipro.h
+++ b/drivers/scsi/ufs/unipro.h
@@ -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,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v4 4/5] scsi: ufs: Do not clear the DL layer timers

[Avri Altman]

 
> 
> 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.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by Avri Altman <avri.altman@wdc.com>

RE: [PATCH v4 4/5] scsi: ufs: Do not clear the DL layer timers

[Avri Altman]

> >
> > 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.
> >
> > Signed-off-by: Can Guo <cang@codeaurora.org>
> Reviewed-by Avri Altman <avri.altman@wdc.com>
BTW, I noticed that you are only updating the TC0 registers.
Why not setting the TC1 registers as well?

Thanks,
Avri

Re: [PATCH v4 4/5] scsi: ufs: Do not clear the DL layer timers

[cang]

On 2019-11-25 15:22, Avri Altman wrote:
>> >
>> > 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.
>> >
>> > Signed-off-by: Can Guo <cang@codeaurora.org>
>> Reviewed-by Avri Altman <avri.altman@wdc.com>
> BTW, I noticed that you are only updating the TC0 registers.
> Why not setting the TC1 registers as well?
> 
> Thanks,
> Avri

Hi Avri,

In the HCI spec, it goes

Currently, UFS uses TC0 only. Therefore, setting the following values 
are not needed:
 DME_ Local_FC1ProtectionTimeOutVal
 DME_ Local_TC1ReplayTimeOutVal
 DME_ Local_ AFC1ReqTimeOutVal

Best Regards,
Can Guo.

RE: [PATCH v4 4/5] scsi: ufs: Do not clear the DL layer timers

[Avri Altman]

> 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.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

RE: [EXT] RE: [PATCH v4 4/5] scsi: ufs: Do not clear the DL layer timers

[Bean Huo (beanhuo)]

> >
> > Signed-off-by: Can Guo <cang@codeaurora.org>
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>

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

[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.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 086d359..c449b68 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -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;
 	}
 }
@@ -7930,9 +7922,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)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v4 5/5] scsi: ufs: Do not free irq in suspend

[Avri Altman]

 
> 
> 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.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Your approach seems reasonable,
However I failed to locate in the kernel PM_QOS_REQ_AFFINE_IRQ,
Just in codeaurora.

Is the WARN_ON in free_irq still applies?

Thanks,
Avri

Re: [PATCH v4 5/5] scsi: ufs: Do not free irq in suspend

[cang]

On 2019-11-20 23:36, Avri Altman wrote:
>> 
>> 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.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
> Your approach seems reasonable,
> However I failed to locate in the kernel PM_QOS_REQ_AFFINE_IRQ,
> Just in codeaurora.
> 
> Is the WARN_ON in free_irq still applies?
> 
> Thanks,
> Avri

Hi Avri,

Thanks for pointing. It seems PM_QOS_REQ_AFFINE_IRQ is not present
on upstream yet. But this change is still applicable.
How about changing the commit message to below?

"Since ufshcd irq resource is allocated with the device resource
management aware IRQ request implementation, we don't really need
to free up irq during suspend, disabling it during suspend and
reenabling it during resume should be good enough."

Thanks,
Can Guo

RE: [PATCH v4 5/5] scsi: ufs: Do not free irq in suspend

[Avri Altman]

 
> 
> On 2019-11-20 23:36, Avri Altman wrote:
> >>
> >> 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.
> >>
> >> Signed-off-by: Can Guo <cang@codeaurora.org>
> > Your approach seems reasonable,
> > However I failed to locate in the kernel PM_QOS_REQ_AFFINE_IRQ, Just
> > in codeaurora.
> >
> > Is the WARN_ON in free_irq still applies?
> >
> > Thanks,
> > Avri
> 
> Hi Avri,
> 
> Thanks for pointing. It seems PM_QOS_REQ_AFFINE_IRQ is not present on
> upstream yet. But this change is still applicable.
> How about changing the commit message to below?
> 
> "Since ufshcd irq resource is allocated with the device resource management
> aware IRQ request implementation, we don't really need to free up irq during
> suspend, disabling it during suspend and reenabling it during resume should be
> good enough."

Looks good to me.
Thanks,
Avri

> 
> Thanks,
> Can Guo