[PATCH 0/4] scsi: ufs: Few HPB fixes

[PATCH 0/4] scsi: ufs: Few HPB fixes

[Avri Altman]

Hi Martin,

This patch series include several hpb fixes, most of them host mode.
Please consider this patch series for kernel v5.15.

Thanks,
Avri

Avri Altman (4):
  scsi: ufshpb: re-wind the read timeout on every read
  scsi: ufshpb: Use a correct max multi chunk
  scsi: ufshpb: Verify that num_inflight_map_req is non-negative
  scsi: ufshpb: Do not report victim error in HCM

 drivers/scsi/ufs/ufshpb.c | 29 +++++++++++++++++++++++++----
 drivers/scsi/ufs/ufshpb.h |  6 ++++--
 2 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/4] scsi: ufshpb: re-wind the read timeout on every read

[Avri Altman]

The "cold"-timer purpose is not to hang-on to active regions with no
reads.  Therefore the read-timeout should be re-wind on every read, and
not just when the region is activated.

Fixes: 13c044e91678 (scsi: ufs: ufshpb: Add "cold" regions timer)
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index d0eb14be47a3..8e92c61ed9d4 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -178,9 +178,19 @@ static void ufshpb_iterate_rgn(struct ufshpb_lu *hpb, int rgn_idx, int srgn_idx,
 		set_bit_len = cnt;
 
 	spin_lock_irqsave(&hpb->rgn_state_lock, flags);
-	if (set_dirty && rgn->rgn_state != HPB_RGN_INACTIVE &&
-	    srgn->srgn_state == HPB_SRGN_VALID)
-		bitmap_set(srgn->mctx->ppn_dirty, srgn_offset, set_bit_len);
+	if (rgn->rgn_state != HPB_RGN_INACTIVE) {
+		if (set_dirty) {
+			if (srgn->srgn_state == HPB_SRGN_VALID)
+				bitmap_set(srgn->mctx->ppn_dirty, srgn_offset,
+					   set_bit_len);
+		} else if (hpb->is_hcm) {
+			 /* rewind the read timer for lru regions */
+			rgn->read_timeout = ktime_add_ms(ktime_get(),
+					rgn->hpb->params.read_timeout_ms);
+			rgn->read_timeout_expiries =
+				rgn->hpb->params.read_timeout_expiries;
+		}
+	}
 	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
 
 	if (hpb->is_hcm && prev_srgn != srgn) {
-- 
2.17.1

[PATCH 2/4] scsi: ufshpb: Use a correct max multi chunk

[Avri Altman]

In HPB2.0, if pre_req_min_tr_len < transfer_len < pre_req_max_tr_len
the driver is expected to send a HPB-WRITE-BUFFER companion to HPB-READ.

The upper bound should fit into a single byte, regardless of
bMAX_ DATA_SIZE_FOR_HPB_SINGLE_CMD which being an attribute (u32) can
be significantly larger.

To further illustrate the issue let us consider the following scenario:
 - SCSI_DEFAULT_MAX_SECTORS is 1024 limiting the IO chunks to 512KB
 - The OEM changes scsi_host_template .max_sectors to be 2048, which
   allows a 1M requests: transfer_len = 256
 - pre_req_max_tr_len = HPB_MULTI_CHUNK_HIGH = 256
 - ufshpb_is_supported_chunk returns true (256 <= 256)
 - WARN_ON_ONCE(256 > 256) doesn't warn
 - ufshpb_set_hpb_read_to_upiu cast transfer_len to u8: transfer_len = 0
 - the command is failing with illegal request

Fixes: 41d8a9333cc9 (scsi: ufs: ufshpb: Add HPB 2.0 support)
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index c74a6c35a446..6df317dfe034 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -32,7 +32,7 @@
 /* hpb support chunk size */
 #define HPB_LEGACY_CHUNK_HIGH			1
 #define HPB_MULTI_CHUNK_LOW			7
-#define HPB_MULTI_CHUNK_HIGH			256
+#define HPB_MULTI_CHUNK_HIGH			255
 
 /* hpb vender defined opcode */
 #define UFSHPB_READ				0xF8
-- 
2.17.1

[PATCH 3/4] scsi: ufshpb: Verify that num_inflight_map_req is non-negative

[Avri Altman]

num_inflight_map_req should not be negative.  It is incremented and
decremented without any protection, allowing it theoretically to be
negative, should some weird unbalanced count occur.

Verify that the those calls are properly serialized.

Fixes: 33845a2d844b (scsi: ufs: ufshpb: Limit the number of in-flight map requests)
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 10 ++++++++++
 drivers/scsi/ufs/ufshpb.h |  4 +++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 8e92c61ed9d4..cd48367f94cc 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -756,6 +756,7 @@ static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
 {
 	struct ufshpb_req *map_req;
 	struct bio *bio;
+	unsigned long flags;
 
 	if (hpb->is_hcm &&
 	    hpb->num_inflight_map_req >= hpb->params.inflight_map_req) {
@@ -780,7 +781,10 @@ static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
 
 	map_req->rb.srgn_idx = srgn->srgn_idx;
 	map_req->rb.mctx = srgn->mctx;
+
+	spin_lock_irqsave(&hpb->param_lock, flags);
 	hpb->num_inflight_map_req++;
+	spin_unlock_irqrestore(&hpb->param_lock, flags);
 
 	return map_req;
 }
@@ -788,9 +792,14 @@ static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
 static void ufshpb_put_map_req(struct ufshpb_lu *hpb,
 			       struct ufshpb_req *map_req)
 {
+	unsigned long flags;
+
 	bio_put(map_req->bio);
 	ufshpb_put_req(hpb, map_req);
+
+	spin_lock_irqsave(&hpb->param_lock, flags);
 	hpb->num_inflight_map_req--;
+	spin_unlock_irqrestore(&hpb->param_lock, flags);
 }
 
 static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
@@ -2387,6 +2396,7 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 
 	spin_lock_init(&hpb->rgn_state_lock);
 	spin_lock_init(&hpb->rsp_list_lock);
+	spin_lock_init(&hpb->param_lock);
 
 	INIT_LIST_HEAD(&hpb->lru_info.lh_lru_rgn);
 	INIT_LIST_HEAD(&hpb->lh_act_srgn);
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 6df317dfe034..a79e07398970 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -237,7 +237,9 @@ struct ufshpb_lu {
 	struct ufshpb_req *pre_req;
 	int num_inflight_pre_req;
 	int throttle_pre_req;
-	int num_inflight_map_req;
+	int num_inflight_map_req; /* hold param_lock */
+	spinlock_t param_lock;
+
 	struct list_head lh_pre_req_free;
 	int cur_read_id;
 	int pre_req_min_tr_len;
-- 
2.17.1

[PATCH 4/4] scsi: ufshpb: Do not report victim error in HCM

[Avri Altman]

In host control mode, eviction is perceived as an extreme measure.
There are several conditions that both the entering and exiting regions
should meet, so that eviction will take place.

The common case however, is that those conditions are rarely met, so it
is normal that the act of eviction fails.  Therefore, Do not report an
error in host control mode if eviction fails.

Fixes: 6c59cb501b86 (scsi: ufs: ufshpb: Make eviction depend on region's reads)
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index cd48367f94cc..aafb55136c7e 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1385,7 +1385,8 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 			victim_rgn = ufshpb_victim_lru_info(hpb);
 			if (!victim_rgn) {
 				dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
-				    "cannot get victim region error\n");
+				    "cannot get victim region %s\n",
+				    hpb->is_hcm ? "" : "error");
 				ret = -ENOMEM;
 				goto out;
 			}
-- 
2.17.1

Re: [PATCH 0/4] scsi: ufs: Few HPB fixes

[Martin K. Petersen]

Avri,

> This patch series include several hpb fixes, most of them host mode.
> Please consider this patch series for kernel v5.15.

Applied to 5.15/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/4] scsi: ufs: Few HPB fixes

[Martin K. Petersen]

On Sun, 8 Aug 2021 12:00:20 +0300, Avri Altman wrote:

> This patch series include several hpb fixes, most of them host mode.
> Please consider this patch series for kernel v5.15.
> 
> Thanks,
> Avri
> 
> Avri Altman (4):
>   scsi: ufshpb: re-wind the read timeout on every read
>   scsi: ufshpb: Use a correct max multi chunk
>   scsi: ufshpb: Verify that num_inflight_map_req is non-negative
>   scsi: ufshpb: Do not report victim error in HCM
> 
> [...]

Applied to 5.15/scsi-queue, thanks!

[1/4] scsi: ufshpb: re-wind the read timeout on every read
      https://git.kernel.org/mkp/scsi/c/283e61c5a9be
[2/4] scsi: ufshpb: Use a correct max multi chunk
      https://git.kernel.org/mkp/scsi/c/07106f86ae13
[3/4] scsi: ufshpb: Verify that num_inflight_map_req is non-negative
      https://git.kernel.org/mkp/scsi/c/22aede9f48b6
[4/4] scsi: ufshpb: Do not report victim error in HCM
      https://git.kernel.org/mkp/scsi/c/10163cee1f06

-- 
Martin K. Petersen	Oracle Linux Engineering