[PATCH 0/4] UFS core patches

[PATCH 0/4] UFS core patches

[Bart Van Assche]

Hi Martin,

Please consider these UFS core patches for the next merge window.

Thanks,

Bart.

Bart Van Assche (4):
  scsi: ufs: Return in case of an invalid tag
  scsi: ufs: Move the 4K alignment code into the Exynos driver
  scsi: ufs: Simplify ufshcd_comp_scsi_upiu()
  scsi: ufs: Set the Command Priority (CP) flag for RT requests

 drivers/ufs/core/ufshcd.c     | 42 +++++++++++++++++++----------------
 drivers/ufs/host/ufs-exynos.c |  9 ++++++--
 include/ufs/ufs.h             |  3 ++-
 include/ufs/ufshcd.h          |  7 ++----
 4 files changed, 34 insertions(+), 27 deletions(-)

[PATCH 1/4] scsi: ufs: Return in case of an invalid tag

[Bart Van Assche]

If a tag is invalid, instead of only issuing a kernel warning, also
return. This patch suppresses Coverity warnings about left shifts with a
negative right hand operand.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index dc1285351336..5fccec3c1091 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2822,7 +2822,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	int err = 0;
 	struct ufs_hw_queue *hwq = NULL;
 
-	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
+	if (WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag))
+		return 0;
 
 	switch (hba->ufshcd_state) {
 	case UFSHCD_STATE_OPERATIONAL:
@@ -6923,8 +6924,11 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	spin_lock_irqsave(host->host_lock, flags);
 
 	task_tag = req->tag;
-	WARN_ONCE(task_tag < 0 || task_tag >= hba->nutmrs, "Invalid tag %d\n",
-		  task_tag);
+	if (WARN_ONCE(task_tag < 0 || task_tag >= hba->nutmrs,
+		      "Invalid tag %d\n", task_tag)) {
+		err = -EINVAL;
+		goto unlock;
+	}
 	hba->tmf_rqs[req->tag] = req;
 	treq->upiu_req.req_header.task_tag = task_tag;
 
@@ -6963,6 +6967,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->tmf_rqs[req->tag] = NULL;
 	__clear_bit(task_tag, &hba->outstanding_tasks);
+unlock:
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	ufshcd_release(hba);
@@ -7485,7 +7490,7 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
  * ufshcd_abort - scsi host template eh_abort_handler callback
  * @cmd: SCSI command pointer
  *
- * Return: SUCCESS or FAILED.
+ * Return: SUCCESS, FAILED or FAST_IO_FAIL.
  */
 static int ufshcd_abort(struct scsi_cmnd *cmd)
 {
@@ -7498,7 +7503,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	bool outstanding;
 	u32 reg;
 
-	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
+	if (WARN_ONCE(tag < 0, "Invalid tag %d\n", tag))
+		return FAST_IO_FAIL;
 
 	ufshcd_hold(hba);
 

[PATCH 2/4] scsi: ufs: Move the 4K alignment code into the Exynos driver

[Bart Van Assche]

The DMA alignment for the Exynos controller follows directly from the
PRDT segment size configured in ufs-exynos.c. Hence, move the DMA
alignment code into the Exynos driver source code.

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c     | 6 ++++--
 drivers/ufs/host/ufs-exynos.c | 9 +++++++--
 include/ufs/ufshcd.h          | 7 ++-----
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 5fccec3c1091..100729981738 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5098,8 +5098,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
 	struct request_queue *q = sdev->request_queue;
 
 	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
-	if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
-		blk_queue_update_dma_alignment(q, SZ_4K - 1);
+
 	/*
 	 * Block runtime-pm until all consumers are added.
 	 * Refer ufshcd_setup_links().
@@ -5115,6 +5114,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
 	 */
 	sdev->silence_suspend = 1;
 
+	if (hba->vops && hba->vops->config_scsi_dev)
+		hba->vops->config_scsi_dev(sdev);
+
 	ufshcd_crypto_register(hba, q);
 
 	return 0;
diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 3396e0388512..e5d145a2676e 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1511,6 +1511,11 @@ static int fsd_ufs_pre_link(struct exynos_ufs *ufs)
 	return 0;
 }
 
+static void exynos_ufs_config_scsi_dev(struct scsi_device *sdev)
+{
+	blk_queue_update_dma_alignment(sdev->request_queue, SZ_4K - 1);
+}
+
 static int fsd_ufs_post_link(struct exynos_ufs *ufs)
 {
 	int i;
@@ -1579,6 +1584,7 @@ static const struct ufs_hba_variant_ops ufs_hba_exynos_ops = {
 	.hibern8_notify			= exynos_ufs_hibern8_notify,
 	.suspend			= exynos_ufs_suspend,
 	.resume				= exynos_ufs_resume,
+	.config_scsi_dev		= exynos_ufs_config_scsi_dev,
 };
 
 static struct ufs_hba_variant_ops ufs_hba_exynosauto_vh_ops = {
@@ -1680,8 +1686,7 @@ static const struct exynos_ufs_drv_data exynos_ufs_drvs = {
 				  UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR |
 				  UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR |
 				  UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL |
-				  UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING |
-				  UFSHCD_QUIRK_4KB_DMA_ALIGNMENT,
+				  UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING,
 	.opts			= EXYNOS_UFS_OPT_HAS_APB_CLK_CTRL |
 				  EXYNOS_UFS_OPT_BROKEN_AUTO_CLK_CTRL |
 				  EXYNOS_UFS_OPT_BROKEN_RX_SEL_IDX |
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 7d07b256e906..e0d6590d163d 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -28,6 +28,7 @@
 
 #define UFSHCD "ufshcd"
 
+struct scsi_device;
 struct ufs_hba;
 
 enum dev_cmd_type {
@@ -371,6 +372,7 @@ struct ufs_hba_variant_ops {
 	int	(*get_outstanding_cqs)(struct ufs_hba *hba,
 				       unsigned long *ocqs);
 	int	(*config_esi)(struct ufs_hba *hba);
+	void	(*config_scsi_dev)(struct scsi_device *sdev);
 };
 
 /* clock gating state  */
@@ -596,11 +598,6 @@ enum ufshcd_quirks {
 	 */
 	UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING = 1 << 13,
 
-	/*
-	 * Align DMA SG entries on a 4 KiB boundary.
-	 */
-	UFSHCD_QUIRK_4KB_DMA_ALIGNMENT			= 1 << 14,
-
 	/*
 	 * This quirk needs to be enabled if the host controller does not
 	 * support UIC command

[PATCH 3/4] scsi: ufs: Simplify ufshcd_comp_scsi_upiu()

[Bart Van Assche]

ufshcd_comp_scsi_upiu() has one caller and that caller ensures that
lrbp->cmd != NULL. Hence leave out the lrbp->cmd check from
ufshcd_comp_scsi_upiu().

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 100729981738..c69bf532c4ab 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2714,27 +2714,19 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba *hba,
  *			   for SCSI Purposes
  * @hba: per adapter instance
  * @lrbp: pointer to local reference block
- *
- * Return: 0 upon success; < 0 upon failure.
  */
-static int ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 {
 	u8 upiu_flags;
-	int ret = 0;
 
 	if (hba->ufs_version <= ufshci_version(1, 1))
 		lrbp->command_type = UTP_CMD_TYPE_SCSI;
 	else
 		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
 
-	if (likely(lrbp->cmd)) {
-		ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, lrbp->cmd->sc_data_direction, 0);
-		ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
-	} else {
-		ret = -EINVAL;
-	}
-
-	return ret;
+	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags,
+				    lrbp->cmd->sc_data_direction, 0);
+	ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
 }
 
 /**

[PATCH 4/4] scsi: ufs: Set the Command Priority (CP) flag for RT requests

[Bart Van Assche]

Make the UFS device execute realtime (RT) requests before other requests.
This will be used in Android to reduce the I/O latency of the foreground
app.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 4 ++++
 include/ufs/ufs.h         | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index c69bf532c4ab..54c3811d5534 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2717,6 +2717,8 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba *hba,
  */
 static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 {
+	struct request *rq = scsi_cmd_to_rq(lrbp->cmd);
+	unsigned int ioprio_class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
 	u8 upiu_flags;
 
 	if (hba->ufs_version <= ufshci_version(1, 1))
@@ -2726,6 +2728,8 @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 
 	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags,
 				    lrbp->cmd->sc_data_direction, 0);
+	if (ioprio_class == IOPRIO_CLASS_RT)
+		upiu_flags |= UPIU_CMD_FLAGS_CP;
 	ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
 }
 
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 0cced88f4531..e77ab1786856 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -98,9 +98,10 @@ enum upiu_response_transaction {
 	UPIU_TRANSACTION_REJECT_UPIU	= 0x3F,
 };
 
-/* UPIU Read/Write flags */
+/* UPIU Read/Write flags. See also table "UPIU Flags" in the UFS standard. */
 enum {
 	UPIU_CMD_FLAGS_NONE	= 0x00,
+	UPIU_CMD_FLAGS_CP	= 0x04,
 	UPIU_CMD_FLAGS_WRITE	= 0x20,
 	UPIU_CMD_FLAGS_READ	= 0x40,
 };

RE: [PATCH 4/4] scsi: ufs: Set the Command Priority (CP) flag for RT requests

[Avri Altman]

 
> Make the UFS device execute realtime (RT) requests before other requests.
> This will be used in Android to reduce the I/O latency of the foreground
> app.
Maybe one more sentence, explaining that ufs CP is agnostic to scsi CDL,
And can be implemented regardless.

Thanks,
Avri
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 4 ++++
>  include/ufs/ufs.h         | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index c69bf532c4ab..54c3811d5534 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2717,6 +2717,8 @@ static int ufshcd_compose_devman_upiu(struct
> ufs_hba *hba,
>   */
>  static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb
> *lrbp)
>  {
> +       struct request *rq = scsi_cmd_to_rq(lrbp->cmd);
> +       unsigned int ioprio_class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
>         u8 upiu_flags;
> 
>         if (hba->ufs_version <= ufshci_version(1, 1))
> @@ -2726,6 +2728,8 @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba
> *hba, struct ufshcd_lrb *lrbp)
> 
>         ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags,
>                                     lrbp->cmd->sc_data_direction, 0);
> +       if (ioprio_class == IOPRIO_CLASS_RT)
> +               upiu_flags |= UPIU_CMD_FLAGS_CP;
>         ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
>  }
> 
> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
> index 0cced88f4531..e77ab1786856 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> @@ -98,9 +98,10 @@ enum upiu_response_transaction {
>         UPIU_TRANSACTION_REJECT_UPIU    = 0x3F,
>  };
> 
> -/* UPIU Read/Write flags */
> +/* UPIU Read/Write flags. See also table "UPIU Flags" in the UFS standard.
> */
>  enum {
>         UPIU_CMD_FLAGS_NONE     = 0x00,
> +       UPIU_CMD_FLAGS_CP       = 0x04,
>         UPIU_CMD_FLAGS_WRITE    = 0x20,
>         UPIU_CMD_FLAGS_READ     = 0x40,
>  };

Re: [PATCH 4/4] scsi: ufs: Set the Command Priority (CP) flag for RT requests

[Bart Van Assche]

On 9/19/23 11:37, Avri Altman wrote:
> Bart Van Assche wrote:
>> Make the UFS device execute realtime (RT) requests before other
>> requests. This will be used in Android to reduce the I/O latency of
>> the foreground app.
>
> Maybe one more sentence, explaining that ufs CP is agnostic to scsi
> CDL, And can be implemented regardless.

Hmm ... is it really necessary to add a reference to CDL in the patch
description? I'm not aware of any UFS devices that support CDL.
Additionally, if CDL will ever be supported then I think that
ufshcd_comp_scsi_upiu() should be modified such that it selects a 
duration limit instead of setting the CP bit.

Thanks,

Bart.

RE: [PATCH 1/4] scsi: ufs: Return in case of an invalid tag

[Daejun Park]

Hi Bart,

> If a tag is invalid, instead of only issuing a kernel warning, also
> return. This patch suppresses Coverity warnings about left shifts with a
> negative right hand operand.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index dc1285351336..5fccec3c1091 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2822,7 +2822,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>          int err = 0;
>          struct ufs_hw_queue *hwq = NULL;
>  
> -        WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
> +        if (WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag))
> +                return 0;

As far as I know, a return 0 from a queuecommand means that the request was accepted by LLD. 

Thanks,
Daejun

Re: [PATCH 1/4] scsi: ufs: Return in case of an invalid tag

[Bart Van Assche]

On 9/19/23 15:47, Daejun Park wrote:
 > Bart Van Assche wrote:
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index dc1285351336..5fccec3c1091 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -2822,7 +2822,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>>           int err = 0;
>>           struct ufs_hw_queue *hwq = NULL;
>>   
>> -        WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
>> +        if (WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag))
>> +                return 0;
> 
> As far as I know, a return 0 from a queuecommand means that the request was accepted by LLD.

This is on purpose. I think that it is better to issue a warning and to 
cause command processing to hang rather than to return a code that 
causes the SCSI core to resubmit a command with an invalid tag and to 
cause an infinite loop.

Thanks,

Bart.

Re: [PATCH 1/4] scsi: ufs: Return in case of an invalid tag

[John Garry]

On 20/09/2023 00:29, Bart Van Assche wrote:
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index dc1285351336..5fccec3c1091 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -2822,7 +2822,8 @@ static int ufshcd_queuecommand(struct Scsi_Host 
>> *host, struct scsi_cmnd *cmd)
>>           int err = 0;
>>           struct ufs_hw_queue *hwq = NULL;
>> -        WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", 
>> tag);

How is tag < 0 even possible? Indeed, I doubt the tag >= hba->nutrs 
check also, since shost->can_queue is set to hba->nutrs - 
UFSHCD_NUM_RESERVED AFAICS

John

>> +        if (WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag 
>> %d\n", tag))
>> +                return 0;
> 
> As far as I know, a return 0 from a queuecommand means that the request 
> was accepted by LLD.

Re: [PATCH 1/4] scsi: ufs: Return in case of an invalid tag

[Bart Van Assche]

On 9/20/23 00:13, John Garry wrote:
> On 20/09/2023 00:29, Bart Van Assche wrote:
>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>> index dc1285351336..5fccec3c1091 100644
>>> --- a/drivers/ufs/core/ufshcd.c
>>> +++ b/drivers/ufs/core/ufshcd.c
>>> @@ -2822,7 +2822,8 @@ static int ufshcd_queuecommand(struct Scsi_Host 
>>> *host, struct scsi_cmnd *cmd)
>>>           int err = 0;
>>>           struct ufs_hw_queue *hwq = NULL;
>>> -        WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", 
>>> tag);
> 
> How is tag < 0 even possible? Indeed, I doubt the tag >= hba->nutrs 
> check also, since shost->can_queue is set to hba->nutrs - 
> UFSHCD_NUM_RESERVED AFAICS

Hi John,

In my opinion neither tag < 0 nor tag >= hba->nutrs can happen. Hence, 
another possibility is to remove the above warning statement.

Thanks,

Bart.