Re: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

["Thomas Weißschuh" <thomas@t-8ch.de>]

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
>