| Message ID | 20201130181143.5739-1-huobean@gmail.com |
|---|---|
| Headers | show |
| Series | Three changes for UFS WriteBooster | expand |
On 11/30/2020 10:11 AM, Bean Huo wrote: > From: Bean Huo <beanhuo@micron.com> > > Currently we let UFS WriteBooster driver use clock scaling > up/down to set WB on/off, for the platform which doesn't > support UFSHCD_CAP_CLK_SCALING, WB will be always on. Provide > a sysfs attribute to enable/disable WB during runtime. > > Signed-off-by: Bean Huo <beanhuo@micron.com> > --- > drivers/scsi/ufs/ufs-sysfs.c | 33 +++++++++++++++++++++++++++++++++ > drivers/scsi/ufs/ufshcd.c | 3 +-- > drivers/scsi/ufs/ufshcd.h | 2 ++ > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c > index 08e72b7eef6a..e41d8eb779ec 100644 > --- a/drivers/scsi/ufs/ufs-sysfs.c > +++ b/drivers/scsi/ufs/ufs-sysfs.c > @@ -189,6 +189,37 @@ static ssize_t auto_hibern8_store(struct device *dev, > return count; > } > > +static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + > + return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled); > +} > + > +static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + unsigned int wb_enable; > + ssize_t res; > + > + if (!ufshcd_is_wb_allowed(hba)) > + return -EOPNOTSUPP; > + > + if (kstrtouint(buf, 0, &wb_enable)) > + return -EINVAL; > + > + if (wb_enable != 0 && wb_enable != 1) > + return -EINVAL; > + > + pm_runtime_get_sync(hba->dev); > + res = ufshcd_wb_ctrl(hba, wb_enable); Say, a platform supports clock-scaling and this bit is toggled. The control goes into ufshcd_wb_ctrl for both this sysfs and clock-scaling contexts. The clock-scaling context passes all checks and blocks on waiting for this wb control to be disabled and then tries to enable wb when it's already disabled. Perhaps that's a race there? > + pm_runtime_put_sync(hba->dev); > + > + return res < 0 ? res : count; > +} > + > static DEVICE_ATTR_RW(rpm_lvl); > static DEVICE_ATTR_RO(rpm_target_dev_state); > static DEVICE_ATTR_RO(rpm_target_link_state); > @@ -196,6 +227,7 @@ static DEVICE_ATTR_RW(spm_lvl); > static DEVICE_ATTR_RO(spm_target_dev_state); > static DEVICE_ATTR_RO(spm_target_link_state); > static DEVICE_ATTR_RW(auto_hibern8); > +static DEVICE_ATTR_RW(wb_on); > > static struct attribute *ufs_sysfs_ufshcd_attrs[] = { > &dev_attr_rpm_lvl.attr, > @@ -205,6 +237,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = { > &dev_attr_spm_target_dev_state.attr, > &dev_attr_spm_target_link_state.attr, > &dev_attr_auto_hibern8.attr, > + &dev_attr_wb_on.attr, > NULL > }; > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index d169db41ee16..639ba9d1ccbb 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -247,7 +247,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba, > static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag); > static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba); > static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba); > -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable); > static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set); > static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable); > static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba); > @@ -5299,7 +5298,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba) > __func__, err); > } > > -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable) > +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable) > { > int ret; > u8 index; > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index d0b68df07eef..c7bb61a4e484 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -1067,6 +1067,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, > u8 *desc_buff, int *buff_len, > enum query_opcode desc_op); > > +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable); > + > /* Wrapper functions for safely calling variant operations */ > static inline const char *ufshcd_get_var_name(struct ufs_hba *hba) > { >
On Mon, 2020-11-30 at 15:19 -0800, Asutosh Das (asd) wrote: > > + return -EINVAL; > > + > > + pm_runtime_get_sync(hba->dev); > > + res = ufshcd_wb_ctrl(hba, wb_enable); > > Say, a platform supports clock-scaling and this bit is toggled. > The control goes into ufshcd_wb_ctrl for both this sysfs and > clock-scaling contexts. The clock-scaling context passes all checks > and > blocks on waiting for this wb control to be disabled and then tries > to > enable wb when it's already disabled. Perhaps that's a race there? Hi Asutosh Appreciate your review. There is only inconsistent problem between clock-scaling and sysfs, since hba->dev_cmd.lock can garantee there is only one can change fWriteBoosterEn. But this is only happening on user willfully wants to control WB through sysfs even they know the platform supports clock- scaling. Since this is for the platform which doesn't support clock-scaling, I think based on your comments, it should be acceptable for you like this: +static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + unsigned int wb_enable; + ssize_t res; + + if (ufshcd_is_clkscaling_supported(hba)) { + dev_err(dev, "supports dynamic clk scaling, control WB + through sysfs is not allowed!"); + return -EOPNOTSUPP; + } + if (!ufshcd_is_wb_allowed(hba)) + return -EOPNOTSUPP; + + if (kstrtouint(buf, 0, &wb_enable)) + return -EINVAL; + + if (wb_enable != 0 && wb_enable != 1) + return -EINVAL; + + pm_runtime_get_sync(hba->dev); + res = ufshcd_wb_ctrl(hba, wb_enable); + pm_runtime_put_sync(hba->dev); + + return res < 0 ? res : count; +} thanks, Bean
On 12/2/2020 8:20 AM, Bean Huo wrote: > On Mon, 2020-11-30 at 15:19 -0800, Asutosh Das (asd) wrote: >>> + return -EINVAL; >>> + >>> + pm_runtime_get_sync(hba->dev); >>> + res = ufshcd_wb_ctrl(hba, wb_enable); >> >> Say, a platform supports clock-scaling and this bit is toggled. >> The control goes into ufshcd_wb_ctrl for both this sysfs and >> clock-scaling contexts. The clock-scaling context passes all checks >> and >> blocks on waiting for this wb control to be disabled and then tries >> to >> enable wb when it's already disabled. Perhaps that's a race there? > > Hi Asutosh > Appreciate your review. > There is only inconsistent problem between clock-scaling and sysfs, > since hba->dev_cmd.lock can garantee there is only one can change > fWriteBoosterEn. But this is only happening on user willfully wants to > control WB through sysfs even they know the platform supports clock- > scaling. > > Since this is for the platform which doesn't support clock-scaling, I > think based on your comments, it should be acceptable for you like > this: Or a synchronization primitive b/w the 2 contexts would work just as well. However, I don't have an issue if the user is denied toggling of wb anyway. LGTM. > > > +static ssize_t wb_on_store(struct device *dev, struct device_attribute > *attr, > + const char *buf, size_t count) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + unsigned int wb_enable; > + ssize_t res; > + > + if (ufshcd_is_clkscaling_supported(hba)) { > + dev_err(dev, "supports dynamic clk scaling, control WB > + through sysfs is not allowed!"); > + return -EOPNOTSUPP; > + } > + if (!ufshcd_is_wb_allowed(hba)) > + return -EOPNOTSUPP; > + > + if (kstrtouint(buf, 0, &wb_enable)) > + return -EINVAL; > + > + if (wb_enable != 0 && wb_enable != 1) > + return -EINVAL; > + > + pm_runtime_get_sync(hba->dev); > + res = ufshcd_wb_ctrl(hba, wb_enable); > + pm_runtime_put_sync(hba->dev); > + > + return res < 0 ? res : count; > +} > > thanks, > Bean > > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, Linux Foundation Collaborative Project
On 2020-12-01 02:11, Bean Huo wrote: > From: Bean Huo <beanhuo@micron.com> > > USFHCD supports WriteBooster "LU dedicated buffer” mode and > “shared buffer” mode both, so changes the comment in the > function ufshcd_wb_probe(). > > Signed-off-by: Bean Huo <beanhuo@micron.com> Reviewed-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufshcd.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index eb7a2534b072..2091775ed3e2 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -7166,10 +7166,9 @@ static void ufshcd_wb_probe(struct ufs_hba > *hba, u8 *desc_buf) > goto wb_disabled; > > /* > - * WB may be supported but not configured while provisioning. > - * The spec says, in dedicated wb buffer mode, > - * a max of 1 lun would have wb buffer configured. > - * Now only shared buffer mode is supported. > + * WB may be supported but not configured while provisioning. The > spec > + * says, in dedicated wb buffer mode, a max of 1 lun would have wb > + * buffer configured. > */ > dev_info->b_wb_buffer_type = > desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
From: Bean Huo <beanhuo@micron.com> Bean Huo (3): scsi: ufs: Add "wb_on" sysfs node to control WB on/off scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 scsi: ufs: Changes comment in the function ufshcd_wb_probe() drivers/scsi/ufs/ufs-sysfs.c | 33 +++++++++++++++++++++++++++++++++ drivers/scsi/ufs/ufs.h | 2 ++ drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++------- drivers/scsi/ufs/ufshcd.h | 2 ++ 4 files changed, 51 insertions(+), 7 deletions(-)