[PATCH 0/8] wlcore/wl12xx/wl18xx: some updates for 3.6

[PATCH 0/8] wlcore/wl12xx/wl18xx: some updates for 3.6

[Luciano Coelho]

Hi,

A few more updates intended for 3.6.  Mostly small bugfixes.

Please review.

Cheers,
Luca.

Arik Nemtsov (5):
  wlcore: don't set SDIO_FAILED flag when driver state is off
  wlcore: define number of supported bands internally
  wl12xx/wl18xx: use a dynamic PS timeout of 1.5sec
  wl18xx: don't send static global struct to FW
  wlcore: determine AP extra rates correctly

Eliad Peller (1):
  wlcore: check ssid length against the correct element

Ido Yariv (1):
  wlcore: Prevent processing of work items during op_stop

Yoni Divinsky (1):
  wlcore: change the wait for event mechanism

 drivers/net/wireless/ti/wl12xx/main.c     |    2 +-
 drivers/net/wireless/ti/wl18xx/main.c     |   37 +++++++++++++++-----
 drivers/net/wireless/ti/wlcore/cmd.c      |    8 ++++-
 drivers/net/wireless/ti/wlcore/io.c       |    6 ++++
 drivers/net/wireless/ti/wlcore/io.h       |   13 ++++----
 drivers/net/wireless/ti/wlcore/main.c     |   52 ++++++++++++++---------------
 drivers/net/wireless/ti/wlcore/scan.c     |    2 +-
 drivers/net/wireless/ti/wlcore/wlcore.h   |    4 +--
 drivers/net/wireless/ti/wlcore/wlcore_i.h |    7 ++--
 9 files changed, 84 insertions(+), 47 deletions(-)

-- 
1.7.10

[PATCH 1/8] wlcore: Prevent processing of work items during op_stop

[Luciano Coelho]

From: Ido Yariv <ido@wizery.com>

The interrupt line is disabled in op_stop using disable_irq. Since
pending interrupts are synchronized, the mutex has to be released before
disabling the interrupt to avoid a deadlock with the interrupt handler.

In addition, the internal state of the driver is only set to 'off'
after the interrupt is disabled. Otherwise, if an interrupt fires after
the state is set but before the interrupt line is disabled, the
interrupt handler will not be able to acknowledge the interrupt
resulting in an interrupt storm.

The driver's operations might be called during recovery. If these
acquire the mutex after it was released by op_stop, but before the
driver's state is changed, they may queue new work items instead of just
failing. This is especially problematic in the case of scans, in which a
new scan may be scheduled after all scan requests were cancelled.

Signed-off-by: Ido Yariv <ido@wizery.com>
Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/ti/wlcore/io.c   |    6 ++++
 drivers/net/wireless/ti/wlcore/io.h   |    1 +
 drivers/net/wireless/ti/wlcore/main.c |   50 ++++++++++++++++-----------------
 3 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/io.c b/drivers/net/wireless/ti/wlcore/io.c
index 9976219..68e74ee 100644
--- a/drivers/net/wireless/ti/wlcore/io.c
+++ b/drivers/net/wireless/ti/wlcore/io.c
@@ -60,6 +60,12 @@ void wlcore_enable_interrupts(struct wl1271 *wl)
 }
 EXPORT_SYMBOL_GPL(wlcore_enable_interrupts);
 
+void wlcore_synchronize_interrupts(struct wl1271 *wl)
+{
+	synchronize_irq(wl->irq);
+}
+EXPORT_SYMBOL_GPL(wlcore_synchronize_interrupts);
+
 int wlcore_translate_addr(struct wl1271 *wl, int addr)
 {
 	struct wlcore_partition_set *part = &wl->curr_part;
diff --git a/drivers/net/wireless/ti/wlcore/io.h b/drivers/net/wireless/ti/wlcore/io.h
index fef80ad..458da55 100644
--- a/drivers/net/wireless/ti/wlcore/io.h
+++ b/drivers/net/wireless/ti/wlcore/io.h
@@ -47,6 +47,7 @@ struct wl1271;
 void wlcore_disable_interrupts(struct wl1271 *wl);
 void wlcore_disable_interrupts_nosync(struct wl1271 *wl);
 void wlcore_enable_interrupts(struct wl1271 *wl);
+void wlcore_synchronize_interrupts(struct wl1271 *wl);
 
 void wl1271_io_reset(struct wl1271 *wl);
 void wl1271_io_init(struct wl1271 *wl);
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index 6ac3323..5632194 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -62,7 +62,7 @@ static bool no_recovery;
 static void __wl1271_op_remove_interface(struct wl1271 *wl,
 					 struct ieee80211_vif *vif,
 					 bool reset_tx_queues);
-static void wl1271_op_stop(struct ieee80211_hw *hw);
+static void wlcore_op_stop_locked(struct wl1271 *wl);
 static void wl1271_free_ap_keys(struct wl1271 *wl, struct wl12xx_vif *wlvif);
 
 static int wl12xx_set_authorized(struct wl1271 *wl,
@@ -956,9 +956,8 @@ static void wl1271_recovery_work(struct work_struct *work)
 		vif = wl12xx_wlvif_to_vif(wlvif);
 		__wl1271_op_remove_interface(wl, vif, false);
 	}
-        wl->watchdog_recovery = false;
-	mutex_unlock(&wl->mutex);
-	wl1271_op_stop(wl->hw);
+
+	wlcore_op_stop_locked(wl);
 
 	ieee80211_restart_hw(wl->hw);
 
@@ -967,7 +966,7 @@ static void wl1271_recovery_work(struct work_struct *work)
 	 * to restart the HW.
 	 */
 	wlcore_wake_queues(wl, WLCORE_QUEUE_STOP_REASON_FW_RESTART);
-	return;
+
 out_unlock:
 	wl->watchdog_recovery = false;
 	clear_bit(WL1271_FLAG_RECOVERY_IN_PROGRESS, &wl->flags);
@@ -1800,33 +1799,15 @@ static int wl1271_op_start(struct ieee80211_hw *hw)
 	return 0;
 }
 
-static void wl1271_op_stop(struct ieee80211_hw *hw)
+static void wlcore_op_stop_locked(struct wl1271 *wl)
 {
-	struct wl1271 *wl = hw->priv;
 	int i;
 
-	wl1271_debug(DEBUG_MAC80211, "mac80211 stop");
-
-	/*
-	 * Interrupts must be disabled before setting the state to OFF.
-	 * Otherwise, the interrupt handler might be called and exit without
-	 * reading the interrupt status.
-	 */
-	wlcore_disable_interrupts(wl);
-	mutex_lock(&wl->mutex);
 	if (wl->state == WL1271_STATE_OFF) {
 		if (test_and_clear_bit(WL1271_FLAG_RECOVERY_IN_PROGRESS,
 					&wl->flags))
 			wlcore_enable_interrupts(wl);
 
-		mutex_unlock(&wl->mutex);
-
-		/*
-		 * This will not necessarily enable interrupts as interrupts
-		 * may have been disabled when op_stop was called. It will,
-		 * however, balance the above call to disable_interrupts().
-		 */
-		wlcore_enable_interrupts(wl);
 		return;
 	}
 
@@ -1835,8 +1816,16 @@ static void wl1271_op_stop(struct ieee80211_hw *hw)
 	 * functions don't perform further work.
 	 */
 	wl->state = WL1271_STATE_OFF;
+
+	/*
+	 * Use the nosync variant to disable interrupts, so the mutex could be
+	 * held while doing so without deadlocking.
+	 */
+	wlcore_disable_interrupts_nosync(wl);
+
 	mutex_unlock(&wl->mutex);
 
+	wlcore_synchronize_interrupts(wl);
 	wl1271_flush_deferred_work(wl);
 	cancel_delayed_work_sync(&wl->scan_complete_work);
 	cancel_work_sync(&wl->netstack_work);
@@ -1903,6 +1892,17 @@ static void wl1271_op_stop(struct ieee80211_hw *hw)
 	wl->tx_res_if = NULL;
 	kfree(wl->target_mem_map);
 	wl->target_mem_map = NULL;
+}
+
+static void wlcore_op_stop(struct ieee80211_hw *hw)
+{
+	struct wl1271 *wl = hw->priv;
+
+	wl1271_debug(DEBUG_MAC80211, "mac80211 stop");
+
+	mutex_lock(&wl->mutex);
+
+	wlcore_op_stop_locked(wl);
 
 	mutex_unlock(&wl->mutex);
 }
@@ -4806,7 +4806,7 @@ static struct ieee80211_supported_band wl1271_band_5ghz = {
 
 static const struct ieee80211_ops wl1271_ops = {
 	.start = wl1271_op_start,
-	.stop = wl1271_op_stop,
+	.stop = wlcore_op_stop,
 	.add_interface = wl1271_op_add_interface,
 	.remove_interface = wl1271_op_remove_interface,
 	.change_interface = wl12xx_op_change_interface,
-- 
1.7.10

[PATCH 2/8] wlcore: change the wait for event mechanism

[Luciano Coelho]

From: Yoni Divinsky <yoni.divinsky@ti.com>

wlcore needs to wait for certain events for example
for roc complete event. Usually the events are received
from the FW very fast, therefore wlcore can poll with
a short delay and if after a second the event was
not received yet poll with a long (1-5 msec) delay.

This implementation is similar to the sending of
commands to the FW.

Empirically the change reduced the wait for roc event
from ~10-40msec to 100s of usecs.

[replace udelay/msleep with usleep_range - Arik]

Signed-off-by: Yoni Divinsky <yoni.divinsky@ti.com>
Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/ti/wlcore/cmd.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/cmd.c b/drivers/net/wireless/ti/wlcore/cmd.c
index 087cb01..a23949c 100644
--- a/drivers/net/wireless/ti/wlcore/cmd.c
+++ b/drivers/net/wireless/ti/wlcore/cmd.c
@@ -39,6 +39,7 @@
 #include "hw_ops.h"
 
 #define WL1271_CMD_FAST_POLL_COUNT       50
+#define WL1271_WAIT_EVENT_FAST_POLL_COUNT 20
 
 /*
  * send command to firmware
@@ -138,6 +139,7 @@ static int wl1271_cmd_wait_for_event_or_timeout(struct wl1271 *wl,
 	u32 *events_vector;
 	u32 event;
 	unsigned long timeout_time;
+	u16 poll_count = 0;
 	int ret = 0;
 
 	*timeout = false;
@@ -156,7 +158,11 @@ static int wl1271_cmd_wait_for_event_or_timeout(struct wl1271 *wl,
 			goto out;
 		}
 
-		msleep(1);
+		poll_count++;
+		if (poll_count < WL1271_WAIT_EVENT_FAST_POLL_COUNT)
+			usleep_range(50, 51);
+		else
+			usleep_range(1000, 5000);
 
 		/* read from both event fields */
 		ret = wlcore_read(wl, wl->mbox_ptr[0], events_vector,
-- 
1.7.10

[PATCH 3/8] wlcore: don't set SDIO_FAILED flag when driver state is off

[Luciano Coelho]

From: Arik Nemtsov <arik@wizery.com>

If some IO read/write fails while the FW is not loaded, a recovery
will not take place. This means the SDIO_FAILED flag will stay in place
forever and prevent further read/writes.

This can happen if a check for STATE_OFF was forgotten in some routine.

Take this opportunity to rename the flag to IO_FAILED, since we support
other buses as well.

Reported-by: Ido Yariv <ido@wizery.com>
Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/ti/wlcore/io.h       |   12 ++++++------
 drivers/net/wireless/ti/wlcore/wlcore_i.h |    2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/io.h b/drivers/net/wireless/ti/wlcore/io.h
index 458da55..259149f 100644
--- a/drivers/net/wireless/ti/wlcore/io.h
+++ b/drivers/net/wireless/ti/wlcore/io.h
@@ -60,12 +60,12 @@ static inline int __must_check wlcore_raw_write(struct wl1271 *wl, int addr,
 {
 	int ret;
 
-	if (test_bit(WL1271_FLAG_SDIO_FAILED, &wl->flags))
+	if (test_bit(WL1271_FLAG_IO_FAILED, &wl->flags))
 		return -EIO;
 
 	ret = wl->if_ops->write(wl->dev, addr, buf, len, fixed);
-	if (ret)
-		set_bit(WL1271_FLAG_SDIO_FAILED, &wl->flags);
+	if (ret && wl->state != WL1271_STATE_OFF)
+		set_bit(WL1271_FLAG_IO_FAILED, &wl->flags);
 
 	return ret;
 }
@@ -76,12 +76,12 @@ static inline int __must_check wlcore_raw_read(struct wl1271 *wl, int addr,
 {
 	int ret;
 
-	if (test_bit(WL1271_FLAG_SDIO_FAILED, &wl->flags))
+	if (test_bit(WL1271_FLAG_IO_FAILED, &wl->flags))
 		return -EIO;
 
 	ret = wl->if_ops->read(wl->dev, addr, buf, len, fixed);
-	if (ret)
-		set_bit(WL1271_FLAG_SDIO_FAILED, &wl->flags);
+	if (ret && wl->state != WL1271_STATE_OFF)
+		set_bit(WL1271_FLAG_IO_FAILED, &wl->flags);
 
 	return ret;
 }
diff --git a/drivers/net/wireless/ti/wlcore/wlcore_i.h b/drivers/net/wireless/ti/wlcore/wlcore_i.h
index a760407..2a0e896 100644
--- a/drivers/net/wireless/ti/wlcore/wlcore_i.h
+++ b/drivers/net/wireless/ti/wlcore/wlcore_i.h
@@ -238,7 +238,7 @@ enum wl12xx_flags {
 	WL1271_FLAG_RECOVERY_IN_PROGRESS,
 	WL1271_FLAG_VIF_CHANGE_IN_PROGRESS,
 	WL1271_FLAG_INTENDED_FW_RECOVERY,
-	WL1271_FLAG_SDIO_FAILED,
+	WL1271_FLAG_IO_FAILED,
 };
 
 enum wl12xx_vif_flags {
-- 
1.7.10

[PATCH 4/8] wlcore: define number of supported bands internally

[Luciano Coelho]

From: Arik Nemtsov <arik@wizery.com>

Avoid using the IEEE80211_NUM_BANDS constant for arrays sizes etc, as
this can contain bands unsupported by the driver (e.g. 60Ghz). Use an
internal constant to determine the number of bands.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/ti/wlcore/main.c     |    2 +-
 drivers/net/wireless/ti/wlcore/wlcore.h   |    4 ++--
 drivers/net/wireless/ti/wlcore/wlcore_i.h |    5 ++++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index 5632194..1aecc46 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -4569,7 +4569,7 @@ static int wl12xx_set_bitrate_mask(struct ieee80211_hw *hw,
 
 	mutex_lock(&wl->mutex);
 
-	for (i = 0; i < IEEE80211_NUM_BANDS; i++)
+	for (i = 0; i < WLCORE_NUM_BANDS; i++)
 		wlvif->bitrate_masks[i] =
 			wl1271_tx_enabled_rates_get(wl,
 						    mask->control[i].legacy,
diff --git a/drivers/net/wireless/ti/wlcore/wlcore.h b/drivers/net/wireless/ti/wlcore/wlcore.h
index 0df731a..27ccc27 100644
--- a/drivers/net/wireless/ti/wlcore/wlcore.h
+++ b/drivers/net/wireless/ti/wlcore/wlcore.h
@@ -304,7 +304,7 @@ struct wl1271 {
 	s8 noise;
 
 	/* bands supported by this instance of wl12xx */
-	struct ieee80211_supported_band bands[IEEE80211_NUM_BANDS];
+	struct ieee80211_supported_band bands[WLCORE_NUM_BANDS];
 
 	/*
 	 * wowlan trigger was configured during suspend.
@@ -371,7 +371,7 @@ struct wl1271 {
 	u8 hw_min_ht_rate;
 
 	/* HW HT (11n) capabilities */
-	struct ieee80211_sta_ht_cap ht_cap[IEEE80211_NUM_BANDS];
+	struct ieee80211_sta_ht_cap ht_cap[WLCORE_NUM_BANDS];
 
 	/* size of the private FW status data */
 	size_t fw_status_priv_len;
diff --git a/drivers/net/wireless/ti/wlcore/wlcore_i.h b/drivers/net/wireless/ti/wlcore/wlcore_i.h
index 2a0e896..0187eef 100644
--- a/drivers/net/wireless/ti/wlcore/wlcore_i.h
+++ b/drivers/net/wireless/ti/wlcore/wlcore_i.h
@@ -62,6 +62,9 @@
 #define WL12XX_INVALID_ROLE_ID     0xff
 #define WL12XX_INVALID_LINK_ID     0xff
 
+/* the driver supports the 2.4Ghz and 5Ghz bands */
+#define WLCORE_NUM_BANDS           2
+
 #define WL12XX_MAX_RATE_POLICIES 16
 
 /* Defined by FW as 0. Will not be freed or allocated. */
@@ -360,7 +363,7 @@ struct wl12xx_vif {
 	int channel;
 	enum nl80211_channel_type channel_type;
 
-	u32 bitrate_masks[IEEE80211_NUM_BANDS];
+	u32 bitrate_masks[WLCORE_NUM_BANDS];
 	u32 basic_rate_set;
 
 	/*
-- 
1.7.10

[PATCH 5/8] wl12xx/wl18xx: use a dynamic PS timeout of 1.5sec

[Luciano Coelho]

From: Arik Nemtsov <arik@wizery.com>

It seems some parties have bad user experience when smaller values
are used. This should have little implications for power consumption,
since traffic is bursty in nature.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/ti/wl12xx/main.c |    2 +-
 drivers/net/wireless/ti/wl18xx/main.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wl12xx/main.c b/drivers/net/wireless/ti/wl12xx/main.c
index da261cf..3d6c71b 100644
--- a/drivers/net/wireless/ti/wl12xx/main.c
+++ b/drivers/net/wireless/ti/wl12xx/main.c
@@ -242,7 +242,7 @@ static struct wlcore_conf wl12xx_conf = {
 		.psm_entry_retries           = 8,
 		.psm_exit_retries            = 16,
 		.psm_entry_nullfunc_retries  = 3,
-		.dynamic_ps_timeout          = 200,
+		.dynamic_ps_timeout          = 1500,
 		.forced_ps                   = false,
 		.keep_alive_interval         = 55000,
 		.max_listen_interval         = 20,
diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index 341e878..23f100a3 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -369,7 +369,7 @@ static struct wlcore_conf wl18xx_conf = {
 		.psm_entry_retries           = 8,
 		.psm_exit_retries            = 16,
 		.psm_entry_nullfunc_retries  = 3,
-		.dynamic_ps_timeout          = 200,
+		.dynamic_ps_timeout          = 1500,
 		.forced_ps                   = false,
 		.keep_alive_interval         = 55000,
 		.max_listen_interval         = 20,
-- 
1.7.10

[PATCH 6/8] wlcore: check ssid length against the correct element

[Luciano Coelho]

From: Eliad Peller <eliad@wizery.com>

commit 587cc28 ("wlcore: compare ssid_len before comparing
ssids") introduced a new bug - the ssid length from the
request struct was compared against the ssid length of
another request, instead the one of the cmd.

This might cause the sched scan request to fail
(with -EINVAL) in many cases.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/ti/wlcore/scan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/scan.c b/drivers/net/wireless/ti/wlcore/scan.c
index b03eb9a..dbeca1b 100644
--- a/drivers/net/wireless/ti/wlcore/scan.c
+++ b/drivers/net/wireless/ti/wlcore/scan.c
@@ -633,7 +633,7 @@ wl12xx_scan_sched_scan_ssid_list(struct wl1271 *wl,
 
 				for (j = 0; j < cmd->n_ssids; j++)
 					if ((req->ssids[i].ssid_len ==
-					     req->ssids[j].ssid_len) &&
+					     cmd->ssids[j].len) &&
 					    !memcmp(req->ssids[i].ssid,
 						   cmd->ssids[j].ssid,
 						   req->ssids[i].ssid_len)) {
-- 
1.7.10

[PATCH 7/8] wl18xx: don't send static global struct to FW

[Luciano Coelho]

From: Arik Nemtsov <arik@wizery.com>

We get DMA alignment trouble if the beginning of the struct is
unaligned. Allocate memory and send it to FW.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/ti/wl18xx/main.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index 23f100a3..aea2e32 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -772,16 +772,27 @@ out:
 static int wl18xx_set_mac_and_phy(struct wl1271 *wl)
 {
 	struct wl18xx_priv *priv = wl->priv;
+	struct wl18xx_mac_and_phy_params *params;
 	int ret;
 
+	params = kzalloc(sizeof(*params), GFP_KERNEL);
+	if (!params) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* use aligned memory for the copy to FW */
+	memcpy(params, &priv->conf.phy, sizeof(*params));
+
 	ret = wlcore_set_partition(wl, &wl->ptable[PART_PHY_INIT]);
 	if (ret < 0)
 		goto out;
 
-	ret = wlcore_write(wl, WL18XX_PHY_INIT_MEM_ADDR, (u8 *)&priv->conf.phy,
-			   sizeof(struct wl18xx_mac_and_phy_params), false);
+	ret = wlcore_write(wl, WL18XX_PHY_INIT_MEM_ADDR, params,
+			   sizeof(*params), false);
 
 out:
+	kfree(params);
 	return ret;
 }
 
-- 
1.7.10

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Johannes Berg]

So the subject is wrong since "&priv->conf.phy" can hardly be "static
global", but 

> We get DMA alignment trouble if the beginning of the struct is
> unaligned. Allocate memory and send it to FW.

If this is all about alignment, and

> +	params = kzalloc(sizeof(*params), GFP_KERNEL);

kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
then most likely you could just put "__aligned(4)" or something on the
conf.phy struct member and not allocate memory at all?

johannes

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Luciano Coelho]

On Sun, 2012-07-08 at 11:35 +0200, Johannes Berg wrote:
> So the subject is wrong since "&priv->conf.phy" can hardly be "static
> global", but 
> 
> > We get DMA alignment trouble if the beginning of the struct is
> > unaligned. Allocate memory and send it to FW.
> 
> If this is all about alignment, and
> 
> > +	params = kzalloc(sizeof(*params), GFP_KERNEL);
> 
> kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
> then most likely you could just put "__aligned(4)" or something on the
> conf.phy struct member and not allocate memory at all?

I'll leave this one out for now until Arik has the time to look into it.

Thanks for your comments, Johannes!

--
Luca.

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Arik Nemtsov]

On Sun, Jul 8, 2012 at 12:35 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> So the subject is wrong since "&priv->conf.phy" can hardly be "static
> global", but

Yea you're right. The static global pointer was used before, now it's
a different location we copy stuff into.

>
>> We get DMA alignment trouble if the beginning of the struct is
>> unaligned. Allocate memory and send it to FW.
>
> If this is all about alignment, and
>
>> +     params = kzalloc(sizeof(*params), GFP_KERNEL);
>
> kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
> then most likely you could just put "__aligned(4)" or something on the
> conf.phy struct member and not allocate memory at all?

Yea we need 4 bytes alignment. The kzalloc is a sort of convention in
the driver (see acx.c), but probably here we can use kmemdup.
We can't use fancy __aligned(4) notations most likely, since a
usermode tool also parses these header files, and it's not too smart
:)

Anyway I'll fix the subject and use kmemdup. Thanks for the review.

Arik

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Johannes Berg]

On Sun, 2012-07-08 at 17:08 +0300, Arik Nemtsov wrote:

> > kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
> > then most likely you could just put "__aligned(4)" or something on the
> > conf.phy struct member and not allocate memory at all?
> 
> Yea we need 4 bytes alignment. The kzalloc is a sort of convention in
> the driver (see acx.c), but probably here we can use kmemdup.
> We can't use fancy __aligned(4) notations most likely, since a
> usermode tool also parses these header files, and it's not too smart
> :)

This seems like a rather bad excuse for making the runtime code more
complex and less performant ... I'll let you sort it out with John
though, I wouldn't let you get away with this though if I maintained the
tree ;-)

FWIW, I think your actual problem is that you mark wl18xx_priv_conf as
packed unnecessarily. Otherwise it might actually get alignment, at
least on ARM I hear.

johannes

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Arik Nemtsov]

On Mon, Jul 9, 2012 at 11:03 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sun, 2012-07-08 at 17:08 +0300, Arik Nemtsov wrote:
>
>> > kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
>> > then most likely you could just put "__aligned(4)" or something on the
>> > conf.phy struct member and not allocate memory at all?
>>
>> Yea we need 4 bytes alignment. The kzalloc is a sort of convention in
>> the driver (see acx.c), but probably here we can use kmemdup.
>> We can't use fancy __aligned(4) notations most likely, since a
>> usermode tool also parses these header files, and it's not too smart
>> :)
>
> This seems like a rather bad excuse for making the runtime code more
> complex and less performant ... I'll let you sort it out with John
> though, I wouldn't let you get away with this though if I maintained the
> tree ;-)
>
> FWIW, I think your actual problem is that you mark wl18xx_priv_conf as
> packed unnecessarily. Otherwise it might actually get alignment, at
> least on ARM I hear.

It's not an excuse, we really have a usermode tool (called wlconf)
preparing binary conf files. And it usually runs on a different arch
(x86), so we mark everything packed to avoid errors.
I'm pretty sure __aligned(4) would screw up the parser there. It has
to build an internal representation of all the struct, and fill it out
using an ini file.

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Julian Calaby]

Hi Arik,

On Mon, Jul 9, 2012 at 6:14 PM, Arik Nemtsov <arik@wizery.com> wrote:
> On Mon, Jul 9, 2012 at 11:03 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Sun, 2012-07-08 at 17:08 +0300, Arik Nemtsov wrote:
>>
>>> > kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
>>> > then most likely you could just put "__aligned(4)" or something on the
>>> > conf.phy struct member and not allocate memory at all?
>>>
>>> Yea we need 4 bytes alignment. The kzalloc is a sort of convention in
>>> the driver (see acx.c), but probably here we can use kmemdup.
>>> We can't use fancy __aligned(4) notations most likely, since a
>>> usermode tool also parses these header files, and it's not too smart
>>> :)
>>
>> This seems like a rather bad excuse for making the runtime code more
>> complex and less performant ... I'll let you sort it out with John
>> though, I wouldn't let you get away with this though if I maintained the
>> tree ;-)
>>
>> FWIW, I think your actual problem is that you mark wl18xx_priv_conf as
>> packed unnecessarily. Otherwise it might actually get alignment, at
>> least on ARM I hear.
>
> It's not an excuse, we really have a usermode tool (called wlconf)
> preparing binary conf files. And it usually runs on a different arch
> (x86), so we mark everything packed to avoid errors.
> I'm pretty sure __aligned(4) would screw up the parser there. It has
> to build an internal representation of all the struct, and fill it out
> using an ini file.

Does this tool parse the compiled code or the actual source files?
Because if it parses the source files, surely you could adjust the
parser so it doesn't trip over a potential __aligned(4) notation.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Arik Nemtsov]

Hey Julian,

On Tue, Jul 10, 2012 at 7:32 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Arik,
>
> On Mon, Jul 9, 2012 at 6:14 PM, Arik Nemtsov <arik@wizery.com> wrote:
>> On Mon, Jul 9, 2012 at 11:03 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>>> On Sun, 2012-07-08 at 17:08 +0300, Arik Nemtsov wrote:
>>>
>>>> > kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
>>>> > then most likely you could just put "__aligned(4)" or something on the
>>>> > conf.phy struct member and not allocate memory at all?
>>>>
>>>> Yea we need 4 bytes alignment. The kzalloc is a sort of convention in
>>>> the driver (see acx.c), but probably here we can use kmemdup.
>>>> We can't use fancy __aligned(4) notations most likely, since a
>>>> usermode tool also parses these header files, and it's not too smart
>>>> :)
>>>
>>> This seems like a rather bad excuse for making the runtime code more
>>> complex and less performant ... I'll let you sort it out with John
>>> though, I wouldn't let you get away with this though if I maintained the
>>> tree ;-)
>>>
>>> FWIW, I think your actual problem is that you mark wl18xx_priv_conf as
>>> packed unnecessarily. Otherwise it might actually get alignment, at
>>> least on ARM I hear.
>>
>> It's not an excuse, we really have a usermode tool (called wlconf)
>> preparing binary conf files. And it usually runs on a different arch
>> (x86), so we mark everything packed to avoid errors.
>> I'm pretty sure __aligned(4) would screw up the parser there. It has
>> to build an internal representation of all the struct, and fill it out
>> using an ini file.
>
> Does this tool parse the compiled code or the actual source files?
> Because if it parses the source files, surely you could adjust the
> parser so it doesn't trip over a potential __aligned(4) notation.

The tool goes over source files, but it's not just a matter of
skipping the token - it has to actually align the next element to 4,
so it needs to start keeping track of alignment etc.

I'm afraid we have bigger issues with this tool right now. Currently
it can't even parse arrays properly :)

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Julian Calaby]

Hi Arik,

On Tue, Jul 10, 2012 at 3:42 PM, Arik Nemtsov <arik@wizery.com> wrote:
> Hey Julian,
>
> On Tue, Jul 10, 2012 at 7:32 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Arik,
>>
>> On Mon, Jul 9, 2012 at 6:14 PM, Arik Nemtsov <arik@wizery.com> wrote:
>>> On Mon, Jul 9, 2012 at 11:03 AM, Johannes Berg
>>> <johannes@sipsolutions.net> wrote:
>>>> On Sun, 2012-07-08 at 17:08 +0300, Arik Nemtsov wrote:
>>>>
>>>>> > kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
>>>>> > then most likely you could just put "__aligned(4)" or something on the
>>>>> > conf.phy struct member and not allocate memory at all?
>>>>>
>>>>> Yea we need 4 bytes alignment. The kzalloc is a sort of convention in
>>>>> the driver (see acx.c), but probably here we can use kmemdup.
>>>>> We can't use fancy __aligned(4) notations most likely, since a
>>>>> usermode tool also parses these header files, and it's not too smart
>>>>> :)
>>>>
>>>> This seems like a rather bad excuse for making the runtime code more
>>>> complex and less performant ... I'll let you sort it out with John
>>>> though, I wouldn't let you get away with this though if I maintained the
>>>> tree ;-)
>>>>
>>>> FWIW, I think your actual problem is that you mark wl18xx_priv_conf as
>>>> packed unnecessarily. Otherwise it might actually get alignment, at
>>>> least on ARM I hear.
>>>
>>> It's not an excuse, we really have a usermode tool (called wlconf)
>>> preparing binary conf files. And it usually runs on a different arch
>>> (x86), so we mark everything packed to avoid errors.
>>> I'm pretty sure __aligned(4) would screw up the parser there. It has
>>> to build an internal representation of all the struct, and fill it out
>>> using an ini file.
>>
>> Does this tool parse the compiled code or the actual source files?
>> Because if it parses the source files, surely you could adjust the
>> parser so it doesn't trip over a potential __aligned(4) notation.
>
> The tool goes over source files, but it's not just a matter of
> skipping the token - it has to actually align the next element to 4,
> so it needs to start keeping track of alignment etc.

Ah, does it produce binary chunks of data that are then "parsed" with
this structure?

> I'm afraid we have bigger issues with this tool right now. Currently
> it can't even parse arrays properly :)

That's a big problem =)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH 7/8] wl18xx: don't send static global struct to FW

[Arik Nemtsov]

On Tue, Jul 10, 2012 at 8:46 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Arik,
>
> On Tue, Jul 10, 2012 at 3:42 PM, Arik Nemtsov <arik@wizery.com> wrote:
>> Hey Julian,
>>
>> On Tue, Jul 10, 2012 at 7:32 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
>>> Hi Arik,
>>>
>>> On Mon, Jul 9, 2012 at 6:14 PM, Arik Nemtsov <arik@wizery.com> wrote:
>>>> On Mon, Jul 9, 2012 at 11:03 AM, Johannes Berg
>>>> <johannes@sipsolutions.net> wrote:
>>>>> On Sun, 2012-07-08 at 17:08 +0300, Arik Nemtsov wrote:
>>>>>
>>>>>> > kzalloc (which is pointless -- use kmemdup) alignment is sufficient,
>>>>>> > then most likely you could just put "__aligned(4)" or something on the
>>>>>> > conf.phy struct member and not allocate memory at all?
>>>>>>
>>>>>> Yea we need 4 bytes alignment. The kzalloc is a sort of convention in
>>>>>> the driver (see acx.c), but probably here we can use kmemdup.
>>>>>> We can't use fancy __aligned(4) notations most likely, since a
>>>>>> usermode tool also parses these header files, and it's not too smart
>>>>>> :)
>>>>>
>>>>> This seems like a rather bad excuse for making the runtime code more
>>>>> complex and less performant ... I'll let you sort it out with John
>>>>> though, I wouldn't let you get away with this though if I maintained the
>>>>> tree ;-)
>>>>>
>>>>> FWIW, I think your actual problem is that you mark wl18xx_priv_conf as
>>>>> packed unnecessarily. Otherwise it might actually get alignment, at
>>>>> least on ARM I hear.
>>>>
>>>> It's not an excuse, we really have a usermode tool (called wlconf)
>>>> preparing binary conf files. And it usually runs on a different arch
>>>> (x86), so we mark everything packed to avoid errors.
>>>> I'm pretty sure __aligned(4) would screw up the parser there. It has
>>>> to build an internal representation of all the struct, and fill it out
>>>> using an ini file.
>>>
>>> Does this tool parse the compiled code or the actual source files?
>>> Because if it parses the source files, surely you could adjust the
>>> parser so it doesn't trip over a potential __aligned(4) notation.
>>
>> The tool goes over source files, but it's not just a matter of
>> skipping the token - it has to actually align the next element to 4,
>> so it needs to start keeping track of alignment etc.
>
> Ah, does it produce binary chunks of data that are then "parsed" with
> this structure?

Yea. It produces a bin file that the driver takes using
request_firmware(). The driver uses this file as its internal
configuration struct.

>
>> I'm afraid we have bigger issues with this tool right now. Currently
>> it can't even parse arrays properly :)
>
> That's a big problem =)

Tell me about it :)

[PATCH 8/8] wlcore: determine AP extra rates correctly

[Luciano Coelho]

From: Arik Nemtsov <arik@wizery.com>

Don't use the ht_mode module parameter for determining AP supported
rates. We can rely on channel type, since HT40 won't be enabled if our
HT cap doesn't support it.

Enable MIMO only if there enough antennas, and rely on per-peer rate
limitation to prevent IOPs.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 drivers/net/wireless/ti/wl18xx/main.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index aea2e32..1b2fb1d 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -1036,14 +1036,24 @@ static u32 wl18xx_sta_get_ap_rate_mask(struct wl1271 *wl,
 static u32 wl18xx_ap_get_mimo_wide_rate_mask(struct wl1271 *wl,
 					     struct wl12xx_vif *wlvif)
 {
-	if ((wlvif->channel_type == NL80211_CHAN_HT40MINUS ||
-	     wlvif->channel_type == NL80211_CHAN_HT40PLUS) &&
-	    !strcmp(ht_mode_param, "wide")) {
+	struct wl18xx_priv *priv = wl->priv;
+
+	if (wlvif->channel_type == NL80211_CHAN_HT40MINUS ||
+	    wlvif->channel_type == NL80211_CHAN_HT40PLUS) {
 		wl1271_debug(DEBUG_ACX, "using wide channel rate mask");
+
+		/* sanity check - we don't support this */
+		if (WARN_ON(wlvif->band != IEEE80211_BAND_5GHZ))
+			return 0;
+
 		return CONF_TX_RATE_USE_WIDE_CHAN;
-	} else if (!strcmp(ht_mode_param, "mimo")) {
+	} else if (priv->conf.phy.number_of_assembled_ant2_4 >= 2 &&
+		   wlvif->band == IEEE80211_BAND_2GHZ) {
 		wl1271_debug(DEBUG_ACX, "using MIMO rate mask");
-
+		/*
+		 * we don't care about HT channel here - if a peer doesn't
+		 * support MIMO, we won't enable it in its rates
+		 */
 		return CONF_TX_MIMO_RATES;
 	} else {
 		return 0;
-- 
1.7.10