| Message ID | 20200530151337.6182-2-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | scsi: ufs: Support WriteBooster on Samsung UFS devices | expand |
> @@ -2801,11 +2801,17 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum > query_opcode opcode, > { > struct ufs_query_req *request = NULL; > struct ufs_query_res *response = NULL; > - int err, selector = 0; > + int err; > int timeout = QUERY_REQ_TIMEOUT; > + u8 selector = 0; > > BUG_ON(!hba); > > + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { > + if (ufshcd_is_wb_flags(idn)) > + selector = 1; > + } > + Why not make the caller set the applicable selector, Instead of checking this for every flag? > ufshcd_hold(hba, false); > mutex_lock(&hba->dev_cmd.lock); > ufshcd_init_query(hba, &request, &response, opcode, idn, index, > @@ -2882,6 +2888,11 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum > query_opcode opcode, > goto out; > } > > + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { > + if (ufshcd_is_wb_attrs(idn)) > + selector = 1; > + } > + Same here > mutex_lock(&hba->dev_cmd.lock); > ufshcd_init_query(hba, &request, &response, opcode, idn, index, > selector); > @@ -3042,6 +3053,11 @@ int ufshcd_query_descriptor_retry(struct ufs_hba > *hba, > int err; > int retries; > > + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { > + if (ufshcd_is_wb_desc(idn, index)) > + selector = 1; > + } > + And here. But this can't be true - Are you setting the selector = 1 for reading any field for those descriptors? Shouldn't it be for the wb specific fields? > for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) { > err = __ufshcd_query_descriptor(hba, opcode, idn, index, > selector, desc_buf, buf_len); > @@ -6907,8 +6923,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba) > size_t buff_len; > u8 model_index; > u8 *desc_buf; > + u8 retry_cnt = 0; > struct ufs_dev_info *dev_info = &hba->dev_info; > > +retry: > buff_len = max_t(size_t, hba->desc_size.dev_desc, > QUERY_DESC_MAX_SIZE + 1); > desc_buf = kmalloc(buff_len, GFP_KERNEL); > @@ -6948,6 +6966,29 @@ static int ufs_get_device_desc(struct ufs_hba *hba) > > ufs_fixup_device_setup(hba); > > + if (!retry_cnt && (hba->dev_quirks & > + UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR)) { If you only want to enter this clause once - you should use something other than retry_cnt, Which the reader expects to performs retries.... Also, this is becoming too wired - From your commit log I get that for specific Samsung devices, You need to query wb descriptor fields/attributes/flags using selectore = 1. But what it has to do with descriptor sizes? > + /* > + * Update WriteBooster related descriptor length with specific > + * seletor used. > + */ > + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_DEVICE, 0, > + &hba->desc_size.dev_desc); > + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_CONFIGURATION, > 0, > + &hba->desc_size.conf_desc); > + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_UNIT, 0, > + &hba->desc_size.unit_desc); > + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_GEOMETRY, 0, > + &hba->desc_size.geom_desc); > + /* > + * Read device descriptor again with specific selector used to > + * get WriteBooster related fileds. > + */ > + kfree(desc_buf); > + retry_cnt++; > + goto retry; > + } > + > /* > * Probe WB only for UFS-3.1 devices or UFS devices with quirk > * UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index bf97d616e597..d850c47e8ae0 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -876,6 +876,26 @@ static inline u8 ufshcd_wb_get_query_index(struct > ufs_hba *hba) > return 0; > } > > +static inline bool ufshcd_is_wb_attrs(enum attr_idn idn) > +{ > + return ((idn >= QUERY_ATTR_IDN_WB_FLUSH_STATUS) && > + (idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE)); > +} > + > +static inline bool ufshcd_is_wb_desc(enum desc_idn idn, u8 index) > +{ > + return (idn <= QUERY_DESC_IDN_CONFIGURATION) || > + ((idn == QUERY_DESC_IDN_UNIT) && > + (index != UFS_UPIU_RPMB_QUERY_INDEX)) || > + (idn == QUERY_DESC_IDN_GEOMETRY); > +} > + > +static inline bool ufshcd_is_wb_flags(enum flag_idn idn) > +{ > + return ((idn >= QUERY_FLAG_IDN_WB_EN) && > + (idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8)); > +} > + > extern int ufshcd_runtime_suspend(struct ufs_hba *hba); > extern int ufshcd_runtime_resume(struct ufs_hba *hba); > extern int ufshcd_runtime_idle(struct ufs_hba *hba); > -- > 2.18.0
Hi Avri, On Sat, 2020-05-30 at 20:37 +0000, Avri Altman wrote: > > @@ -2801,11 +2801,17 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum > > query_opcode opcode, > > { > > struct ufs_query_req *request = NULL; > > struct ufs_query_res *response = NULL; > > - int err, selector = 0; > > + int err; > > int timeout = QUERY_REQ_TIMEOUT; > > + u8 selector = 0; > > > > BUG_ON(!hba); > > > > + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { > > + if (ufshcd_is_wb_flags(idn)) > > + selector = 1; > > + } > > + > Why not make the caller set the applicable selector, > Instead of checking this for every flag? This way have the minimum modification efforts and places compared to other ways. However it looks a little wired because the selector control is better assigned by users. I will submit next version with changing the way selector assigned for comparison. > > > ufshcd_hold(hba, false); > > mutex_lock(&hba->dev_cmd.lock); > > ufshcd_init_query(hba, &request, &response, opcode, idn, index, > > @@ -2882,6 +2888,11 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum > > query_opcode opcode, > > goto out; > > } > > > > + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { > > + if (ufshcd_is_wb_attrs(idn)) > > + selector = 1; > > + } > > + > Same here > > > mutex_lock(&hba->dev_cmd.lock); > > ufshcd_init_query(hba, &request, &response, opcode, idn, index, > > selector); > > @@ -3042,6 +3053,11 @@ int ufshcd_query_descriptor_retry(struct ufs_hba > > *hba, > > int err; > > int retries; > > > > + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { > > + if (ufshcd_is_wb_desc(idn, index)) > > + selector = 1; > > + } > > + > And here. > But this can't be true - > Are you setting the selector = 1 for reading any field for those descriptors? > Shouldn't it be for the wb specific fields? Yes, thanks for remind this. I shall assign selector = 1 for WB related fields only in descriptors. > > > > for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) { > > err = __ufshcd_query_descriptor(hba, opcode, idn, index, > > selector, desc_buf, buf_len); > > @@ -6907,8 +6923,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba) > > size_t buff_len; > > u8 model_index; > > u8 *desc_buf; > > + u8 retry_cnt = 0; > > struct ufs_dev_info *dev_info = &hba->dev_info; > > > > +retry: > > buff_len = max_t(size_t, hba->desc_size.dev_desc, > > QUERY_DESC_MAX_SIZE + 1); > > desc_buf = kmalloc(buff_len, GFP_KERNEL); > > @@ -6948,6 +6966,29 @@ static int ufs_get_device_desc(struct ufs_hba *hba) > > > > ufs_fixup_device_setup(hba); > > > > + if (!retry_cnt && (hba->dev_quirks & > > + UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR)) { > If you only want to enter this clause once - you should use something other than retry_cnt, > Which the reader expects to performs retries.... OK! I will fix this label by using another more comprehensible name. > > Also, this is becoming too wired - > From your commit log I get that for specific Samsung devices, > You need to query wb descriptor fields/attributes/flags using selectore = 1. > But what it has to do with descriptor sizes? Sorry to not mention clearly in the commit log. Here driver needs to update the descriptor size to a "longer size" which includes the "hidden WB related fields" which can be "found" by selector = 1. If descriptor size is not updated, any query can only get the fields offset within current descriptor size even if selector = 1, and out-of-boundary desc_buf[] access will happen in ufshcd_read_desc_param(). PS. The check of "param_offset" to prevent possible out-of-boundary desc_buf[] access can be patched as well. Thanks, Stanley Chu
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c index 2d71d232a69d..fa5fdfcd2611 100644 --- a/drivers/scsi/ufs/ufs-sysfs.c +++ b/drivers/scsi/ufs/ufs-sysfs.c @@ -622,12 +622,6 @@ static const struct attribute_group ufs_sysfs_string_descriptors_group = { .attrs = ufs_sysfs_string_descriptors, }; -static inline bool ufshcd_is_wb_flags(enum flag_idn idn) -{ - return ((idn >= QUERY_FLAG_IDN_WB_EN) && - (idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8)); -} - #define UFS_FLAG(_name, _uname) \ static ssize_t _name##_show(struct device *dev, \ struct device_attribute *attr, char *buf) \ @@ -680,12 +674,6 @@ static const struct attribute_group ufs_sysfs_flags_group = { .attrs = ufs_sysfs_device_flags, }; -static inline bool ufshcd_is_wb_attrs(enum attr_idn idn) -{ - return ((idn >= QUERY_ATTR_IDN_WB_FLUSH_STATUS) && - (idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE)); -} - #define UFS_ATTRIBUTE(_name, _uname) \ static ssize_t _name##_show(struct device *dev, \ struct device_attribute *attr, char *buf) \ diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index c70845d41449..88ec87e2811c 100644 --- a/drivers/scsi/ufs/ufs.h +++ b/drivers/scsi/ufs/ufs.h @@ -61,6 +61,7 @@ * This means max. LUN number reported from UFS device could be 0xC17F. */ #define UFS_UPIU_MAX_UNIT_NUM_ID 0x7F +#define UFS_UPIU_RPMB_QUERY_INDEX 0xC4 #define UFS_MAX_LUNS (SCSI_W_LUN_BASE + UFS_UPIU_MAX_UNIT_NUM_ID) #define UFS_UPIU_WLUN_ID (1 << 7) diff --git a/drivers/scsi/ufs/ufs_quirks.h b/drivers/scsi/ufs/ufs_quirks.h index e3175a63c676..f9ed868cf330 100644 --- a/drivers/scsi/ufs/ufs_quirks.h +++ b/drivers/scsi/ufs/ufs_quirks.h @@ -108,4 +108,11 @@ struct ufs_dev_fix { */ #define UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES (1 << 10) +/* + * Some UFS devices need to use special selector to operate WriteBooster + * related flags and attributes. Enable this quirk to make these devices + * work normally. + */ +#define UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR (1 << 11) + #endif /* UFS_QUIRKS_H_ */ diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index f11be69e50e9..5e38c471877c 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -2801,11 +2801,17 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode, { struct ufs_query_req *request = NULL; struct ufs_query_res *response = NULL; - int err, selector = 0; + int err; int timeout = QUERY_REQ_TIMEOUT; + u8 selector = 0; BUG_ON(!hba); + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { + if (ufshcd_is_wb_flags(idn)) + selector = 1; + } + ufshcd_hold(hba, false); mutex_lock(&hba->dev_cmd.lock); ufshcd_init_query(hba, &request, &response, opcode, idn, index, @@ -2882,6 +2888,11 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode, goto out; } + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { + if (ufshcd_is_wb_attrs(idn)) + selector = 1; + } + mutex_lock(&hba->dev_cmd.lock); ufshcd_init_query(hba, &request, &response, opcode, idn, index, selector); @@ -3042,6 +3053,11 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba, int err; int retries; + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) { + if (ufshcd_is_wb_desc(idn, index)) + selector = 1; + } + for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) { err = __ufshcd_query_descriptor(hba, opcode, idn, index, selector, desc_buf, buf_len); @@ -6907,8 +6923,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba) size_t buff_len; u8 model_index; u8 *desc_buf; + u8 retry_cnt = 0; struct ufs_dev_info *dev_info = &hba->dev_info; +retry: buff_len = max_t(size_t, hba->desc_size.dev_desc, QUERY_DESC_MAX_SIZE + 1); desc_buf = kmalloc(buff_len, GFP_KERNEL); @@ -6948,6 +6966,29 @@ static int ufs_get_device_desc(struct ufs_hba *hba) ufs_fixup_device_setup(hba); + if (!retry_cnt && (hba->dev_quirks & + UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR)) { + /* + * Update WriteBooster related descriptor length with specific + * seletor used. + */ + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_DEVICE, 0, + &hba->desc_size.dev_desc); + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_CONFIGURATION, 0, + &hba->desc_size.conf_desc); + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_UNIT, 0, + &hba->desc_size.unit_desc); + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_GEOMETRY, 0, + &hba->desc_size.geom_desc); + /* + * Read device descriptor again with specific selector used to + * get WriteBooster related fileds. + */ + kfree(desc_buf); + retry_cnt++; + goto retry; + } + /* * Probe WB only for UFS-3.1 devices or UFS devices with quirk * UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index bf97d616e597..d850c47e8ae0 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -876,6 +876,26 @@ static inline u8 ufshcd_wb_get_query_index(struct ufs_hba *hba) return 0; } +static inline bool ufshcd_is_wb_attrs(enum attr_idn idn) +{ + return ((idn >= QUERY_ATTR_IDN_WB_FLUSH_STATUS) && + (idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE)); +} + +static inline bool ufshcd_is_wb_desc(enum desc_idn idn, u8 index) +{ + return (idn <= QUERY_DESC_IDN_CONFIGURATION) || + ((idn == QUERY_DESC_IDN_UNIT) && + (index != UFS_UPIU_RPMB_QUERY_INDEX)) || + (idn == QUERY_DESC_IDN_GEOMETRY); +} + +static inline bool ufshcd_is_wb_flags(enum flag_idn idn) +{ + return ((idn >= QUERY_FLAG_IDN_WB_EN) && + (idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8)); +} + extern int ufshcd_runtime_suspend(struct ufs_hba *hba); extern int ufshcd_runtime_resume(struct ufs_hba *hba); extern int ufshcd_runtime_idle(struct ufs_hba *hba);
Samsung UFS devices are widely used in the market, however these devices need some special handling to support WriteBooster. The major part is that Samsung UFS devices need to use specific "selector" value for WriteBooster related query operations. Therefore, introduce a device quirk to handle the special requirement and then WriteBooster can be enabled on these devices. Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> --- drivers/scsi/ufs/ufs-sysfs.c | 12 ---------- drivers/scsi/ufs/ufs.h | 1 + drivers/scsi/ufs/ufs_quirks.h | 7 ++++++ drivers/scsi/ufs/ufshcd.c | 43 ++++++++++++++++++++++++++++++++++- drivers/scsi/ufs/ufshcd.h | 20 ++++++++++++++++ 5 files changed, 70 insertions(+), 13 deletions(-)