Linux wireless drivers development
 help / color / mirror / Atom feed
* [PATCH 3/3] ath10k:  Improve logging message.
From: greearb @ 2016-08-19  1:26 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, Ben Greear
In-Reply-To: <1471569995-10028-1-git-send-email-greearb@candelatech.com>

From: Ben Greear <greearb@candelatech.com>

Helps to know the sta pointer.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index d96c06e..82bc0da 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -6491,8 +6491,8 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
 		 * Existing station deletion.
 		 */
 		ath10k_dbg(ar, ATH10K_DBG_MAC,
-			   "mac vdev %d peer delete %pM (sta gone)\n",
-			   arvif->vdev_id, sta->addr);
+			   "mac vdev %d peer delete %pM (sta gone) sta: %p\n",
+			   arvif->vdev_id, sta->addr, sta);
 
 		ret = ath10k_peer_delete(ar, arvif->vdev_id, sta->addr);
 		if (ret)
-- 
2.4.11

^ permalink raw reply related

* [PATCH 1/3] ath10k:  Ensure there are no stale ar->txqs entries.
From: greearb @ 2016-08-19  1:26 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, Ben Greear

From: Ben Greear <greearb@candelatech.com>

I was seeing kernel crashes due to accessing freed memory
while debugging a 9984 firmware that was crashing often.

This patch fixes the crashes.  I am not certain if there
is a better way or not.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 5659ef1..916119c 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4172,8 +4172,10 @@ static void ath10k_mac_txq_init(struct ieee80211_txq *txq)
 static void ath10k_mac_txq_unref(struct ath10k *ar, struct ieee80211_txq *txq)
 {
 	struct ath10k_txq *artxq = (void *)txq->drv_priv;
+	struct ath10k_txq *tmp, *walker;
 	struct ath10k_skb_cb *cb;
 	struct sk_buff *msdu;
+	struct ieee80211_txq *txq_tmp;
 	int msdu_id;
 
 	if (!txq)
@@ -4182,6 +4184,14 @@ static void ath10k_mac_txq_unref(struct ath10k *ar, struct ieee80211_txq *txq)
 	spin_lock_bh(&ar->txqs_lock);
 	if (!list_empty(&artxq->list))
 		list_del_init(&artxq->list);
+
+	/* Remove from ar->txqs in case it still exists there. */
+	list_for_each_entry_safe(walker, tmp, &ar->txqs, list) {
+		txq_tmp = container_of((void *)walker, struct ieee80211_txq,
+				       drv_priv);
+		if (txq_tmp == txq)
+			list_del(&walker->list);
+	}
 	spin_unlock_bh(&ar->txqs_lock);
 
 	spin_lock_bh(&ar->htt.tx_lock);
-- 
2.4.11

^ permalink raw reply related

* [PATCH 2/3] ath10k:  Grab rcu_read_lock before the txqs spinlock.
From: greearb @ 2016-08-19  1:26 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, Ben Greear
In-Reply-To: <1471569995-10028-1-git-send-email-greearb@candelatech.com>

From: Ben Greear <greearb@candelatech.com>

I was seeing some spin-lock hangs in this area of the code,
and it seems more proper to do the rcu-read-lock outside of
the spin lock.  I am not sure how much this matters, however.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 916119c..d96c06e 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4307,8 +4307,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
 	int max;
 	int loop_max = 2000;
 
-	spin_lock_bh(&ar->txqs_lock);
 	rcu_read_lock();
+	spin_lock_bh(&ar->txqs_lock);
 
 	last = list_last_entry(&ar->txqs, struct ath10k_txq, list);
 	while (!list_empty(&ar->txqs)) {
@@ -4342,8 +4342,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
 			break;
 	}
 
-	rcu_read_unlock();
 	spin_unlock_bh(&ar->txqs_lock);
+	rcu_read_unlock();
 }
 
 /************/
-- 
2.4.11

^ permalink raw reply related

* Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()
From: Javier Martinez Canillas @ 2016-08-18 19:29 UTC (permalink / raw)
  To: Arend van Spriel, linux-kernel
  Cc: Amitkumar Karwar, Kalle Valo, netdev, linux-wireless,
	Nishant Sarmukadam
In-Reply-To: <10fb1110-4c0b-00a2-522c-7011ca397b8c@broadcom.com>

Hello Arend,

Thanks a lot for your feedback.

On 08/18/2016 03:14 PM, Arend van Spriel wrote:
> On 18-08-16 16:17, Javier Martinez Canillas wrote:
>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>> is printed but the actual error is not propagated to the caller function.
> 
> Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care.
>

Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of()
return value.

If the IRQ request failing is not an error, then at the very least the call
to disable_irq() should be avoided if request_irq() fails, and the message
should be changed from dev_err() to dev_dgb() or dev_info().
 
> The device may still function without this wake interrupt.
>

That's correct, the binding says that the "interrupts" property in the child
node is optional since is just a wakeup IRQ. Now the question is if should
be an error if the IRQ is defined but fails to be requested.

> Regards,
> Arend
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

^ permalink raw reply

* Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()
From: Arend van Spriel @ 2016-08-18 19:49 UTC (permalink / raw)
  To: Javier Martinez Canillas, linux-kernel
  Cc: Amitkumar Karwar, Kalle Valo, netdev, linux-wireless,
	Nishant Sarmukadam
In-Reply-To: <70b6a0d1-8cc4-280e-3e53-8c42cb43b721@osg.samsung.com>



On 18-08-16 21:29, Javier Martinez Canillas wrote:
> Hello Arend,
> 
> Thanks a lot for your feedback.
> 
> On 08/18/2016 03:14 PM, Arend van Spriel wrote:
>> On 18-08-16 16:17, Javier Martinez Canillas wrote:
>>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>>> is printed but the actual error is not propagated to the caller function.
>>
>> Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care.
>>
> 
> Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of()
> return value.

Ok. I looked at 4.7 sources on lxr [1].

> If the IRQ request failing is not an error, then at the very least the call
> to disable_irq() should be avoided if request_irq() fails, and the message
> should be changed from dev_err() to dev_dgb() or dev_info().

agree.

>> The device may still function without this wake interrupt.
>>
> 
> That's correct, the binding says that the "interrupts" property in the child
> node is optional since is just a wakeup IRQ. Now the question is if should
> be an error if the IRQ is defined but fails to be requested.

Clearly it indicates an error in the DT specification so behavior is not
as expected. Personally I would indeed consider it an error, but I was
just indicating that it might have done like this intentionally.

Regards,
Arend

[1]
http://lxr.free-electrons.com/source/drivers/net/wireless/marvell/mwifiex/sdio.c#L192

>> Regards,
>> Arend
>>
> 
> Best regards,
> 

^ permalink raw reply

* Re: [PATCH 2/3] wlcore: Pass win_size taken from ieee80211_sta to FW
From: Johannes Berg @ 2016-08-18 21:23 UTC (permalink / raw)
  To: Maxim Altshul, linux-wireless; +Cc: kvalo
In-Reply-To: <20160818073627.30972-3-maxim.altshul@ti.com>

On Thu, 2016-08-18 at 10:36 +0300, Maxim Altshul wrote:
> When starting a new BA session, we must pass the win_size to the FW.
> 
> To do this we take max_rx_aggregation_subframes (BA RX win size)
> which is stored in ieee80211_sta structure (e.g per link and not per
> HW)

I believe this is wrong, and you should use the struct
ieee80211_ampdu_params::buf_size value that's passed to ampdu_action
when the action is IEEE80211_AMPDU_RX_START.

johannes

^ permalink raw reply

* Re: [PATCH 1/3] mac80211: RX BA support for sta max_rx_aggregation_subframes
From: Johannes Berg @ 2016-08-18 21:21 UTC (permalink / raw)
  To: Maxim Altshul, linux-wireless; +Cc: kvalo
In-Reply-To: <20160818073627.30972-2-maxim.altshul@ti.com>

On Thu, 2016-08-18 at 10:36 +0300, Maxim Altshul wrote:
> 
> @@ -1735,6 +1735,9 @@ struct ieee80211_sta_rates {
>   * @supp_rates: Bitmap of supported rates (per band)
>   * @ht_cap: HT capabilities of this STA; restricted to our own
> capabilities
>   * @vht_cap: VHT capabilities of this STA; restricted to our own
> capabilities
> + * @max_rx_aggregation_subframes: restriction on rx buff size for
> this active
> + *	aggregation. Initially set to local-
> >hw.max_rx_aggregation_subframes but
> + *	can be modified by driver.

The documentation for this makes no sense, it's clearly not a per
"active aggregation" parameter in any way.

>  /**
> + * ieee80211_change_rx_ba_max_subframes - callback to change
> + * sta.max_rx_aggregation_subframes and stop existing BA sessions
> + *
> + * This capability is useful in cases of IOP, i.e. cases where peer
> sta
> + * or ap doesn't respect the max size (Kbps) of an AMPDU.
> + * In these cases the driver/chip may recover by decreasing the
> + * max_rx_aggregation_subframes, which will in turn reduce the size
> of
> + * the whole aggregation.
> + *
> + * @vif: &struct ieee80211_vif pointer from the add_interface
> callback.
> + * @addr: & to bssid mac address
> + * @max_subframes: new max_rx_aggregation_subframes for this sta
> + */
> +void ieee80211_change_rx_ba_max_subframes(struct ieee80211_vif *vif,
> +					  const u8 *addr,
> +					  u8 max_subframes);

I see no reason for this to exist, it's practically equivalent to
something like this:

sta = ieee80211_find_sta(vif, addr);
if (sta)
	sta->max_subframes = max_subframes;
ieee80211_stop_rx_ba_session(vif, 0xff, addr);

so there's no real need for this. What you did has an advantage if the
driver is doing something stupid like calling the function with the
same argument multiple times, but that's easily fixed or prevented; I
don't really see any other advantages?

johannes

^ permalink raw reply

* Re: [PATCH 0/2] wireless: Use complete() instead complete_all()
From: Luis R. Rodriguez @ 2016-08-18 19:16 UTC (permalink / raw)
  To: Daniel Wagner
  Cc: Kalle Valo, Christian Lamparter, Luis R . Rodriguez, ath10k,
	linux-wireless, netdev, linux-kernel, Daniel Wagner,
	Greg Kroah-Hartman
In-Reply-To: <1471525926-20384-1-git-send-email-wagi@monom.org>

On Thu, Aug 18, 2016 at 03:12:04PM +0200, Daniel Wagner wrote:
> This series ignores all complete_all() usages in the firmware loading
> path. They will be hopefully address by Luis' sysdata patches [0].
> That leaves a couple of complete_all() calls.

I had not considered this as a gain, but glad to know the sysdata series
could help with RT as well, thanks for the clarification.

 Luis

^ permalink raw reply

* [PATCH] net: wireless: rtl8xxxu: Make rtl8xxxu_ampdu_action less chatty
From: Hans de Goede @ 2016-08-18 17:52 UTC (permalink / raw)
  To: Jes Sorensen, Kalle Valo; +Cc: linux-wireless, netdev, Hans de Goede

On my home network rtl8xxxu is spamming the log with
IEEE80211_AMPDU_RX_START / IEEE80211_AMPDU_RX_STOP every few seconds
turn these messages into debug messages.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 77048db..28b5ac95 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -5704,7 +5704,7 @@ rtl8xxxu_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 
 	switch (action) {
 	case IEEE80211_AMPDU_TX_START:
-		dev_info(dev, "%s: IEEE80211_AMPDU_TX_START\n", __func__);
+		dev_dbg(dev, "%s: IEEE80211_AMPDU_TX_START\n", __func__);
 		ampdu_factor = sta->ht_cap.ampdu_factor;
 		ampdu_density = sta->ht_cap.ampdu_density;
 		rtl8xxxu_set_ampdu_factor(priv, ampdu_factor);
@@ -5714,21 +5714,21 @@ rtl8xxxu_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			ampdu_factor, ampdu_density);
 		break;
 	case IEEE80211_AMPDU_TX_STOP_FLUSH:
-		dev_info(dev, "%s: IEEE80211_AMPDU_TX_STOP_FLUSH\n", __func__);
+		dev_dbg(dev, "%s: IEEE80211_AMPDU_TX_STOP_FLUSH\n", __func__);
 		rtl8xxxu_set_ampdu_factor(priv, 0);
 		rtl8xxxu_set_ampdu_min_space(priv, 0);
 		break;
 	case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
-		dev_info(dev, "%s: IEEE80211_AMPDU_TX_STOP_FLUSH_CONT\n",
+		dev_dbg(dev, "%s: IEEE80211_AMPDU_TX_STOP_FLUSH_CONT\n",
 			 __func__);
 		rtl8xxxu_set_ampdu_factor(priv, 0);
 		rtl8xxxu_set_ampdu_min_space(priv, 0);
 		break;
 	case IEEE80211_AMPDU_RX_START:
-		dev_info(dev, "%s: IEEE80211_AMPDU_RX_START\n", __func__);
+		dev_dbg(dev, "%s: IEEE80211_AMPDU_RX_START\n", __func__);
 		break;
 	case IEEE80211_AMPDU_RX_STOP:
-		dev_info(dev, "%s: IEEE80211_AMPDU_RX_STOP\n", __func__);
+		dev_dbg(dev, "%s: IEEE80211_AMPDU_RX_STOP\n", __func__);
 		break;
 	default:
 		break;
-- 
2.7.4

^ permalink raw reply related

* [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()
From: Javier Martinez Canillas @ 2016-08-18 14:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: Javier Martinez Canillas, Amitkumar Karwar, Kalle Valo, netdev,
	linux-wireless, Nishant Sarmukadam

If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
is printed but the actual error is not propagated to the caller function.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index d3e1561ca075..00727936ad6e 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -125,6 +125,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
 				dev_err(dev,
 					"Failed to request irq_wifi %d (%d)\n",
 					cfg->irq_wifi, ret);
+				return ret;
 			}
 			disable_irq(cfg->irq_wifi);
 		}
-- 
2.5.5

^ permalink raw reply related

* [PATCH 2/2] ath10k: use complete() instead complete_all()
From: Daniel Wagner @ 2016-08-18 13:12 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Christian Lamparter, Luis R . Rodriguez, ath10k, linux-wireless,
	netdev, linux-kernel, Daniel Wagner
In-Reply-To: <1471525926-20384-1-git-send-email-wagi@monom.org>

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

There is only one waiter for the completion, therefore there
is no need to use complete_all(). Let's make that clear by
using complete() instead of complete_all().

The usage pattern of the completion is:

waiter context                          waker context

scan.started
------------

ath10k_start_scan()
  lockdep_assert_held(conf_mutex)
  auth10k_wmi_start_scan()
  wait_for_completion_timeout(scan.started)

					ath10k_wmi_event_scan_start_failed()
					  complete(scan.started)

					ath10k_wmi_event_scan_started()
					  complete(scan.started)

scan.completed
--------------

ath10k_scan_stop()
  lockdep_assert_held(conf_mutex)
  ath10k_wmi_stop_scan()
  wait_for_completion_timeout(scan.completed)

					__ath10k_scan_finish()
					  complete(scan.completed)

scan.on_channel
---------------

ath10k_remain_on_channel()
  mutex_lock(conf_mutex)
  ath10k_start_scan()
  wait_for_completion_timeout(scan.on_channel)

					ath10k_wmi_event_scan_foreign_chan()
					  complete(scan.on_channel)

offchan_tx_completed
--------------------

ath10k_offchan_tx_work()
  mutex_lock(conf_mutex)
  reinit_completion(offchan_tx_completed)
  wait_for_completion_timeout(offchan_tx_completed)

					ath10k_report_offchain_tx()
					  complete(offchan_tx_completed)

install_key_done
----------------
ath10k_install_key()
  lockep_assert_held(conf_mutex)
  reinit_completion(install_key_done)
  wait_for_completion_timeout(install_key_done)

				        ath10k_htt_t2h_msg_handler()
					  complete(install_key_done)

vdev_setup_done
---------------

ath10k_monitor_vdev_start()
  lockdep_assert_held(conf_mutex)
   reinit_completion(vdev_setup_done)
  ath10k_vdev_setup_sync()
    wait_for_completion_timeout(vdev_setup_done)

					ath10k_wmi_event_vdev_start_resp()
					  complete(vdev_setup_done)

ath10k_monitor_vdev_stop()
  lockdep_assert_held(conf_mutex)
  reinit_completion(vdev_setup_done()
  ath10k_vdev_setup_sync()
    wait_for_completion_timeout(vdev_setup_done)

					ath10k_wmi_event_vdev_stopped()
					 complete(vdev_setup_done)

thermal.wmi_sync
----------------
ath10k_thermal_show_temp()
  mutex_lock(conf_mutex)
  reinit_completion(thermal.wmi_sync)
  wait_for_completion_timeout(thermal.wmi_sync)

					ath10k_thermal_event_temperature()
					  complete(thermal.wmi_sync)

bss_survey_done
---------------
ath10k_mac_update_bss_chan_survey
  lockdep_assert_held(conf_mutex)
  reinit_completion(bss_survey_done)
  wait_for_completion_timeout(bss_survey_done)

					ath10k_wmi_event_pdev_bss_chan_info()
					  complete(bss_survey_done)

All complete() calls happen while the conf_mutex is taken. That means
at max one waiter is possible.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/net/wireless/ath/ath10k/core.c | 16 ++++++++--------
 drivers/net/wireless/ath/ath10k/mac.c  |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index e889829..ed76601 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1497,14 +1497,14 @@ static void ath10k_core_restart(struct work_struct *work)
 
 	ieee80211_stop_queues(ar->hw);
 	ath10k_drain_tx(ar);
-	complete_all(&ar->scan.started);
-	complete_all(&ar->scan.completed);
-	complete_all(&ar->scan.on_channel);
-	complete_all(&ar->offchan_tx_completed);
-	complete_all(&ar->install_key_done);
-	complete_all(&ar->vdev_setup_done);
-	complete_all(&ar->thermal.wmi_sync);
-	complete_all(&ar->bss_survey_done);
+	complete(&ar->scan.started);
+	complete(&ar->scan.completed);
+	complete(&ar->scan.on_channel);
+	complete(&ar->offchan_tx_completed);
+	complete(&ar->install_key_done);
+	complete(&ar->vdev_setup_done);
+	complete(&ar->thermal.wmi_sync);
+	complete(&ar->bss_survey_done);
 	wake_up(&ar->htt.empty_tx_wq);
 	wake_up(&ar->wmi.tx_credits_wq);
 	wake_up(&ar->peer_mapping_wq);
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 0bbd0a0..c3c1c25 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3894,7 +3894,7 @@ void __ath10k_scan_finish(struct ath10k *ar)
 		ar->scan.roc_freq = 0;
 		ath10k_offchan_tx_purge(ar);
 		cancel_delayed_work(&ar->scan.timeout);
-		complete_all(&ar->scan.completed);
+		complete(&ar->scan.completed);
 		break;
 	}
 }
-- 
2.7.4

^ permalink raw reply related

* [PATCH 1/2] carl9170: Fix wrong completion usage
From: Daniel Wagner @ 2016-08-18 13:12 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Christian Lamparter, Luis R . Rodriguez, ath10k, linux-wireless,
	netdev, linux-kernel, Daniel Wagner
In-Reply-To: <1471525926-20384-1-git-send-email-wagi@monom.org>

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

carl9170_usb_stop() is used from several places to flush and cleanup any
pending work. The normal pattern is to send a request and wait for the
irq handler to call complete(). The completion is not reinitialized
during normal operation and as the old comment indicates it is important
to keep calls to wait_for_completion_timeout() and complete() balanced.

Calling complete_all() brings this equilibirum out of balance and needs
to be fixed by a reinit_completion(). But that opens a small race
window. It is possible that the sequence of complete_all(),
reinit_completion() is faster than the wait_for_completion_timeout() can
do its work. The wake up is not lost but the done counter test is after
reinit_completion() has been executed. The only reason we don't see
carl9170_exec_cmd() hang forever is we use the timeout version of
wait_for_copletion().

Let's fix this by reinitializing the completion (that is just setting
done counter to 0) just before we send out an request. Now,
carl9170_usb_stop() can be sure a complete() call is enough to make
progess since there is only one waiter at max. This is a common pattern
also seen in various drivers which use completion.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/net/wireless/ath/carl9170/usb.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
index 76842e6..99ab203 100644
--- a/drivers/net/wireless/ath/carl9170/usb.c
+++ b/drivers/net/wireless/ath/carl9170/usb.c
@@ -670,6 +670,7 @@ int carl9170_exec_cmd(struct ar9170 *ar, const enum carl9170_cmd_oids cmd,
 	ar->readlen = outlen;
 	spin_unlock_bh(&ar->cmd_lock);
 
+	reinit_completion(&ar->cmd_wait);
 	err = __carl9170_exec_cmd(ar, &ar->cmd, false);
 
 	if (!(cmd & CARL9170_CMD_ASYNC_FLAG)) {
@@ -778,10 +779,7 @@ void carl9170_usb_stop(struct ar9170 *ar)
 	spin_lock_bh(&ar->cmd_lock);
 	ar->readlen = 0;
 	spin_unlock_bh(&ar->cmd_lock);
-	complete_all(&ar->cmd_wait);
-
-	/* This is required to prevent an early completion on _start */
-	reinit_completion(&ar->cmd_wait);
+	complete(&ar->cmd_wait);
 
 	/*
 	 * Note:
-- 
2.7.4

^ permalink raw reply related

* [PATCH 0/2] wireless: Use complete() instead complete_all()
From: Daniel Wagner @ 2016-08-18 13:12 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Christian Lamparter, Luis R . Rodriguez, ath10k, linux-wireless,
	netdev, linux-kernel, Daniel Wagner

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

Hi,

Using complete_all() is not wrong per se but it suggest that there
might be more than one reader. For -rt I am reviewing all
complete_all() users and would like to leave only the real ones in the
tree. The main problem for -rt about complete_all() is that it can be
uses inside IRQ context and that can lead to unbounded amount work
inside the interrupt handler. That is a no no for -rt.

The patches grouped per subsystem and in small batches to allow
reviewing.

This series ignores all complete_all() usages in the firmware loading
path. They will be hopefully address by Luis' sysdata patches [0].
That leaves a couple of complete_all() calls.

The first patch fixes a real glitch for the carl9170 driver. I was
able to test it because I have the hardware. For the second one I
haven't found any dongle with that chip in my drawers. 

This series against net-next of today.

cheers,
daniel

[0] https://lkml.kernel.org/r/1466117661-22075-1-git-send-email-mcgrof@kernel.org

Daniel Wagner (2):
  carl9170: Fix wrong completion usage
  ath10k: use complete() instead complete_all()

 drivers/net/wireless/ath/ath10k/core.c  | 16 ++++++++--------
 drivers/net/wireless/ath/ath10k/mac.c   |  2 +-
 drivers/net/wireless/ath/carl9170/usb.c |  6 ++----
 3 files changed, 11 insertions(+), 13 deletions(-)

-- 
2.7.4

^ permalink raw reply

* Re: mwifiex: fix large amsdu packets causing firmware hang
From: Kalle Valo @ 2016-08-18 13:02 UTC (permalink / raw)
  To: Amitkumar Karwar
  Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, Amitkumar Karwar
In-Reply-To: <1469098824-8031-1-git-send-email-akarwar@marvell.com>

Amitkumar Karwar <akarwar@marvell.com> wrote:
> From: Cathy Luo <cluo@marvell.com>
> 
> Sometimes host prepares and downloads a large amsdu packet to firmware
> which leads to a memory corruption in firmware.
> The reason is __dev_alloc_skb() may allocate larger buffer than required
> size. This patch solves the problem by checking "adapter->tx_buf_size"
> instead of relying on skb_tailroom().
> 
> Signed-off-by: Cathy Luo <cluo@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>

Thanks, 1 patch applied to wireless-drivers.git:

c81396f3da22 mwifiex: fix large amsdu packets causing firmware hang

-- 
Sent by pwcli
https://patchwork.kernel.org/patch/9241469/

^ permalink raw reply

* wireless-drivers-next is OPEN
From: Kalle Valo @ 2016-08-18 13:25 UTC (permalink / raw)
  To: linux-wireless
In-Reply-To: <20160808.222446.744905683717709496.davem@davemloft.net>

David Miller <davem@davemloft.net> writes:

> I've merged 'net' into 'net-next' and applied a bunch of pending stuff.
>
> Fire at will...

Delayed because of my vacation but wireless-drivers-next is finally open
for new patches. I'll start going through the backlog of patches now.

I have also written some documentation about patchwork, hopefully makes
it easier to understand what's happening:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches?&#checking_state_of_patches_from_patchwork

Feedback very welcome.

-- 
Kalle Valo

^ permalink raw reply

* [PATCH] ath10k: fix sending frame in management path in push txq logic
From: Ashok Raj Nagarajan @ 2016-08-18 10:00 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless, arnagara, Ashok Raj Nagarajan

In the wake tx queue path, we are not checking if the frame to be sent
takes management path or not. For eg. QOS null func frame coming here will
take the management path. Since we are not incrementing the descriptor
counter (num_pending_mgmt_tx) w.r.t tx management, on tx completion it is
possible to see negative values.

When the above counter reaches a negative value, we will not be sending a
probe response out.

    if (is_presp &&
	ar->hw_params.max_probe_resp_desc_thres < htt->num_pending_mgmt_tx)

For IPQ4019, max_probe_resp_desc_thres (u32) is 24 is compared against
num_pending_mgmt_tx (int) and the above condtions comes true if the counter
is negative and we drop the probe response.

To avoid this, check on the wake tx queue path as well for the tx path of
the frame and increment the appropriate counters

Fixes: cac085524cf1 "ath10k: move mgmt descriptor limit handle under mgmt_tx"
Signed-off-by: Ashok Raj Nagarajan <arnagara@qti.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index fb8e38d..0d6afe2 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3777,7 +3777,9 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,
 	enum ath10k_hw_txrx_mode txmode;
 	enum ath10k_mac_tx_path txpath;
 	struct sk_buff *skb;
+	struct ieee80211_hdr *hdr;
 	size_t skb_len;
+	bool is_mgmt, is_presp;
 	int ret;
 
 	spin_lock_bh(&ar->htt.tx_lock);
@@ -3801,6 +3803,22 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,
 	skb_len = skb->len;
 	txmode = ath10k_mac_tx_h_get_txmode(ar, vif, sta, skb);
 	txpath = ath10k_mac_tx_h_get_txpath(ar, skb, txmode);
+	is_mgmt = (txpath == ATH10K_MAC_TX_HTT_MGMT);
+
+	if (is_mgmt) {
+		hdr = (struct ieee80211_hdr *)skb->data;
+		is_presp = ieee80211_is_probe_resp(hdr->frame_control);
+
+		spin_lock_bh(&ar->htt.tx_lock);
+		ret = ath10k_htt_tx_mgmt_inc_pending(htt, is_mgmt, is_presp);
+
+		if (ret) {
+			ath10k_htt_tx_dec_pending(htt);
+			spin_unlock_bh(&ar->htt.tx_lock);
+			return ret;
+		}
+		spin_unlock_bh(&ar->htt.tx_lock);
+	}
 
 	ret = ath10k_mac_tx(ar, vif, sta, txmode, txpath, skb);
 	if (unlikely(ret)) {
@@ -3808,6 +3826,8 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,
 
 		spin_lock_bh(&ar->htt.tx_lock);
 		ath10k_htt_tx_dec_pending(htt);
+		if (is_mgmt)
+			ath10k_htt_tx_mgmt_dec_pending(htt);
 		spin_unlock_bh(&ar->htt.tx_lock);
 
 		return ret;
-- 
1.9.1

^ permalink raw reply related

* [PATCH 3/3] wlcore: Add RX_BA_WIN_SIZE_CHANGE_EVENT event
From: Maxim Altshul @ 2016-08-18  7:36 UTC (permalink / raw)
  To: linux-wireless; +Cc: kvalo, johannes, Maxim Altshul
In-Reply-To: <20160818073627.30972-3-maxim.altshul@ti.com>

This event is used by the Firmware to limit the RX BA win size
for a specific link.

The event handler updates the new size in the mac's sta->sta struct.

BA sessions opened for that link will use the new restricted
win_size. This limitation remains until a new update is received or
until the link is closed.

Signed-off-by: Maxim Altshul <maxim.altshul@ti.com>
---
 drivers/net/wireless/ti/wl18xx/event.c | 22 ++++++++++++++++++++++
 drivers/net/wireless/ti/wl18xx/event.h |  1 +
 drivers/net/wireless/ti/wl18xx/main.c  |  3 ++-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wl18xx/event.c b/drivers/net/wireless/ti/wl18xx/event.c
index 2c5df43..a69be96 100644
--- a/drivers/net/wireless/ti/wl18xx/event.c
+++ b/drivers/net/wireless/ti/wl18xx/event.c
@@ -217,5 +217,27 @@ int wl18xx_process_mailbox_events(struct wl1271 *wl)
 	if (vector & FW_LOGGER_INDICATION)
 		wlcore_event_fw_logger(wl);
 
+	if (vector & RX_BA_WIN_SIZE_CHANGE_EVENT_ID) {
+		struct wl12xx_vif *wlvif;
+		struct ieee80211_vif *vif;
+		u8 link_id = mbox->rx_ba_link_id;
+		u8 win_size = mbox->rx_ba_win_size;
+		const u8 *addr;
+
+		wlvif = wl->links[link_id].wlvif;
+		vif = wl12xx_wlvif_to_vif(wlvif);
+
+		/* Call MAC routine to update win_size and stop all link active
+		 * BA sessions. This routine returns 0 on failure or previous
+		 * win_size on success
+		 */
+		if (wlvif->bss_type != BSS_TYPE_AP_BSS)
+			addr = vif->bss_conf.bssid;
+		else
+			addr = wl->links[link_id].addr;
+
+		ieee80211_change_rx_ba_max_subframes(vif, addr, win_size);
+	}
+
 	return 0;
 }
diff --git a/drivers/net/wireless/ti/wl18xx/event.h b/drivers/net/wireless/ti/wl18xx/event.h
index ce8ea9c0..4af297f 100644
--- a/drivers/net/wireless/ti/wl18xx/event.h
+++ b/drivers/net/wireless/ti/wl18xx/event.h
@@ -38,6 +38,7 @@ enum {
 	REMAIN_ON_CHANNEL_COMPLETE_EVENT_ID      = BIT(18),
 	DFS_CHANNELS_CONFIG_COMPLETE_EVENT       = BIT(19),
 	PERIODIC_SCAN_REPORT_EVENT_ID            = BIT(20),
+	RX_BA_WIN_SIZE_CHANGE_EVENT_ID           = BIT(21),
 	SMART_CONFIG_SYNC_EVENT_ID               = BIT(22),
 	SMART_CONFIG_DECODE_EVENT_ID             = BIT(23),
 	TIME_SYNC_EVENT_ID                       = BIT(24),
diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index 00a04df..dafb2fd 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -1041,7 +1041,8 @@ static int wl18xx_boot(struct wl1271 *wl)
 		SMART_CONFIG_SYNC_EVENT_ID |
 		SMART_CONFIG_DECODE_EVENT_ID |
 		TIME_SYNC_EVENT_ID |
-		FW_LOGGER_INDICATION;
+		FW_LOGGER_INDICATION |
+		RX_BA_WIN_SIZE_CHANGE_EVENT_ID;
 
 	wl->ap_event_mask = MAX_TX_FAILURE_EVENT_ID;
 
-- 
2.9.0

^ permalink raw reply related

* [PATCH 1/3] mac80211: RX BA support for sta max_rx_aggregation_subframes
From: Maxim Altshul @ 2016-08-18  7:36 UTC (permalink / raw)
  To: linux-wireless; +Cc: kvalo, johannes, Maxim Altshul
In-Reply-To: <20160818073627.30972-1-maxim.altshul@ti.com>

The ability to change the max_rx_aggregation frames is useful
in cases of IOP.

There exist some devices (latest mobile phones and some AP's)
that tend to not respect a BA sessions maximum size (in Kbps).
These devices won't respect the AMPDU size that was negotiated during
associasion (even though they do respect the maximal number of packets).

This violation is characterized by a valid number of packets in
a single AMPDU. Even so, the total size will exceed the size negotiated
during association.

Eventually, this will cause some undefined behavior, which in turn
causes the hw to drop packets, causing the throughput to plummet.

This patch will:
a. Make the subframe limitation to be held by each station,
instead of being held only by hw.
b. Create an api for the driver to call which will remove violating
BA sessions with a specific peer. When the session is reopened, it
will use the new size, limiting the aggregation.

Signed-off-by: Maxim Altshul <maxim.altshul@ti.com>
---
 include/net/mac80211.h  | 22 ++++++++++++++++++++++
 net/mac80211/agg-rx.c   | 31 +++++++++++++++++++++++++++----
 net/mac80211/ht.c       | 18 ++++++++++++++++++
 net/mac80211/sta_info.c |  3 +++
 4 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cca510a..eec97d5 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1735,6 +1735,9 @@ struct ieee80211_sta_rates {
  * @supp_rates: Bitmap of supported rates (per band)
  * @ht_cap: HT capabilities of this STA; restricted to our own capabilities
  * @vht_cap: VHT capabilities of this STA; restricted to our own capabilities
+ * @max_rx_aggregation_subframes: restriction on rx buff size for this active
+ *	aggregation. Initially set to local->hw.max_rx_aggregation_subframes but
+ *	can be modified by driver.
  * @wme: indicates whether the STA supports QoS/WME (if local devices does,
  *	otherwise always false)
  * @drv_priv: data area for driver use, will always be aligned to
@@ -1775,6 +1778,7 @@ struct ieee80211_sta {
 	u16 aid;
 	struct ieee80211_sta_ht_cap ht_cap;
 	struct ieee80211_sta_vht_cap vht_cap;
+	u8 max_rx_aggregation_subframes;
 	bool wme;
 	u8 uapsd_queues;
 	u8 max_sp;
@@ -5281,6 +5285,24 @@ void ieee80211_mark_rx_ba_filtered_frames(struct ieee80211_sta *pubsta, u8 tid,
 					  u16 received_mpdus);
 
 /**
+ * ieee80211_change_rx_ba_max_subframes - callback to change
+ * sta.max_rx_aggregation_subframes and stop existing BA sessions
+ *
+ * This capability is useful in cases of IOP, i.e. cases where peer sta
+ * or ap doesn't respect the max size (Kbps) of an AMPDU.
+ * In these cases the driver/chip may recover by decreasing the
+ * max_rx_aggregation_subframes, which will in turn reduce the size of
+ * the whole aggregation.
+ *
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ * @addr: & to bssid mac address
+ * @max_subframes: new max_rx_aggregation_subframes for this sta
+ */
+void ieee80211_change_rx_ba_max_subframes(struct ieee80211_vif *vif,
+					  const u8 *addr,
+					  u8 max_subframes);
+
+/**
  * ieee80211_send_bar - send a BlockAckReq frame
  *
  * can be used to flush pending frames from the peer's aggregation reorder
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index a9aff60..8089234 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -147,6 +147,27 @@ void ieee80211_stop_rx_ba_session(struct ieee80211_vif *vif, u16 ba_rx_bitmap,
 }
 EXPORT_SYMBOL(ieee80211_stop_rx_ba_session);
 
+void ieee80211_change_rx_ba_max_subframes(struct ieee80211_vif *vif,
+					  const u8 *addr,
+					  u8 max_subframes)
+{
+	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+	struct sta_info *sta;
+
+	rcu_read_lock();
+	sta = sta_info_get_bss(sdata, addr);
+
+	if (!sta) {
+		rcu_read_unlock();
+		return;
+	}
+
+	sta->sta.max_rx_aggregation_subframes = max_subframes;
+	ieee80211_queue_work(&sta->local->hw, &sta->ampdu_mlme.work);
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL(ieee80211_change_rx_ba_max_subframes);
+
 /*
  * After accepting the AddBA Request we activated a timer,
  * resetting it after each frame that arrives from the originator.
@@ -297,13 +318,15 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 	if (buf_size == 0)
 		buf_size = IEEE80211_MAX_AMPDU_BUF;
 
+	/* examine state machine */
+	mutex_lock(&sta->ampdu_mlme.mtx);
+
 	/* make sure the size doesn't exceed the maximum supported by the hw */
-	if (buf_size > local->hw.max_rx_aggregation_subframes)
-		buf_size = local->hw.max_rx_aggregation_subframes;
+	if (buf_size > sta->sta.max_rx_aggregation_subframes)
+		buf_size = sta->sta.max_rx_aggregation_subframes;
 	params.buf_size = buf_size;
 
-	/* examine state machine */
-	mutex_lock(&sta->ampdu_mlme.mtx);
+	ht_dbg(sta->sdata, "AddBA Req buf_size=%d\n", buf_size);
 
 	if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
 		tid_agg_rx = rcu_dereference_protected(
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index f4a5287..eaff7a8 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -305,6 +305,7 @@ void ieee80211_ba_session_work(struct work_struct *work)
 	struct sta_info *sta =
 		container_of(work, struct sta_info, ampdu_mlme.work);
 	struct tid_ampdu_tx *tid_tx;
+	struct tid_ampdu_rx *tid_rx;
 	int tid;
 
 	/*
@@ -329,6 +330,23 @@ void ieee80211_ba_session_work(struct work_struct *work)
 				sta, tid, WLAN_BACK_RECIPIENT,
 				WLAN_REASON_UNSPECIFIED, true);
 
+		/* Stop RX BA sessions affected by change of
+		 * sta.max_rx_aggregation_subframe
+		 */
+		tid_rx = sta->ampdu_mlme.tid_rx[tid];
+		if (tid_rx &&
+		    tid_rx->buf_size > sta->sta.max_rx_aggregation_subframes) {
+			ht_dbg(sta->sdata,
+			       "buf_size(%d) > max_subframes(%d) stopping tid %d\n",
+			       tid_rx->buf_size,
+			       sta->sta.max_rx_aggregation_subframes,
+			       tid);
+
+			___ieee80211_stop_rx_ba_session(
+				sta, tid, WLAN_BACK_RECIPIENT,
+				WLAN_REASON_UNSPECIFIED, true);
+		}
+
 		spin_lock_bh(&sta->lock);
 
 		tid_tx = sta->ampdu_mlme.tid_start_tx[tid];
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 19f14c9..5e70fa5 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -340,6 +340,9 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 
 	memcpy(sta->addr, addr, ETH_ALEN);
 	memcpy(sta->sta.addr, addr, ETH_ALEN);
+	sta->sta.max_rx_aggregation_subframes =
+		local->hw.max_rx_aggregation_subframes;
+
 	sta->local = local;
 	sta->sdata = sdata;
 	sta->rx_stats.last_rx = jiffies;
-- 
2.9.0

^ permalink raw reply related

* [PATCH 2/3] wlcore: Pass win_size taken from ieee80211_sta to FW
From: Maxim Altshul @ 2016-08-18  7:36 UTC (permalink / raw)
  To: linux-wireless; +Cc: kvalo, johannes, Maxim Altshul
In-Reply-To: <20160818073627.30972-2-maxim.altshul@ti.com>

When starting a new BA session, we must pass the win_size to the FW.

To do this we take max_rx_aggregation_subframes (BA RX win size)
which is stored in ieee80211_sta structure (e.g per link and not per HW)

We will use the value stored per link when passing the win_size to
firmware through the ACX_BA_SESSION_RX_SETUP command.

Signed-off-by: Maxim Altshul <maxim.altshul@ti.com>
---
 drivers/net/wireless/ti/wlcore/acx.c  | 5 +++--
 drivers/net/wireless/ti/wlcore/acx.h  | 3 ++-
 drivers/net/wireless/ti/wlcore/main.c | 6 ++++--
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index 26cc23f..a485999 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -1419,7 +1419,8 @@ out:
 
 /* setup BA session receiver setting in the FW. */
 int wl12xx_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
-				       u16 ssn, bool enable, u8 peer_hlid)
+				       u16 ssn, bool enable, u8 peer_hlid,
+				       u8 win_size)
 {
 	struct wl1271_acx_ba_receiver_setup *acx;
 	int ret;
@@ -1435,7 +1436,7 @@ int wl12xx_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
 	acx->hlid = peer_hlid;
 	acx->tid = tid_index;
 	acx->enable = enable;
-	acx->win_size = wl->conf.ht.rx_ba_win_size;
+	acx->win_size =	win_size;
 	acx->ssn = ssn;
 
 	ret = wlcore_cmd_configure_failsafe(wl, ACX_BA_SESSION_RX_SETUP, acx,
diff --git a/drivers/net/wireless/ti/wlcore/acx.h b/drivers/net/wireless/ti/wlcore/acx.h
index 6321ed4..f46d7fd 100644
--- a/drivers/net/wireless/ti/wlcore/acx.h
+++ b/drivers/net/wireless/ti/wlcore/acx.h
@@ -1113,7 +1113,8 @@ int wl1271_acx_set_ht_information(struct wl1271 *wl,
 int wl12xx_acx_set_ba_initiator_policy(struct wl1271 *wl,
 				       struct wl12xx_vif *wlvif);
 int wl12xx_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
-				       u16 ssn, bool enable, u8 peer_hlid);
+				       u16 ssn, bool enable, u8 peer_hlid,
+				       u8 win_size);
 int wl12xx_acx_tsf_info(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 			u64 *mactime);
 int wl1271_acx_ps_rx_streaming(struct wl1271 *wl, struct wl12xx_vif *wlvif,
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index 9e1f2d9..9ea477e 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -5286,7 +5286,9 @@ static int wl1271_op_ampdu_action(struct ieee80211_hw *hw,
 		}
 
 		ret = wl12xx_acx_set_ba_receiver_session(wl, tid, *ssn, true,
-							 hlid);
+				hlid,
+				sta->max_rx_aggregation_subframes);
+
 		if (!ret) {
 			*ba_bitmap |= BIT(tid);
 			wl->ba_rx_session_count++;
@@ -5307,7 +5309,7 @@ static int wl1271_op_ampdu_action(struct ieee80211_hw *hw,
 		}
 
 		ret = wl12xx_acx_set_ba_receiver_session(wl, tid, 0, false,
-							 hlid);
+							 hlid, 0);
 		if (!ret) {
 			*ba_bitmap &= ~BIT(tid);
 			wl->ba_rx_session_count--;
-- 
2.9.0

^ permalink raw reply related

* [PATCH 0/3] IOP RX BA session issues
From: Maxim Altshul @ 2016-08-18  7:36 UTC (permalink / raw)
  To: linux-wireless; +Cc: kvalo, johannes, Maxim Altshul

Some background: 

There exist some devices (latest mobile phones and some AP's e.g LG G4 H815, 
Samsung Galaxy S5 G900H) that tend to not respect a BA 
sessions maximum size (in Kbps). 

These devices won't respect the AMPDU size that was negotiated 
during associasion (even though they do respect the maximal number of packets).

This violation is characterized by a valid number of 
packets in a single AMPDU who all have the same size.
Even so, the total size will exceed the size negotiated during association.

Eventually, this will cause some undefined behavior, 
which in turn causes the hw to drop packets, causing the throughput to plummet.

The fix is separated to 3 patches:
wlcore patches (patches 2+3):
Add a functionality in which the firmware is able 
to notify the driver that the aggregation window size 
must be changed, and in turn the driver will notify mac80211.

mac80211 patch (patch 1) which will:
a. Make the subframe limitation to be held by each station, 
instead of being held only by hw.
b. Create an api for the driver to call which will remove all violating
BA sessions with a specific peer.

Reopening the BA sessions with a different size will limit the aggregation 
window size, which will in turn limit the maximum size in bytes of the whole aggregation.

I hope that the commit messages are clear enough, and if not then I will change them upon request.

Maxim Altshul (3):
  mac80211: RX BA support for sta max_rx_aggregation_subframes
  wlcore: Pass win_size taken from ieee80211_sta to FW
  wlcore: Add RX_BA_WIN_SIZE_CHANGE_EVENT event

 drivers/net/wireless/ti/wl18xx/event.c | 22 ++++++++++++++++++++++
 drivers/net/wireless/ti/wl18xx/event.h |  1 +
 drivers/net/wireless/ti/wl18xx/main.c  |  3 ++-
 drivers/net/wireless/ti/wlcore/acx.c   |  5 +++--
 drivers/net/wireless/ti/wlcore/acx.h   |  3 ++-
 drivers/net/wireless/ti/wlcore/main.c  |  6 ++++--
 include/net/mac80211.h                 | 22 ++++++++++++++++++++++
 net/mac80211/agg-rx.c                  | 31 +++++++++++++++++++++++++++----
 net/mac80211/ht.c                      | 18 ++++++++++++++++++
 net/mac80211/sta_info.c                |  3 +++
 10 files changed, 104 insertions(+), 10 deletions(-)

-- 
2.9.0

^ permalink raw reply

* [PATCH net-next] net: bgmac: support Ethernet core on BCM53573 SoCs
From: Rafał Miłecki @ 2016-08-17 21:00 UTC (permalink / raw)
  To: David S. Miller, netdev
  Cc: Rafał Miłecki, Rafał Miłecki, Jon Mason,
	Arnd Bergmann, Florian Fainelli, Philippe Reynes, Felix Fietkau,
	open list, open list:BROADCOM SPECIFIC AMBA DRIVER (BCMA)

From: Rafał Miłecki <rafal@milecki.pl>

BCM53573 is a new series of Broadcom's SoCs. It's based on ARM and can
be found in two packages (versions): BCM53573 and BCM47189. It shares
some code with the Northstar family, but also requires some new quirks.

First of all there can be up to 2 Ethernet cores on this SoC. If that is
the case, they are connected to two different switch ports allowing some
more complex/optimized setups. It seems the second unit doesn't come
fully configured and requires some IRQ quirk.

Other than that only the first core is connected to the PHY. For the
second one we have to register fixed PHY (similarly to the Northstar),
otherwise generic PHY driver would get some invalid info.

This has been successfully tested on Tenda AC9 (BCM47189B0).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c | 19 ++++++++++++++++++-
 drivers/net/ethernet/broadcom/bgmac.c      | 25 +++++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bgmac.h      | 19 +++++++++++++++++++
 include/linux/bcma/bcma.h                  |  3 +++
 include/linux/bcma/bcma_regs.h             |  1 +
 5 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 9a9745c4..3bc0a04 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -92,6 +92,7 @@ MODULE_DEVICE_TABLE(bcma, bgmac_bcma_tbl);
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipattach */
 static int bgmac_probe(struct bcma_device *core)
 {
+	struct bcma_chipinfo *ci = &core->bus->chipinfo;
 	struct ssb_sprom *sprom = &core->bus->sprom;
 	struct mii_bus *mii_bus;
 	struct bgmac *bgmac;
@@ -157,7 +158,8 @@ static int bgmac_probe(struct bcma_device *core)
 	dev_info(bgmac->dev, "Found PHY addr: %d%s\n", bgmac->phyaddr,
 		 bgmac->phyaddr == BGMAC_PHY_NOREGS ? " (NOREGS)" : "");
 
-	if (!bgmac_is_bcm4707_family(core)) {
+	if (!bgmac_is_bcm4707_family(core) &&
+	    !(ci->id == BCMA_CHIP_ID_BCM53573 && core->core_unit == 1)) {
 		mii_bus = bcma_mdio_mii_register(core, bgmac->phyaddr);
 		if (!IS_ERR(mii_bus)) {
 			err = PTR_ERR(mii_bus);
@@ -230,6 +232,21 @@ static int bgmac_probe(struct bcma_device *core)
 		bgmac->feature_flags |= BGMAC_FEAT_NO_RESET;
 		bgmac->feature_flags |= BGMAC_FEAT_FORCE_SPEED_2500;
 		break;
+	case BCMA_CHIP_ID_BCM53573:
+		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
+		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+		if (ci->pkg == BCMA_PKG_ID_BCM47189)
+			bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+		if (core->core_unit == 0) {
+			bgmac->feature_flags |= BGMAC_FEAT_CC4_IF_SW_TYPE;
+			if (ci->pkg == BCMA_PKG_ID_BCM47189)
+				bgmac->feature_flags |=
+					BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII;
+		} else if (core->core_unit == 1) {
+			bgmac->feature_flags |= BGMAC_FEAT_IRQ_ID_OOB_6;
+			bgmac->feature_flags |= BGMAC_FEAT_CC7_IF_TYPE_RGMII;
+		}
+		break;
 	default:
 		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
 		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index c4751ec..63ef7235 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -940,6 +940,27 @@ static void bgmac_chip_reset(struct bgmac *bgmac)
 		bgmac_cco_ctl_maskset(bgmac, 1, ~(BGMAC_CHIPCTL_1_IF_TYPE_MASK |
 						  BGMAC_CHIPCTL_1_SW_TYPE_MASK),
 				      sw_type);
+	} else if (bgmac->feature_flags & BGMAC_FEAT_CC4_IF_SW_TYPE) {
+		u32 sw_type = BGMAC_CHIPCTL_4_IF_TYPE_MII |
+			      BGMAC_CHIPCTL_4_SW_TYPE_EPHY;
+		u8 et_swtype = 0;
+		char buf[4];
+
+		if (bcm47xx_nvram_getenv("et_swtype", buf, sizeof(buf)) > 0) {
+			if (kstrtou8(buf, 0, &et_swtype))
+				dev_err(bgmac->dev, "Failed to parse et_swtype (%s)\n",
+					buf);
+			sw_type = (et_swtype & 0x0f) << 12;
+		} else if (bgmac->feature_flags & BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII) {
+			sw_type = BGMAC_CHIPCTL_4_IF_TYPE_RGMII |
+				  BGMAC_CHIPCTL_4_SW_TYPE_RGMII;
+		}
+		bgmac_cco_ctl_maskset(bgmac, 4, ~(BGMAC_CHIPCTL_4_IF_TYPE_MASK |
+						  BGMAC_CHIPCTL_4_SW_TYPE_MASK),
+				      sw_type);
+	} else if (bgmac->feature_flags & BGMAC_FEAT_CC7_IF_TYPE_RGMII) {
+		bgmac_cco_ctl_maskset(bgmac, 7, ~BGMAC_CHIPCTL_7_IF_TYPE_MASK,
+				      BGMAC_CHIPCTL_7_IF_TYPE_RGMII);
 	}
 
 	if (iost & BGMAC_BCMA_IOST_ATTACHED && !bgmac->has_robosw)
@@ -1467,6 +1488,10 @@ int bgmac_enet_probe(struct bgmac *info)
 	 */
 	bgmac_clk_enable(bgmac, 0);
 
+	/* This seems to be fixing IRQ by assigning OOB #6 to the core */
+	if (bgmac->feature_flags & BGMAC_FEAT_IRQ_ID_OOB_6)
+		bgmac_idm_write(bgmac, BCMA_OOB_SEL_OUT_A30, 0x86);
+
 	bgmac_chip_reset(bgmac);
 
 	err = bgmac_dma_alloc(bgmac);
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 24a2502..80836b4 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -369,6 +369,21 @@
 #define BGMAC_CHIPCTL_1_SW_TYPE_RGMII		0x000000C0
 #define BGMAC_CHIPCTL_1_RXC_DLL_BYPASS		0x00010000
 
+#define BGMAC_CHIPCTL_4_IF_TYPE_MASK		0x00003000
+#define BGMAC_CHIPCTL_4_IF_TYPE_RMII		0x00000000
+#define BGMAC_CHIPCTL_4_IF_TYPE_MII		0x00001000
+#define BGMAC_CHIPCTL_4_IF_TYPE_RGMII		0x00002000
+#define BGMAC_CHIPCTL_4_SW_TYPE_MASK		0x0000C000
+#define BGMAC_CHIPCTL_4_SW_TYPE_EPHY		0x00000000
+#define BGMAC_CHIPCTL_4_SW_TYPE_EPHYMII		0x00004000
+#define BGMAC_CHIPCTL_4_SW_TYPE_EPHYRMII	0x00008000
+#define BGMAC_CHIPCTL_4_SW_TYPE_RGMII		0x0000C000
+
+#define BGMAC_CHIPCTL_7_IF_TYPE_MASK		0x000000C0
+#define BGMAC_CHIPCTL_7_IF_TYPE_RMII		0x00000000
+#define BGMAC_CHIPCTL_7_IF_TYPE_MII		0x00000040
+#define BGMAC_CHIPCTL_7_IF_TYPE_RGMII		0x00000080
+
 #define BGMAC_WEIGHT	64
 
 #define ETHER_MAX_LEN   1518
@@ -390,6 +405,10 @@
 #define BGMAC_FEAT_NO_CLR_MIB		BIT(13)
 #define BGMAC_FEAT_FORCE_SPEED_2500	BIT(14)
 #define BGMAC_FEAT_CMDCFG_SR_REV4	BIT(15)
+#define BGMAC_FEAT_IRQ_ID_OOB_6		BIT(16)
+#define BGMAC_FEAT_CC4_IF_SW_TYPE	BIT(17)
+#define BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII	BIT(18)
+#define BGMAC_FEAT_CC7_IF_TYPE_RGMII	BIT(19)
 
 struct bgmac_slot_info {
 	union {
diff --git a/include/linux/bcma/bcma.h b/include/linux/bcma/bcma.h
index 3db25df..8eeedb2 100644
--- a/include/linux/bcma/bcma.h
+++ b/include/linux/bcma/bcma.h
@@ -205,6 +205,9 @@ struct bcma_host_ops {
 #define  BCMA_PKG_ID_BCM4709	0
 #define BCMA_CHIP_ID_BCM47094	53030
 #define BCMA_CHIP_ID_BCM53018	53018
+#define BCMA_CHIP_ID_BCM53573	53573
+#define  BCMA_PKG_ID_BCM53573	0
+#define  BCMA_PKG_ID_BCM47189	1
 
 /* Board types (on PCI usually equals to the subsystem dev id) */
 /* BCM4313 */
diff --git a/include/linux/bcma/bcma_regs.h b/include/linux/bcma/bcma_regs.h
index ebd5c1f..c607fce 100644
--- a/include/linux/bcma/bcma_regs.h
+++ b/include/linux/bcma/bcma_regs.h
@@ -23,6 +23,7 @@
 #define  BCMA_CLKCTLST_4328A0_HAVEALP	0x00020000 /* 4328a0 has reversed bits */
 
 /* Agent registers (common for every core) */
+#define BCMA_OOB_SEL_OUT_A30		0x0100
 #define BCMA_IOCTL			0x0408 /* IO control */
 #define  BCMA_IOCTL_CLK			0x0001
 #define  BCMA_IOCTL_FGC			0x0002
-- 
1.8.4.5

^ permalink raw reply related

* Re: [Make-wifi-fast] [PATCH v2] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Johannes Berg @ 2016-08-17 20:43 UTC (permalink / raw)
  To: Dave Taht
  Cc: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless,
	Felix Fietkau
In-Reply-To: <CAA93jw4+WTiFAdUJkPaf5aRDy81vMW34TPQt7xSgv95tsXU=cA@mail.gmail.com>


> well, we're getting there. the results of both patch attempts were
> really nice, and brought encrypted performance with fq back into line
> with unencrypted. Still running crypted tests as I write...
> 
> So fixing TKIP would be next, forcing the AP to use that? What other
> scenarios do we have to worry about? WDS?
> 

I don't think there's anything else, I just don't really feel it's
getting anywhere. This is a mere symptom of the design.

Felix had worked around the SN assignment in a similar way, but I feel
that perhaps the whole thing isn't quite the right architecture. Why
are we applying FQ after the wifi conversion, when clearly that doesn't
work well? Seems to me that it would make more sense to let the frames
sit on the queues as they come in, and do most of the wifi handling
only when needed (obviously, things like control port would still have
to be done).
We even count those packets that are dropped for TX statistics, which
would seem to be a big behavioural difference vs. applying a qdisc.

Now, it's unlikely to be that simple - fragmentation, for example,
might mess this up.

Overall though, I'm definitely wondering if it should be this way,
since all the special cases just add complexity.

johannes

^ permalink raw reply

* Re: [PATCH v2] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Johannes Berg @ 2016-08-17 19:49 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
  Cc: Felix Fietkau
In-Reply-To: <20160817144531.4285-1-toke@toke.dk>

Hi,

You need to work on coding style, a lot of your indentation is
completely messed up.

> +	switch (sdata->vif.type) {
> +	case NL80211_IFTYPE_STATION:
> +		if (sdata->u.mgd.use_4addr) {
> +			pn_offs = 30;
> +			break;
> +		}
> +		pn_offs = 24;
> +		break;
> +	case NL80211_IFTYPE_AP_VLAN:
> +		if (sdata->wdev.use_4addr) {
> +			pn_offs = 30;
> +			break;
> +		}
> +		/* fall through */
> +	case NL80211_IFTYPE_ADHOC:
> +	case NL80211_IFTYPE_AP:
> +		pn_offs = 24;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	if (sta->sta.wme) {
> +		pn_offs += 2;
> +	}

I think you just reinvented ieee80211_hdrlen(). No?

> -	if (fast_tx->pn_offs) {
> -		u64 pn;
> -		u8 *crypto_hdr = skb->data + fast_tx->pn_offs;

No need to undo the pn_offs optimisation for the !txq case, you can
pass it in to the new function that will fill it.

However, you're still doing it wrong - now you haven't fixed anything
for TKIP, which won't hit the fastpath.

johannes

^ permalink raw reply

* Re: [Make-wifi-fast] [PATCH v2] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Dave Taht @ 2016-08-17 20:07 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless,
	Felix Fietkau
In-Reply-To: <1471463348.5173.16.camel@sipsolutions.net>

On Wed, Aug 17, 2016 at 9:49 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Hi,
>
> You need to work on coding style, a lot of your indentation is
> completely messed up.
>
>> +     switch (sdata->vif.type) {
>> +     case NL80211_IFTYPE_STATION:
>> +             if (sdata->u.mgd.use_4addr) {
>> +                     pn_offs =3D 30;
>> +                     break;
>> +             }
>> +             pn_offs =3D 24;
>> +             break;
>> +     case NL80211_IFTYPE_AP_VLAN:
>> +             if (sdata->wdev.use_4addr) {
>> +                     pn_offs =3D 30;
>> +                     break;
>> +             }
>> +             /* fall through */
>> +     case NL80211_IFTYPE_ADHOC:
>> +     case NL80211_IFTYPE_AP:
>> +             pn_offs =3D 24;
>> +             break;
>> +     default:
>> +             return;
>> +     }
>> +
>> +     if (sta->sta.wme) {
>> +             pn_offs +=3D 2;
>> +     }
>
> I think you just reinvented ieee80211_hdrlen(). No?
>
>> -     if (fast_tx->pn_offs) {
>> -             u64 pn;
>> -             u8 *crypto_hdr =3D skb->data + fast_tx->pn_offs;
>
> No need to undo the pn_offs optimisation for the !txq case, you can
> pass it in to the new function that will fill it.
>
> However, you're still doing it wrong - now you haven't fixed anything
> for TKIP, which won't hit the fastpath.

well, we're getting there. the results of both patch attempts were
really nice, and brought encrypted performance with fq back into line
with unencrypted. Still running crypted tests as I write...

So fixing TKIP would be next, forcing the AP to use that? What other
scenarios do we have to worry about? WDS?


> johannes
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast



--=20
Dave T=C3=A4ht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org

^ permalink raw reply

* Re: [PATCHv6 1/2] add basic register-field manipulation macros
From: Linus Torvalds @ 2016-08-17 17:16 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Kalle Valo, Andrew Morton, Greg Kroah-Hartman, David Miller,
	Linux Wireless List, Linux Kernel Mailing List, dinan.gunawardena
In-Reply-To: <20160817181109.080edf7a@jkicinski-Precision-T1700>

On Wed, Aug 17, 2016 at 10:11 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Wed, 17 Aug 2016 09:33:26 -0700, Linus Torvalds wrote:
>>
>> I'm not a huge fan, since the interface fundamentally seems to be
>> oddly designed (why pass in the mask rather than "start bit +
>> length"?).
>
> Would that not require start and length to have separate defines?

Yeah.

> I assume doing:
>
>     #define REG_BLA_FIELD_FOO  3, 4
>     val = FIELD_GET(REG_BLA_FIELD_FOO, reg);
>
> is not acceptable.  Attempts to define a single value brought us to the
> shifted mask.

Agreed. Maybe the mask with the complexity to then undo it (at compile
time) is the better approach.

            Linus

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox