| Message ID | 20180111070932.9929-4-pkshih@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 01/11/2018 01:09 AM, pkshih@realtek.com wrote: > From: Ping-Ke Shih <pkshih@realtek.com> > > When using iwconfig to check wifi status, wext uses 'static struct' of > sinfo to get station info so sinfo->filled will be persistent. Since the > commit 2b9a7e1bac24 ("mac80211: allow drivers to provide most station > statistics") assumes driver initializes sinfo->filled to declare supported > fields, without initialization it will report wrong info. This commit > simply set 'filled' to be zero simply, and left sinfo to be filled by > mac80211. > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > --- > drivers/net/wireless/realtek/rtlwifi/core.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) Acked-by: Larry Finger <Larry.Finger@lwfinger.net> > > diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c > index a78b828f531a..ec639fa8095e 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/core.c > +++ b/drivers/net/wireless/realtek/rtlwifi/core.c > @@ -992,6 +992,15 @@ static int _rtl_get_hal_qnum(u16 queue) > return qnum; > } > > +static void rtl_op_sta_statistics(struct ieee80211_hw *hw, > + struct ieee80211_vif *vif, > + struct ieee80211_sta *sta, > + struct station_info *sinfo) > +{ > + /* nothing filled by driver, so mac80211 will update all info */ > + sinfo->filled = 0; > +} > + > /* > *for mac80211 VO = 0, VI = 1, BE = 2, BK = 3 > *for rtl819x BE = 0, BK = 1, VI = 2, VO = 3 > @@ -1878,6 +1887,7 @@ const struct ieee80211_ops rtl_ops = { > .config = rtl_op_config, > .configure_filter = rtl_op_configure_filter, > .set_key = rtl_op_set_key, > + .sta_statistics = rtl_op_sta_statistics, > .conf_tx = rtl_op_conf_tx, > .bss_info_changed = rtl_op_bss_info_changed, > .get_tsf = rtl_op_get_tsf, >
<pkshih@realtek.com> writes: > From: Ping-Ke Shih <pkshih@realtek.com> > > When using iwconfig to check wifi status, wext uses 'static struct' of > sinfo to get station info so sinfo->filled will be persistent. Since the > commit 2b9a7e1bac24 ("mac80211: allow drivers to provide most station > statistics") assumes driver initializes sinfo->filled to declare supported > fields, without initialization it will report wrong info. This commit > simply set 'filled' to be zero simply, and left sinfo to be filled by > mac80211. > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > --- > drivers/net/wireless/realtek/rtlwifi/core.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c > index a78b828f531a..ec639fa8095e 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/core.c > +++ b/drivers/net/wireless/realtek/rtlwifi/core.c > @@ -992,6 +992,15 @@ static int _rtl_get_hal_qnum(u16 queue) > return qnum; > } > > +static void rtl_op_sta_statistics(struct ieee80211_hw *hw, > + struct ieee80211_vif *vif, > + struct ieee80211_sta *sta, > + struct station_info *sinfo) > +{ > + /* nothing filled by driver, so mac80211 will update all info */ > + sinfo->filled = 0; > +} > + > /* > *for mac80211 VO = 0, VI = 1, BE = 2, BK = 3 > *for rtl819x BE = 0, BK = 1, VI = 2, VO = 3 > @@ -1878,6 +1887,7 @@ const struct ieee80211_ops rtl_ops = { > .config = rtl_op_config, > .configure_filter = rtl_op_configure_filter, > .set_key = rtl_op_set_key, > + .sta_statistics = rtl_op_sta_statistics, Adding an empty op like that feels pointless, IMHO (but without checking mac80211 sources) not having the op should be the same as filled = 0. To me this smells like a bug in mac80211. Johannes, what do you think?
On Tue, 2018-01-16 at 17:42 +0200, Kalle Valo wrote: > > When using iwconfig to check wifi status, wext uses 'static struct' of > > sinfo to get station info so sinfo->filled will be persistent. Since the > > commit 2b9a7e1bac24 ("mac80211: allow drivers to provide most station > > statistics") assumes driver initializes sinfo->filled to declare supported > > fields, without initialization it will report wrong info. This commit > > simply set 'filled' to be zero simply, and left sinfo to be filled by > > mac80211. Huh, this can't be right. > Adding an empty op like that feels pointless, IMHO (but without checking > mac80211 sources) not having the op should be the same as filled = 0. To > me this smells like a bug in mac80211. > > Johannes, what do you think? It can't be right. It would've broken each and every driver out there, other than the one or two who implement this. However, it looks like PK is actually correct - *wext* does appear to be broken! nl80211 does this: memset(&sinfo, 0, sizeof(sinfo)); err = rdev_dump_station(rdev, wdev->netdev, sta_idx, mac_addr, &sinfo); and memset(&sinfo, 0, sizeof(sinfo)); ... err = rdev_get_station(rdev, dev, mac_addr, &sinfo); and has a bug in cfg80211_cqm_rssi_update(). Wext also has bugs, I'll send out a fix. johannes
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c index a78b828f531a..ec639fa8095e 100644 --- a/drivers/net/wireless/realtek/rtlwifi/core.c +++ b/drivers/net/wireless/realtek/rtlwifi/core.c @@ -992,6 +992,15 @@ static int _rtl_get_hal_qnum(u16 queue) return qnum; } +static void rtl_op_sta_statistics(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + struct ieee80211_sta *sta, + struct station_info *sinfo) +{ + /* nothing filled by driver, so mac80211 will update all info */ + sinfo->filled = 0; +} + /* *for mac80211 VO = 0, VI = 1, BE = 2, BK = 3 *for rtl819x BE = 0, BK = 1, VI = 2, VO = 3 @@ -1878,6 +1887,7 @@ const struct ieee80211_ops rtl_ops = { .config = rtl_op_config, .configure_filter = rtl_op_configure_filter, .set_key = rtl_op_set_key, + .sta_statistics = rtl_op_sta_statistics, .conf_tx = rtl_op_conf_tx, .bss_info_changed = rtl_op_bss_info_changed, .get_tsf = rtl_op_get_tsf,