When drivers use HW crypto, reservation for tail room is not needed for
any crypto suite. Do not reserve tail room in such cases, this helps in
optimizing the transmit path.
Signed-off-by: Yogesh Ashok Powar <[email protected]>
---
include/net/mac80211.h | 5 +++++
net/mac80211/tx.c | 9 +++++----
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 6c9c4e9..dcd7828 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1081,6 +1081,10 @@ enum ieee80211_tkip_key_type {
* stations based on the PM bit of incoming frames.
* Use ieee80211_start_ps()/ieee8021_end_ps() to manually configure
* the PS mode of connected stations.
+ *
+ * @IEEE80211_HW_SUPPORTS_HW_CRYPTO:
+ * Set by drivers when crypto is being done in the hardware. Drivers using SW
+ * crypto should not set this flag.
*/
enum ieee80211_hw_flags {
IEEE80211_HW_HAS_RATE_CONTROL = 1<<0,
@@ -1106,6 +1110,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_SUPPORTS_CQM_RSSI = 1<<20,
IEEE80211_HW_SUPPORTS_PER_STA_GTK = 1<<21,
IEEE80211_HW_AP_LINK_PS = 1<<22,
+ IEEE80211_HW_CRYPTO_ENABLED = 1<<23,
};
/**
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 17b10be..a80fc1a 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1483,11 +1483,12 @@ static int ieee80211_skb_resize(struct ieee80211_local *local,
int tail_need = 0;
/*
- * This could be optimised, devices that do full hardware
- * crypto (including TKIP MMIC) need no tailroom... But we
- * have no drivers for such devices currently.
+ * When full HW crypto is being used by the driver,
+ * no tail room is needed. Hence do not ask for tail room
+ * in such cases. This will avoid copying the skb in
+ * pskb_expand_head.
*/
- if (may_encrypt) {
+ if (!(local->hw.flags & IEEE80211_HW_CRYPTO_ENABLED) && may_encrypt) {
tail_need = IEEE80211_ENCRYPT_TAILROOM;
tail_need -= skb_tailroom(skb);
tail_need = max_t(int, tail_need, 0);
--
1.7.3.5
On Wed, 2011-04-27 at 16:33 +0530, Yogesh Ashok Powar wrote:
> On Thu, Apr 21, 2011 at 05:59:48AM -0700, Johannes Berg wrote:
> > Well, it kinda goes like this:
> >
> > key added -> need tailroom code
> > key put into HW -> no longer need tailroom code
> > key removed from HW -> need tailroom code again
> > key deleted -> no longer need tailroom code
> >
> Following patch takes care of these four states. Kindly review.
Looks fine, but still I think it might get out of sync when HW restart
happens?
johannes
On Fri, 2011-04-15 at 14:10 +0530, Yogesh Ashok Powar wrote:
> Define a flag say IEEE80211_CRYPTO_NO_TAILROOM_NEEDED per key. Drivers
> need to set this flag in set_key handler for the keys which requires no
> extra tailroom.
This isn't necessary. You already know from the existing flags, the only
relevant one is IEEE80211_KEY_FLAG_GENERATE_MMIC which must be unset.
> Then Skip the code which expands the skb iff
> IEEE80211_CRYPTO_NO_TAILROOM_NEEDED is set and the key is programmed
> into the hardware (checking KEY_FLAG_UPLOADED_TO_HARDWARE).
You don't know the key at this point, so you have to keep track of
whether this is true for all keys, which depends on whether or not
they're already programmed into the HW (since for SW crypto we need the
tailroom)
johannes
On Fri, 2011-04-15 at 16:21 +0530, Yogesh Ashok Powar wrote:
> > > Then Skip the code which expands the skb iff
> > > IEEE80211_CRYPTO_NO_TAILROOM_NEEDED is set and the key is programmed
> > > into the hardware (checking KEY_FLAG_UPLOADED_TO_HARDWARE).
> >
> > You don't know the key at this point, so you have to keep track of
> > whether this is true for all keys, which depends on whether or not
> > they're already programmed into the HW (since for SW crypto we need the
> > tailroom)
> From this it seems that we do not reserve tailroom iff
> IEEE80211_KEY_FLAG_GENERATE_MMIC flag is unset for all keys and all the keys
> are programmed into the hardware.
>
> Also, say in mixed mode if TKIP and CCMP keys are configured and this
> flag is set for TKIP MMIC, we will end up reserving tailroom even for
> CCMP. Is my understanding correct?
Yes, correct.
> Also, though I have not looked into this part of the code very closely,
> how about deriving key information at this place? Will that be feasible?
No, we can't do it here. I suppose we could postpone it for per key
stuff, but that's more complex and probably good for a separate patch.
johannes
On Fri, Apr 15, 2011 at 01:52:34AM -0700, Johannes Berg wrote:
> On Fri, 2011-04-15 at 14:10 +0530, Yogesh Ashok Powar wrote:
>
> > Define a flag say IEEE80211_CRYPTO_NO_TAILROOM_NEEDED per key. Drivers
> > need to set this flag in set_key handler for the keys which requires no
> > extra tailroom.
>
> This isn't necessary. You already know from the existing flags, the only
> relevant one is IEEE80211_KEY_FLAG_GENERATE_MMIC which must be unset.
Ok. We will use this flag.
>
> > Then Skip the code which expands the skb iff
> > IEEE80211_CRYPTO_NO_TAILROOM_NEEDED is set and the key is programmed
> > into the hardware (checking KEY_FLAG_UPLOADED_TO_HARDWARE).
>
> You don't know the key at this point, so you have to keep track of
> whether this is true for all keys, which depends on whether or not
> they're already programmed into the HW (since for SW crypto we need the
> tailroom)
>From this it seems that we do not reserve tailroom iff
IEEE80211_KEY_FLAG_GENERATE_MMIC flag is unset for all keys and all the keys
are programmed into the hardware.
Also, say in mixed mode if TKIP and CCMP keys are configured and this
flag is set for TKIP MMIC, we will end up reserving tailroom even for
CCMP. Is my understanding correct?
Also, though I have not looked into this part of the code very closely,
how about deriving key information at this place? Will that be feasible?
Thanks
Yogesh
On Thu, 2011-04-21 at 18:16 +0530, Yogesh Ashok Powar wrote:
> I thought about this initially, but then realized that this will not
> handle the scenario where we disable the key which needs tailroom space
> reservation? Once the last key, that needs tailroom reservation, is
> disabled, we can skip the tailroom code again?
Well, it kinda goes like this:
key added -> need tailroom code
key put into HW -> no longer need tailroom code
key removed from HW -> need tailroom code again
key deleted -> no longer need tailroom code
johannes
On Wed, Apr 27, 2011 at 04:27:56AM -0700, Johannes Berg wrote:
> On Wed, 2011-04-27 at 16:33 +0530, Yogesh Ashok Powar wrote:
> > On Thu, Apr 21, 2011 at 05:59:48AM -0700, Johannes Berg wrote:
> > > Well, it kinda goes like this:
> > >
> > > key added -> need tailroom code
> > > key put into HW -> no longer need tailroom code
> > > key removed from HW -> need tailroom code again
> > > key deleted -> no longer need tailroom code
> > >
> > Following patch takes care of these four states. Kindly review.
>
> Looks fine, but still I think it might get out of sync when HW restart
> happens?
How about this?
Thanks
Yogesh
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index e180149..ca3c626 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -503,8 +503,12 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
mutex_lock(&sdata->local->key_mtx);
- list_for_each_entry(key, &sdata->key_list, list)
+ sdata->local->crypto_tx_tailroom_needed_cnt = 0;
+
+ list_for_each_entry(key, &sdata->key_list, list) {
+ sdata->local->crypto_tx_tailroom_needed_cnt++;
ieee80211_key_enable_hw_accel(key);
+ }
mutex_unlock(&sdata->local->key_mtx);
}
On Thu, Apr 21, 2011 at 05:33:56AM -0700, Johannes Berg wrote:
> On Thu, 2011-04-21 at 17:45 +0530, Yogesh Ashok Powar wrote:
>
> > @@ -101,6 +101,9 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
> >
> > if (!ret) {
> > key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
> > + if (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC)
> > + key->local->crypto_tx_tailroom_needed_cnt++;
> > +
> > return 0;
> > }
> >
> > @@ -117,6 +120,11 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
> > case WLAN_CIPHER_SUITE_CCMP:
> > case WLAN_CIPHER_SUITE_AES_CMAC:
> > /* all of these we can do in software */
> > +
> > + /* SW encryption need tailroom reservation */
> > + BUG_ON(!key->local);
>
> BUG_ON? Seriously? It was already dereferenced about 10 times here... In
> any case, this isn't right.
>
> > @@ -156,6 +164,11 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
> > key->conf.keyidx, sta ? sta->addr : bcast_addr, ret);
> >
> > key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
> > +
> > + if ((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC)
> > + && key->local->crypto_tx_tailroom_needed_cnt)
> > + key->local->crypto_tx_tailroom_needed_cnt--;
>
> And that if (cnt) can't be right either.
>
>
> > + if (key_sw_programmed && local->crypto_tx_tailroom_needed_cnt)
> > + local->crypto_tx_tailroom_needed_cnt--;
>
> So you're putting if (needed_cnt) everywhere because you have
> refcounting bugs? Better fix those bugs ....
I will address these comments.
>
>
> > + local->crypto_tx_tailroom_needed_cnt = 0;
>
> Not necessary, it's already 0.
>
>
> I don't think you understood what I said how it should work. You must
> increase the counter when the key is created, and then you can reduce it
> again when the key is uploaded and no mmic is needed.
I thought about this initially, but then realized that this will not
handle the scenario where we disable the key which needs tailroom space
reservation? Once the last key, that needs tailroom reservation, is
disabled, we can skip the tailroom code again?
thanks
Yogesh
On Fri, 2011-04-15 at 10:23 +0530, Yogesh Ashok Powar wrote:
> When drivers use HW crypto, reservation for tail room is not needed for
> any crypto suite. Do not reserve tail room in such cases, this helps in
> optimizing the transmit path.
>
> Signed-off-by: Yogesh Ashok Powar <[email protected]>
NACK.
> /*
> - * This could be optimised, devices that do full hardware
> - * crypto (including TKIP MMIC) need no tailroom... But we
> - * have no drivers for such devices currently.
> + * When full HW crypto is being used by the driver,
> + * no tail room is needed. Hence do not ask for tail room
> + * in such cases. This will avoid copying the skb in
> + * pskb_expand_head.
> */
> - if (may_encrypt) {
> + if (!(local->hw.flags & IEEE80211_HW_CRYPTO_ENABLED) && may_encrypt) {
There's no need for this as a HW flag, and in its present form it is
EXTREMELY likely to be misused completely. And realise that even drivers
that implement HW crypto may need the extra tailroom for TKIP MMIC, so
your description of the flag is completely bogus.
You can implement the performance feature, but only by keeping track of
which keys need tailroom and skipping the code here once they are all
programmed into the device which will handle them including MMIC.
johannes
On Thu, Apr 14, 2011 at 11:55:27PM -0700, Johannes Berg wrote:
> On Fri, 2011-04-15 at 10:23 +0530, Yogesh Ashok Powar wrote:
> > When drivers use HW crypto, reservation for tail room is not needed for
> > any crypto suite. Do not reserve tail room in such cases, this helps in
> > optimizing the transmit path.
> >
> > Signed-off-by: Yogesh Ashok Powar <[email protected]>
>
> NACK.
>
> > /*
> > - * This could be optimised, devices that do full hardware
> > - * crypto (including TKIP MMIC) need no tailroom... But we
> > - * have no drivers for such devices currently.
> > + * When full HW crypto is being used by the driver,
> > + * no tail room is needed. Hence do not ask for tail room
> > + * in such cases. This will avoid copying the skb in
> > + * pskb_expand_head.
> > */
> > - if (may_encrypt) {
> > + if (!(local->hw.flags & IEEE80211_HW_CRYPTO_ENABLED) && may_encrypt) {
>
> There's no need for this as a HW flag, and in its present form it is
> EXTREMELY likely to be misused completely. And realise that even drivers
> that implement HW crypto may need the extra tailroom for TKIP MMIC, so
> your description of the flag is completely bogus.
>
> You can implement the performance feature, but only by keeping track of
> which keys need tailroom and skipping the code here once they are all
> programmed into the device which will handle them including MMIC.
Thanks for the information.
Just for my understanding before I attempt patch set V2, please let me
know if the approach given below makes sense.
Define a flag say IEEE80211_CRYPTO_NO_TAILROOM_NEEDED per key. Drivers
need to set this flag in set_key handler for the keys which requires no
extra tailroom. Then Skip the code which expands the skb iff
IEEE80211_CRYPTO_NO_TAILROOM_NEEDED is set and the key is programmed
into the hardware (checking KEY_FLAG_UPLOADED_TO_HARDWARE).
Thanks
Yogesh
>
> johannes
>
On Thu, 2011-04-21 at 14:33 +0200, Johannes Berg wrote:
> So you're putting if (needed_cnt) everywhere because you have
> refcounting bugs? Better fix those bugs ....
Come to think of it, this will be interesting for HW restart code path.
Not sure how to handle that really. Maybe just recompute the counter.
johannes
Hello Johannes,
Please take a look at the patch below which handles all the
comments that have been discussed in this thread.
I will appreciate if you review the same and let me know your thoughts
before I send the final patch.
On Fri, Apr 15, 2011 at 04:01:44AM -0700, Johannes Berg wrote:
> On Fri, 2011-04-15 at 16:21 +0530, Yogesh Ashok Powar wrote:
>
> > > > Then Skip the code which expands the skb iff
> > > > IEEE80211_CRYPTO_NO_TAILROOM_NEEDED is set and the key is programmed
> > > > into the hardware (checking KEY_FLAG_UPLOADED_TO_HARDWARE).
> > >
> > > You don't know the key at this point, so you have to keep track of
> > > whether this is true for all keys, which depends on whether or not
> > > they're already programmed into the HW (since for SW crypto we need the
> > > tailroom)
> > From this it seems that we do not reserve tailroom iff
> > IEEE80211_KEY_FLAG_GENERATE_MMIC flag is unset for all keys and all the keys
> > are programmed into the hardware.
> >
> > Also, say in mixed mode if TKIP and CCMP keys are configured and this
> > flag is set for TKIP MMIC, we will end up reserving tailroom even for
> > CCMP. Is my understanding correct?
>
> Yes, correct.
>
> > Also, though I have not looked into this part of the code very closely,
> > how about deriving key information at this place? Will that be feasible?
>
> No, we can't do it here. I suppose we could postpone it for per key
> stuff, but that's more complex and probably good for a separate patch.
>
> johannes
>
Thanks
Yogesh
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a778499..20476ee 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -764,6 +764,9 @@ struct ieee80211_local {
/* device is started */
bool started;
+ /* count for keys needing tailroom space allocation */
+ int crypto_tx_tailroom_needed_cnt;
+
int tx_headroom; /* required headroom for hardware/radiotap */
/* Tasklet and skb queue to process calls from IRQ mode. All frames
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index af3c564..86b88fa 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -101,6 +101,9 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
if (!ret) {
key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
+ if (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC)
+ key->local->crypto_tx_tailroom_needed_cnt++;
+
return 0;
}
@@ -117,6 +120,11 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
case WLAN_CIPHER_SUITE_CCMP:
case WLAN_CIPHER_SUITE_AES_CMAC:
/* all of these we can do in software */
+
+ /* SW encryption need tailroom reservation */
+ BUG_ON(!key->local);
+ key->local->crypto_tx_tailroom_needed_cnt++;
+
return 0;
default:
return -EINVAL;
@@ -156,6 +164,11 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
key->conf.keyidx, sta ? sta->addr : bcast_addr, ret);
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
+
+ if ((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC)
+ && key->local->crypto_tx_tailroom_needed_cnt)
+ key->local->crypto_tx_tailroom_needed_cnt--;
+
}
void ieee80211_key_removed(struct ieee80211_key_conf *key_conf)
@@ -370,8 +383,11 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
return key;
}
-static void __ieee80211_key_destroy(struct ieee80211_key *key)
+static void __ieee80211_key_destroy(struct ieee80211_local *local,
+ struct ieee80211_key *key)
{
+ bool key_sw_programmed = true;
+
if (!key)
return;
@@ -381,6 +397,8 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key)
*/
synchronize_rcu();
+ key_sw_programmed = !(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE);
+
if (key->local)
ieee80211_key_disable_hw_accel(key);
@@ -391,6 +409,9 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key)
if (key->local)
ieee80211_debugfs_key_remove(key);
+ if (key_sw_programmed && local->crypto_tx_tailroom_needed_cnt)
+ local->crypto_tx_tailroom_needed_cnt--;
+
kfree(key);
}
@@ -447,7 +468,7 @@ int ieee80211_key_link(struct ieee80211_key *key,
old_key = sdata->keys[idx];
__ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
- __ieee80211_key_destroy(old_key);
+ __ieee80211_key_destroy(sdata->local, old_key);
ieee80211_debugfs_key_add(key);
@@ -458,7 +479,8 @@ int ieee80211_key_link(struct ieee80211_key *key,
return ret;
}
-static void __ieee80211_key_free(struct ieee80211_key *key)
+static void __ieee80211_key_free(struct ieee80211_local *local,
+ struct ieee80211_key *key)
{
/*
* Replace key with nothingness if it was ever used.
@@ -467,7 +489,7 @@ static void __ieee80211_key_free(struct ieee80211_key *key)
__ieee80211_key_replace(key->sdata, key->sta,
key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE,
key, NULL);
- __ieee80211_key_destroy(key);
+ __ieee80211_key_destroy(local, key);
}
void ieee80211_key_free(struct ieee80211_local *local,
@@ -477,7 +499,7 @@ void ieee80211_key_free(struct ieee80211_local *local,
return;
mutex_lock(&local->key_mtx);
- __ieee80211_key_free(key);
+ __ieee80211_key_free(local, key);
mutex_unlock(&local->key_mtx);
}
@@ -521,7 +543,7 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata)
ieee80211_debugfs_key_remove_mgmt_default(sdata);
list_for_each_entry_safe(key, tmp, &sdata->key_list, list)
- __ieee80211_key_free(key);
+ __ieee80211_key_free(sdata->local, key);
ieee80211_debugfs_key_update_default(sdata);
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 0ab2a8d..1f2aa4d 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -619,6 +619,7 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
local->user_power_level = -1;
local->uapsd_queues = IEEE80211_DEFAULT_UAPSD_QUEUES;
local->uapsd_max_sp_len = IEEE80211_DEFAULT_MAX_SP_LEN;
+ local->crypto_tx_tailroom_needed_cnt = 0;
INIT_LIST_HEAD(&local->interfaces);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 17b10be..39cdcf5 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1487,7 +1487,7 @@ static int ieee80211_skb_resize(struct ieee80211_local *local,
* crypto (including TKIP MMIC) need no tailroom... But we
* have no drivers for such devices currently.
*/
- if (may_encrypt) {
+ if (may_encrypt && local->crypto_tx_tailroom_needed_cnt) {
tail_need = IEEE80211_ENCRYPT_TAILROOM;
tail_need -= skb_tailroom(skb);
tail_need = max_t(int, tail_need, 0);
--
1.5.4.1
On Thu, 2011-04-21 at 17:45 +0530, Yogesh Ashok Powar wrote:
> @@ -101,6 +101,9 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
>
> if (!ret) {
> key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
> + if (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC)
> + key->local->crypto_tx_tailroom_needed_cnt++;
> +
> return 0;
> }
>
> @@ -117,6 +120,11 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
> case WLAN_CIPHER_SUITE_CCMP:
> case WLAN_CIPHER_SUITE_AES_CMAC:
> /* all of these we can do in software */
> +
> + /* SW encryption need tailroom reservation */
> + BUG_ON(!key->local);
BUG_ON? Seriously? It was already dereferenced about 10 times here... In
any case, this isn't right.
> @@ -156,6 +164,11 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
> key->conf.keyidx, sta ? sta->addr : bcast_addr, ret);
>
> key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
> +
> + if ((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC)
> + && key->local->crypto_tx_tailroom_needed_cnt)
> + key->local->crypto_tx_tailroom_needed_cnt--;
And that if (cnt) can't be right either.
> + if (key_sw_programmed && local->crypto_tx_tailroom_needed_cnt)
> + local->crypto_tx_tailroom_needed_cnt--;
So you're putting if (needed_cnt) everywhere because you have
refcounting bugs? Better fix those bugs ....
> + local->crypto_tx_tailroom_needed_cnt = 0;
Not necessary, it's already 0.
I don't think you understood what I said how it should work. You must
increase the counter when the key is created, and then you can reduce it
again when the key is uploaded and no mmic is needed.
johannes
On Thu, Apr 21, 2011 at 05:59:48AM -0700, Johannes Berg wrote:
> Well, it kinda goes like this:
>
> key added -> need tailroom code
> key put into HW -> no longer need tailroom code
> key removed from HW -> need tailroom code again
> key deleted -> no longer need tailroom code
>
Following patch takes care of these four states. Kindly review.
Thanks
Yogesh
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a778499..027c046 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -766,6 +766,9 @@ struct ieee80211_local {
int tx_headroom; /* required headroom for hardware/radiotap */
+ /* count for keys needing tailroom space allocation */
+ int crypto_tx_tailroom_needed_cnt;
+
/* Tasklet and skb queue to process calls from IRQ mode. All frames
* added to skb_queue will be processed, but frames in
* skb_queue_unreliable may be dropped if the total length of these
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index af3c564..e180149 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -101,6 +101,10 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
if (!ret) {
key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
+
+ if (!(key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC))
+ key->local->crypto_tx_tailroom_needed_cnt--;
+
return 0;
}
@@ -156,6 +160,9 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
key->conf.keyidx, sta ? sta->addr : bcast_addr, ret);
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
+
+ if (!(key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC))
+ key->local->crypto_tx_tailroom_needed_cnt++;
}
void ieee80211_key_removed(struct ieee80211_key_conf *key_conf)
@@ -388,8 +395,10 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key)
ieee80211_aes_key_free(key->u.ccmp.tfm);
if (key->conf.cipher == WLAN_CIPHER_SUITE_AES_CMAC)
ieee80211_aes_cmac_key_free(key->u.aes_cmac.tfm);
- if (key->local)
+ if (key->local) {
ieee80211_debugfs_key_remove(key);
+ key->local->crypto_tx_tailroom_needed_cnt--;
+ }
kfree(key);
}
@@ -451,6 +460,8 @@ int ieee80211_key_link(struct ieee80211_key *key,
ieee80211_debugfs_key_add(key);
+ key->local->crypto_tx_tailroom_needed_cnt++;
+
ret = ieee80211_key_enable_hw_accel(key);
mutex_unlock(&sdata->local->key_mtx);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index a2043e4..87456b4 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1481,7 +1481,7 @@ static int ieee80211_skb_resize(struct ieee80211_local *local,
* crypto (including TKIP MMIC) need no tailroom... But we
* have no drivers for such devices currently.
*/
- if (may_encrypt) {
+ if (may_encrypt && local->crypto_tx_tailroom_needed_cnt) {
tail_need = IEEE80211_ENCRYPT_TAILROOM;
tail_need -= skb_tailroom(skb);
tail_need = max_t(int, tail_need, 0);
--
1.5.4.1
On Wed, 2011-04-27 at 17:10 +0530, Yogesh Ashok Powar wrote:
> On Wed, Apr 27, 2011 at 04:27:56AM -0700, Johannes Berg wrote:
> > On Wed, 2011-04-27 at 16:33 +0530, Yogesh Ashok Powar wrote:
> > > On Thu, Apr 21, 2011 at 05:59:48AM -0700, Johannes Berg wrote:
> > > > Well, it kinda goes like this:
> > > >
> > > > key added -> need tailroom code
> > > > key put into HW -> no longer need tailroom code
> > > > key removed from HW -> need tailroom code again
> > > > key deleted -> no longer need tailroom code
> > > >
> > > Following patch takes care of these four states. Kindly review.
> >
> > Looks fine, but still I think it might get out of sync when HW restart
> > happens?
> How about this?
>
> Thanks
> Yogesh
>
> diff --git a/net/mac80211/key.c b/net/mac80211/key.c
> index e180149..ca3c626 100644
> --- a/net/mac80211/key.c
> +++ b/net/mac80211/key.c
> @@ -503,8 +503,12 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
>
> mutex_lock(&sdata->local->key_mtx);
>
> - list_for_each_entry(key, &sdata->key_list, list)
> + sdata->local->crypto_tx_tailroom_needed_cnt = 0;
> +
> + list_for_each_entry(key, &sdata->key_list, list) {
> + sdata->local->crypto_tx_tailroom_needed_cnt++;
> ieee80211_key_enable_hw_accel(key);
> + }
>
> mutex_unlock(&sdata->local->key_mtx);
> }
Yeah that looks good.
johannes