Re: [PATCH] rtl8xxxu: Don't printk raw binary if serial number is not burned in.

[Stefano Brivio]

On Fri,  8 Sep 2017 01:51:03 +0200
Adam Borowski  wrote:

> I assume that a blank efuse comes with all ones, thus I did not bother
> recognizing other possible junk values.  This matches 100% of dongles
> I've seen (a single Gembird 8192eu).
> 
> Signed-off-by: Adam Borowski 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> index 80fee699f58a..bdc37e7272ca 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> @@ -614,7 +614,11 @@ static int rtl8192eu_parse_efuse(struct rtl8xxxu_priv 
> *priv)
>  
>   dev_info(>udev->dev, "Vendor: %.7s\n", efuse->vendor_name);
>   dev_info(>udev->dev, "Product: %.11s\n", efuse->device_name);
> - dev_info(>udev->dev, "Serial: %.11s\n", efuse->serial);
> + if (strncmp(efuse->serial,
> + "\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff", 11))

You might want to use memchr_inv():

if (memchr_inv(efuse->serial, 0xff, 11))
dev_info(>udev->dev, "Serial: %.11s\n", efuse->serial);
...

Mostly cosmetic though.

--
Stefano

Re: hung task in mac80211

[Stefano Brivio]

On Wed, 06 Sep 2017 14:58:48 +0200
Johannes Berg  wrote:

> +void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> +  u8 dialog_token, u16 timeout,
> +  u16 start_seq_num, u16 ba_policy, u16 tid,
> +  u16 buf_size, bool tx, bool auto_seq)
> +{
> + mutex_lock(>ampdu_mlme.mtx);
> + ___ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
> +  start_seq_num, ba_policy, tid,
> +  buf_size, tx, auto_seq);
> + mutex_unlock(>ampdu_mlme.mtx);
> +}
> +

Sorry for the extended bothering :) but here, you're extending quite a
bit the scope of the lock also when__ieee80211_start_rx_ba_session() is
called by ieee80211_process_addba_request(). No idea what the hit can
be, but we can't safely assume it's nothing either. What about simply
introducing a 'ampdu_mlme_lock_held' argument instead? Something like:

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 2b36eff5d97e..35a9eff1ec66 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -248,7 +248,8 @@ static void ieee80211_send_addba_resp(struct 
ieee80211_sub_if_data *sdata, u8 *d
 void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 u8 dialog_token, u16 timeout,
 u16 start_seq_num, u16 ba_policy, u16 tid,
-u16 buf_size, bool tx, bool auto_seq)
+u16 buf_size, bool tx, bool auto_seq,
+bool ampdu_mlme_lock_held)
 {
struct ieee80211_local *local = sta->sdata->local;
struct tid_ampdu_rx *tid_agg_rx;
@@ -311,7 +312,8 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
   buf_size, sta->sta.addr);
 
/* examine state machine */
-   mutex_lock(>ampdu_mlme.mtx);
+   if (!ampdu_mlme_lock_held)
+   mutex_lock(>ampdu_mlme.mtx);
 
if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
if (sta->ampdu_mlme.tid_rx_token[tid] == dialog_token) {
@@ -415,7 +417,8 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
__clear_bit(tid, sta->ampdu_mlme.unexpected_agg);
sta->ampdu_mlme.tid_rx_token[tid] = dialog_token;
}
-   mutex_unlock(>ampdu_mlme.mtx);
+   if (!ampdu_mlme_lock_held)
+   mutex_unlock(>ampdu_mlme.mtx);
 
 end_no_lock:
if (tx)
@@ -445,7 +448,7 @@ void ieee80211_process_addba_request(struct ieee80211_local 
*local,
 
__ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
start_seq_num, ba_policy, tid,
-   buf_size, true, false);
+   buf_size, true, false, false);
 }
 
 void ieee80211_manage_rx_ba_offl(struct ieee80211_vif *vif,
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..59ba67e8942f 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -335,7 +335,7 @@ void ieee80211_ba_session_work(struct work_struct *work)
   sta->ampdu_mlme.tid_rx_manage_offl))
__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
IEEE80211_MAX_AMPDU_BUF,
-   false, true);
+   false, true, true);
 
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
   sta->ampdu_mlme.tid_rx_manage_offl))
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2197c62a0a6e..5d494ac65853 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1759,7 +1759,8 @@ void __ieee80211_stop_rx_ba_session(struct sta_info *sta, 
u16 tid,
 void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 u8 dialog_token, u16 timeout,
 u16 start_seq_num, u16 ba_policy, u16 tid,
-u16 buf_size, bool tx, bool auto_seq);
+u16 buf_size, bool tx, bool auto_seq,
+bool ampdu_mlme_lock_held);
 void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
 enum ieee80211_agg_stop_reason reason);
 void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,

-- 
Stefano

Re: hung task in mac80211

[Stefano Brivio]

On Wed, 06 Sep 2017 15:30:10 +0200
Johannes Berg  wrote:

> So for example replacing the loop of tid = 0..NUM_TIDS-1 with a
> list_for_each_entry() would already be unsafe with the dropping if the
> list were to require the mutex for locking.

Sure. Still, it would need another code change to break, but in general
I do agree indeed. :)


--
Stefano

Re: hung task in mac80211

[Stefano Brivio]

On Wed, 06 Sep 2017 15:21:00 +0200
Johannes Berg <johan...@sipsolutions.net> wrote:

> On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote:
> > On Wed, 06 Sep 2017 14:48:35 +0200
> > Johannes Berg <johan...@sipsolutions.net> wrote:
> >   
> > > I'll look in a bit - but
> > >   
> > > > +   mutex_unlock(>ampdu_mlme.mtx);
> > > >     ___ieee80211_stop_rx_ba_session(
> > > >     sta, tid, WLAN_BACK_RECIPIENT,
> > > >     WLAN_REASON_QSTA_TIMEOUT,
> > > > true);    
> > > 
> > > This already has three underscores so shouldn't drop.  
> > 
> > Right, of course.
> >   
> > > [...]  
> > > > +   mutex_unlock(>ampdu_mlme.mtx);
> > > >     __ieee80211_start_rx_ba_session(sta, 0,
> > > > 0,
> > > > 0, 1, tid,    
> > > 
> > > maybe this one needs a ___ version then?  
> > 
> > Either that, or as it's a single call, perhaps just the following?
> > Matter of taste I guess...  
> 
> I don't think it's a matter of taste - for me, in principle, dropping
> locks for small sections of code where the larger section holds it is a
> bug waiting to happen. It may (may, I don't even know) be OK here, but
> in general it's something to avoid.

Yes, that was based on the assumption that the initial part of
__ieee80211_start_rx_ba_session() can't really affect the AMPDU
state-machine in any way.

But sure, one small change there in the future and the assumption
doesn't hold anymore.


--
Stefano

Re: hung task in mac80211

[Stefano Brivio]

On Wed, 06 Sep 2017 14:48:35 +0200
Johannes Berg  wrote:

> I'll look in a bit - but
> 
> > +   mutex_unlock(>ampdu_mlme.mtx);
> >     ___ieee80211_stop_rx_ba_session(
> >     sta, tid, WLAN_BACK_RECIPIENT,
> >     WLAN_REASON_QSTA_TIMEOUT, true);  
> 
> This already has three underscores so shouldn't drop.

Right, of course.

> [...]
> > +   mutex_unlock(>ampdu_mlme.mtx);
> >     __ieee80211_start_rx_ba_session(sta, 0, 0,
> > 0, 1, tid,  
> 
> maybe this one needs a ___ version then?

Either that, or as it's a single call, perhaps just the following?
Matter of taste I guess...

diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..377dd3c233d3 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -332,10 +332,13 @@ void ieee80211_ba_session_work(struct work_struct *work)
WLAN_REASON_UNSPECIFIED, true);
 
if (test_and_clear_bit(tid,
-  sta->ampdu_mlme.tid_rx_manage_offl))
+  sta->ampdu_mlme.tid_rx_manage_offl)) {
+   mutex_unlock(>ampdu_mlme.mtx);
__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
IEEE80211_MAX_AMPDU_BUF,
false, true);
+   mutex_lock(>ampdu_mlme.mtx);
+   }
 
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
   sta->ampdu_mlme.tid_rx_manage_offl))
-- 
Stefano

Re: hung task in mac80211

[Stefano Brivio]

On Wed, 6 Sep 2017 13:57:47 +0200
Matteo Croce  wrote:

> Hi,
> 
> I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
> The problem is present both on my AP and on my notebook,
> so it seems it affects AP and STA mode as well.
> The generated messages are:
> 
> INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
>   Not tainted 4.13.0 #57
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/u16:6   D0   120  2 0x
> Workqueue: phy0 ieee80211_ba_session_work [mac80211]
> Call Trace:
>  ? __schedule+0x174/0x5b0
>  ? schedule+0x31/0x80
>  ? schedule_preempt_disabled+0x9/0x10
>  ? __mutex_lock.isra.2+0x163/0x480
>  ? select_task_rq_fair+0xb9f/0xc60
>  ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>  ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]

This is ugly and maybe wrong, but you could check perhaps...:

diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..bd7512a656f2 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -320,28 +320,40 @@ void ieee80211_ba_session_work(struct work_struct *work)
 
mutex_lock(>ampdu_mlme.mtx);
for (tid = 0; tid < IEEE80211_NUM_TIDS; tid++) {
-   if (test_and_clear_bit(tid, 
sta->ampdu_mlme.tid_rx_timer_expired))
+   if (test_and_clear_bit(tid, 
sta->ampdu_mlme.tid_rx_timer_expired)) {
+   mutex_unlock(>ampdu_mlme.mtx);
___ieee80211_stop_rx_ba_session(
sta, tid, WLAN_BACK_RECIPIENT,
WLAN_REASON_QSTA_TIMEOUT, true);
+   mutex_lock(>ampdu_mlme.mtx);
+   }
 
if (test_and_clear_bit(tid,
-  sta->ampdu_mlme.tid_rx_stop_requested))
+  sta->ampdu_mlme.tid_rx_stop_requested)) {
+   mutex_unlock(>ampdu_mlme.mtx);
___ieee80211_stop_rx_ba_session(
sta, tid, WLAN_BACK_RECIPIENT,
WLAN_REASON_UNSPECIFIED, true);
+   mutex_lock(>ampdu_mlme.mtx);
+   }
 
if (test_and_clear_bit(tid,
-  sta->ampdu_mlme.tid_rx_manage_offl))
+  sta->ampdu_mlme.tid_rx_manage_offl)) {
+   mutex_unlock(>ampdu_mlme.mtx);
__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
IEEE80211_MAX_AMPDU_BUF,
false, true);
+   mutex_lock(>ampdu_mlme.mtx);
+   }
 
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
-  sta->ampdu_mlme.tid_rx_manage_offl))
+  sta->ampdu_mlme.tid_rx_manage_offl)) {
+   mutex_unlock(>ampdu_mlme.mtx);
___ieee80211_stop_rx_ba_session(
sta, tid, WLAN_BACK_RECIPIENT,
0, false);
+   mutex_lock(>ampdu_mlme.mtx);
+   }
 
spin_lock_bh(>lock);
 
-- 
Stefano

[PATCH net] intersil/hostap: Fix outdated comment about dev->destructor

[Stefano Brivio]

After commit cf124db566e6 ("net: Fix inconsistent teardown and
release of private netdev state."), setting
'dev->needs_free_netdev' ensures device data is released, and
'dev->destructor' is not used anymore.

Fixes: cf124db566e6 ("net: Fix inconsistent teardown and release of private 
netdev state.")
Signed-off-by: Stefano Brivio <sbri...@redhat.com>
---
Despite checkpatch.pl warning against unnecessary changes, this
outdated comment might lead to confusion -- making this change
actually necessary, perhaps.

 drivers/net/wireless/intersil/hostap/hostap_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_main.c 
b/drivers/net/wireless/intersil/hostap/hostap_main.c
index a3c066f90afc..012930d35434 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_main.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_main.c
@@ -125,8 +125,8 @@ void hostap_remove_interface(struct net_device *dev, int 
rtnl_locked,
else
unregister_netdev(dev);
 
-   /* dev->destructor = free_netdev() will free the device data, including
-* private data, when removing the device */
+   /* 'dev->needs_free_netdev = true' implies device data, including
+* private data, will be freed when the device is removed */
 }
 
 
-- 
2.9.4