lkml.org 
[lkml]   [2019]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@codeaurora.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

\
 
 \ /
  Last update: 2019-10-29 17:30    [W:0.040 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site