| Message ID | 20231109125217.185462-3-beanhuo@iokpp.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add UFS RTC support | expand |
On 2023-11-09 13:52:17+0100, Bean Huo wrote: > From: Bean Huo <beanhuo@micron.com> > > This patch introduces a sysfs node named 'rtc_update_ms' within the kernel, enabling users to > adjust the RTC periodic update frequency to suit the specific requirements of the system and > UFS. Also, this patch allows the user to disable periodic update RTC in the UFS idle time. > > Signed-off-by: Bean Huo <beanhuo@micron.com> > --- > drivers/ufs/core/ufs-sysfs.c | 31 +++++++++++++++++++++++++++++++ > drivers/ufs/core/ufshcd.c | 4 ++-- > 2 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c > index c95906443d5f..d42846316a86 100644 > --- a/drivers/ufs/core/ufs-sysfs.c > +++ b/drivers/ufs/core/ufs-sysfs.c > @@ -255,6 +255,35 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr, > return res < 0 ? res : count; > } > > +static ssize_t rtc_update_ms_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%d\n", hba->dev_info.rtc_update_period); > +} > + > +static ssize_t rtc_update_ms_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + unsigned int ms; > + bool resume_period_update; > + > + if (kstrtouint(buf, 0, &ms)) > + return -EINVAL; > + > + if (!hba->dev_info.rtc_update_period && ms > 0) > + resume_period_update = true; > + /* Minimum and maximum update frequency should be synchronized with all UFS vendors */ > + hba->dev_info.rtc_update_period = ms; > + > + if (resume_period_update) Variable will be unitialized when if() above did not trigger. > + schedule_delayed_work(&hba->ufs_rtc_delayed_work, > + msecs_to_jiffies(hba->dev_info.rtc_update_period)); What about the other work that has already been scheduled? > + return count; > +} > + > static ssize_t enable_wb_buf_flush_show(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -339,6 +368,7 @@ static DEVICE_ATTR_RW(auto_hibern8); > static DEVICE_ATTR_RW(wb_on); > static DEVICE_ATTR_RW(enable_wb_buf_flush); > static DEVICE_ATTR_RW(wb_flush_threshold); > +static DEVICE_ATTR_RW(rtc_update_ms); The whole attribute needs documentation. > > static struct attribute *ufs_sysfs_ufshcd_attrs[] = { > &dev_attr_rpm_lvl.attr, > @@ -351,6 +381,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = { > &dev_attr_wb_on.attr, > &dev_attr_enable_wb_buf_flush.attr, > &dev_attr_wb_flush_threshold.attr, > + &dev_attr_rtc_update_ms.attr, > NULL > }; > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index f0e3dd3dd280..ae9b60619fd3 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8234,9 +8234,9 @@ static void ufshcd_rtc_work(struct work_struct *work) > > ufshcd_update_rtc(hba); > out: > - if (ufshcd_is_ufs_dev_active(hba)) > + if (ufshcd_is_ufs_dev_active(hba) && hba->dev_info.rtc_update_period) > schedule_delayed_work(&hba->ufs_rtc_delayed_work, > - msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS)); > + msecs_to_jiffies(hba->dev_info.rtc_update_period)); > return; > } > > -- > 2.34.1 >
> From: Bean Huo <beanhuo@micron.com> > > This patch introduces a sysfs node named 'rtc_update_ms' within the kernel, > enabling users to adjust the RTC periodic update frequency to suit the > specific requirements of the system and UFS. Also, this patch allows the user > to disable periodic update RTC in the UFS idle time. > > Signed-off-by: Bean Huo <beanhuo@micron.com> Forgot to add doc? Thanks, Avri > --- > drivers/ufs/core/ufs-sysfs.c | 31 +++++++++++++++++++++++++++++++ > drivers/ufs/core/ufshcd.c | 4 ++-- > 2 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index > c95906443d5f..d42846316a86 100644 > --- a/drivers/ufs/core/ufs-sysfs.c > +++ b/drivers/ufs/core/ufs-sysfs.c > @@ -255,6 +255,35 @@ static ssize_t wb_on_store(struct device *dev, > struct device_attribute *attr, > return res < 0 ? res : count; > } > > +static ssize_t rtc_update_ms_show(struct device *dev, struct > device_attribute *attr, > + char *buf) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%d\n", hba->dev_info.rtc_update_period); > +} > + > +static ssize_t rtc_update_ms_store(struct device *dev, struct > device_attribute *attr, > + const char *buf, size_t count) { > + struct ufs_hba *hba = dev_get_drvdata(dev); > + unsigned int ms; > + bool resume_period_update; > + > + if (kstrtouint(buf, 0, &ms)) > + return -EINVAL; > + > + if (!hba->dev_info.rtc_update_period && ms > 0) > + resume_period_update = true; > + /* Minimum and maximum update frequency should be synchronized > with all UFS vendors */ > + hba->dev_info.rtc_update_period = ms; > + > + if (resume_period_update) > + schedule_delayed_work(&hba->ufs_rtc_delayed_work, > + msecs_to_jiffies(hba- > >dev_info.rtc_update_period)); > + return count; > +} > + > static ssize_t enable_wb_buf_flush_show(struct device *dev, > struct device_attribute *attr, > char *buf) @@ -339,6 +368,7 @@ static > DEVICE_ATTR_RW(auto_hibern8); static DEVICE_ATTR_RW(wb_on); static > DEVICE_ATTR_RW(enable_wb_buf_flush); > static DEVICE_ATTR_RW(wb_flush_threshold); > +static DEVICE_ATTR_RW(rtc_update_ms); > > static struct attribute *ufs_sysfs_ufshcd_attrs[] = { > &dev_attr_rpm_lvl.attr, > @@ -351,6 +381,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = { > &dev_attr_wb_on.attr, > &dev_attr_enable_wb_buf_flush.attr, > &dev_attr_wb_flush_threshold.attr, > + &dev_attr_rtc_update_ms.attr, > NULL > }; > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > f0e3dd3dd280..ae9b60619fd3 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8234,9 +8234,9 @@ static void ufshcd_rtc_work(struct work_struct > *work) > > ufshcd_update_rtc(hba); > out: > - if (ufshcd_is_ufs_dev_active(hba)) > + if (ufshcd_is_ufs_dev_active(hba) && > + hba->dev_info.rtc_update_period) > schedule_delayed_work(&hba->ufs_rtc_delayed_work, > - > msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS)); > + > + msecs_to_jiffies(hba->dev_info.rtc_update_period)); > return; > } > > -- > 2.34.1
On 11/9/23 04:52, Bean Huo wrote: > This patch introduces a sysfs node named 'rtc_update_ms' within the kernel, enabling users to > adjust the RTC periodic update frequency to suit the specific requirements of the system and > UFS. Also, this patch allows the user to disable periodic update RTC in the UFS idle time. Why is this behavior enabled by default instead of disabled by default? Thanks, Bart.
Hi Bean, kernel test robot noticed the following build warnings: [auto build test WARNING on mkp-scsi/for-next] [also build test WARNING on jejb-scsi/for-next linus/master v6.6 next-20231110] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Bean-Huo/scsi-ufs-core-Add-UFS-RTC-support/20231110-051048 base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next patch link: https://lore.kernel.org/r/20231109125217.185462-3-beanhuo%40iokpp.de patch subject: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20231112/202311120923.S1Zbpb0s-lkp@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231112/202311120923.S1Zbpb0s-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311120923.S1Zbpb0s-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/ufs/core/ufs-sysfs.c:276:6: warning: variable 'resume_period_update' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] 276 | if (!hba->dev_info.rtc_update_period && ms > 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/ufs/core/ufs-sysfs.c:281:6: note: uninitialized use occurs here 281 | if (resume_period_update) | ^~~~~~~~~~~~~~~~~~~~ drivers/ufs/core/ufs-sysfs.c:276:2: note: remove the 'if' if its condition is always true 276 | if (!hba->dev_info.rtc_update_period && ms > 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 277 | resume_period_update = true; | ~~~~~~~~~~~~~~~~ >> drivers/ufs/core/ufs-sysfs.c:276:6: warning: variable 'resume_period_update' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized] 276 | if (!hba->dev_info.rtc_update_period && ms > 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/ufs/core/ufs-sysfs.c:281:6: note: uninitialized use occurs here 281 | if (resume_period_update) | ^~~~~~~~~~~~~~~~~~~~ drivers/ufs/core/ufs-sysfs.c:276:6: note: remove the '&&' if its condition is always true 276 | if (!hba->dev_info.rtc_update_period && ms > 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/ufs/core/ufs-sysfs.c:271:27: note: initialize the variable 'resume_period_update' to silence this warning 271 | bool resume_period_update; | ^ | = 0 2 warnings generated. vim +276 drivers/ufs/core/ufs-sysfs.c 265 266 static ssize_t rtc_update_ms_store(struct device *dev, struct device_attribute *attr, 267 const char *buf, size_t count) 268 { 269 struct ufs_hba *hba = dev_get_drvdata(dev); 270 unsigned int ms; 271 bool resume_period_update; 272 273 if (kstrtouint(buf, 0, &ms)) 274 return -EINVAL; 275 > 276 if (!hba->dev_info.rtc_update_period && ms > 0) 277 resume_period_update = true; 278 /* Minimum and maximum update frequency should be synchronized with all UFS vendors */ 279 hba->dev_info.rtc_update_period = ms; 280 281 if (resume_period_update) 282 schedule_delayed_work(&hba->ufs_rtc_delayed_work, 283 msecs_to_jiffies(hba->dev_info.rtc_update_period)); 284 return count; 285 } 286
Hi Thomas, On Thu, 2023-11-09 at 15:05 +0100, Thomas Weißschuh wrote: > > + schedule_delayed_work(&hba->ufs_rtc_delayed_work, > > + msecs_to_jiffies(hb > > a->dev_info.rtc_update_period)); > > What about the other work that has already been scheduled? I don't quite understand your concern here, I need to schedule the work when needed because of resume_period_update. Work may have been scheduled, but that's okay, there will be one more RTC update as expected. Kind regards, Bean
Hi Bart, On Thu, 2023-11-09 at 10:07 -0800, Bart Van Assche wrote: > On 11/9/23 04:52, Bean Huo wrote: > > This patch introduces a sysfs node named 'rtc_update_ms' within the > > kernel, enabling users to > > adjust the RTC periodic update frequency to suit the specific > > requirements of the system and > > UFS. Also, this patch allows the user to disable periodic update > > RTC in the UFS idle time. > > Why is this behavior enabled by default instead of disabled by > default? No problem, I will disable it by default in the next version and let customers choose on a case-by-case basis. Kind regards, Bean
On 2023-11-14 19:39:55+0100, Bean Huo wrote: > On Thu, 2023-11-09 at 15:05 +0100, Thomas Weißschuh wrote: > > > + schedule_delayed_work(&hba->ufs_rtc_delayed_work, > > > + msecs_to_jiffies(hb > > > a->dev_info.rtc_update_period)); > > > > What about the other work that has already been scheduled? > > I don't quite understand your concern here, I need to schedule the > work when needed because of resume_period_update. > > Work may have been scheduled, but that's okay, there will be one more > RTC update as expected. If it's fine to have this additional update, all is good. Thomas
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index c95906443d5f..d42846316a86 100644 --- a/drivers/ufs/core/ufs-sysfs.c +++ b/drivers/ufs/core/ufs-sysfs.c @@ -255,6 +255,35 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr, return res < 0 ? res : count; } +static ssize_t rtc_update_ms_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + + return sysfs_emit(buf, "%d\n", hba->dev_info.rtc_update_period); +} + +static ssize_t rtc_update_ms_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + unsigned int ms; + bool resume_period_update; + + if (kstrtouint(buf, 0, &ms)) + return -EINVAL; + + if (!hba->dev_info.rtc_update_period && ms > 0) + resume_period_update = true; + /* Minimum and maximum update frequency should be synchronized with all UFS vendors */ + hba->dev_info.rtc_update_period = ms; + + if (resume_period_update) + schedule_delayed_work(&hba->ufs_rtc_delayed_work, + msecs_to_jiffies(hba->dev_info.rtc_update_period)); + return count; +} + static ssize_t enable_wb_buf_flush_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -339,6 +368,7 @@ static DEVICE_ATTR_RW(auto_hibern8); static DEVICE_ATTR_RW(wb_on); static DEVICE_ATTR_RW(enable_wb_buf_flush); static DEVICE_ATTR_RW(wb_flush_threshold); +static DEVICE_ATTR_RW(rtc_update_ms); static struct attribute *ufs_sysfs_ufshcd_attrs[] = { &dev_attr_rpm_lvl.attr, @@ -351,6 +381,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = { &dev_attr_wb_on.attr, &dev_attr_enable_wb_buf_flush.attr, &dev_attr_wb_flush_threshold.attr, + &dev_attr_rtc_update_ms.attr, NULL }; diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index f0e3dd3dd280..ae9b60619fd3 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8234,9 +8234,9 @@ static void ufshcd_rtc_work(struct work_struct *work) ufshcd_update_rtc(hba); out: - if (ufshcd_is_ufs_dev_active(hba)) + if (ufshcd_is_ufs_dev_active(hba) && hba->dev_info.rtc_update_period) schedule_delayed_work(&hba->ufs_rtc_delayed_work, - msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS)); + msecs_to_jiffies(hba->dev_info.rtc_update_period)); return; }