lkml.org 
[lkml]   [2020]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module
    Date

    >
    > 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
    \
     
     \ /
      Last update: 2020-06-06 16:59    [W:3.161 / U:1.288 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site