| Message ID | 20221010092937.520013-2-beanhuo@iokpp.de |
|---|---|
| State | New |
| Headers | show |
| Series | Changes for ufshcd.c | expand |
Hi Bean Huo, I think ufs_is_valid_unit_desc_lun() is also used for wb_buf_alloc_units_show() in ufs-sysfs.c. So just removing this if-checkup will make different result when check lun value. Thanks, Daejun >From: Bean Huo <beanhuo@micron.com> > >LUs with WB potential support are properly checked in ufshcd_wb_probe() >before calling ufshcd_read_unit_desc_param(), so remove this unnecessary >if-checkup in ufs_is_valid_unit_desc_lun() to match its function definition. > >Signed-off-by: Bean Huo <beanhuo@micron.com> >--- > drivers/ufs/core/ufshcd-priv.h | 3 --- > 1 file changed, 3 deletions(-) > >diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h >index f68ca33f6ac7..2457b005101a 100644 >--- a/drivers/ufs/core/ufshcd-priv.h >+++ b/drivers/ufs/core/ufshcd-priv.h >@@ -300,9 +300,6 @@ static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info, > pr_err("Max General LU supported by UFS isn't initialized\n"); > return false; > } >- /* WB is available only for the logical unit from 0 to 7 */ >- if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS) >- return lun < UFS_UPIU_MAX_WB_LUN_ID; > return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported); > } > >-- >2.34.1
On Tue, 2022-10-11 at 11:21 +0900, Daejun Park wrote: > Hi Bean Huo, > > I think ufs_is_valid_unit_desc_lun() is also used for > wb_buf_alloc_units_show() in ufs-sysfs.c. > So just removing this if-checkup will make different result when > check lun value. > Hi Daejun, Thanks for your review on the patch. Yes, I understood what you mean. But I don't think that's the problem. Without this patch, access on sysfs node "wb_shared_alloc_units" would get "invalid argument", while with this patch sysfs would return 00. According to the UFS specification: "If this value is ‘0’, then the WriteBooster is not supported for this LU. The Logical unit among LU0 ~ LU7 can be configured for WriteBooster Buffer. Otherwise, whole WriteBooster Buffer configuration in this device is invalid." Per my understanding, with this patch, there is still no miss- explanation of this sysfs node. The key purpose of this patch is to remove any nonsense logical during the booting stage. please have a think my comments. thanks. Kind regards, Bean > Thanks, > Daejun > > > From: Bean Huo <beanhuo@micron.com> > > > > LUs with WB potential support are properly checked in ufshcd_wb_pro > > be() > > before calling ufshcd_read_unit_desc_param(), so remove this unnece > > ssary > > if- > > checkup in ufs_is_valid_unit_desc_lun() to match its function defin > > ition.
Hi Bean, > LUs with WB potential support are properly checked in ufshcd_wb_probe() > before calling ufshcd_read_unit_desc_param(), so remove this unnecessary > if-checkup in ufs_is_valid_unit_desc_lun() to match its function definition. > > Signed-off-by: Bean Huo <beanhuo@micron.com> > --- > drivers/ufs/core/ufshcd-priv.h | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h > index f68ca33f6ac7..2457b005101a 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -300,9 +300,6 @@ static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info, > pr_err("Max General LU supported by UFS isn't initialized\n"); > return false; > } > - /* WB is available only for the logical unit from 0 to 7 */ > - if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS) Then, there is no requirement for "param_offset" argument. Thanks, Daejun
On 10/10/22 02:29, Bean Huo wrote: > From: Bean Huo <beanhuo@micron.com> > > LUs with WB potential support are properly checked in ufshcd_wb_probe() > before calling ufshcd_read_unit_desc_param(), so remove this unnecessary > if-checkup in ufs_is_valid_unit_desc_lun() to match its function definition. > > Signed-off-by: Bean Huo <beanhuo@micron.com> > --- > drivers/ufs/core/ufshcd-priv.h | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h > index f68ca33f6ac7..2457b005101a 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -300,9 +300,6 @@ static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info, > pr_err("Max General LU supported by UFS isn't initialized\n"); > return false; > } > - /* WB is available only for the logical unit from 0 to 7 */ > - if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS) > - return lun < UFS_UPIU_MAX_WB_LUN_ID; > return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported); > } Hi Bean, I think the above patch reintroduces the stack overflow issue fixed by commit a2fca52ee640 ("scsi: ufs: WB is only available on LUN #0 to #7"). How about reverting commit a2fca52ee640 and fixing the stack overflow issue in another way than by modifying ufs_is_valid_unit_desc_lun()? Thanks, Bart.
On Fri, 2022-10-14 at 11:37 -0700, Bart Van Assche wrote: > > @@ -300,9 +300,6 @@ static inline bool > > ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info, > > pr_err("Max General LU supported by UFS isn't > > initialized\n"); > > return false; > > } > > - /* WB is available only for the logical unit from 0 to 7 */ > > - if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS) > > - return lun < UFS_UPIU_MAX_WB_LUN_ID; > > return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info- > > >max_lu_supported); > > } > > Hi Bean, > > I think the above patch reintroduces the stack overflow issue fixed > by > commit a2fca52ee640 ("scsi: ufs: WB is only available on LUN #0 to > #7"). > > How about reverting commit a2fca52ee640 and fixing the stack overflow > issue in another way than by modifying ufs_is_valid_unit_desc_lun()? > > Thanks, > > Bart Hi Bart, I knew that fix, it was because the user tried to poll dLUNumWriteBoosterBufferAllocUnits from RPMB LU, as you know, RPMB doesn't support WB, but the root cause is that we don't separate normal logical unit descriptors and RPMB unit descriptor when we create sysfs group, in ufshcd_driver_template { ... .sdev_groups = ufshcd_driver_groups, } ufshcd_driver_groups { ... &ufs_sysfs_unit_descriptor_group, ... } so all the logical units will have the unified unit descriptor sysfs node. This is wrong. Another problem is that Boot and device LU don't have unit descriptors, but we still create a unit descriptor sysfs node group for boot and device LU. I am working on the Advanced RPMB, and trying to seperate normal unit descriptor and RPMB unit descriptor, will let you know if it is possible. or if you know the solution, please let me know, thanks. Kind regards, Bean
On Fri, 2022-10-14 at 11:37 -0700, Bart Van Assche wrote: > > pr_err("Max General LU supported by UFS isn't > > initialized\n"); > > return false; > > } > > - /* WB is available only for the logical unit from 0 to 7 */ > > - if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS) > > - return lun < UFS_UPIU_MAX_WB_LUN_ID; > > return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info- > > >max_lu_supported); > > } > > Hi Bean, > > I think the above patch reintroduces the stack overflow issue fixed > by > commit a2fca52ee640 ("scsi: ufs: WB is only available on LUN #0 to > #7"). > > How about reverting commit a2fca52ee640 and fixing the stack overflow > issue in another way than by modifying ufs_is_valid_unit_desc_lun()? > > Thanks, > > Bart. Hi Bart, I double-checked the changelog and the stack overflow issue was double fixed by your commit: commit d3d9c4570285 ("scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()"), For example, if the user wants to read wb_buf_alloc_units in the RPMB unit descriptor, parameter offset = 41, parameter size = 4, buff_len = 45; After ufshcd_query_descriptor_retry(), buff_len will be updated to 35. param_offset > buff_len, then -EINVAL will be returned. So we can safely remove this check, and if you still have concerns, I can verify when I get back to the office. Kind regards, Bean
On 10/14/22 12:20, Bean Huo wrote: > I am working on the Advanced RPMB, and trying to seperate normal unit > descriptor and RPMB unit descriptor, will let you know if it is > possible. or if you know the solution, please let me know, thanks. Hi Bean, How about setting .is_visible member of struct attribute_group in the UFS driver and letting that callback decide which sysfs attributes are visible depending on the logical unit type? Thanks, Bart.
On 10/14/22 13:30, Bean Huo wrote: > I double-checked the changelog and the stack overflow issue was double > fixed by your commit: > > commit d3d9c4570285 ("scsi: ufs: Fix memory corruption by > ufshcd_read_desc_param()"), > > For example, if the user wants to read wb_buf_alloc_units in the RPMB > unit descriptor, > > parameter offset = 41, parameter size = 4, > buff_len = 45; > > After ufshcd_query_descriptor_retry(), buff_len will be updated to 35. > > param_offset > buff_len, then -EINVAL will be returned. > > So we can safely remove this check, and if you still have concerns, I > can verify when I get back to the office. Hi Bean, Thank you for having looked this up. I agree with the above. Bart.
On Fri, 2022-10-14 at 13:41 -0700, Bart Van Assche wrote: > On 10/14/22 12:20, Bean Huo wrote: > > I am working on the Advanced RPMB, and trying to seperate normal > > unit > > descriptor and RPMB unit descriptor, will let you know if it is > > possible. or if you know the solution, please let me know, thanks. > > Hi Bean, > > How about setting .is_visible member of struct attribute_group in the > UFS > driver and letting that callback decide which sysfs attributes are > visible > depending on the logical unit type? > > Thanks, > > Bart. Thanks, it looks like what I expected, I'll verify it further. Kind regards, Bean
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index f68ca33f6ac7..2457b005101a 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -300,9 +300,6 @@ static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info, pr_err("Max General LU supported by UFS isn't initialized\n"); return false; } - /* WB is available only for the logical unit from 0 to 7 */ - if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS) - return lun < UFS_UPIU_MAX_WB_LUN_ID; return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported); }