RE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Daejun Park]

> Why not just allow for max-active-regions per the device's configuration?
> The platform vendor can provision it per its need.
The max-active-region is configured as device config. The module parameter which you mentioned is just minimum value of the memory pool.

> > +
> > +       total_srgn_cnt = hpb->srgns_per_lu;
> > +       for (rgn_idx = 0, srgn_cnt = 0; rgn_idx < hpb->rgns_per_lu;
> > +            rgn_idx++, total_srgn_cnt -= srgn_cnt) {
> Maybe simplify and improve readability by moving the srgn_cnt into the for clause:
> 	int srgn_cnt = hpb->srgns_per_rgn;
OK, I will apply this for patch v2.

> > +
> > +static void ufshpb_destroy_region_tbl(struct ufshpb_lu *hpb)
> > +{
> > +       int rgn_idx;
> > +
> > +       for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
> > +               struct ufshpb_region *rgn;
> > +
> > +               rgn = hpb->rgn_tbl + rgn_idx;
> > +               if (rgn->rgn_state != HPB_RGN_INACTIVE) {
> > +                       rgn->rgn_state = HPB_RGN_INACTIVE;
> > +
> > +                       ufshpb_destroy_subregion_tbl(hpb, rgn);
> > +               }
> > +
> > +               kvfree(rgn->srgn_tbl);
> This looks like it belongs to ufshpb_destroy_subregion_tbl?
Yes, it will be changed.

> How about calling ufshpb_issue_hpb_reset_query right after ufshpb_get_dev_info?
> This way waiting for the device to complete its reset can be done while scsi is scanning the luns?
I will change the call path as follows:
- ufshpb_probe_async
  - ufshpb_get_dev_info
 ï - ufshpb_issue_hpb_reset_query 1/2 (query part)
  - ufshpb_scan_hpb_lu
 ï - ufshpb_issue_hpb_reset_query 2/2 (wait part)

> > +
> > +static void ufshpb_reset(struct ufs_hba *hba)
> > +static void ufshpb_reset_host(struct ufs_hba *hba)
> > +static void ufshpb_suspend(struct ufs_hba *hba)
> > +static void ufshpb_resume(struct ufs_hba *hba)
> The above 4 functions essentially runs the same code but set a different state.
> Maybe call a helper?
OK, I will.

> > +static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb)
> Why separate from ufs-sysfs?
> Also you might want to introduce all the stats not as part of the functional patch.
The HPB feature is implemented as a device. So, We added the hpb-sysfs separated from ufs-sysfs.

> > +
> > +static int ufshpb_get_geo_info(struct ufs_hba *hba, u8 *geo_buf,
> > +                              struct ufshpb_dev_info *hpb_dev_info)
> > +{
> > +       int hpb_device_max_active_rgns = 0;
> > +       int hpb_num_lu;
> > +
> > +       hpb_dev_info->max_num_lun =
> > +               geo_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0x00 ? 8 :
> > 32;
> You already have this in hba->dev_info.max_lu_supported
> > +
> > +       hpb_num_lu = geo_buf[GEOMETRY_DESC_HPB_NUMBER_LU];
> You should capture hpb_dev_info->max_num_lun = hpb_num_lu
You are right. And hpb_dev_info->max_num_lun will be deleted.

> > +
> > +       ret = ufshpb_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, SELECTOR,
> > +                            desc_buf, hba->desc_size.dev_desc);
> What with this SELECTOR stuff?
> Why not the default 0?
Right, SELECTOR should be 0. I will fix it. 

> What about the other hpb fields in the device descriptor:
> DEVICE_DESC_PARAM_HPB_VER and DEVICE_DESC_PARAM_HPB_CONTROL ?
I will add codes that checks these fields on initialization.

> > +unsigned int ufshpb_host_map_kbytes = 1 * 1024;
> > +module_param(ufshpb_host_map_kbytes, uint, 0644);
> > +MODULE_PARM_DESC(ufshpb_host_map_kbytes,
> > +        "ufshpb host mapping memory kilo-bytes for ufshpb memory-pool");
> You should introduce this module parameter in the patch that uses it.
OK, could you recommend good location of introducing message? At the patch letter or in the codes?

Thanks,
Daejun