| 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
| ||
|
Message-ID: <d8850f39c67de9a8fb478a6a738093d1@codeaurora.org>
Date: Tue, 29 Oct 2019 09:26:48 -0700
From: asutoshd@...eaurora.org
To: Can Guo <cang@...eaurora.org>
Cc: nguyenb@...eaurora.org, rnayak@...eaurora.org,
linux-scsi@...r.kernel.org, kernel-team@...roid.com,
saravanak@...gle.com, salyzyn@...gle.com,
Alim Akhtar <alim.akhtar@...sung.com>,
Avri Altman <avri.altman@....com>,
Pedro Sousa <pedrom.sousa@...opsys.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
Stanley Chu <stanley.chu@...iatek.com>,
Bean Huo <beanhuo@...ron.com>,
Tomas Winkler <tomas.winkler@...el.com>,
Subhash Jadavani <subhashj@...eaurora.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] scsi: ufs: Do not rely on prefetched data
On 2019-10-29 05:23, Can Guo wrote:
> We were setting bActiveICCLevel attribute for UFS device only once but
> type of this attribute has changed from persistent to volatile since
> UFS
> device specification v2.1. This attribute is set to the default value
> after
> power cycle or hardware reset event. It isn't safe to rely on
> prefetched
> data (only used for bActiveICCLevel attribute now). Hence this change
> removes the code related to data prefetching and set this parameter on
> every attempt to probe the UFS device.
>
> Signed-off-by: Can Guo <cang@...eaurora.org>
> ---
> drivers/scsi/ufs/ufshcd.c | 30 +++++++++++++++---------------
> drivers/scsi/ufs/ufshcd.h | 13 -------------
> 2 files changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3a0b99b..0026199 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6424,11 +6424,12 @@ static u32
> ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
> return icc_level;
> }
>
> -static void ufshcd_init_icc_levels(struct ufs_hba *hba)
> +static void ufshcd_set_active_icc_lvl(struct ufs_hba *hba)
> {
> int ret;
> int buff_len = hba->desc_size.pwr_desc;
> u8 *desc_buf;
> + u32 icc_level;
>
> desc_buf = kmalloc(buff_len, GFP_KERNEL);
> if (!desc_buf)
> @@ -6442,20 +6443,17 @@ static void ufshcd_init_icc_levels(struct
> ufs_hba *hba)
> goto out;
> }
>
> - hba->init_prefetch_data.icc_level =
> - ufshcd_find_max_sup_active_icc_level(hba,
> - desc_buf, buff_len);
> - dev_dbg(hba->dev, "%s: setting icc_level 0x%x",
> - __func__, hba->init_prefetch_data.icc_level);
> + icc_level = ufshcd_find_max_sup_active_icc_level(hba, desc_buf,
> + buff_len);
> + dev_dbg(hba->dev, "%s: setting icc_level 0x%x", __func__, icc_level);
>
> ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> - QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0,
> - &hba->init_prefetch_data.icc_level);
> + QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0, &icc_level);
>
> if (ret)
> dev_err(hba->dev,
> "%s: Failed configuring bActiveICCLevel = %d ret = %d",
> - __func__, hba->init_prefetch_data.icc_level , ret);
> + __func__, icc_level, ret);
>
> out:
> kfree(desc_buf);
> @@ -6963,6 +6961,14 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> }
> }
>
> + /*
> + * bActiveICCLevel is volatile for UFS device (as per latest v2.1
> spec)
> + * and for removable UFS card as well, hence always set the
> parameter.
> + * Note: Error handler may issue the device reset hence resetting
> + * bActiveICCLevel as well so it is always safe to set this
> here.
> + */
> + ufshcd_set_active_icc_lvl(hba);
> +
> /* set the state as operational after switching to desired gear */
> hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
>
> @@ -6979,9 +6985,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
> hba->dev_info.f_power_on_wp_en = flag;
>
> - if (!hba->is_init_prefetch)
> - ufshcd_init_icc_levels(hba);
> -
> /* Add required well known logical units to scsi mid layer */
> if (ufshcd_scsi_add_wlus(hba))
> goto out;
> @@ -7006,9 +7009,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> pm_runtime_put_sync(hba->dev);
> }
>
> - if (!hba->is_init_prefetch)
> - hba->is_init_prefetch = true;
> -
> out:
> /*
> * If we failed to initialize the device or the device is not
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index e0fe247..3089b81 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -405,15 +405,6 @@ struct ufs_clk_scaling {
> bool is_suspended;
> };
>
> -/**
> - * struct ufs_init_prefetch - contains data that is pre-fetched once
> during
> - * initialization
> - * @icc_level: icc level which was read during initialization
> - */
> -struct ufs_init_prefetch {
> - u32 icc_level;
> -};
> -
> #define UFS_ERR_REG_HIST_LENGTH 8
> /**
> * struct ufs_err_reg_hist - keeps history of errors
> @@ -505,8 +496,6 @@ struct ufs_stats {
> * @intr_mask: Interrupt Mask Bits
> * @ee_ctrl_mask: Exception event control mask
> * @is_powered: flag to check if HBA is powered
> - * @is_init_prefetch: flag to check if data was pre-fetched in
> initialization
> - * @init_prefetch_data: data pre-fetched during initialization
> * @eh_work: Worker to handle UFS errors that require s/w attention
> * @eeh_work: Worker to handle exception events
> * @errors: HBA errors
> @@ -657,8 +646,6 @@ struct ufs_hba {
> u32 intr_mask;
> u16 ee_ctrl_mask;
> bool is_powered;
> - bool is_init_prefetch;
> - struct ufs_init_prefetch init_prefetch_data;
>
> /* Work Queues */
> struct work_struct eh_work;
Looks good to me.
-Asutosh
Powered by blists - more mailing lists