| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 6 Jun 2020 14:58:08 +0000
From: Avri Altman <Avri.Altman@....com>
To: "daejun7.park@...sung.com" <daejun7.park@...sung.com>,
ALIM AKHTAR <alim.akhtar@...sung.com>,
"jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
"beanhuo@...ron.com" <beanhuo@...ron.com>,
"stanley.chu@...iatek.com" <stanley.chu@...iatek.com>,
"cang@...eaurora.org" <cang@...eaurora.org>,
"bvanassche@....org" <bvanassche@....org>,
"tomas.winkler@...el.com" <tomas.winkler@...el.com>
CC: "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sang-yoon Oh <sangyoon.oh@...sung.com>,
Sung-Jun Park <sungjun07.park@...sung.com>,
yongmyung lee <ymhungry.lee@...sung.com>,
Jinyoung CHOI <j-young.choi@...sung.com>,
Adel Choi <adel.choi@...sung.com>,
BoRam Shin <boram.shin@...sung.com>
Subject: RE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module
>
> 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
Powered by blists - more mailing lists