| Message ID | 20200503113415.21034-2-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi: ufs: support LU Dedicated buffer mode for WriteBooster | expand |
> > static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf) > { > + if (!ufshcd_is_wb_allowed(hba)) > + return; > + > + if (hba->desc_size.dev_desc <= > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) Should be DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4 > + goto wb_disabled; > + > hba->dev_info.d_ext_ufs_feature_sup = > get_unaligned_be32(desc_buf + > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP); > > static int ufs_get_device_desc(struct ufs_hba *hba) > @@ -6862,10 +6890,6 @@ static int ufs_get_device_desc(struct ufs_hba > *hba) > > model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME]; > > - /* Enable WB only for UFS-3.1 */ > - if (dev_info->wspecversion >= 0x310) > - ufshcd_wb_probe(hba, desc_buf); > - > err = ufshcd_read_string_desc(hba, model_index, > &dev_info->model, SD_ASCII_STD); > if (err < 0) { > @@ -6874,6 +6898,16 @@ static int ufs_get_device_desc(struct ufs_hba > *hba) > goto out; > } > > + ufs_fixup_device_setup(hba); I don't think you should "hide" ufs_fixup_device_setup inside ufs_get_device_desc. Thanks, Avri
Hi Avri, On Mon, 2020-05-04 at 10:37 +0000, Avri Altman wrote: > > > > static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf) > > { > > + if (!ufshcd_is_wb_allowed(hba)) > > + return; > > + > > + if (hba->desc_size.dev_desc <= > > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) > Should be > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4 I think this description length check is redundant because the device quirk shall be added only after WriteBooster supportat is confirmed in attached UFS device. So I will remove this in next version. > > > + goto wb_disabled; > > + > > hba->dev_info.d_ext_ufs_feature_sup = > > get_unaligned_be32(desc_buf + > > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP); > > > > > > static int ufs_get_device_desc(struct ufs_hba *hba) > > @@ -6862,10 +6890,6 @@ static int ufs_get_device_desc(struct ufs_hba > > *hba) > > > > model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME]; > > > > - /* Enable WB only for UFS-3.1 */ > > - if (dev_info->wspecversion >= 0x310) > > - ufshcd_wb_probe(hba, desc_buf); > > - > > err = ufshcd_read_string_desc(hba, model_index, > > &dev_info->model, SD_ASCII_STD); > > if (err < 0) { > > @@ -6874,6 +6898,16 @@ static int ufs_get_device_desc(struct ufs_hba > > *hba) > > goto out; > > } > > > > + ufs_fixup_device_setup(hba); > I don't think you should "hide" ufs_fixup_device_setup inside ufs_get_device_desc. The reason is as below, ufshcd_wb_probe() needs the contents of Device Descriptor for initialization. To avoid double reading the Device Descriptor, I keep ufshcd_wb_probe() inside ufs_get_device_desc() to use its "desc_buf". And ufshcd_wb_probe() needs well-configured device quirk for entrance check, thus ufs_fixup_device_setup() shall be moved before ufshcd_wb_probe(). This change does not affect the behavior and functionality of ufs_fixup_device_setup() since it is still executed once only during ufshcd_init() flow and not be executed again in the future. Thanks, Stanley Chu > > Thanks, > Avri > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Hi Avri, On Mon, 2020-05-04 at 22:33 +0800, Stanley Chu wrote: > Hi Avri, > > On Mon, 2020-05-04 at 10:37 +0000, Avri Altman wrote: > > > > > > static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf) > > > { > > > + if (!ufshcd_is_wb_allowed(hba)) > > > + return; > > > + > > > + if (hba->desc_size.dev_desc <= > > > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) > > Should be > > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4 > > I think this description length check is redundant because the device > quirk shall be added only after WriteBooster supportat is confirmed in > attached UFS device. So I will remove this in next version. Sorry this statement is incorrect because this kind on devices may have short (without DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP field) before firmware upgrading. So the checking for descriptor length is still required to avoid out-of-boundary access in below codes. I will add it back in next version and also fix the length. Thanks, Stanley Chu > > > > _______________________________________________ > > Linux-mediatek mailing list > > Linux-mediatek@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-mediatek >
diff --git a/drivers/scsi/ufs/ufs_quirks.h b/drivers/scsi/ufs/ufs_quirks.h index df7a1e6805a3..e3175a63c676 100644 --- a/drivers/scsi/ufs/ufs_quirks.h +++ b/drivers/scsi/ufs/ufs_quirks.h @@ -101,4 +101,11 @@ struct ufs_dev_fix { */ #define UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME (1 << 9) +/* + * Some pre-3.1 UFS devices can support extended features by upgrading + * the firmware. Enable this quirk to make UFS core driver probe and enable + * supported features on such devices. + */ +#define UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES (1 << 10) + #endif /* UFS_QUIRKS_H_ */ diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 915e963398c4..04ddfb15e858 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -6800,9 +6800,19 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba) static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf) { + if (!ufshcd_is_wb_allowed(hba)) + return; + + if (hba->desc_size.dev_desc <= DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) + goto wb_disabled; + hba->dev_info.d_ext_ufs_feature_sup = get_unaligned_be32(desc_buf + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP); + + if (!(hba->dev_info.d_ext_ufs_feature_sup & UFS_DEV_WRITE_BOOSTER_SUP)) + goto wb_disabled; + /* * WB may be supported but not configured while provisioning. * The spec says, in dedicated wb buffer mode, @@ -6818,11 +6828,29 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf) hba->dev_info.b_presrv_uspc_en = desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN]; - if (!((hba->dev_info.d_ext_ufs_feature_sup & - UFS_DEV_WRITE_BOOSTER_SUP) && - hba->dev_info.b_wb_buffer_type && + if (!(hba->dev_info.b_wb_buffer_type && hba->dev_info.d_wb_alloc_units)) - hba->caps &= ~UFSHCD_CAP_WB_EN; + goto wb_disabled; + + return; + +wb_disabled: + hba->caps &= ~UFSHCD_CAP_WB_EN; +} + +static void ufs_fixup_device_setup(struct ufs_hba *hba) +{ + struct ufs_dev_fix *f; + struct ufs_dev_info *dev_info = &hba->dev_info; + + for (f = ufs_fixups; f->quirk; f++) { + if ((f->wmanufacturerid == dev_info->wmanufacturerid || + f->wmanufacturerid == UFS_ANY_VENDOR) && + ((dev_info->model && + STR_PRFX_EQUAL(f->model, dev_info->model)) || + !strcmp(f->model, UFS_ANY_MODEL))) + hba->dev_quirks |= f->quirk; + } } static int ufs_get_device_desc(struct ufs_hba *hba) @@ -6862,10 +6890,6 @@ static int ufs_get_device_desc(struct ufs_hba *hba) model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME]; - /* Enable WB only for UFS-3.1 */ - if (dev_info->wspecversion >= 0x310) - ufshcd_wb_probe(hba, desc_buf); - err = ufshcd_read_string_desc(hba, model_index, &dev_info->model, SD_ASCII_STD); if (err < 0) { @@ -6874,6 +6898,16 @@ static int ufs_get_device_desc(struct ufs_hba *hba) goto out; } + ufs_fixup_device_setup(hba); + + /* + * Probe WB only for UFS-3.1 devices or UFS devices with quirk + * UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled + */ + if (dev_info->wspecversion >= 0x310 || + (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES)) + ufshcd_wb_probe(hba, desc_buf); + /* * ufshcd_read_string_desc returns size of the string * reset the error value @@ -6893,21 +6927,6 @@ static void ufs_put_device_desc(struct ufs_hba *hba) dev_info->model = NULL; } -static void ufs_fixup_device_setup(struct ufs_hba *hba) -{ - struct ufs_dev_fix *f; - struct ufs_dev_info *dev_info = &hba->dev_info; - - for (f = ufs_fixups; f->quirk; f++) { - if ((f->wmanufacturerid == dev_info->wmanufacturerid || - f->wmanufacturerid == UFS_ANY_VENDOR) && - ((dev_info->model && - STR_PRFX_EQUAL(f->model, dev_info->model)) || - !strcmp(f->model, UFS_ANY_MODEL))) - hba->dev_quirks |= f->quirk; - } -} - /** * ufshcd_tune_pa_tactivate - Tunes PA_TActivate of local UniPro * @hba: per-adapter instance @@ -7244,8 +7263,6 @@ static int ufshcd_device_params_init(struct ufs_hba *hba) ufshcd_get_ref_clk_gating_wait(hba); - ufs_fixup_device_setup(hba); - if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG, QUERY_FLAG_IDN_PWR_ON_WPE, &flag)) hba->dev_info.f_power_on_wp_en = flag;
WriteBooster feature can be supported by some pre-3.1 UFS devices by upgrading firmware. To enable WriteBooster feature in such devices, introduce a device quirk to relax the entrance condition of ufshcd_wb_probe() to allow host driver to check those devices' WriteBooster capability. WriteBooster feature can be available if below all conditions are satisfied, 1. Host enables WriteBooster capability 2. UFS 3.1 device or UFS pre-3.1 device with quirk UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled 3. Device descriptor has dExtendedUFSFeaturesSupport field 4. WriteBooster support is specified in above field Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> --- drivers/scsi/ufs/ufs_quirks.h | 7 ++++ drivers/scsi/ufs/ufshcd.c | 67 ++++++++++++++++++++++------------- 2 files changed, 49 insertions(+), 25 deletions(-)