| Message ID | 20231109125217.185462-3-beanhuo@iokpp.de |
|---|---|
| State | New |
| Headers | show |
| Series | Add UFS RTC support | expand |
> 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
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; }