| Message ID | 1669550910-9672-2-git-send-email-Arthur.Simchaev@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ufs: core: Always read the descriptors with max length | expand |
On 11/27/22 04:08, Arthur Simchaev wrote: > We used to use the extended-feature field in the device descriptor, > as an indication that the device supports ufs2.2 or later. > Remove that as this check is specifically done few lines above. > > Reviewed-by: Bean Huo <beanhuo@micron.com> > Signed-off-by: Arthur Simchaev <Arthur.Simchaev@wdc.com> > --- > drivers/ufs/core/ufshcd.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 2dbe249..2e47c69 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -7608,10 +7608,6 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, const u8 *desc_buf) > (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))) > goto wb_disabled; > > - if (hba->desc_size[QUERY_DESC_IDN_DEVICE] < > - DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4) > - goto wb_disabled; > - > ext_ufs_feature = get_unaligned_be32(desc_buf + > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP); Does this code really have to be removed? I see a check of the UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES flag above the removed code but no check of the descriptor size? Thanks, Bart.
On 08.12.22 12:31 AM, Bart Van Assche wrote: > On 11/27/22 04:08, Arthur Simchaev wrote: >> We used to use the extended-feature field in the device descriptor, >> as an indication that the device supports ufs2.2 or later. >> Remove that as this check is specifically done few lines above. >> >> Reviewed-by: Bean Huo <beanhuo@micron.com> >> Signed-off-by: Arthur Simchaev <Arthur.Simchaev@wdc.com> >> --- >> drivers/ufs/core/ufshcd.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index 2dbe249..2e47c69 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -7608,10 +7608,6 @@ static void ufshcd_wb_probe(struct ufs_hba >> *hba, const u8 *desc_buf) >> (hba->dev_quirks & >> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))) >> goto wb_disabled; >> - if (hba->desc_size[QUERY_DESC_IDN_DEVICE] < >> - DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4) >> - goto wb_disabled; >> - >> ext_ufs_feature = get_unaligned_be32(desc_buf + >> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP); > > Does this code really have to be removed? I see a check of the > UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES flag above the removed > code but no check of the descriptor size? > it is not necessary to check this, but if you have concern, we could change to like this: if (desc_buf[DEVICE_DESC_PARAM_LEN] < DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4) goto wb_disabled; then hba->desc_size could be removed. Kind regards, Bean
On 12/8/22 04:22, Bean Huo wrote: > > On 08.12.22 12:31 AM, Bart Van Assche wrote: >> On 11/27/22 04:08, Arthur Simchaev wrote: >>> We used to use the extended-feature field in the device descriptor, >>> as an indication that the device supports ufs2.2 or later. >>> Remove that as this check is specifically done few lines above. >>> >>> Reviewed-by: Bean Huo <beanhuo@micron.com> >>> Signed-off-by: Arthur Simchaev <Arthur.Simchaev@wdc.com> >>> --- >>> drivers/ufs/core/ufshcd.c | 4 ---- >>> 1 file changed, 4 deletions(-) >>> >>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >>> index 2dbe249..2e47c69 100644 >>> --- a/drivers/ufs/core/ufshcd.c >>> +++ b/drivers/ufs/core/ufshcd.c >>> @@ -7608,10 +7608,6 @@ static void ufshcd_wb_probe(struct ufs_hba >>> *hba, const u8 *desc_buf) >>> (hba->dev_quirks & >>> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))) >>> goto wb_disabled; >>> - if (hba->desc_size[QUERY_DESC_IDN_DEVICE] < >>> - DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4) >>> - goto wb_disabled; >>> - >>> ext_ufs_feature = get_unaligned_be32(desc_buf + >>> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP); >> >> Does this code really have to be removed? I see a check of the >> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES flag above the removed >> code but no check of the descriptor size? >> > it is not necessary to check this, but if you have concern, we could > change to like this: > > > if (desc_buf[DEVICE_DESC_PARAM_LEN] < > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4) > goto wb_disabled; > > then hba->desc_size could be removed. Hi Bean, My only concern is that this patch conflicts with the pending MCQ patch series. Since that conflict is unavoidable, let's keep this patch. Bart.
On 11/27/22 04:08, Arthur Simchaev wrote: > We used to use the extended-feature field in the device descriptor, > as an indication that the device supports ufs2.2 or later. > Remove that as this check is specifically done few lines above. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 2dbe249..2e47c69 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -7608,10 +7608,6 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, const u8 *desc_buf) (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))) goto wb_disabled; - if (hba->desc_size[QUERY_DESC_IDN_DEVICE] < - DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4) - goto wb_disabled; - ext_ufs_feature = get_unaligned_be32(desc_buf + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);