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

[Avri Altman]

> 
> This is a patch for the HPB module.
> The HPB module queries UFS for device information during initialization.
> We added the export symbol to two functions in ufshcd.c to initialize
> the HPB module.
> 
> The HPB module can be loaded or built-in as needed.
> The memory pool size used in the HPB module is implemented as a module
> parameter, so that it can be configurable by the user.
Why not just allow for max-active-regions per the device's configuration?
The platform vendor can provision it per its need.


> +
> +static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu
> *hpb)
> +{
> +       struct ufshpb_region *rgn_table, *rgn;
> +       struct ufshpb_subregion *srgn;
> +       int rgn_idx, srgn_idx, total_srgn_cnt, srgn_cnt, i;
> +       int ret = 0;
> +
> +       rgn_table = kvcalloc(hpb->rgns_per_lu, sizeof(struct ufshpb_region),
> +                           GFP_KERNEL);
> +       if (!rgn_table)
> +               return -ENOMEM;
> +
> +       hpb->rgn_tbl = rgn_table;
> +
> +       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;

> +               rgn = rgn_table + rgn_idx;
> +               rgn->rgn_idx = rgn_idx;
> +
> +               srgn_cnt = min(total_srgn_cnt, hpb->srgns_per_rgn);
I guess you are carefully counting the sbregions because the spec allows the lun not to be subregion aligned.
So for any region but the last its hpb->srgns_per_rgn, and for the last one its:
	If (rgn_idx == hpb->rgns_per_lu - 1)
		srgn_cnt = ((hpb->srgns_per_lu - 1) % hpb->srgns_per_rgn) + 1;

> +
> +               ret = ufshpb_alloc_subregion_tbl(hpb, rgn, srgn_cnt);
> +               if (ret)
> +                       goto release_srgn_table;
> +               ufshpb_init_subregion_tbl(hpb, rgn);
> +
> +               rgn->rgn_state = HPB_RGN_INACTIVE;
> +               }
> +       }
> +
> +       if (total_srgn_cnt != 0) {
And you won't be needing this anymore

> +               dev_err(hba->dev, "ufshpb(%d) error total_subregion_count %d",
> +                       hpb->lun, total_srgn_cnt);
> +               goto release_srgn_table;
> +       }
> +
> +       return 0;
> +release_srgn_table:
> +       for (i = 0; i < rgn_idx; i++) {
> +               rgn = rgn_table + i;
> +               if (rgn->srgn_tbl)
> +                       kvfree(rgn->srgn_tbl);
> +       }
> +       kvfree(rgn_table);
> +       return ret;
> +}
> +
> +static void ufshpb_destroy_subregion_tbl(struct ufshpb_lu *hpb,
> +                                        struct ufshpb_region *rgn)
> +{
> +       int srgn_idx;
> +
> +       for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
> +               struct ufshpb_subregion *srgn;
> +
> +               srgn = rgn->srgn_tbl + srgn_idx;
> +               srgn->srgn_state = HPB_SRGN_UNUSED;
> +       }
> +}
> +
> +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?

> +       }
> +
> +       kvfree(hpb->rgn_tbl);
> +}
> +


> +static void ufshpb_issue_hpb_reset_query(struct ufs_hba *hba)

> +               return;
> +       }
> +       /* wait for the device to complete HPB reset query */
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?

> +
> +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?

> +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.
> +
> +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

> +       if (hpb_num_lu == 0) {
> +               dev_err(hba->dev, "No HPB LU supported\n");
> +               return -ENODEV;
> +       }
> +
> +       hpb_dev_info->rgn_size =
> geo_buf[GEOMETRY_DESC_HPB_REGION_SIZE];
> +       hpb_dev_info->srgn_size =
> geo_buf[GEOMETRY_DESC_HPB_SUBREGION_SIZE];
> +       hpb_device_max_active_rgns =
> +               get_unaligned_be16(geo_buf +
> +                       GEOMETRY_DESC_HPB_DEVICE_MAX_ACTIVE_REGIONS);
> +
> +       if (hpb_dev_info->rgn_size == 0 || hpb_dev_info->srgn_size == 0 ||
> +           hpb_device_max_active_rgns == 0) {
> +               dev_err(hba->dev, "No HPB supported device\n");
> +               return -ENODEV;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ufshpb_get_dev_info(struct ufs_hba *hba,
> +                              struct ufshpb_dev_info *hpb_dev_info,
> +                              u8 *desc_buf)
> +{
> +       int ret;
> +
> +       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?

> +       if (ret) {
> +               dev_err(hba->dev, "%s: idn: %d query request failed\n",
> +                       __func__, QUERY_DESC_IDN_DEVICE);
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * Get the number of user logical unit to check whether all
> +        * scsi_device finish initialization
> +        */
> +       hpb_dev_info->num_lu = desc_buf[DEVICE_DESC_PARAM_NUM_LU];
What about the other hpb fields in the device descriptor:
DEVICE_DESC_PARAM_HPB_VER and DEVICE_DESC_PARAM_HPB_CONTROL ?

> +
> +       ret = ufshpb_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0,
> SELECTOR,
> +                              desc_buf, hba->desc_size.geom_desc);
> +       if (ret) {
> +               dev_err(hba->dev, "%s: idn: %d query request failed\n",
> +                       __func__, QUERY_DESC_IDN_DEVICE);
> +               return ret;
> +       }
> +
> +       ret = ufshpb_get_geo_info(hba, desc_buf, hpb_dev_info);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +       hpb_lu_info->num_blocks = get_unaligned_be64(
> +                       desc_buf + UNIT_DESC_PARAM_LOGICAL_BLK_COUNT);
> +       hpb_lu_info->pinned_start = get_unaligned_be16(
> +                       desc_buf + UNIT_DESC_HPB_LU_PIN_REGION_START_OFFSET);
> +       hpb_lu_info->num_pinned = get_unaligned_be16(
> +                       desc_buf + UNIT_DESC_HPB_LU_NUM_PIN_REGIONS);
> +       hpb_lu_info->max_active_rgns = get_unaligned_be16(
> +                       desc_buf + UNIT_DESC_HPB_LU_MAX_ACTIVE_REGIONS);
You already have it, its max_active_rgns

> +
> +       return 0;
> +}
> +

> +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.

> +
> +/**
> + * struct ufshpb_dev_info - UFSHPB device related info
> + * @max_num_lun: maximum number of logical unit that HPB is supported
> + * @num_ln: the number of user logical unit to check whether all lu finished
Typo num_lu


Thanks,
Avri