| Message ID | 20231003215839.981227-1-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: ethtool: check link sta if deflink sta not found. | expand |
On Tue, 2023-10-03 at 14:58 -0700, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > While testing with wifi-7 radio put into AX mode, link-0 is created. > sdata->deflink.u.mgd.bssid was 00 in this case, so sta was not > found. > > Use link-0 for sta if it is available to do a better job of reporting > ethtool stats. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > net/mac80211/ethtool.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/net/mac80211/ethtool.c b/net/mac80211/ethtool.c > index 6bd7fba8a867..1b58304fc68a 100644 > --- a/net/mac80211/ethtool.c > +++ b/net/mac80211/ethtool.c > @@ -96,6 +96,7 @@ static void ieee80211_get_stats2(struct net_device *dev, > struct ieee80211_local *local = sdata->local; > struct station_info sinfo; > struct survey_info survey; > + struct ieee80211_link_data *link; > int i, q; > int z; > #define STA_STATS_SURVEY_LEN 7 > @@ -128,6 +129,12 @@ static void ieee80211_get_stats2(struct net_device *dev, > if (sdata->vif.type == NL80211_IFTYPE_STATION) { > sta = sta_info_get_bss(sdata, sdata->deflink.u.mgd.bssid); Just don't even try this, link[0] is always valid. > + if (!sta) { > + link = sdata_dereference(sdata->link[0], sdata); but link[0] is a bad idea anyway ... what if the AP only assigned link 1 and 2? Or you connected only there? I'm not even sure this is really worth fixing, do you really want a random link's statistics? johannes
On 10/4/23 12:36, Johannes Berg wrote: > On Tue, 2023-10-03 at 14:58 -0700, greearb@candelatech.com wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> While testing with wifi-7 radio put into AX mode, link-0 is created. >> sdata->deflink.u.mgd.bssid was 00 in this case, so sta was not >> found. >> >> Use link-0 for sta if it is available to do a better job of reporting >> ethtool stats. >> >> Signed-off-by: Ben Greear <greearb@candelatech.com> >> --- >> net/mac80211/ethtool.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/net/mac80211/ethtool.c b/net/mac80211/ethtool.c >> index 6bd7fba8a867..1b58304fc68a 100644 >> --- a/net/mac80211/ethtool.c >> +++ b/net/mac80211/ethtool.c >> @@ -96,6 +96,7 @@ static void ieee80211_get_stats2(struct net_device *dev, >> struct ieee80211_local *local = sdata->local; >> struct station_info sinfo; >> struct survey_info survey; >> + struct ieee80211_link_data *link; >> int i, q; >> int z; >> #define STA_STATS_SURVEY_LEN 7 >> @@ -128,6 +129,12 @@ static void ieee80211_get_stats2(struct net_device *dev, >> if (sdata->vif.type == NL80211_IFTYPE_STATION) { >> sta = sta_info_get_bss(sdata, sdata->deflink.u.mgd.bssid); > > Just don't even try this, link[0] is always valid. What is 'deflink' even used for then? Is that only for non MLO connections? Or you mean link[0] is valid even in non MLO connections? Note I'm not changing that line above in my patch, though maybe it should be changed... > >> + if (!sta) { >> + link = sdata_dereference(sdata->link[0], sdata); > > but link[0] is a bad idea anyway ... what if the AP only assigned link 1 > and 2? Or you connected only there? > > I'm not even sure this is really worth fixing, do you really want a > random link's statistics? I'd like it to at least have a chance of reporting something useful. So maybe 'sta' can be the connected link with highest band. And then I can add new ethtool stats for link-0, link-1, link-2 (and one of those would be duplicated, since the 'best' stats are already added). Or I could have the default stats always be link-0, and add new ethtool stats for link-1, link-2. That would make it less backwards compatible since link-0 may not be connected and would show no useful stats for the old default ethtool strings. I do not want to change existing ethtool stats strings since that would break my user-space API and I'd rather not hack around that if possible. Thanks, Ben > > johannes >
diff --git a/net/mac80211/ethtool.c b/net/mac80211/ethtool.c index 6bd7fba8a867..1b58304fc68a 100644 --- a/net/mac80211/ethtool.c +++ b/net/mac80211/ethtool.c @@ -96,6 +96,7 @@ static void ieee80211_get_stats2(struct net_device *dev, struct ieee80211_local *local = sdata->local; struct station_info sinfo; struct survey_info survey; + struct ieee80211_link_data *link; int i, q; int z; #define STA_STATS_SURVEY_LEN 7 @@ -128,6 +129,12 @@ static void ieee80211_get_stats2(struct net_device *dev, if (sdata->vif.type == NL80211_IFTYPE_STATION) { sta = sta_info_get_bss(sdata, sdata->deflink.u.mgd.bssid); + if (!sta) { + link = sdata_dereference(sdata->link[0], sdata); + if (link) + sta = sta_info_get_bss(sdata, link->u.mgd.bssid); + } + if (!(sta && !WARN_ON(sta->sdata->dev != dev))) goto do_survey;