* [PATCH v2 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD
2019-10-29 3:10 [PATCH v2 0/5] UFS driver general fixes bundle 1 Can Guo
@ 2019-10-29 3:10 ` Can Guo
2019-10-31 15:20 ` Mark Salyzyn
2019-10-29 3:10 ` [PATCH v2 2/5] scsi: ufs: Set DBD setting in mode sense for caching mode page Can Guo
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Can Guo @ 2019-10-29 3:10 UTC (permalink / raw)
To: asutoshd, nguyenb, rnayak, linux-scsi, kernel-team, saravanak,
salyzyn, cang
Cc: James E.J. Bottomley, Martin K. Petersen, open list
Host sends MODE_SENSE_10 with caching mode page, to check if the device
supports the cache feature.
UFS JEDEC standards require DBD field to be set to 1.
This patch allows LLD to define the setting of DBD if required.
Signed-off-by: Can Guo <cang@codeaurora.org>
---
drivers/scsi/sd.c | 6 +++++-
include/scsi/scsi_host.h | 6 ++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index aab4ed8..6d8194f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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 */
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 2c3f0c5..3900987 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -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];
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD
2019-10-29 3:10 ` [PATCH v2 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD Can Guo
@ 2019-10-31 15:20 ` Mark Salyzyn
2019-10-31 15:40 ` Mark Salyzyn
0 siblings, 1 reply; 9+ messages in thread
From: Mark Salyzyn @ 2019-10-31 15:20 UTC (permalink / raw)
To: Can Guo, asutoshd, nguyenb, rnayak, linux-scsi, kernel-team,
saravanak, salyzyn
Cc: James E.J. Bottomley, Martin K. Petersen, open list
On 10/28/19 8:10 PM, Can Guo wrote:
> Host sends MODE_SENSE_10 with caching mode page, to check if the device
> supports the cache feature.
> UFS JEDEC standards require DBD field to be set to 1.
>
> This patch allows LLD to define the setting of DBD if required.
>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
> drivers/scsi/sd.c | 6 +++++-
> include/scsi/scsi_host.h | 6 ++++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index aab4ed8..6d8194f 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -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;
variable locality
> 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;
> }
>
This simplifies to:
- } else if (sdp->type == TYPE_RBC) {
+ } else if (sdp->type == TYPE_RBC || sdp->host->set_dbd_for_caching) {
> /* cautiously ask */
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 2c3f0c5..3900987 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -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];
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD
2019-10-31 15:20 ` Mark Salyzyn
@ 2019-10-31 15:40 ` Mark Salyzyn
2019-11-01 0:18 ` cang
0 siblings, 1 reply; 9+ messages in thread
From: Mark Salyzyn @ 2019-10-31 15:40 UTC (permalink / raw)
To: Can Guo, asutoshd, nguyenb, rnayak, linux-scsi, kernel-team,
saravanak, salyzyn
Cc: James E.J. Bottomley, Martin K. Petersen, open list
On 10/31/19 8:20 AM, Mark Salyzyn wrote:
> On 10/28/19 8:10 PM, Can Guo wrote:
>> Host sends MODE_SENSE_10 with caching mode page, to check if the device
>> supports the cache feature.
>> UFS JEDEC standards require DBD field to be set to 1.
>>
>> This patch allows LLD to define the setting of DBD if required.
>>
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>> drivers/scsi/sd.c | 6 +++++-
>> include/scsi/scsi_host.h | 6 ++++++
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index aab4ed8..6d8194f 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -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;
> variable locality
>> 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;
>> }
>
> This simplifies to:
>
> - } else if (sdp->type == TYPE_RBC) {
>
> + } else if (sdp->type == TYPE_RBC || sdp->host->set_dbd_for_caching) {
IDK what happened with my mailer sending out an older infant copy (blame
on fumble fingers). My final copy was instead the simplification:
+ dbd = sdp->host->set_dbd_for_caching ? 8 : 0;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD
2019-10-31 15:40 ` Mark Salyzyn
@ 2019-11-01 0:18 ` cang
0 siblings, 0 replies; 9+ messages in thread
From: cang @ 2019-11-01 0:18 UTC (permalink / raw)
To: Mark Salyzyn
Cc: asutoshd, nguyenb, rnayak, linux-scsi, kernel-team, saravanak,
salyzyn, James E.J. Bottomley, Martin K. Petersen, open list
On 2019-10-31 23:40, Mark Salyzyn wrote:
> On 10/31/19 8:20 AM, Mark Salyzyn wrote:
>> On 10/28/19 8:10 PM, Can Guo wrote:
>>> Host sends MODE_SENSE_10 with caching mode page, to check if the
>>> device
>>> supports the cache feature.
>>> UFS JEDEC standards require DBD field to be set to 1.
>>>
>>> This patch allows LLD to define the setting of DBD if required.
>>>
>>> Signed-off-by: Can Guo <cang@codeaurora.org>
>>> ---
>>> drivers/scsi/sd.c | 6 +++++-
>>> include/scsi/scsi_host.h | 6 ++++++
>>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>>> index aab4ed8..6d8194f 100644
>>> --- a/drivers/scsi/sd.c
>>> +++ b/drivers/scsi/sd.c
>>> @@ -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;
>> variable locality
>>> 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;
>>> }
>>
>> This simplifies to:
>>
>> - } else if (sdp->type == TYPE_RBC) {
>>
>> + } else if (sdp->type == TYPE_RBC ||
>> sdp->host->set_dbd_for_caching) {
>
> IDK what happened with my mailer sending out an older infant copy
> (blame on fumble fingers). My final copy was instead the
> simplification:
>
> + dbd = sdp->host->set_dbd_for_caching ? 8 : 0;
Thank you for your review.
Regards,
Can Guo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/5] scsi: ufs: Set DBD setting in mode sense for caching mode page
2019-10-29 3:10 [PATCH v2 0/5] UFS driver general fixes bundle 1 Can Guo
2019-10-29 3:10 ` [PATCH v2 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD Can Guo
@ 2019-10-29 3:10 ` Can Guo
2019-10-29 3:10 ` [PATCH v2 3/5] scsi: ufs: Release clock if DMA map fails Can Guo
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2019-10-29 3:10 UTC (permalink / raw)
To: asutoshd, nguyenb, rnayak, linux-scsi, kernel-team, saravanak,
salyzyn, cang
Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
Martin K. Petersen, Stanley Chu, Bean Huo, Tomas Winkler,
Subhash Jadavani, open list
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.
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 c28c144..101b4d0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -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;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 3/5] scsi: ufs: Release clock if DMA map fails
2019-10-29 3:10 [PATCH v2 0/5] UFS driver general fixes bundle 1 Can Guo
2019-10-29 3:10 ` [PATCH v2 1/5] scsi: Adjust DBD setting in mode sense for caching mode page per LLD Can Guo
2019-10-29 3:10 ` [PATCH v2 2/5] scsi: ufs: Set DBD setting in mode sense for caching mode page Can Guo
@ 2019-10-29 3:10 ` Can Guo
2019-10-29 3:10 ` [PATCH v2 4/5] scsi: ufs: Do not clear the DL layer timers Can Guo
2019-10-29 3:10 ` [PATCH v2 5/5] scsi: ufs: Do not free irq in suspend Can Guo
4 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2019-10-29 3:10 UTC (permalink / raw)
To: asutoshd, nguyenb, rnayak, linux-scsi, kernel-team, saravanak,
salyzyn, cang
Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
Martin K. Petersen, Stanley Chu, Bean Huo, Tomas Winkler,
Subhash Jadavani, open list
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 101b4d0..6e9236a 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 4/5] scsi: ufs: Do not clear the DL layer timers
2019-10-29 3:10 [PATCH v2 0/5] UFS driver general fixes bundle 1 Can Guo
` (2 preceding siblings ...)
2019-10-29 3:10 ` [PATCH v2 3/5] scsi: ufs: Release clock if DMA map fails Can Guo
@ 2019-10-29 3:10 ` Can Guo
2019-10-29 3:10 ` [PATCH v2 5/5] scsi: ufs: Do not free irq in suspend Can Guo
4 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2019-10-29 3:10 UTC (permalink / raw)
To: asutoshd, nguyenb, rnayak, linux-scsi, kernel-team, saravanak,
salyzyn, cang
Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
Martin K. Petersen, Stanley Chu, Bean Huo, Tomas Winkler,
Subhash Jadavani, Allison Randal, Greg Kroah-Hartman,
Thomas Gleixner, open list
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 6e9236a..34efa8a 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 5/5] scsi: ufs: Do not free irq in suspend
2019-10-29 3:10 [PATCH v2 0/5] UFS driver general fixes bundle 1 Can Guo
` (3 preceding siblings ...)
2019-10-29 3:10 ` [PATCH v2 4/5] scsi: ufs: Do not clear the DL layer timers Can Guo
@ 2019-10-29 3:10 ` Can Guo
4 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2019-10-29 3:10 UTC (permalink / raw)
To: asutoshd, nguyenb, rnayak, linux-scsi, kernel-team, saravanak,
salyzyn, cang
Cc: Alim Akhtar, Avri Altman, Pedro Sousa, James E.J. Bottomley,
Martin K. Petersen, Stanley Chu, Bean Huo, Tomas Winkler,
Subhash Jadavani, open list
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 34efa8a..fb36a51 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;
}
}
@@ -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)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 9+ messages in thread