[PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[Stanislaw Gruszka]

Otherwise we are not able to run more than one device per driver:

[   24.743045] kmem_cache_create: duplicate cache iwl_dev_cmd
[   24.743051] Pid: 3165, comm: NetworkManager Not tainted 3.3.0-rc2-wl+ #5
[   24.743054] Call Trace:
[   24.743066]  [<ffffffff811717d5>] kmem_cache_create+0x655/0x700
[   24.743073]  [<ffffffff81061415>] ? destroy_timer_on_stack+0x15/0x20
[   24.743079]  [<ffffffff814e26ed>] ? schedule_timeout+0x1dd/0x330
[   24.743083]  [<ffffffff810617c0>] ? lock_timer_base+0x70/0x70
[   24.743101]  [<ffffffffa03b9f8b>] iwl_alive_notify+0x1cb/0x1f0 [iwlwifi]
[   24.743111]  [<ffffffffa03ba442>] iwl_load_ucode_wait_alive+0x1b2/0x220 [iwlwifi]
[   24.743118]  [<ffffffff810b1fbd>] ? trace_hardirqs_on+0xd/0x10
[   24.743127]  [<ffffffffa03b9700>] ? iwlagn_mac_setup_register+0x2e0/0x2e0 [iwlwifi]
[   24.743133]  [<ffffffff814e60b4>] ? _raw_spin_unlock_bh+0x34/0x40
[   24.743142]  [<ffffffffa03ba893>] iwl_run_init_ucode+0x73/0x100 [iwlwifi]
[   24.743152]  [<ffffffffa03b8fa1>] __iwl_up+0x81/0x220 [iwlwifi]
[   24.743161]  [<ffffffffa03b91c0>] iwlagn_mac_start+0x80/0x190 [iwlwifi]
[   24.743188]  [<ffffffffa03307b3>] ieee80211_do_open+0x293/0x770 [mac80211]

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
I tested patch on wireless testing tree, and then rebased to -iwlwifi
tree, where did compile check only. This is basically a bugfix, but
seems nobody except me (*) use more than one iwlwifi device per system,
so I do not cc stable. 

(*) I wonder why Intel developers do not run many devices in their
systems. I think they should - one new device and a few older. Then
perhaps we will stop seeing so many breakage on older devices caused
by adding support for new hardware and features.

 drivers/net/wireless/iwlwifi/iwl-agn-tx.c |    8 ++++----
 drivers/net/wireless/iwlwifi/iwl-agn.c    |   21 +++++++++++++++------
 drivers/net/wireless/iwlwifi/iwl-core.c   |    3 +--
 drivers/net/wireless/iwlwifi/iwl-dev.h    |    2 +-
 drivers/net/wireless/iwlwifi/iwl-ucode.c  |    9 ---------
 5 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-tx.c b/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
index a7cc0c0..5c00aa7 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
@@ -334,7 +334,7 @@ int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb)
 	if (info->flags & IEEE80211_TX_CTL_AMPDU)
 		is_agg = true;
 
-	dev_cmd = kmem_cache_alloc(priv->tx_cmd_pool, GFP_ATOMIC);
+	dev_cmd = kmem_cache_alloc(iwl_tx_cmd_pool, GFP_ATOMIC);
 
 	if (unlikely(!dev_cmd))
 		goto drop_unlock_priv;
@@ -425,7 +425,7 @@ int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb)
 
 drop_unlock_sta:
 	if (dev_cmd)
-		kmem_cache_free(priv->tx_cmd_pool, dev_cmd);
+		kmem_cache_free(iwl_tx_cmd_pool, dev_cmd);
 	spin_unlock(&priv->sta_lock);
 drop_unlock_priv:
 	return -1;
@@ -1045,7 +1045,7 @@ int iwlagn_rx_reply_tx(struct iwl_priv *priv, struct iwl_rx_mem_buffer *rxb,
 
 			info = IEEE80211_SKB_CB(skb);
 			ctx = info->driver_data[0];
-			kmem_cache_free(priv->tx_cmd_pool,
+			kmem_cache_free(iwl_tx_cmd_pool,
 					(info->driver_data[1]));
 
 			memset(&info->status, 0, sizeof(info->status));
@@ -1192,7 +1192,7 @@ int iwlagn_rx_reply_compressed_ba(struct iwl_priv *priv,
 			WARN_ON_ONCE(1);
 
 		info = IEEE80211_SKB_CB(skb);
-		kmem_cache_free(priv->tx_cmd_pool, (info->driver_data[1]));
+		kmem_cache_free(iwl_tx_cmd_pool, (info->driver_data[1]));
 
 		if (freed == 1) {
 			/* this is the first skb we deliver in this batch */
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index dcfc12c..025feae 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -1099,8 +1099,6 @@ static void iwl_uninit_drv(struct iwl_priv *priv)
 {
 	iwl_free_geos(priv);
 	iwl_free_channel_map(priv);
-	if (priv->tx_cmd_pool)
-		kmem_cache_destroy(priv->tx_cmd_pool);
 	kfree(priv->scan_cmd);
 	kfree(priv->beacon_cmd);
 	kfree(rcu_dereference_raw(priv->noa_data));
@@ -1419,6 +1417,9 @@ const struct iwl_op_mode_ops iwl_dvm_ops = {
  * driver and module entry point
  *
  *****************************************************************************/
+
+struct kmem_cache *iwl_tx_cmd_pool;
+
 static int __init iwl_init(void)
 {
 
@@ -1426,20 +1427,27 @@ static int __init iwl_init(void)
 	pr_info(DRV_DESCRIPTION ", " DRV_VERSION "\n");
 	pr_info(DRV_COPYRIGHT "\n");
 
+	iwl_tx_cmd_pool = kmem_cache_create("iwl_dev_cmd",
+					    sizeof(struct iwl_device_cmd),
+					    sizeof(void *), 0, NULL);
+	if (!iwl_tx_cmd_pool)
+		return -ENOMEM;
+
 	ret = iwlagn_rate_control_register();
 	if (ret) {
 		pr_err("Unable to register rate control algorithm: %d\n", ret);
-		return ret;
+		goto error_rc_register;
 	}
 
 	ret = iwl_pci_register_driver();
-
 	if (ret)
-		goto error_register;
+		goto error_pci_register;
 	return ret;
 
-error_register:
+error_pci_register:
 	iwlagn_rate_control_unregister();
+error_rc_register:
+	kmem_cache_destroy(iwl_tx_cmd_pool);
 	return ret;
 }
 
@@ -1447,6 +1455,7 @@ static void __exit iwl_exit(void)
 {
 	iwl_pci_unregister_driver();
 	iwlagn_rate_control_unregister();
+	kmem_cache_destroy(iwl_tx_cmd_pool);
 }
 
 module_exit(iwl_exit);
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index c3a9e6e..bc9b3b4 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -1482,11 +1482,10 @@ void iwl_nic_config(struct iwl_priv *priv)
 
 void iwl_free_skb(struct iwl_op_mode *op_mode, struct sk_buff *skb)
 {
-	struct iwl_priv *priv = IWL_OP_MODE_GET_DVM(op_mode);
 	struct ieee80211_tx_info *info;
 
 	info = IEEE80211_SKB_CB(skb);
-	kmem_cache_free(priv->tx_cmd_pool, (info->driver_data[1]));
+	kmem_cache_free(iwl_tx_cmd_pool, (info->driver_data[1]));
 	dev_kfree_skb_any(skb);
 }
 
diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h b/drivers/net/wireless/iwlwifi/iwl-dev.h
index 9ea9002..a9551cf 100644
--- a/drivers/net/wireless/iwlwifi/iwl-dev.h
+++ b/drivers/net/wireless/iwlwifi/iwl-dev.h
@@ -722,7 +722,6 @@ struct iwl_priv {
 	struct ieee80211_hw *hw;
 	struct ieee80211_channel *ieee_channels;
 	struct ieee80211_rate *ieee_rates;
-	struct kmem_cache *tx_cmd_pool;
 
 	struct workqueue_struct *workqueue;
 
@@ -968,6 +967,7 @@ struct iwl_priv {
 	bool have_rekey_data;
 }; /*iwl_priv */
 
+extern struct kmem_cache *iwl_tx_cmd_pool;
 extern struct iwl_mod_params iwlagn_mod_params;
 
 static inline struct iwl_rxon_context *
diff --git a/drivers/net/wireless/iwlwifi/iwl-ucode.c b/drivers/net/wireless/iwlwifi/iwl-ucode.c
index b16efc0..7251e81 100644
--- a/drivers/net/wireless/iwlwifi/iwl-ucode.c
+++ b/drivers/net/wireless/iwlwifi/iwl-ucode.c
@@ -366,15 +366,6 @@ static int iwl_alive_notify(struct iwl_trans *trans)
 	struct iwl_rxon_context *ctx;
 	int ret;
 
-	if (!priv->tx_cmd_pool)
-		priv->tx_cmd_pool =
-			kmem_cache_create("iwl_dev_cmd",
-					  sizeof(struct iwl_device_cmd),
-					  sizeof(void *), 0, NULL);
-
-	if (!priv->tx_cmd_pool)
-		return -ENOMEM;
-
 	iwl_trans_fw_alive(trans);
 	for_each_context(priv, ctx)
 		ctx->last_tx_rejected = false;
-- 
1.7.1

Re: [PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[Guy, Wey-Yi]

Hi Stanislaw,

On Mon, 2012-02-27 at 14:52 +0100, Stanislaw Gruszka wrote:
> Otherwise we are not able to run more than one device per driver:
> 
> [   24.743045] kmem_cache_create: duplicate cache iwl_dev_cmd
> [   24.743051] Pid: 3165, comm: NetworkManager Not tainted 3.3.0-rc2-wl+ #5
> [   24.743054] Call Trace:
> [   24.743066]  [<ffffffff811717d5>] kmem_cache_create+0x655/0x700
> [   24.743073]  [<ffffffff81061415>] ? destroy_timer_on_stack+0x15/0x20
> [   24.743079]  [<ffffffff814e26ed>] ? schedule_timeout+0x1dd/0x330
> [   24.743083]  [<ffffffff810617c0>] ? lock_timer_base+0x70/0x70
> [   24.743101]  [<ffffffffa03b9f8b>] iwl_alive_notify+0x1cb/0x1f0 [iwlwifi]
> [   24.743111]  [<ffffffffa03ba442>] iwl_load_ucode_wait_alive+0x1b2/0x220 [iwlwifi]
> [   24.743118]  [<ffffffff810b1fbd>] ? trace_hardirqs_on+0xd/0x10
> [   24.743127]  [<ffffffffa03b9700>] ? iwlagn_mac_setup_register+0x2e0/0x2e0 [iwlwifi]
> [   24.743133]  [<ffffffff814e60b4>] ? _raw_spin_unlock_bh+0x34/0x40
> [   24.743142]  [<ffffffffa03ba893>] iwl_run_init_ucode+0x73/0x100 [iwlwifi]
> [   24.743152]  [<ffffffffa03b8fa1>] __iwl_up+0x81/0x220 [iwlwifi]
> [   24.743161]  [<ffffffffa03b91c0>] iwlagn_mac_start+0x80/0x190 [iwlwifi]
> [   24.743188]  [<ffffffffa03307b3>] ieee80211_do_open+0x293/0x770 [mac80211]
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> I tested patch on wireless testing tree, and then rebased to -iwlwifi
> tree, where did compile check only. This is basically a bugfix, but
> seems nobody except me (*) use more than one iwlwifi device per system,
> so I do not cc stable. 
> 

I am not totally understand what your patch doing, you make a global
iwl_tx_cmd_pool structure, how that fix the multiple devices problem?
global variable is not really a good programming exercise, true?

> (*) I wonder why Intel developers do not run many devices in their
> systems. I think they should - one new device and a few older. Then
> perhaps we will stop seeing so many breakage on older devices caused
> by adding support for new hardware and features.
> 

Yes, I am totally agree and we will start doing that

btw, thanks for the help, I will merge all the patches from you on our
internal tree first.

Thanks
Wey

Re: [PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[Guy, Wey-Yi]

On Mon, 2012-02-27 at 19:55 +0200, Emmanuel Grumbach wrote:
> I guess that Sanislaw patch is defining one single pool for all the
> devices, which means that we won't try to allocate a few pools with
> the same name, which leads to this warning.
> 
That I got it, but is really the "only" way to do it, I am really not a
fan of global variable.

Wey

Re: [PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[Emmanuel Grumbach]

I guess that Sanislaw patch is defining one single pool for all the
devices, which means that we won't try to allocate a few pools with
the same name, which leads to this warning.

Emmanuel Grumbach
egrumbach@gmail.com



On Mon, Feb 27, 2012 at 16:30, Guy, Wey-Yi <wey-yi.w.guy@intel.com> wrote:
> Hi Stanislaw,
>
> On Mon, 2012-02-27 at 14:52 +0100, Stanislaw Gruszka wrote:
>> Otherwise we are not able to run more than one device per driver:
>>
>> [   24.743045] kmem_cache_create: duplicate cache iwl_dev_cmd
>> [   24.743051] Pid: 3165, comm: NetworkManager Not tainted 3.3.0-rc2-wl+ #5
>> [   24.743054] Call Trace:
>> [   24.743066]  [<ffffffff811717d5>] kmem_cache_create+0x655/0x700
>> [   24.743073]  [<ffffffff81061415>] ? destroy_timer_on_stack+0x15/0x20
>> [   24.743079]  [<ffffffff814e26ed>] ? schedule_timeout+0x1dd/0x330
>> [   24.743083]  [<ffffffff810617c0>] ? lock_timer_base+0x70/0x70
>> [   24.743101]  [<ffffffffa03b9f8b>] iwl_alive_notify+0x1cb/0x1f0 [iwlwifi]
>> [   24.743111]  [<ffffffffa03ba442>] iwl_load_ucode_wait_alive+0x1b2/0x220 [iwlwifi]
>> [   24.743118]  [<ffffffff810b1fbd>] ? trace_hardirqs_on+0xd/0x10
>> [   24.743127]  [<ffffffffa03b9700>] ? iwlagn_mac_setup_register+0x2e0/0x2e0 [iwlwifi]
>> [   24.743133]  [<ffffffff814e60b4>] ? _raw_spin_unlock_bh+0x34/0x40
>> [   24.743142]  [<ffffffffa03ba893>] iwl_run_init_ucode+0x73/0x100 [iwlwifi]
>> [   24.743152]  [<ffffffffa03b8fa1>] __iwl_up+0x81/0x220 [iwlwifi]
>> [   24.743161]  [<ffffffffa03b91c0>] iwlagn_mac_start+0x80/0x190 [iwlwifi]
>> [   24.743188]  [<ffffffffa03307b3>] ieee80211_do_open+0x293/0x770 [mac80211]
>>
>> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
>> ---
>> I tested patch on wireless testing tree, and then rebased to -iwlwifi
>> tree, where did compile check only. This is basically a bugfix, but
>> seems nobody except me (*) use more than one iwlwifi device per system,
>> so I do not cc stable.
>>
>
> I am not totally understand what your patch doing, you make a global
> iwl_tx_cmd_pool structure, how that fix the multiple devices problem?
> global variable is not really a good programming exercise, true?
>
>> (*) I wonder why Intel developers do not run many devices in their
>> systems. I think they should - one new device and a few older. Then
>> perhaps we will stop seeing so many breakage on older devices caused
>> by adding support for new hardware and features.
>>
>
> Yes, I am totally agree and we will start doing that
>
> btw, thanks for the help, I will merge all the patches from you on our
> internal tree first.
>
> Thanks
> Wey
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[Emmanuel Grumbach]

On Mon, Feb 27, 2012 at 19:48, Guy, Wey-Yi <wey-yi.w.guy@intel.com> wrote:
> On Mon, 2012-02-27 at 19:55 +0200, Emmanuel Grumbach wrote:
>> I guess that Sanislaw patch is defining one single pool for all the
>> devices, which means that we won't try to allocate a few pools with
>> the same name, which leads to this warning.
>>
> That I got it, but is really the "only" way to do it, I am really not a
> fan of global variable.
>
I guess we can have a pool for "iwl_dev_cmd_<PCI_DEV_ID>" or something
like this. But I don't see what is the problem with the global
variable besides the namespace pollution which is not very problematic
since we don't export this variable. It is a singleton in terms of
"Software design pattern" (I hate these buzzwords), and this is the
way to implement this in C AFAICT.

Re: [PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[Guy, Wey-Yi]

On Mon, 2012-02-27 at 22:20 +0200, Emmanuel Grumbach wrote:
> On Mon, Feb 27, 2012 at 19:48, Guy, Wey-Yi <wey-yi.w.guy@intel.com> wrote:
> > On Mon, 2012-02-27 at 19:55 +0200, Emmanuel Grumbach wrote:
> >> I guess that Sanislaw patch is defining one single pool for all the
> >> devices, which means that we won't try to allocate a few pools with
> >> the same name, which leads to this warning.
> >>
> > That I got it, but is really the "only" way to do it, I am really not a
> > fan of global variable.
> >
> I guess we can have a pool for "iwl_dev_cmd_<PCI_DEV_ID>" or something
> like this. But I don't see what is the problem with the global
> variable besides the namespace pollution which is not very problematic
> since we don't export this variable. It is a singleton in terms of
> "Software design pattern" (I hate these buzzwords), and this is the
> way to implement this in C AFAICT.

it is not a problem, just "global" is not something what I prefer, but
it is ok if it is the most easy way to address the problem and not
having side-effect.

Thanks
Wey

Re: [PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[Johannes Berg]

On Mon, 2012-02-27 at 14:52 +0100, Stanislaw Gruszka wrote:
> Otherwise we are not able to run more than one device per driver:
> 
> [   24.743045] kmem_cache_create: duplicate cache iwl_dev_cmd
> [   24.743051] Pid: 3165, comm: NetworkManager Not tainted 3.3.0-rc2-wl+ #5
> [   24.743054] Call Trace:
> [   24.743066]  [<ffffffff811717d5>] kmem_cache_create+0x655/0x700
> [   24.743073]  [<ffffffff81061415>] ? destroy_timer_on_stack+0x15/0x20
> [   24.743079]  [<ffffffff814e26ed>] ? schedule_timeout+0x1dd/0x330
> [   24.743083]  [<ffffffff810617c0>] ? lock_timer_base+0x70/0x70
> [   24.743101]  [<ffffffffa03b9f8b>] iwl_alive_notify+0x1cb/0x1f0 [iwlwifi]
> [   24.743111]  [<ffffffffa03ba442>] iwl_load_ucode_wait_alive+0x1b2/0x220 [iwlwifi]
> [   24.743118]  [<ffffffff810b1fbd>] ? trace_hardirqs_on+0xd/0x10
> [   24.743127]  [<ffffffffa03b9700>] ? iwlagn_mac_setup_register+0x2e0/0x2e0 [iwlwifi]
> [   24.743133]  [<ffffffff814e60b4>] ? _raw_spin_unlock_bh+0x34/0x40
> [   24.743142]  [<ffffffffa03ba893>] iwl_run_init_ucode+0x73/0x100 [iwlwifi]
> [   24.743152]  [<ffffffffa03b8fa1>] __iwl_up+0x81/0x220 [iwlwifi]
> [   24.743161]  [<ffffffffa03b91c0>] iwlagn_mac_start+0x80/0x190 [iwlwifi]
> [   24.743188]  [<ffffffffa03307b3>] ieee80211_do_open+0x293/0x770 [mac80211]

I'll pick this up.

johannes

Re: [PATCH -iwlwifi] iwlwifi: make tx_cmd_pool kmem cache global

[Guy, Wey-Yi]

On Wed, 2012-02-29 at 10:00 +0100, Johannes Berg wrote:
> On Mon, 2012-02-27 at 14:52 +0100, Stanislaw Gruszka wrote:
> > Otherwise we are not able to run more than one device per driver:
> > 
> > [   24.743045] kmem_cache_create: duplicate cache iwl_dev_cmd
> > [   24.743051] Pid: 3165, comm: NetworkManager Not tainted 3.3.0-rc2-wl+ #5
> > [   24.743054] Call Trace:
> > [   24.743066]  [<ffffffff811717d5>] kmem_cache_create+0x655/0x700
> > [   24.743073]  [<ffffffff81061415>] ? destroy_timer_on_stack+0x15/0x20
> > [   24.743079]  [<ffffffff814e26ed>] ? schedule_timeout+0x1dd/0x330
> > [   24.743083]  [<ffffffff810617c0>] ? lock_timer_base+0x70/0x70
> > [   24.743101]  [<ffffffffa03b9f8b>] iwl_alive_notify+0x1cb/0x1f0 [iwlwifi]
> > [   24.743111]  [<ffffffffa03ba442>] iwl_load_ucode_wait_alive+0x1b2/0x220 [iwlwifi]
> > [   24.743118]  [<ffffffff810b1fbd>] ? trace_hardirqs_on+0xd/0x10
> > [   24.743127]  [<ffffffffa03b9700>] ? iwlagn_mac_setup_register+0x2e0/0x2e0 [iwlwifi]
> > [   24.743133]  [<ffffffff814e60b4>] ? _raw_spin_unlock_bh+0x34/0x40
> > [   24.743142]  [<ffffffffa03ba893>] iwl_run_init_ucode+0x73/0x100 [iwlwifi]
> > [   24.743152]  [<ffffffffa03b8fa1>] __iwl_up+0x81/0x220 [iwlwifi]
> > [   24.743161]  [<ffffffffa03b91c0>] iwlagn_mac_start+0x80/0x190 [iwlwifi]
> > [   24.743188]  [<ffffffffa03307b3>] ieee80211_do_open+0x293/0x770 [mac80211]
> 
> I'll pick this up.
> 
Thanks, I will merge it once it is there.

Wey
>