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

[Bean Huo]

On Fri, 2021-01-29 at 14:30 +0900, Daejun Park wrote:
> +static void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
> +                                          int srgn_idx, int
> srgn_mem_size)
> +{
> +       cdb[0] = UFSHPB_READ_BUFFER;
> +       cdb[1] = UFSHPB_READ_BUFFER_ID;
> +
> +       put_unaligned_be16(rgn_idx, &cdb[2]);
> +       put_unaligned_be16(srgn_idx, &cdb[4]);
> +       put_unaligned_be24(srgn_mem_size, &cdb[6]);
> +
> +       cdb[9] = 0x00;
> +}
> +
> +static int ufshpb_execute_map_req(struct ufshpb_lu *hpb,
> +                                 struct ufshpb_req *map_req)
> +{
> +       struct request_queue *q;
> +       struct request *req;
> +       struct scsi_request *rq;
> +       int ret = 0;
> +       int i;
> +
> +       q = hpb->sdev_ufs_lu->request_queue;
> +       for (i = 0; i < hpb->pages_per_srgn; i++) {
> +               ret = bio_add_pc_page(q, map_req->bio, map_req->mctx-
> >m_page[i],
> +                                     PAGE_SIZE, 0);
> +               if (ret != PAGE_SIZE) {
> +                       dev_err(&hpb->sdev_ufs_lu->sdev_dev,
> +                                  "bio_add_pc_page fail %d - %d\n",
> +                                  map_req->rgn_idx, map_req-
> >srgn_idx);
> +                       return ret;
> +               }
> +       }
> +
> +       req = map_req->req;
> +
> +       blk_rq_append_bio(req, &map_req->bio);
> +
> +       req->end_io_data = map_req;
> +
> +       rq = scsi_req(req);
> +       ufshpb_set_read_buf_cmd(rq->cmd, map_req->rgn_idx,
> +                               map_req->srgn_idx, hpb-
> >srgn_mem_size);

Hi Daejun

Thanks for your hard-working on the HPB driver.

I found you didn't take into account of allocation Length of the last
sub-region of the last region.

UFS HPB spec: 

"If the requested field of the HPB Region or HPB Sub-Region is out of
range, then the device shall terminate the command by sending RESPONSE
UPIU with CHECK CONDITION status, with the SENSE KEY set to ILLEGAL
REQUEST, and the additional sense code set to INVALID FIELD IN CDB"


Below codes are from my RFC HPB patchset:

https://patchwork.kernel.org/project/linux-scsi/patch/20200504142032.16619-6-beanhuo@xxxxxxxxxx/

+	alloc_len = hpb->hba->hpb_geo.subregion_entry_sz;
+	/*
+	 * HPB Sub-Regions are equally sized except for the last one
which is
+	 * smaller if the last hpb Region is not an integer multiple of
+	 * bHPBSubRegionSize.
+	 */
+	if (map_req->region == (hpb->lu_region_cnt - 1) &&
+	    map_req->subregion == (hpb->subregions_in_last_region - 1))
+		alloc_len = hpb->last_subregion_entry_size;
+
+	ufshpb_prepare_read_buf_cmd(ureq->cmd, map_req->region,
+				    map_req->subregion, alloc_len);
+	if (!ureq->req) {
+		ureq->req = blk_get_request(q, REQ_OP_SCSI_IN, 0);
+		if (IS_ERR(ureq->req)) {
+			ret =  PTR_ERR(ureq->req);
+			goto free_mem;
+		}
+	}

please fix it in your next version patch. thanks.


Kind regards,
Bean