[PATCH 1/3] ath10k: add sysfs entry to configure quiet period

[PATCH 1/3] ath10k: add sysfs entry to configure quiet period

[Rajkumar Manoharan]

Add support to configure quiet period via sysfs entry. This will
be helpful to experiment different quiet period values along with
different duty cycle ratio.

To configure quiet period as 30ms,

echo 30 >/sys/class/ieee80211/phyX/device/quiet_period

Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/thermal.c | 57 +++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/thermal.h |  1 +
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/thermal.c b/drivers/net/wireless/ath/ath10k/thermal.c
index 0d89ab5..8992c17 100644
--- a/drivers/net/wireless/ath/ath10k/thermal.c
+++ b/drivers/net/wireless/ath/ath10k/thermal.c
@@ -96,8 +96,7 @@ static int ath10k_thermal_set_cur_dutycycle(struct thermal_cooling_device *cdev,
 		ret = -ENETDOWN;
 		goto out;
 	}
-	period = max(ATH10K_QUIET_PERIOD_MIN,
-		     (ATH10K_QUIET_PERIOD_DEFAULT / num_bss));
+	period = ar->thermal.quiet_period;
 	duration = (period * (100 - duty_cycle)) / 100;
 	enabled = duration ? 1 : 0;
 
@@ -184,6 +183,48 @@ static struct attribute *ath10k_hwmon_attrs[] = {
 };
 ATTRIBUTE_GROUPS(ath10k_hwmon);
 
+static ssize_t ath10k_thermal_show_quiet_period(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct ath10k *ar = dev_get_drvdata(dev);
+	u32 period;
+
+	mutex_lock(&ar->conf_mutex);
+	period = ar->thermal.quiet_period;
+	mutex_unlock(&ar->conf_mutex);
+
+	return sprintf(buf, "%u\n", period);
+}
+
+static ssize_t ath10k_thermal_store_quiet_period(struct device *dev,
+						 struct device_attribute *attr,
+						 const char *buf, size_t count)
+{
+	struct ath10k *ar = dev_get_drvdata(dev);
+	u32 period;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &period);
+	if (ret)
+		return ret;
+
+	if (period < ATH10K_QUIET_PERIOD_MIN) {
+		ath10k_warn(ar, "Quiet period %u can not be lesser than 25ms\n",
+			    period);
+		return -EINVAL;
+	}
+	mutex_lock(&ar->conf_mutex);
+	ar->thermal.quiet_period = period;
+	mutex_unlock(&ar->conf_mutex);
+
+	return count;
+}
+
+static DEVICE_ATTR(quiet_period, S_IRUGO | S_IWUSR,
+		   ath10k_thermal_show_quiet_period,
+		   ath10k_thermal_store_quiet_period);
+
 int ath10k_thermal_register(struct ath10k *ar)
 {
 	struct thermal_cooling_device *cdev;
@@ -208,6 +249,13 @@ int ath10k_thermal_register(struct ath10k *ar)
 
 	ar->thermal.cdev = cdev;
 
+	ret = sysfs_create_file(&ar->dev->kobj, &dev_attr_quiet_period.attr);
+	if (ret) {
+		ath10k_err(ar, "failed to create quiet period sysfs entry\n");
+		goto err_remove_link;
+	}
+	ar->thermal.quiet_period = ATH10K_QUIET_PERIOD_DEFAULT;
+
 	/* Do not register hwmon device when temperature reading is not
 	 * supported by firmware
 	 */
@@ -226,10 +274,12 @@ int ath10k_thermal_register(struct ath10k *ar)
 		ath10k_err(ar, "failed to register hwmon device: %ld\n",
 			   PTR_ERR(hwmon_dev));
 		ret = -EINVAL;
-		goto err_remove_link;
+		goto err_remove_file;
 	}
 	return 0;
 
+err_remove_file:
+	sysfs_remove_file(&ar->dev->kobj, &dev_attr_quiet_period.attr);
 err_remove_link:
 	sysfs_remove_link(&ar->dev->kobj, "thermal_sensor");
 err_cooling_destroy:
@@ -241,4 +291,5 @@ void ath10k_thermal_unregister(struct ath10k *ar)
 {
 	thermal_cooling_device_unregister(ar->thermal.cdev);
 	sysfs_remove_link(&ar->dev->kobj, "cooling_device");
+	sysfs_remove_file(&ar->dev->kobj, &dev_attr_quiet_period.attr);
 }
diff --git a/drivers/net/wireless/ath/ath10k/thermal.h b/drivers/net/wireless/ath/ath10k/thermal.h
index 5e87d9a..050f41d 100644
--- a/drivers/net/wireless/ath/ath10k/thermal.h
+++ b/drivers/net/wireless/ath/ath10k/thermal.h
@@ -29,6 +29,7 @@ struct ath10k_thermal {
 
 	/* protected by conf_mutex */
 	u32 duty_cycle;
+	u32 quiet_period;
 	/* temperature value in Celcius degree
 	 * protected by data_lock
 	 */
-- 
2.3.0

[PATCH 2/3] ath10k: fix wrong symlink name on error path

[Rajkumar Manoharan]

Wrong symlink name is used on error path of thermal registration
and also correcting the error message.

Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/thermal.c b/drivers/net/wireless/ath/ath10k/thermal.c
index 8992c17..7c54bb1 100644
--- a/drivers/net/wireless/ath/ath10k/thermal.c
+++ b/drivers/net/wireless/ath/ath10k/thermal.c
@@ -243,7 +243,7 @@ int ath10k_thermal_register(struct ath10k *ar)
 	ret = sysfs_create_link(&ar->dev->kobj, &cdev->device.kobj,
 				"cooling_device");
 	if (ret) {
-		ath10k_err(ar, "failed to create thermal symlink\n");
+		ath10k_err(ar, "failed to create cooling device symlink\n");
 		goto err_cooling_destroy;
 	}
 
@@ -281,7 +281,7 @@ int ath10k_thermal_register(struct ath10k *ar)
 err_remove_file:
 	sysfs_remove_file(&ar->dev->kobj, &dev_attr_quiet_period.attr);
 err_remove_link:
-	sysfs_remove_link(&ar->dev->kobj, "thermal_sensor");
+	sysfs_remove_link(&ar->dev->kobj, "cooling_device");
 err_cooling_destroy:
 	thermal_cooling_device_unregister(cdev);
 	return ret;
-- 
2.3.0

[PATCH 3/3] ath10k: add symlink for hwmon device

[Rajkumar Manoharan]

Add symbloic link "thermal_sensors" for hwmon devices for better
readability.

/sys/class/ieee80211/phyX/device/thermal_sensors ->
sys/class/ieee80211/phyX/device/hwmon/hwmonY

Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/thermal.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/thermal.c b/drivers/net/wireless/ath/ath10k/thermal.c
index 7c54bb1..f601e3e 100644
--- a/drivers/net/wireless/ath/ath10k/thermal.c
+++ b/drivers/net/wireless/ath/ath10k/thermal.c
@@ -276,6 +276,13 @@ int ath10k_thermal_register(struct ath10k *ar)
 		ret = -EINVAL;
 		goto err_remove_file;
 	}
+	ret = sysfs_create_link(&ar->dev->kobj, &hwmon_dev->kobj,
+				"thermal_sensors");
+	if (ret) {
+		ath10k_err(ar, "failed to create hwmon symlink\n");
+		goto err_remove_file;
+	}
+
 	return 0;
 
 err_remove_file:
@@ -292,4 +299,5 @@ void ath10k_thermal_unregister(struct ath10k *ar)
 	thermal_cooling_device_unregister(ar->thermal.cdev);
 	sysfs_remove_link(&ar->dev->kobj, "cooling_device");
 	sysfs_remove_file(&ar->dev->kobj, &dev_attr_quiet_period.attr);
+	sysfs_remove_link(&ar->dev->kobj, "thermal_sensors");
 }
-- 
2.3.0

Re: [PATCH 3/3] ath10k: add symlink for hwmon device

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> Add symbloic link "thermal_sensors" for hwmon devices for better
> readability.
>
> /sys/class/ieee80211/phyX/device/thermal_sensors ->
> sys/class/ieee80211/phyX/device/hwmon/hwmonY

I don't get it, how does that improve readability? Also I don't see any
other driver doing this, so isn't this just some private hack and not a
proper generic interface?

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: add sysfs entry to configure quiet period

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> Add support to configure quiet period via sysfs entry. This will
> be helpful to experiment different quiet period values along with
> different duty cycle ratio.
>
> To configure quiet period as 30ms,
>
> echo 30 >/sys/class/ieee80211/phyX/device/quiet_period

What's the justification? To me this looks like an ugly driver private
hack. Why can't you use nl80211 or something else?

-- 
Kalle Valo

Re: [PATCH 3/3] ath10k: add symlink for hwmon device

[Rajkumar Manoharan]

On Tue, Mar 10, 2015 at 05:39:46PM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > Add symbloic link "thermal_sensors" for hwmon devices for better
> > readability.
> >
> > /sys/class/ieee80211/phyX/device/thermal_sensors ->
> > sys/class/ieee80211/phyX/device/hwmon/hwmonY
> 
> I don't get it, how does that improve readability? Also I don't see any
> other driver doing this, so isn't this just some private hack and not a
> proper generic interface?
>
The patch is providing same level access to monitoring device similar to
cooling device.

cooling_device -> ../../../../../virtual/thermal/cooling_deviceX
thermal_sensors -> hwmon/hwmonX

-Rajkumar

Re: [PATCH 1/3] ath10k: add sysfs entry to configure quiet period

[Rajkumar Manoharan]

On Tue, Mar 10, 2015 at 05:43:58PM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > Add support to configure quiet period via sysfs entry. This will
> > be helpful to experiment different quiet period values along with
> > different duty cycle ratio.
> >
> > To configure quiet period as 30ms,
> >
> > echo 30 >/sys/class/ieee80211/phyX/device/quiet_period
> 
> What's the justification? To me this looks like an ugly driver private
> hack. Why can't you use nl80211 or something else?

As this is purely for testing purpose to play around with different
quiet period along with various throttling state, sysfs entry is used
instead of netlink testmode command. Instead of debugfs, sysfs entry is
selected to align with existing thermal interface. 

-Rajkumar

Re: [PATCH 2/3] ath10k: fix wrong symlink name on error path

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> Wrong symlink name is used on error path of thermal registration
> and also correcting the error message.
>
> Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>

Thanks, patch 2 applied. Not sure yet what to do with patch 1 and 3.

-- 
Kalle Valo

Re: [PATCH 3/3] ath10k: add symlink for hwmon device

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> On Tue, Mar 10, 2015 at 05:39:46PM +0200, Kalle Valo wrote:
>> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
>> 
>> > Add symbloic link "thermal_sensors" for hwmon devices for better
>> > readability.
>> >
>> > /sys/class/ieee80211/phyX/device/thermal_sensors ->
>> > sys/class/ieee80211/phyX/device/hwmon/hwmonY
>> 
>> I don't get it, how does that improve readability? Also I don't see any
>> other driver doing this, so isn't this just some private hack and not a
>> proper generic interface?
>
> The patch is providing same level access to monitoring device similar to
> cooling device.
>
> cooling_device -> ../../../../../virtual/thermal/cooling_deviceX
> thermal_sensors -> hwmon/hwmonX

Is this documented somewhere? We cannot just start inventing sysfs
interfaces on our own.

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: add sysfs entry to configure quiet period

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> On Tue, Mar 10, 2015 at 05:43:58PM +0200, Kalle Valo wrote:
>> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
>> 
>> > Add support to configure quiet period via sysfs entry. This will
>> > be helpful to experiment different quiet period values along with
>> > different duty cycle ratio.
>> >
>> > To configure quiet period as 30ms,
>> >
>> > echo 30 >/sys/class/ieee80211/phyX/device/quiet_period
>> 
>> What's the justification? To me this looks like an ugly driver private
>> hack. Why can't you use nl80211 or something else?
>
> As this is purely for testing purpose to play around with different
> quiet period along with various throttling state, sysfs entry is used
> instead of netlink testmode command. Instead of debugfs, sysfs entry is
> selected to align with existing thermal interface. 

Please correct me if I'm wrong, but I consider sysfs as a stable user
space interface. It's not meant for random testing stuff.

-- 
Kalle Valo

Re: [PATCH 3/3] ath10k: add symlink for hwmon device

[Rajkumar Manoharan]

On Thu, Mar 12, 2015 at 03:20:33PM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > On Tue, Mar 10, 2015 at 05:39:46PM +0200, Kalle Valo wrote:
> >> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> >> 
> >> > Add symbloic link "thermal_sensors" for hwmon devices for better
> >> > readability.
> >> >
> >> > /sys/class/ieee80211/phyX/device/thermal_sensors ->
> >> > sys/class/ieee80211/phyX/device/hwmon/hwmonY
> >> 
> >> I don't get it, how does that improve readability? Also I don't see any
> >> other driver doing this, so isn't this just some private hack and not a
> >> proper generic interface?
> >
> > The patch is providing same level access to monitoring device similar to
> > cooling device.
> >
> > cooling_device -> ../../../../../virtual/thermal/cooling_deviceX
> > thermal_sensors -> hwmon/hwmonX
> 
> Is this documented somewhere? We cannot just start inventing sysfs
> interfaces on our own.
>
Agree. this is not a standard interface. I added symlink just to provide
user readable name instead of hwmon. Please drop this change, if it does
not add any values to existing interface.

-Rajkumar

Re: [PATCH 1/3] ath10k: add sysfs entry to configure quiet period

[Rajkumar Manoharan]

On Thu, Mar 12, 2015 at 03:21:34PM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > On Tue, Mar 10, 2015 at 05:43:58PM +0200, Kalle Valo wrote:
> >> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> >> 
> >> > Add support to configure quiet period via sysfs entry. This will
> >> > be helpful to experiment different quiet period values along with
> >> > different duty cycle ratio.
> >> >
> >> > To configure quiet period as 30ms,
> >> >
> >> > echo 30 >/sys/class/ieee80211/phyX/device/quiet_period
> >> 
> >> What's the justification? To me this looks like an ugly driver private
> >> hack. Why can't you use nl80211 or something else?
> >
> > As this is purely for testing purpose to play around with different
> > quiet period along with various throttling state, sysfs entry is used
> > instead of netlink testmode command. Instead of debugfs, sysfs entry is
> > selected to align with existing thermal interface. 
> 
> Please correct me if I'm wrong, but I consider sysfs as a stable user
> space interface. It's not meant for random testing stuff.
> 
Completely agree. Will move this change under debugfs. Does it sound
good?

-Rajkumar

Re: [PATCH 2/3] ath10k: fix wrong symlink name on error path

[Rajkumar Manoharan]

On Thu, Mar 12, 2015 at 03:00:32PM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > Wrong symlink name is used on error path of thermal registration
> > and also correcting the error message.
> >
> > Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
> 
> Thanks, patch 2 applied. Not sure yet what to do with patch 1 and 3.
>
Will rework on patch 1 (ath10k: add sysfs entry to configure quiet
period) to use debugfs instead of sysfs. Patch 3 can be dropped.

-Rajkumar