[PATCH] scsi: ufs: Move clock gating sysfs entries to ufs-sysfs.c

[PATCH] scsi: ufs: Move clock gating sysfs entries to ufs-sysfs.c

[Avri Altman]

This commit moves the clock gating sysfs entries from `ufshcd.c` to
`ufs-sysfs.c` where it belongs. This change improves the organization of
the code by consolidating all sysfs-related code into a single file.

The `clkgate_enable` and `clkgate_delay_ms` attributes are now defined
and managed in `ufs-sysfs.c`, and the corresponding initialization and
removal functions in `ufshcd.c` are removed.

No functional change.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufs-sysfs.c | 62 ++++++++++++++++++++++++++
 drivers/ufs/core/ufshcd.c    | 85 ------------------------------------
 2 files changed, 62 insertions(+), 85 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 796e37a1d859..dd0db44b18a6 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -458,6 +458,64 @@ static ssize_t pm_qos_enable_store(struct device *dev,
 	return count;
 }
 
+static ssize_t
+clkgate_enable_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->clk_gating.is_enabled);
+}
+
+static ssize_t
+clkgate_enable_store(struct device *dev, struct device_attribute *attr,
+		     const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u32 value;
+
+	if (kstrtou32(buf, 0, &value))
+		return -EINVAL;
+
+	value = !!value;
+
+	guard(spinlock_irqsave)(&hba->clk_gating.lock);
+
+	if (value == hba->clk_gating.is_enabled)
+		return count;
+
+	if (value)
+		ufshcd_release(hba);
+	else
+		hba->clk_gating.active_reqs++;
+
+	hba->clk_gating.is_enabled = value;
+
+	return count;
+}
+
+static ssize_t
+clkgate_delay_ms_show(struct device *dev, struct device_attribute *attr,
+		      char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%lu\n", hba->clk_gating.delay_ms);
+}
+
+static ssize_t
+clkgate_delay_ms_store(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	unsigned long value;
+
+	if (kstrtoul(buf, 0, &value))
+		return -EINVAL;
+
+	ufshcd_clkgate_delay_set(dev, value);
+	return count;
+}
+
 static DEVICE_ATTR_RW(rpm_lvl);
 static DEVICE_ATTR_RO(rpm_target_dev_state);
 static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -470,6 +528,8 @@ static DEVICE_ATTR_RW(enable_wb_buf_flush);
 static DEVICE_ATTR_RW(wb_flush_threshold);
 static DEVICE_ATTR_RW(rtc_update_ms);
 static DEVICE_ATTR_RW(pm_qos_enable);
+static DEVICE_ATTR_RW(clkgate_delay_ms);
+static DEVICE_ATTR_RW(clkgate_enable);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -484,6 +544,8 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_wb_flush_threshold.attr,
 	&dev_attr_rtc_update_ms.attr,
 	&dev_attr_pm_qos_enable.attr,
+	&dev_attr_clkgate_delay_ms.attr,
+	&dev_attr_clkgate_enable.attr,
 	NULL
 };
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index f6c38cf10382..901aef52a452 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2019,14 +2019,6 @@ void ufshcd_release(struct ufs_hba *hba)
 }
 EXPORT_SYMBOL_GPL(ufshcd_release);
 
-static ssize_t ufshcd_clkgate_delay_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct ufs_hba *hba = dev_get_drvdata(dev);
-
-	return sysfs_emit(buf, "%lu\n", hba->clk_gating.delay_ms);
-}
-
 void ufshcd_clkgate_delay_set(struct device *dev, unsigned long value)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
@@ -2036,79 +2028,6 @@ void ufshcd_clkgate_delay_set(struct device *dev, unsigned long value)
 }
 EXPORT_SYMBOL_GPL(ufshcd_clkgate_delay_set);
 
-static ssize_t ufshcd_clkgate_delay_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	unsigned long value;
-
-	if (kstrtoul(buf, 0, &value))
-		return -EINVAL;
-
-	ufshcd_clkgate_delay_set(dev, value);
-	return count;
-}
-
-static ssize_t ufshcd_clkgate_enable_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->clk_gating.is_enabled);
-}
-
-static ssize_t ufshcd_clkgate_enable_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct ufs_hba *hba = dev_get_drvdata(dev);
-	u32 value;
-
-	if (kstrtou32(buf, 0, &value))
-		return -EINVAL;
-
-	value = !!value;
-
-	guard(spinlock_irqsave)(&hba->clk_gating.lock);
-
-	if (value == hba->clk_gating.is_enabled)
-		return count;
-
-	if (value)
-		__ufshcd_release(hba);
-	else
-		hba->clk_gating.active_reqs++;
-
-	hba->clk_gating.is_enabled = value;
-
-	return count;
-}
-
-static void ufshcd_init_clk_gating_sysfs(struct ufs_hba *hba)
-{
-	hba->clk_gating.delay_attr.show = ufshcd_clkgate_delay_show;
-	hba->clk_gating.delay_attr.store = ufshcd_clkgate_delay_store;
-	sysfs_attr_init(&hba->clk_gating.delay_attr.attr);
-	hba->clk_gating.delay_attr.attr.name = "clkgate_delay_ms";
-	hba->clk_gating.delay_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->clk_gating.delay_attr))
-		dev_err(hba->dev, "Failed to create sysfs for clkgate_delay\n");
-
-	hba->clk_gating.enable_attr.show = ufshcd_clkgate_enable_show;
-	hba->clk_gating.enable_attr.store = ufshcd_clkgate_enable_store;
-	sysfs_attr_init(&hba->clk_gating.enable_attr.attr);
-	hba->clk_gating.enable_attr.attr.name = "clkgate_enable";
-	hba->clk_gating.enable_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->clk_gating.enable_attr))
-		dev_err(hba->dev, "Failed to create sysfs for clkgate_enable\n");
-}
-
-static void ufshcd_remove_clk_gating_sysfs(struct ufs_hba *hba)
-{
-	if (hba->clk_gating.delay_attr.attr.name)
-		device_remove_file(hba->dev, &hba->clk_gating.delay_attr);
-	if (hba->clk_gating.enable_attr.attr.name)
-		device_remove_file(hba->dev, &hba->clk_gating.enable_attr);
-}
-
 static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 {
 	if (!ufshcd_is_clkgating_allowed(hba))
@@ -2126,8 +2045,6 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 		"ufs_clk_gating_%d", WQ_MEM_RECLAIM | WQ_HIGHPRI,
 		hba->host->host_no);
 
-	ufshcd_init_clk_gating_sysfs(hba);
-
 	hba->clk_gating.is_enabled = true;
 	hba->clk_gating.is_initialized = true;
 }
@@ -2137,8 +2054,6 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
 	if (!hba->clk_gating.is_initialized)
 		return;
 
-	ufshcd_remove_clk_gating_sysfs(hba);
-
 	/* Ungate the clock if necessary. */
 	ufshcd_hold(hba);
 	hba->clk_gating.is_initialized = false;
-- 
2.25.1

Re: [PATCH] scsi: ufs: Move clock gating sysfs entries to ufs-sysfs.c

[Manivannan Sadhasivam]

+ Ziqi

On Wed, Jan 22, 2025 at 04:36:05PM +0200, Avri Altman wrote:
> This commit moves the clock gating sysfs entries from `ufshcd.c` to
> `ufs-sysfs.c` where it belongs. This change improves the organization of
> the code by consolidating all sysfs-related code into a single file.
> 
> The `clkgate_enable` and `clkgate_delay_ms` attributes are now defined
> and managed in `ufs-sysfs.c`, and the corresponding initialization and
> removal functions in `ufshcd.c` are removed.
> 
> No functional change.
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>

While this patch is useful, you should also consider adding the ABI
documentation for these. I did share the comment to another patch that touches
this part of the code:
https://lore.kernel.org/linux-scsi/20250119074823.lnlppdpsfnkz7onx@thinkpad/

Maybe it is relevant to add the documentation together with this patch.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

RE: [PATCH] scsi: ufs: Move clock gating sysfs entries to ufs-sysfs.c

[Avri Altman]

 
> + Ziqi
> 
> On Wed, Jan 22, 2025 at 04:36:05PM +0200, Avri Altman wrote:
> > This commit moves the clock gating sysfs entries from `ufshcd.c` to
> > `ufs-sysfs.c` where it belongs. This change improves the organization
> > of the code by consolidating all sysfs-related code into a single file.
> >
> > The `clkgate_enable` and `clkgate_delay_ms` attributes are now defined
> > and managed in `ufs-sysfs.c`, and the corresponding initialization and
> > removal functions in `ufshcd.c` are removed.
> >
> > No functional change.
> >
> > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> 
> While this patch is useful, you should also consider adding the ABI
> documentation for these. I did share the comment to another patch that
> touches this part of the code:
> https://lore.kernel.org/linux-scsi/20250119074823.lnlppdpsfnkz7onx@thinkpad/
> 
> Maybe it is relevant to add the documentation together with this patch.
Thanks.
Ziqi already beat me - https://www.spinics.net/lists/linux-scsi/msg202516.html

Thanks,
Avri

> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH] scsi: ufs: Move clock gating sysfs entries to ufs-sysfs.c

[Bart Van Assche]

On 1/22/25 6:36 AM, Avri Altman wrote:
> +static ssize_t
> +clkgate_enable_show(struct device *dev, struct device_attribute *attr,
> +		    char *buf)
> +{

The above formatting does not conform to the Linux kernel coding style. 
Please fix this, e.g. by running git clang-format HEAD^ && git commit 
--amend on the git branch with this patch.

> +	if (kstrtou32(buf, 0, &value))
> +		return -EINVAL;
> +
> +	value = !!value;

Please use kstrtobool() instead of the above code.

Thanks,

Bart.

RE: [PATCH] scsi: ufs: Move clock gating sysfs entries to ufs-sysfs.c

[Avri Altman]

> On 1/22/25 6:36 AM, Avri Altman wrote:
> > +static ssize_t
> > +clkgate_enable_show(struct device *dev, struct device_attribute *attr,
> > +                 char *buf)
> > +{
> 
> The above formatting does not conform to the Linux kernel coding style.
> Please fix this, e.g. by running git clang-format HEAD^ && git commit --amend
> on the git branch with this patch.
Done.
AFAIK, there hasn't been a formal announcement that `clang-format` should replace `checkpatch` for Linux kernel development.
And yes, while checkpatch --strict accepted the above formatting, clang-format did make changes.


> 
> > +     if (kstrtou32(buf, 0, &value))
> > +             return -EINVAL;
> > +
> > +     value = !!value;
> 
> Please use kstrtobool() instead of the above code.
Done.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

Re: [PATCH] scsi: ufs: Move clock gating sysfs entries to ufs-sysfs.c

[Bart Van Assche]

On 1/22/25 11:31 PM, Avri Altman wrote:
>> On 1/22/25 6:36 AM, Avri Altman wrote:
>>> +static ssize_t
>>> +clkgate_enable_show(struct device *dev, struct device_attribute *attr,
>>> +                 char *buf)
>>> +{
>>
>> The above formatting does not conform to the Linux kernel coding style.
>> Please fix this, e.g. by running git clang-format HEAD^ && git commit --amend
>> on the git branch with this patch.
> Done.
> AFAIK, there hasn't been a formal announcement that `clang-format` should replace `checkpatch` for Linux kernel development.
> And yes, while checkpatch --strict accepted the above formatting, clang-format did make changes.

Hi Avri,

Nobody ever required to use clang-format as far as I know.

With my email I was referring to the requirement that the return type
and the function name occur on the same line.

Thanks,

Bart.