[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Mohammed Shafi Shajakhan]

From: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>

this requires complete testing, disabling seems to work fine but
renabling does seems to be complete

Signed-off-by: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>
---
 drivers/net/wireless/ath/ath.h         |    1 +
 drivers/net/wireless/ath/ath9k/debug.c |   52 ++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/init.c  |    6 ++-
 drivers/net/wireless/ath/ath9k/main.c  |   40 +++++++++++++++++-------
 4 files changed, 85 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 7cf4317..17c4b56 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -161,6 +161,7 @@ struct ath_common {
 	const struct ath_bus_ops *bus_ops;
 
 	bool btcoex_enabled;
+	bool disable_ani;
 };
 
 struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index 7afe332..6c9f6bb 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -176,6 +176,56 @@ static const struct file_operations fops_rx_chainmask = {
 	.llseek = default_llseek,
 };
 
+static ssize_t read_file_disable_ani(struct file *file, char __user *user_buf,
+			     size_t count, loff_t *ppos)
+{
+	struct ath_softc *sc = file->private_data;
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	char buf[32];
+	unsigned int len;
+
+	len = sprintf(buf, "%1d\n", common->disable_ani);
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t write_file_disable_ani(struct file *file,
+		const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	struct ath_softc *sc = file->private_data;
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	unsigned long disable_ani;
+	char buf[32];
+	ssize_t len;
+
+	len = min(count, sizeof(buf) - 1);
+	if (copy_from_user(buf, user_buf, len))
+		return -EFAULT;
+
+	buf[len] = '\0';
+	if (strict_strtoul(buf, 0, &disable_ani))
+		return -EINVAL;
+
+	common->disable_ani = !!(disable_ani);
+
+	if (disable_ani) {
+		sc->sc_flags &= ~SC_OP_ANI_RUN;
+		del_timer_sync(&common->ani.timer);
+	}
+
+	else
+		setup_timer(&common->ani.timer, ath_ani_calibrate,
+						(unsigned long)sc);
+
+	return count;
+}
+
+static const struct file_operations fops_disable_ani = {
+	.read = read_file_disable_ani,
+	.write = write_file_disable_ani,
+	.open = ath9k_debugfs_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
 
 static ssize_t read_file_dma(struct file *file, char __user *user_buf,
 			     size_t count, loff_t *ppos)
@@ -1160,6 +1210,8 @@ int ath9k_init_debug(struct ath_hw *ah)
 			    sc->debug.debugfs_phy, sc, &fops_rx_chainmask);
 	debugfs_create_file("tx_chainmask", S_IRUSR | S_IWUSR,
 			    sc->debug.debugfs_phy, sc, &fops_tx_chainmask);
+	debugfs_create_file("disable_ani", S_IRUSR | S_IWUSR,
+			    sc->debug.debugfs_phy, sc, &fops_disable_ani);
 	debugfs_create_file("regidx", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
 			    sc, &fops_regidx);
 	debugfs_create_file("regval", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index b172d15..bf8ac4e 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -519,8 +519,9 @@ static void ath9k_init_misc(struct ath_softc *sc)
 {
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
 	int i = 0;
-
-	setup_timer(&common->ani.timer, ath_ani_calibrate, (unsigned long)sc);
+	if (!(common->disable_ani))
+		setup_timer(&common->ani.timer,
+				ath_ani_calibrate, (unsigned long)sc);
 
 	sc->config.txpowlimit = ATH_TXPOWER_MAX;
 
@@ -585,6 +586,7 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
 	common->priv = sc;
 	common->debug_mask = ath9k_debug;
 	common->btcoex_enabled = ath9k_btcoex_enable == 1;
+	common->disable_ani = false;
 	spin_lock_init(&common->cc_lock);
 
 	spin_lock_init(&sc->sc_serial_rw);
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 17ebdf1..1ac7c67 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -235,7 +235,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
 
 	sc->hw_busy_count = 0;
 
-	del_timer_sync(&common->ani.timer);
+	if (!(common->disable_ani))
+		del_timer_sync(&common->ani.timer);
 	cancel_work_sync(&sc->paprd_work);
 	cancel_work_sync(&sc->hw_check_work);
 	cancel_delayed_work_sync(&sc->tx_complete_work);
@@ -302,7 +303,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
 			ath_set_beacon(sc);
 		ieee80211_queue_delayed_work(sc->hw, &sc->tx_complete_work, 0);
 		ieee80211_queue_delayed_work(sc->hw, &sc->hw_pll_work, HZ/2);
-		ath_start_ani(common);
+		if (!(common->disable_ani))
+			ath_start_ani(common);
 	}
 
  ps_restore:
@@ -972,7 +974,9 @@ int ath_reset(struct ath_softc *sc, bool retry_tx)
 	sc->hw_busy_count = 0;
 
 	/* Stop ANI */
-	del_timer_sync(&common->ani.timer);
+
+	if (!(common->disable_ani))
+		del_timer_sync(&common->ani.timer);
 
 	ath9k_ps_wakeup(sc);
 	spin_lock_bh(&sc->sc_pcu_lock);
@@ -1021,7 +1025,8 @@ int ath_reset(struct ath_softc *sc, bool retry_tx)
 	spin_unlock_bh(&sc->sc_pcu_lock);
 
 	/* Start ANI */
-	ath_start_ani(common);
+	if (!(common->disable_ani))
+		ath_start_ani(common);
 	ath9k_ps_restore(sc);
 
 	return r;
@@ -1412,11 +1417,18 @@ static void ath9k_calculate_summary_state(struct ieee80211_hw *hw,
 	/* Set up ANI */
 	if ((iter_data.naps + iter_data.nadhocs) > 0) {
 		sc->sc_ah->stats.avgbrssi = ATH_RSSI_DUMMY_MARKER;
-		sc->sc_flags |= SC_OP_ANI_RUN;
-		ath_start_ani(common);
+
+		if (!(common->disable_ani)) {
+			sc->sc_flags |= SC_OP_ANI_RUN;
+			ath_start_ani(common);
+		}
 	} else {
-		sc->sc_flags &= ~SC_OP_ANI_RUN;
-		del_timer_sync(&common->ani.timer);
+
+		if (!(common->disable_ani)) {
+			sc->sc_flags &= ~SC_OP_ANI_RUN;
+			del_timer_sync(&common->ani.timer);
+		}
+
 	}
 }
 
@@ -1988,8 +2000,10 @@ static void ath9k_bss_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 			sc->last_rssi = ATH_RSSI_DUMMY_MARKER;
 			sc->sc_ah->stats.avgbrssi = ATH_RSSI_DUMMY_MARKER;
 
-			sc->sc_flags |= SC_OP_ANI_RUN;
-			ath_start_ani(common);
+			if (!(common->disable_ani)) {
+				sc->sc_flags |= SC_OP_ANI_RUN;
+				ath_start_ani(common);
+			}
 		}
 		break;
 	default:
@@ -2025,8 +2039,10 @@ static void ath9k_config_bss(struct ath_softc *sc, struct ieee80211_vif *vif)
 	    !(sc->sc_flags & SC_OP_PRIM_STA_VIF)) {
 		ath9k_hw_write_associd(sc->sc_ah);
 		/* Stop ANI */
-		sc->sc_flags &= ~SC_OP_ANI_RUN;
-		del_timer_sync(&common->ani.timer);
+		if (!(common->disable_ani)) {
+			sc->sc_flags &= ~SC_OP_ANI_RUN;
+			del_timer_sync(&common->ani.timer);
+		}
 	}
 }
 
-- 
1.7.0.4

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Vivek Natarajan]

On Tue, May 17, 2011 at 3:59 PM, Mohammed Shafi Shajakhan
<mshajakhan@atheros.com> wrote:
> From: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>
>
> this requires complete testing, disabling seems to work fine but
> renabling does seems to be complete
>
> Signed-off-by: Mohammed Shafi Shajakhan <mshajakhan@atheros.com>
> diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
> index 7afe332..6c9f6bb 100644
> --- a/drivers/net/wireless/ath/ath9k/debug.c
> +++ b/drivers/net/wireless/ath/ath9k/debug.c
> @@ -176,6 +176,56 @@ static const struct file_operations fops_rx_chainmask = {
> ? ? ? ?.llseek = default_llseek,
> ?};
>
> +static ssize_t read_file_disable_ani(struct file *file, char __user *user_buf,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?size_t count, loff_t *ppos)
> +{
> + ? ? ? struct ath_softc *sc = file->private_data;
> + ? ? ? struct ath_common *common = ath9k_hw_common(sc->sc_ah);
> + ? ? ? char buf[32];
> + ? ? ? unsigned int len;
> +
> + ? ? ? len = sprintf(buf, "%1d\n", common->disable_ani);
> + ? ? ? return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static ssize_t write_file_disable_ani(struct file *file,
> + ? ? ? ? ? ? ? const char __user *user_buf, size_t count, loff_t *ppos)
> +{
> + ? ? ? struct ath_softc *sc = file->private_data;
> + ? ? ? struct ath_common *common = ath9k_hw_common(sc->sc_ah);
> + ? ? ? unsigned long disable_ani;
> + ? ? ? char buf[32];
> + ? ? ? ssize_t len;
> +
> + ? ? ? len = min(count, sizeof(buf) - 1);
> + ? ? ? if (copy_from_user(buf, user_buf, len))
> + ? ? ? ? ? ? ? return -EFAULT;
> +
> + ? ? ? buf[len] = '\0';
> + ? ? ? if (strict_strtoul(buf, 0, &disable_ani))
> + ? ? ? ? ? ? ? return -EINVAL;
> +
> + ? ? ? common->disable_ani = !!(disable_ani);
> +
> + ? ? ? if (disable_ani) {
> + ? ? ? ? ? ? ? sc->sc_flags &= ~SC_OP_ANI_RUN;
> + ? ? ? ? ? ? ? del_timer_sync(&common->ani.timer);
> + ? ? ? }
> +
> + ? ? ? else
> + ? ? ? ? ? ? ? setup_timer(&common->ani.timer, ath_ani_calibrate,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (unsigned long)sc);

For restarting the timer, you need to do mod_timer and not setup_timer.

> +
> + ? ? ? return count;
> +}
> +
> +static const struct file_operations fops_disable_ani = {
> + ? ? ? .read = read_file_disable_ani,
> + ? ? ? .write = write_file_disable_ani,
> + ? ? ? .open = ath9k_debugfs_open,
> + ? ? ? .owner = THIS_MODULE,
> + ? ? ? .llseek = default_llseek,
> +};
>
> ?static ssize_t read_file_dma(struct file *file, char __user *user_buf,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? size_t count, loff_t *ppos)
> @@ -1160,6 +1210,8 @@ int ath9k_init_debug(struct ath_hw *ah)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?sc->debug.debugfs_phy, sc, &fops_rx_chainmask);
> ? ? ? ?debugfs_create_file("tx_chainmask", S_IRUSR | S_IWUSR,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?sc->debug.debugfs_phy, sc, &fops_tx_chainmask);
> + ? ? ? debugfs_create_file("disable_ani", S_IRUSR | S_IWUSR,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? sc->debug.debugfs_phy, sc, &fops_disable_ani);
> ? ? ? ?debugfs_create_file("regidx", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?sc, &fops_regidx);
> ? ? ? ?debugfs_create_file("regval", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> index b172d15..bf8ac4e 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -519,8 +519,9 @@ static void ath9k_init_misc(struct ath_softc *sc)
> ?{
> ? ? ? ?struct ath_common *common = ath9k_hw_common(sc->sc_ah);
> ? ? ? ?int i = 0;
> -
> - ? ? ? setup_timer(&common->ani.timer, ath_ani_calibrate, (unsigned long)sc);
> + ? ? ? if (!(common->disable_ani))
> + ? ? ? ? ? ? ? setup_timer(&common->ani.timer,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ath_ani_calibrate, (unsigned long)sc);

This need not depend on disable_ani since you can stop/restart using
del_timer and mod_timer.

>
> ? ? ? ?sc->config.txpowlimit = ATH_TXPOWER_MAX;
>
> @@ -585,6 +586,7 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
> ? ? ? ?common->priv = sc;
> ? ? ? ?common->debug_mask = ath9k_debug;
> ? ? ? ?common->btcoex_enabled = ath9k_btcoex_enable == 1;
> + ? ? ? common->disable_ani = false;
> ? ? ? ?spin_lock_init(&common->cc_lock);
>
> ? ? ? ?spin_lock_init(&sc->sc_serial_rw);
> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> index 17ebdf1..1ac7c67 100644
> --- a/drivers/net/wireless/ath/ath9k/main.c
> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -235,7 +235,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>
> ? ? ? ?sc->hw_busy_count = 0;
>
> - ? ? ? del_timer_sync(&common->ani.timer);
> + ? ? ? if (!(common->disable_ani))
> + ? ? ? ? ? ? ? del_timer_sync(&common->ani.timer);
> ? ? ? ?cancel_work_sync(&sc->paprd_work);
> ? ? ? ?cancel_work_sync(&sc->hw_check_work);
> ? ? ? ?cancel_delayed_work_sync(&sc->tx_complete_work);
> @@ -302,7 +303,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
> ? ? ? ? ? ? ? ? ? ? ? ?ath_set_beacon(sc);
> ? ? ? ? ? ? ? ?ieee80211_queue_delayed_work(sc->hw, &sc->tx_complete_work, 0);
> ? ? ? ? ? ? ? ?ieee80211_queue_delayed_work(sc->hw, &sc->hw_pll_work, HZ/2);
> - ? ? ? ? ? ? ? ath_start_ani(common);
> + ? ? ? ? ? ? ? if (!(common->disable_ani))
> + ? ? ? ? ? ? ? ? ? ? ? ath_start_ani(common);

The same applies here. If the requirement is just to stop/restart
dynamically during runtime, ani initialisation need not depend on
disable_ani.

Vivek.

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Mohammed Shafi]

On Tuesday 17 May 2011 04:43 PM, Vivek Natarajan wrote:
> On Tue, May 17, 2011 at 3:59 PM, Mohammed Shafi Shajakhan
> <mshajakhan@atheros.com>  wrote:
>> From: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>>
>> this requires complete testing, disabling seems to work fine but
>> renabling does seems to be complete
>>
>> Signed-off-by: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>> diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
>> index 7afe332..6c9f6bb 100644
>> --- a/drivers/net/wireless/ath/ath9k/debug.c
>> +++ b/drivers/net/wireless/ath/ath9k/debug.c
>> @@ -176,6 +176,56 @@ static const struct file_operations fops_rx_chainmask = {
>>         .llseek = default_llseek,
>>   };
>>
>> +static ssize_t read_file_disable_ani(struct file *file, char __user *user_buf,
>> +                            size_t count, loff_t *ppos)
>> +{
>> +       struct ath_softc *sc = file->private_data;
>> +       struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>> +       char buf[32];
>> +       unsigned int len;
>> +
>> +       len = sprintf(buf, "%1d\n", common->disable_ani);
>> +       return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>> +}
>> +
>> +static ssize_t write_file_disable_ani(struct file *file,
>> +               const char __user *user_buf, size_t count, loff_t *ppos)
>> +{
>> +       struct ath_softc *sc = file->private_data;
>> +       struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>> +       unsigned long disable_ani;
>> +       char buf[32];
>> +       ssize_t len;
>> +
>> +       len = min(count, sizeof(buf) - 1);
>> +       if (copy_from_user(buf, user_buf, len))
>> +               return -EFAULT;
>> +
>> +       buf[len] = '\0';
>> +       if (strict_strtoul(buf, 0,&disable_ani))
>> +               return -EINVAL;
>> +
>> +       common->disable_ani = !!(disable_ani);
>> +
>> +       if (disable_ani) {
>> +               sc->sc_flags&= ~SC_OP_ANI_RUN;
>> +               del_timer_sync(&common->ani.timer);
>> +       }
>> +
>> +       else
>> +               setup_timer(&common->ani.timer, ath_ani_calibrate,
>> +                                               (unsigned long)sc);
>
> For restarting the timer, you need to do mod_timer and not setup_timer.

Thanks! I will change that

>
>> +
>> +       return count;
>> +}
>> +
>> +static const struct file_operations fops_disable_ani = {
>> +       .read = read_file_disable_ani,
>> +       .write = write_file_disable_ani,
>> +       .open = ath9k_debugfs_open,
>> +       .owner = THIS_MODULE,
>> +       .llseek = default_llseek,
>> +};
>>
>>   static ssize_t read_file_dma(struct file *file, char __user *user_buf,
>>                              size_t count, loff_t *ppos)
>> @@ -1160,6 +1210,8 @@ int ath9k_init_debug(struct ath_hw *ah)
>>                             sc->debug.debugfs_phy, sc,&fops_rx_chainmask);
>>         debugfs_create_file("tx_chainmask", S_IRUSR | S_IWUSR,
>>                             sc->debug.debugfs_phy, sc,&fops_tx_chainmask);
>> +       debugfs_create_file("disable_ani", S_IRUSR | S_IWUSR,
>> +                           sc->debug.debugfs_phy, sc,&fops_disable_ani);
>>         debugfs_create_file("regidx", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
>>                             sc,&fops_regidx);
>>         debugfs_create_file("regval", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
>> index b172d15..bf8ac4e 100644
>> --- a/drivers/net/wireless/ath/ath9k/init.c
>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>> @@ -519,8 +519,9 @@ static void ath9k_init_misc(struct ath_softc *sc)
>>   {
>>         struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>>         int i = 0;
>> -
>> -       setup_timer(&common->ani.timer, ath_ani_calibrate, (unsigned long)sc);
>> +       if (!(common->disable_ani))
>> +               setup_timer(&common->ani.timer,
>> +                               ath_ani_calibrate, (unsigned long)sc);
>
> This need not depend on disable_ani since you can stop/restart using
> del_timer and mod_timer.

incase of disabling ANI completely right from the start by hardcoding 
disable_ani true in driver initialization, I thought this might help.. 
if this looks redundant I will revert it back.

>
>>
>>         sc->config.txpowlimit = ATH_TXPOWER_MAX;
>>
>> @@ -585,6 +586,7 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
>>         common->priv = sc;
>>         common->debug_mask = ath9k_debug;
>>         common->btcoex_enabled = ath9k_btcoex_enable == 1;
>> +       common->disable_ani = false;
>>         spin_lock_init(&common->cc_lock);
>>
>>         spin_lock_init(&sc->sc_serial_rw);
>> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
>> index 17ebdf1..1ac7c67 100644
>> --- a/drivers/net/wireless/ath/ath9k/main.c
>> +++ b/drivers/net/wireless/ath/ath9k/main.c
>> @@ -235,7 +235,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>>
>>         sc->hw_busy_count = 0;
>>
>> -       del_timer_sync(&common->ani.timer);
>> +       if (!(common->disable_ani))
>> +               del_timer_sync(&common->ani.timer);
>>         cancel_work_sync(&sc->paprd_work);
>>         cancel_work_sync(&sc->hw_check_work);
>>         cancel_delayed_work_sync(&sc->tx_complete_work);
>> @@ -302,7 +303,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>>                         ath_set_beacon(sc);
>>                 ieee80211_queue_delayed_work(sc->hw,&sc->tx_complete_work, 0);
>>                 ieee80211_queue_delayed_work(sc->hw,&sc->hw_pll_work, HZ/2);
>> -               ath_start_ani(common);
>> +               if (!(common->disable_ani))
>> +                       ath_start_ani(common);
>
> The same applies here. If the requirement is just to stop/restart
> dynamically during runtime, ani initialisation need not depend on
> disable_ani.

as you had later suggested, this looks fine.


Thanks a lot for the review! I will change the setup_timer to mod_timer 
and this will fix the ANI getting restarted properly.

>
> Vivek.
> .
>

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Mohammed Shafi]

On Tuesday 17 May 2011 05:08 PM, Mohammed Shajakhan wrote:
> On Tuesday 17 May 2011 04:43 PM, Vivek Natarajan wrote:
>> On Tue, May 17, 2011 at 3:59 PM, Mohammed Shafi Shajakhan
>> <mshajakhan@atheros.com>   wrote:
>>> From: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>>>
>>> this requires complete testing, disabling seems to work fine but
>>> renabling does seems to be complete
>>>
>>> Signed-off-by: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>>> diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
>>> index 7afe332..6c9f6bb 100644
>>> --- a/drivers/net/wireless/ath/ath9k/debug.c
>>> +++ b/drivers/net/wireless/ath/ath9k/debug.c
>>> @@ -176,6 +176,56 @@ static const struct file_operations fops_rx_chainmask = {
>>>          .llseek = default_llseek,
>>>    };
>>>
>>> +static ssize_t read_file_disable_ani(struct file *file, char __user *user_buf,
>>> +                            size_t count, loff_t *ppos)
>>> +{
>>> +       struct ath_softc *sc = file->private_data;
>>> +       struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>>> +       char buf[32];
>>> +       unsigned int len;
>>> +
>>> +       len = sprintf(buf, "%1d\n", common->disable_ani);
>>> +       return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>>> +}
>>> +
>>> +static ssize_t write_file_disable_ani(struct file *file,
>>> +               const char __user *user_buf, size_t count, loff_t *ppos)
>>> +{
>>> +       struct ath_softc *sc = file->private_data;
>>> +       struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>>> +       unsigned long disable_ani;
>>> +       char buf[32];
>>> +       ssize_t len;
>>> +
>>> +       len = min(count, sizeof(buf) - 1);
>>> +       if (copy_from_user(buf, user_buf, len))
>>> +               return -EFAULT;
>>> +
>>> +       buf[len] = '\0';
>>> +       if (strict_strtoul(buf, 0,&disable_ani))
>>> +               return -EINVAL;
>>> +
>>> +       common->disable_ani = !!(disable_ani);
>>> +
>>> +       if (disable_ani) {
>>> +               sc->sc_flags&= ~SC_OP_ANI_RUN;
>>> +               del_timer_sync(&common->ani.timer);
>>> +       }
>>> +
>>> +       else
>>> +               setup_timer(&common->ani.timer, ath_ani_calibrate,
>>> +                                               (unsigned long)sc);
>>
>> For restarting the timer, you need to do mod_timer and not setup_timer.
>
> Thanks! I will change that
>
>>
>>> +
>>> +       return count;
>>> +}
>>> +
>>> +static const struct file_operations fops_disable_ani = {
>>> +       .read = read_file_disable_ani,
>>> +       .write = write_file_disable_ani,
>>> +       .open = ath9k_debugfs_open,
>>> +       .owner = THIS_MODULE,
>>> +       .llseek = default_llseek,
>>> +};
>>>
>>>    static ssize_t read_file_dma(struct file *file, char __user *user_buf,
>>>                               size_t count, loff_t *ppos)
>>> @@ -1160,6 +1210,8 @@ int ath9k_init_debug(struct ath_hw *ah)
>>>                              sc->debug.debugfs_phy, sc,&fops_rx_chainmask);
>>>          debugfs_create_file("tx_chainmask", S_IRUSR | S_IWUSR,
>>>                              sc->debug.debugfs_phy, sc,&fops_tx_chainmask);
>>> +       debugfs_create_file("disable_ani", S_IRUSR | S_IWUSR,
>>> +                           sc->debug.debugfs_phy, sc,&fops_disable_ani);
>>>          debugfs_create_file("regidx", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
>>>                              sc,&fops_regidx);
>>>          debugfs_create_file("regval", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
>>> index b172d15..bf8ac4e 100644
>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>> @@ -519,8 +519,9 @@ static void ath9k_init_misc(struct ath_softc *sc)
>>>    {
>>>          struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>>>          int i = 0;
>>> -
>>> -       setup_timer(&common->ani.timer, ath_ani_calibrate, (unsigned long)sc);
>>> +       if (!(common->disable_ani))
>>> +               setup_timer(&common->ani.timer,
>>> +                               ath_ani_calibrate, (unsigned long)sc);
>>
>> This need not depend on disable_ani since you can stop/restart using
>> del_timer and mod_timer.
>
> incase of disabling ANI completely right from the start by hardcoding
> disable_ani true in driver initialization, I thought this might help..
> if this looks redundant I will revert it back.
>
>>
>>>
>>>          sc->config.txpowlimit = ATH_TXPOWER_MAX;
>>>
>>> @@ -585,6 +586,7 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
>>>          common->priv = sc;
>>>          common->debug_mask = ath9k_debug;
>>>          common->btcoex_enabled = ath9k_btcoex_enable == 1;
>>> +       common->disable_ani = false;
>>>          spin_lock_init(&common->cc_lock);
>>>
>>>          spin_lock_init(&sc->sc_serial_rw);
>>> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
>>> index 17ebdf1..1ac7c67 100644
>>> --- a/drivers/net/wireless/ath/ath9k/main.c
>>> +++ b/drivers/net/wireless/ath/ath9k/main.c
>>> @@ -235,7 +235,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>>>
>>>          sc->hw_busy_count = 0;
>>>
>>> -       del_timer_sync(&common->ani.timer);
>>> +       if (!(common->disable_ani))
>>> +               del_timer_sync(&common->ani.timer);
>>>          cancel_work_sync(&sc->paprd_work);
>>>          cancel_work_sync(&sc->hw_check_work);
>>>          cancel_delayed_work_sync(&sc->tx_complete_work);
>>> @@ -302,7 +303,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>>>                          ath_set_beacon(sc);
>>>                  ieee80211_queue_delayed_work(sc->hw,&sc->tx_complete_work, 0);
>>>                  ieee80211_queue_delayed_work(sc->hw,&sc->hw_pll_work, HZ/2);
>>> -               ath_start_ani(common);
>>> +               if (!(common->disable_ani))
>>> +                       ath_start_ani(common);
>>
>> The same applies here. If the requirement is just to stop/restart
>> dynamically during runtime, ani initialisation need not depend on
>> disable_ani.
>
> as you had later suggested, this looks fine.
>
>
> Thanks a lot for the review! I will change the setup_timer to mod_timer
> and this will fix the ANI getting restarted properly.
>
>>

did not help in perfectly re starting the ANI, still I am missing 
something, i need to look into it more

>> Vivek.
>> .
>>
> .
>

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Mohammed Shafi]

On Tuesday 17 May 2011 05:08 PM, Mohammed Shajakhan wrote:
> On Tuesday 17 May 2011 04:43 PM, Vivek Natarajan wrote:
>> On Tue, May 17, 2011 at 3:59 PM, Mohammed Shafi Shajakhan
>> <mshajakhan@atheros.com>   wrote:
>>> From: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>>>
>>> this requires complete testing, disabling seems to work fine but
>>> renabling does seems to be complete
>>>
>>> Signed-off-by: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>>> diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
>>> index 7afe332..6c9f6bb 100644
>>> --- a/drivers/net/wireless/ath/ath9k/debug.c
>>> +++ b/drivers/net/wireless/ath/ath9k/debug.c
>>> @@ -176,6 +176,56 @@ static const struct file_operations fops_rx_chainmask = {
>>>          .llseek = default_llseek,
>>>    };
>>>
>>> +static ssize_t read_file_disable_ani(struct file *file, char __user *user_buf,
>>> +                            size_t count, loff_t *ppos)
>>> +{
>>> +       struct ath_softc *sc = file->private_data;
>>> +       struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>>> +       char buf[32];
>>> +       unsigned int len;
>>> +
>>> +       len = sprintf(buf, "%1d\n", common->disable_ani);
>>> +       return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>>> +}
>>> +
>>> +static ssize_t write_file_disable_ani(struct file *file,
>>> +               const char __user *user_buf, size_t count, loff_t *ppos)
>>> +{
>>> +       struct ath_softc *sc = file->private_data;
>>> +       struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>>> +       unsigned long disable_ani;
>>> +       char buf[32];
>>> +       ssize_t len;
>>> +
>>> +       len = min(count, sizeof(buf) - 1);
>>> +       if (copy_from_user(buf, user_buf, len))
>>> +               return -EFAULT;
>>> +
>>> +       buf[len] = '\0';
>>> +       if (strict_strtoul(buf, 0,&disable_ani))
>>> +               return -EINVAL;
>>> +
>>> +       common->disable_ani = !!(disable_ani);
>>> +
>>> +       if (disable_ani) {
>>> +               sc->sc_flags&= ~SC_OP_ANI_RUN;
>>> +               del_timer_sync(&common->ani.timer);
>>> +       }
>>> +
>>> +       else
>>> +               setup_timer(&common->ani.timer, ath_ani_calibrate,
>>> +                                               (unsigned long)sc);

*need to enable SC_OP_ANI_RUN flag, thanks to Raj who pointed out this. 
This will properly restart the ANI and mod_timer as suggested by Vivek.
*As pointed out by Raj, we also need to address the issue, that when we 
are not associated we can start the ANI but this should not be the case.
*Also no need to have a disable_ani check for every del_timer_sync

>>
>> For restarting the timer, you need to do mod_timer and not setup_timer.
>
> Thanks! I will change that
>
>>
>>> +
>>> +       return count;
>>> +}
>>> +
>>> +static const struct file_operations fops_disable_ani = {
>>> +       .read = read_file_disable_ani,
>>> +       .write = write_file_disable_ani,
>>> +       .open = ath9k_debugfs_open,
>>> +       .owner = THIS_MODULE,
>>> +       .llseek = default_llseek,
>>> +};
>>>
>>>    static ssize_t read_file_dma(struct file *file, char __user *user_buf,
>>>                               size_t count, loff_t *ppos)
>>> @@ -1160,6 +1210,8 @@ int ath9k_init_debug(struct ath_hw *ah)
>>>                              sc->debug.debugfs_phy, sc,&fops_rx_chainmask);
>>>          debugfs_create_file("tx_chainmask", S_IRUSR | S_IWUSR,
>>>                              sc->debug.debugfs_phy, sc,&fops_tx_chainmask);
>>> +       debugfs_create_file("disable_ani", S_IRUSR | S_IWUSR,
>>> +                           sc->debug.debugfs_phy, sc,&fops_disable_ani);
>>>          debugfs_create_file("regidx", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
>>>                              sc,&fops_regidx);
>>>          debugfs_create_file("regval", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
>>> index b172d15..bf8ac4e 100644
>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>> @@ -519,8 +519,9 @@ static void ath9k_init_misc(struct ath_softc *sc)
>>>    {
>>>          struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>>>          int i = 0;
>>> -
>>> -       setup_timer(&common->ani.timer, ath_ani_calibrate, (unsigned long)sc);
>>> +       if (!(common->disable_ani))
>>> +               setup_timer(&common->ani.timer,
>>> +                               ath_ani_calibrate, (unsigned long)sc);
>>
>> This need not depend on disable_ani since you can stop/restart using
>> del_timer and mod_timer.
>
> incase of disabling ANI completely right from the start by hardcoding
> disable_ani true in driver initialization, I thought this might help..
> if this looks redundant I will revert it back.
>
>>
>>>
>>>          sc->config.txpowlimit = ATH_TXPOWER_MAX;
>>>
>>> @@ -585,6 +586,7 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
>>>          common->priv = sc;
>>>          common->debug_mask = ath9k_debug;
>>>          common->btcoex_enabled = ath9k_btcoex_enable == 1;
>>> +       common->disable_ani = false;
>>>          spin_lock_init(&common->cc_lock);
>>>
>>>          spin_lock_init(&sc->sc_serial_rw);
>>> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
>>> index 17ebdf1..1ac7c67 100644
>>> --- a/drivers/net/wireless/ath/ath9k/main.c
>>> +++ b/drivers/net/wireless/ath/ath9k/main.c
>>> @@ -235,7 +235,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>>>
>>>          sc->hw_busy_count = 0;
>>>
>>> -       del_timer_sync(&common->ani.timer);
>>> +       if (!(common->disable_ani))
>>> +               del_timer_sync(&common->ani.timer);
>>>          cancel_work_sync(&sc->paprd_work);
>>>          cancel_work_sync(&sc->hw_check_work);
>>>          cancel_delayed_work_sync(&sc->tx_complete_work);
>>> @@ -302,7 +303,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>>>                          ath_set_beacon(sc);
>>>                  ieee80211_queue_delayed_work(sc->hw,&sc->tx_complete_work, 0);
>>>                  ieee80211_queue_delayed_work(sc->hw,&sc->hw_pll_work, HZ/2);
>>> -               ath_start_ani(common);
>>> +               if (!(common->disable_ani))
>>> +                       ath_start_ani(common);
>>
>> The same applies here. If the requirement is just to stop/restart
>> dynamically during runtime, ani initialisation need not depend on
>> disable_ani.
>
> as you had later suggested, this looks fine.
>
>
> Thanks a lot for the review! I will change the setup_timer to mod_timer
> and this will fix the ANI getting restarted properly.
>
>>
>> Vivek.
>> .
>>
> .
>

-- 
shafi

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Peter Stuge]

Mohammed Shafi Shajakhan wrote:
> +static ssize_t read_file_disable_ani(struct file *file, char __user *user_buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct ath_softc *sc = file->private_data;
> +	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
> +	char buf[32];
> +	unsigned int len;
> +
> +	len = sprintf(buf, "%1d\n", common->disable_ani);
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}

Note that %1d does not really guarantee the output length. The buffer
is 32 chars so it should be fine, but to really make sure I would

len = sprintf(buf, "%d\n", !!common->disable_ani);

..even though this is also done on input.


> +static ssize_t write_file_disable_ani(struct file *file,
> +		const char __user *user_buf, size_t count, loff_t *ppos)
> +{
..
> +	common->disable_ani = !!(disable_ani);

No () strictly needed.  common->disable_ani = !!disable_ani; should
be fine.


> +	if (disable_ani) {

Would suggest to only use the processed and assigned value once the
processing and assignment has been done, to avoid any accidents where
the unprocessed value is misinterpreted, and to avoid unneccessary
duplication of the processing.


> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -235,7 +235,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>  
>  	sc->hw_busy_count = 0;
>  
> -	del_timer_sync(&common->ani.timer);
> +	if (!(common->disable_ani))
> +		del_timer_sync(&common->ani.timer);
>  	cancel_work_sync(&sc->paprd_work);
>  	cancel_work_sync(&sc->hw_check_work);
>  	cancel_delayed_work_sync(&sc->tx_complete_work);
> @@ -302,7 +303,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>  			ath_set_beacon(sc);
>  		ieee80211_queue_delayed_work(sc->hw, &sc->tx_complete_work, 0);
>  		ieee80211_queue_delayed_work(sc->hw, &sc->hw_pll_work, HZ/2);
> -		ath_start_ani(common);
> +		if (!(common->disable_ani))
> +			ath_start_ani(common);

Again I personally find without the extra () to be much more
readable, but maybe that's even a style violation.


//Peter

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Adrian Chadd]

On 18 May 2011 09:16, Peter Stuge <peter@stuge.se> wrote:

>> + ? ? len = sprintf(buf, "%1d\n", common->disable_ani);
>> + ? ? return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>> +}
>
> Note that %1d does not really guarantee the output length. The buffer
> is 32 chars so it should be fine, but to really make sure I would
>
> len = sprintf(buf, "%d\n", !!common->disable_ani);
>
> ..even though this is also done on input.

Or:

len = sprintf(buf, "%s\n", (!!common->disable_ani) ? "disabled" : "enabled");




Adrian

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Mohammed Shafi]

On Wed, May 18, 2011 at 7:49 AM, Adrian Chadd <adrian@freebsd.org> wrote:
> On 18 May 2011 09:16, Peter Stuge <peter@stuge.se> wrote:
>
>>> + ? ? len = sprintf(buf, "%1d\n", common->disable_ani);
>>> + ? ? return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>>> +}
>>
>> Note that %1d does not really guarantee the output length. The buffer
>> is 32 chars so it should be fine, but to really make sure I would
>>
>> len = sprintf(buf, "%d\n", !!common->disable_ani);
>>
>> ..even though this is also done on input.
>
> Or:
>
> len = sprintf(buf, "%s\n", (!!common->disable_ani) ? "disabled" : "enabled");

we will get the output like this
sudo cat disable_ani
0
echo 1 > disable_ani
sudo cat disable_ani
1
>
>
>
>
> Adrian
> _______________________________________________
> ath9k-devel mailing list
> ath9k-devel at lists.ath9k.org
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel
>



-- 
shafi

[ath9k-devel] [RFC] ath9k: Add a debug entry to stop/start ANI

[Mohammed Shafi]

On Wed, May 18, 2011 at 6:46 AM, Peter Stuge <peter@stuge.se> wrote:
> Mohammed Shafi Shajakhan wrote:
>> +static ssize_t read_file_disable_ani(struct file *file, char __user *user_buf,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?size_t count, loff_t *ppos)
>> +{
>> + ? ? struct ath_softc *sc = file->private_data;
>> + ? ? struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>> + ? ? char buf[32];
>> + ? ? unsigned int len;
>> +
>> + ? ? len = sprintf(buf, "%1d\n", common->disable_ani);
>> + ? ? return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>> +}
>
> Note that %1d does not really guarantee the output length. The buffer
> is 32 chars so it should be fine, but to really make sure I would
>
> len = sprintf(buf, "%d\n", !!common->disable_ani);
>
> ..even though this is also done on input.

Ok.

>
>
>> +static ssize_t write_file_disable_ani(struct file *file,
>> + ? ? ? ? ? ? const char __user *user_buf, size_t count, loff_t *ppos)
>> +{
> ..
>> + ? ? common->disable_ani = !!(disable_ani);
>
> No () strictly needed. ?common->disable_ani = !!disable_ani; should
> be fine.

yes, did not look :)

>
>
>> + ? ? if (disable_ani) {
>
> Would suggest to only use the processed and assigned value once the
> processing and assignment has been done, to avoid any accidents where
> the unprocessed value is misinterpreted, and to avoid unneccessary
> duplication of the processing.


we can both enable/disable via debugfs, so i think its necessary.

>
>
>> +++ b/drivers/net/wireless/ath/ath9k/main.c
>> @@ -235,7 +235,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>>
>> ? ? ? sc->hw_busy_count = 0;
>>
>> - ? ? del_timer_sync(&common->ani.timer);
>> + ? ? if (!(common->disable_ani))
>> + ? ? ? ? ? ? del_timer_sync(&common->ani.timer);
>> ? ? ? cancel_work_sync(&sc->paprd_work);
>> ? ? ? cancel_work_sync(&sc->hw_check_work);
>> ? ? ? cancel_delayed_work_sync(&sc->tx_complete_work);
>> @@ -302,7 +303,8 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>> ? ? ? ? ? ? ? ? ? ? ? ath_set_beacon(sc);
>> ? ? ? ? ? ? ? ieee80211_queue_delayed_work(sc->hw, &sc->tx_complete_work, 0);
>> ? ? ? ? ? ? ? ieee80211_queue_delayed_work(sc->hw, &sc->hw_pll_work, HZ/2);
>> - ? ? ? ? ? ? ath_start_ani(common);
>> + ? ? ? ? ? ? if (!(common->disable_ani))
>> + ? ? ? ? ? ? ? ? ? ? ath_start_ani(common);
>
> Again I personally find without the extra () to be much more
> readable, but maybe that's even a style violation.

agree, I will look into that

>
>
> //Peter
> _______________________________________________
> ath9k-devel mailing list
> ath9k-devel at lists.ath9k.org
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel
>