RE: Re: [PATCH v19 2/3] scsi: ufs: L2P map management for HPB read

+

quoted

+static void ufshpb_kick_map_work(struct ufshpb_lu *hpb)
+{
+        bool ret = true;

 
-> ret = false;
 

quoted

+        unsigned long flags;
+
+        spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+        if (!list_empty(&hpb->lh_inact_rgn) || 
!list_empty(&hpb->lh_act_srgn))
+                ret = false;

 
-> ret = true;

Thanks, I will fix it.
  

quoted

+        spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+
+        if (ret)
+                queue_work(ufshpb_wq, &hpb->map_work);
+}
+
+static bool ufshpb_is_hpb_rsp_valid(struct ufs_hba *hba,
+                                         struct ufshcd_lrb *lrbp,
+                                         struct utp_hpb_rsp *rsp_field)
+{
+        if (be16_to_cpu(rsp_field->sense_data_len) != DEV_SENSE_SEG_LEN ||
+            rsp_field->desc_type != DEV_DES_TYPE ||
+            rsp_field->additional_len != DEV_ADDITIONAL_LEN ||
+            rsp_field->hpb_op == HPB_RSP_NONE ||

 
HPB_RSP_NONE is checked again in switch-case, no need of this line.

OK, I agree.

quoted

+static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
+                                         struct utp_hpb_rsp *rsp_field)
+{
+        int i, rgn_idx, srgn_idx;
+
+        BUILD_BUG_ON(sizeof(struct ufshpb_active_field) != 
HPB_ACT_FIELD_SIZE);
+        /*
+         * If the active region and the inactive region are the same,
+         * we will inactivate this region.
+         * The device could check this (region inactivated) and
+         * will response the proper active region information
+         */
+        spin_lock(&hpb->rsp_list_lock);
+        for (i = 0; i < rsp_field->active_rgn_cnt; i++) {
+                rgn_idx =
+                        be16_to_cpu(rsp_field->hpb_active_field[i].active_rgn);
+                srgn_idx =
+                        be16_to_cpu(rsp_field->hpb_active_field[i].active_srgn);
+
+                dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
+                        "activate(%d) region %d - %d\n", i, rgn_idx, srgn_idx);
+                ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
+                hpb->stats.rb_active_cnt++;
+        }
+
+        for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
+                rgn_idx = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
+                dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
+                        "inactivate(%d) region %d\n", i, rgn_idx);
+                ufshpb_update_inactive_info(hpb, rgn_idx);
+                hpb->stats.rb_inactive_cnt++;
+        }
+        spin_unlock(&hpb->rsp_list_lock);
+
+        dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "Noti: #ACT %u #INACT %u\n",
+                rsp_field->active_rgn_cnt, rsp_field->inactive_rgn_cnt);
+
+        queue_work(ufshpb_wq, &hpb->map_work);
+}
+
+/*
+ * This function will parse recommended active subregion information 
in sense
+ * data field of response UPIU with SAM_STAT_GOOD state.
+ */
+void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+{
+        struct ufshpb_lu *hpb = ufshpb_get_hpb_data(lrbp->cmd->device);
+        struct utp_hpb_rsp *rsp_field;
+        int data_seg_len;
+
+        if (!hpb)
+                return;

 
You are assuming HPB recommandations only come in responses to LUs
with HPB enabled, but the specs says the recommandations can come
in any responses with GOOD status, meaning you should check the *hpb
which belongs to the LUN in res_field, but not the one belongs to
lrbp->cmd->device.

I will add codes for checking lun to prevent getting wrong HPB
recommandations.

Thanks,
Daejun