Messages in this thread | | | Date | Tue, 29 Oct 2019 09:26:48 -0700 | From | asutoshd@codeauro ... | 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@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
| |