| 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: Sun, 12 Apr 2020 12:43:03 +0000
From: Avri Altman <Avri.Altman@....com>
To: Asutosh Das <asutoshd@...eaurora.org>,
"cang@...eaurora.org" <cang@...eaurora.org>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>
CC: "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
Subhash Jadavani <subhashj@...eaurora.org>,
Alim Akhtar <alim.akhtar@...sung.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
Bean Huo <beanhuo@...ron.com>,
Stanley Chu <stanley.chu@...iatek.com>,
Tomas Winkler <tomas.winkler@...el.com>,
Colin Ian King <colin.king@...onical.com>,
Bart Van Assche <bvanassche@....org>,
open list <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1 1/3] scsi: ufs: add write booster feature support
Hi,
>
> enum ufs_desc_def_size {
> - QUERY_DESC_DEVICE_DEF_SIZE = 0x40,
> + QUERY_DESC_DEVICE_DEF_SIZE = 0x59,
> QUERY_DESC_CONFIGURATION_DEF_SIZE = 0x90,
> QUERY_DESC_UNIT_DEF_SIZE = 0x23,
Might as well update the unit descriptor size - its 0x2D now,
As you are adding its new fields
> QUERY_DESC_INTERCONNECT_DEF_SIZE = 0x06,
> @@ -219,6 +226,7 @@ enum unit_desc_param {
> UNIT_DESC_PARAM_PHY_MEM_RSRC_CNT = 0x18,
> UNIT_DESC_PARAM_CTX_CAPABILITIES = 0x20,
> UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1 = 0x22,
> + UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS = 0x29,
> };
>
> /* Device descriptor parameters offsets in bytes*/
> @@ -258,6 +266,9 @@ enum device_desc_param {
> DEVICE_DESC_PARAM_PSA_MAX_DATA = 0x25,
> DEVICE_DESC_PARAM_PSA_TMT = 0x29,
> DEVICE_DESC_PARAM_PRDCT_REV = 0x2A,
> + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP = 0x4F,
DEVICE_DESC_PARAM_WB_USER_TYPE = 0x53,
> + DEVICE_DESC_PARAM_WB_TYPE = 0x54,
> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55,
> };
>
> +enum ufs_dev_wb_buf_user_cap_config {
> + UFS_WB_BUFF_PRESERVE_USER_SPACE = 1,
> + UFS_WB_BUFF_USER_SPACE_RED_EN = 2,
> +};
Maybe better to follow the spec here:
Reduced - 0
Preserve - 1
> +static inline void ufshcd_wb_config(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + if (!ufshcd_wb_sup(hba))
> + return;
> +
> + ret = ufshcd_wb_ctrl(hba, true);
Why are you setting WB on init?
> + if (ret)
> + dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
> + else
> + dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
> + ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
> + if (ret)
> + dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
> + __func__, ret);
> + ufshcd_wb_toggle_flush(hba, true);
Why set explicit flush on init?
> +
> +
> +static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba)
> +{
> + int ret;
> + u32 cur_buf, avail_buf;
> +
> + if (!ufshcd_wb_sup(hba))
> + return false;
> + /*
> + * The ufs device needs the vcc to be ON to flush.
> + * With user-space reduction enabled, it's enough to enable flush
> + * by checking only the available buffer. The threshold
> + * defined here is > 90% full.
> + * With user-space preserved enabled, the current-buffer
> + * should be checked too because the wb buffer size can reduce
> + * when disk tends to be full. This info is provided by current
> + * buffer (dCurrentWriteBoosterBufferSize). There's no point in
> + * keeping vcc on when current buffer is empty.
> + */
> + ret = ufshcd_query_attr_retry(hba,
> UPIU_QUERY_OPCODE_READ_ATTR,
> + QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE,
> + 0, 0, &avail_buf);
> + if (ret) {
> + dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read
> failed %d\n",
> + __func__, ret);
> + return false;
> + }
> +
> + ret = ufshcd_vops_get_user_cap_mode(hba);
The Preserve User Space mode should be read from -
bWriteBoosterBufferPreserveUserSpaceEn of the device descriptor - 0ffset 0x53.
The driver should have no judgement here.
Based on what is configured, better to attach a helper pointer that will perform the below check,
e.g. ufshcd_wb_preserve_keep_vcc_on() and ufshcd_wb_reduced_keep_vcc_on().
Which will simplify the logic here and make it much more readable.
This makes the preparations you made for ufshcd_vops_get_user_cap_mode,
and your second patch unneeded.
> /**
> * ufshcd_exception_event_handler - handle exceptions raised by device
> * @work: pointer to work data
> @@ -6632,6 +6829,28 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
> desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
>
> model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
> + /* Enable WB only for UFS-3.1 */
> + if (dev_info->wspecversion >= 0x310) {
> + hba->dev_info.d_ext_ufs_feature_sup =
> + get_unaligned_be32(desc_buf +
> + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> + /*
> + * WB may be supported but not configured while provisioning.
> + * The spec says, in dedicated wb buffer mode,
> + * a max of 1 lun would have wb buffer configured.
> + * Now only shared buffer mode is supported.
> + */
> + hba->dev_info.b_wb_buffer_type =
> + desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
> +
> + if (!hba->dev_info.b_wb_buffer_type)
> + goto skip_alloc_unit;
> + hba->dev_info.d_wb_alloc_units =
> + get_unaligned_be32(desc_buf +
> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
> + }
Maybe pack the above code in a wb_probe() designated helper,
which will establish if WB is supported or not.
If one of your validation tests fails, maybe you can just
hba->caps &= ~UFSHCD_CAP_WB_EN;
which will further simplify your ufshcd_wb_sup()
> if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
> - ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> - !ufshcd_is_runtime_pm(pm_op))) {
> + ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> + !ufshcd_is_runtime_pm(pm_op))) {
Redundant space removal
Thanks,
Avri
Powered by blists - more mailing lists