| Message ID | 152586802515.18089.12700714478750075562.stgit@alrua-kau (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
> WARN_ON is used where the function has no way to signal errors to the > caller. There isn't really much point in that - failing allocations are already *very* noisy. Please respin with that removed (and then I guess you can fix the Fixes: too) You didn't actually do that in this patch though :-) johannes > + bool filled; > int ret; > > /* if the user specified a customised value for this interface, then > @@ -102,7 +103,17 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) > if (!real_netdev) > goto default_throughput; > > - ret = cfg80211_get_station(real_netdev, neigh->addr, &sinfo); > + sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL); > + if (!sinfo) > + goto default_throughput; > + > + ret = cfg80211_get_station(real_netdev, neigh->addr, sinfo); > + > + /* just save these here instead of having complex free logic below */ > + throughput = sinfo.expected_throughput / 100; > + filled = !!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT)); It's bool so you don't need the !!(...) :-) johannes
Johannes Berg <johannes@sipsolutions.net> writes: >> WARN_ON is used where the function has no way to signal errors to the >> caller. > > There isn't really much point in that - failing allocations are already > *very* noisy. Please respin with that removed (and then I guess you can > fix the Fixes: too) OK, will do. > You didn't actually do that in this patch though :-) Well, I may have copy-pasted that from the commit message of the previous patch ;) > It's bool so you don't need the !!(...) :-) But what if I want it to be extra super duper bool? ;) -Toke
Hi Toke,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v4.17-rc4 next-20180509]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/wireless-drivers-Dynamically-allocate-struct-station_info/20180510-034416
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: i386-randconfig-x000-201818 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
net/batman-adv/bat_v_elp.c: In function 'batadv_v_elp_get_throughput':
>> net/batman-adv/bat_v_elp.c:113:21: error: 'sinfo' is a pointer; did you mean to use '->'?
throughput = sinfo.expected_throughput / 100;
^
->
net/batman-adv/bat_v_elp.c:114:20: error: 'sinfo' is a pointer; did you mean to use '->'?
filled = !!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT));
^
->
vim +113 net/batman-adv/bat_v_elp.c
69
70 /**
71 * batadv_v_elp_get_throughput() - get the throughput towards a neighbour
72 * @neigh: the neighbour for which the throughput has to be obtained
73 *
74 * Return: The throughput towards the given neighbour in multiples of 100kpbs
75 * (a value of '1' equals to 0.1Mbps, '10' equals 1Mbps, etc).
76 */
77 static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh)
78 {
79 struct batadv_hard_iface *hard_iface = neigh->if_incoming;
80 struct ethtool_link_ksettings link_settings;
81 struct net_device *real_netdev;
82 struct station_info *sinfo;
83 u32 throughput;
84 bool filled;
85 int ret;
86
87 /* if the user specified a customised value for this interface, then
88 * return it directly
89 */
90 throughput = atomic_read(&hard_iface->bat_v.throughput_override);
91 if (throughput != 0)
92 return throughput;
93
94 /* if this is a wireless device, then ask its throughput through
95 * cfg80211 API
96 */
97 if (batadv_is_wifi_hardif(hard_iface)) {
98 if (!batadv_is_cfg80211_hardif(hard_iface))
99 /* unsupported WiFi driver version */
100 goto default_throughput;
101
102 real_netdev = batadv_get_real_netdev(hard_iface->net_dev);
103 if (!real_netdev)
104 goto default_throughput;
105
106 sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
107 if (!sinfo)
108 goto default_throughput;
109
110 ret = cfg80211_get_station(real_netdev, neigh->addr, sinfo);
111
112 /* just save these here instead of having complex free logic below */
> 113 throughput = sinfo.expected_throughput / 100;
114 filled = !!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT));
115
116 kfree(sinfo);
117
118 dev_put(real_netdev);
119 if (ret == -ENOENT) {
120 /* Node is not associated anymore! It would be
121 * possible to delete this neighbor. For now set
122 * the throughput metric to 0.
123 */
124 return 0;
125 }
126 if (ret || !filled)
127 goto default_throughput;
128
129 return throughput;
130 }
131
132 /* if not a wifi interface, check if this device provides data via
133 * ethtool (e.g. an Ethernet adapter)
134 */
135 memset(&link_settings, 0, sizeof(link_settings));
136 rtnl_lock();
137 ret = __ethtool_get_link_ksettings(hard_iface->net_dev, &link_settings);
138 rtnl_unlock();
139 if (ret == 0) {
140 /* link characteristics might change over time */
141 if (link_settings.base.duplex == DUPLEX_FULL)
142 hard_iface->bat_v.flags |= BATADV_FULL_DUPLEX;
143 else
144 hard_iface->bat_v.flags &= ~BATADV_FULL_DUPLEX;
145
146 throughput = link_settings.base.speed;
147 if (throughput && throughput != SPEED_UNKNOWN)
148 return throughput * 10;
149 }
150
151 default_throughput:
152 if (!(hard_iface->bat_v.flags & BATADV_WARNING_DEFAULT)) {
153 batadv_info(hard_iface->soft_iface,
154 "WiFi driver or ethtool info does not provide information about link speeds on interface %s, therefore defaulting to hardcoded throughput values of %u.%1u Mbps. Consider overriding the throughput manually or checking your driver.\n",
155 hard_iface->net_dev->name,
156 BATADV_THROUGHPUT_DEFAULT_VALUE / 10,
157 BATADV_THROUGHPUT_DEFAULT_VALUE % 10);
158 hard_iface->bat_v.flags |= BATADV_WARNING_DEFAULT;
159 }
160
161 /* if none of the above cases apply, return the base_throughput */
162 return BATADV_THROUGHPUT_DEFAULT_VALUE;
163 }
164
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 10 May 2018 00:55:01 CEST, kbuild test robot <lkp@intel.com> wrote: >Hi Toke, > >Thank you for the patch! Yet something to improve: > >[auto build test ERROR on wireless-drivers-next/master] >[also build test ERROR on v4.17-rc4 next-20180509] >[if your patch is applied to the wrong git tree, please drop us a note >to help improve the system] > >url: >https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/wireless-drivers-Dynamically-allocate-struct-station_info/20180510-034416 >base: >https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git >master >config: i386-randconfig-x000-201818 (attached as .config) >compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 >reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > >All errors (new ones prefixed by >>): > > net/batman-adv/bat_v_elp.c: In function 'batadv_v_elp_get_throughput': >>> net/batman-adv/bat_v_elp.c:113:21: error: 'sinfo' is a pointer; did >you mean to use '->'? > throughput = sinfo.expected_throughput / 100; > ^ > -> >net/batman-adv/bat_v_elp.c:114:20: error: 'sinfo' is a pointer; did you >mean to use '->'? > filled = !!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT)); > ^ > -> Huh, I could swear I compile tested this. Will send another version tomorrow, sorry about that :/ -Toke > >vim +113 net/batman-adv/bat_v_elp.c > > 69 > 70 /** >71 * batadv_v_elp_get_throughput() - get the throughput towards a >neighbour >72 * @neigh: the neighbour for which the throughput has to be obtained > 73 * >74 * Return: The throughput towards the given neighbour in multiples >of 100kpbs >75 * (a value of '1' equals to 0.1Mbps, '10' equals 1Mbps, >etc). > 76 */ >77 static u32 batadv_v_elp_get_throughput(struct >batadv_hardif_neigh_node *neigh) > 78 { > 79 struct batadv_hard_iface *hard_iface = neigh->if_incoming; > 80 struct ethtool_link_ksettings link_settings; > 81 struct net_device *real_netdev; > 82 struct station_info *sinfo; > 83 u32 throughput; > 84 bool filled; > 85 int ret; > 86 >87 /* if the user specified a customised value for this interface, >then > 88 * return it directly > 89 */ > 90 throughput = atomic_read(&hard_iface->bat_v.throughput_override); > 91 if (throughput != 0) > 92 return throughput; > 93 > 94 /* if this is a wireless device, then ask its throughput through > 95 * cfg80211 API > 96 */ > 97 if (batadv_is_wifi_hardif(hard_iface)) { > 98 if (!batadv_is_cfg80211_hardif(hard_iface)) > 99 /* unsupported WiFi driver version */ > 100 goto default_throughput; > 101 > 102 real_netdev = batadv_get_real_netdev(hard_iface->net_dev); > 103 if (!real_netdev) > 104 goto default_throughput; > 105 > 106 sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL); > 107 if (!sinfo) > 108 goto default_throughput; > 109 > 110 ret = cfg80211_get_station(real_netdev, neigh->addr, sinfo); > 111 >112 /* just save these here instead of having complex free logic >below */ > > 113 throughput = sinfo.expected_throughput / 100; >114 filled = !!(sinfo.filled & >BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT)); > 115 > 116 kfree(sinfo); > 117 > 118 dev_put(real_netdev); > 119 if (ret == -ENOENT) { > 120 /* Node is not associated anymore! It would be > 121 * possible to delete this neighbor. For now set > 122 * the throughput metric to 0. > 123 */ > 124 return 0; > 125 } > 126 if (ret || !filled) > 127 goto default_throughput; > 128 > 129 return throughput; > 130 } > 131 >132 /* if not a wifi interface, check if this device provides data via > 133 * ethtool (e.g. an Ethernet adapter) > 134 */ > 135 memset(&link_settings, 0, sizeof(link_settings)); > 136 rtnl_lock(); >137 ret = __ethtool_get_link_ksettings(hard_iface->net_dev, >&link_settings); > 138 rtnl_unlock(); > 139 if (ret == 0) { > 140 /* link characteristics might change over time */ > 141 if (link_settings.base.duplex == DUPLEX_FULL) > 142 hard_iface->bat_v.flags |= BATADV_FULL_DUPLEX; > 143 else > 144 hard_iface->bat_v.flags &= ~BATADV_FULL_DUPLEX; > 145 > 146 throughput = link_settings.base.speed; > 147 if (throughput && throughput != SPEED_UNKNOWN) > 148 return throughput * 10; > 149 } > 150 > 151 default_throughput: > 152 if (!(hard_iface->bat_v.flags & BATADV_WARNING_DEFAULT)) { > 153 batadv_info(hard_iface->soft_iface, >154 "WiFi driver or ethtool info does not provide information >about link speeds on interface %s, therefore defaulting to hardcoded >throughput values of %u.%1u Mbps. Consider overriding the throughput >manually or checking your driver.\n", > 155 hard_iface->net_dev->name, > 156 BATADV_THROUGHPUT_DEFAULT_VALUE / 10, > 157 BATADV_THROUGHPUT_DEFAULT_VALUE % 10); > 158 hard_iface->bat_v.flags |= BATADV_WARNING_DEFAULT; > 159 } > 160 >161 /* if none of the above cases apply, return the base_throughput */ > 162 return BATADV_THROUGHPUT_DEFAULT_VALUE; > 163 } > 164 > >--- >0-DAY kernel test infrastructure Open Source Technology >Center >https://lists.01.org/pipermail/kbuild-all Intel >Corporation
Hi Toke,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v4.17-rc4 next-20180509]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/wireless-drivers-Dynamically-allocate-struct-station_info/20180510-034416
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: i386-randconfig-x0-05100327 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
net//batman-adv/bat_v_elp.c: In function 'batadv_v_elp_get_throughput':
>> net//batman-adv/bat_v_elp.c:113:21: error: request for member 'expected_throughput' in something not a structure or union
throughput = sinfo.expected_throughput / 100;
^
>> net//batman-adv/bat_v_elp.c:114:20: error: request for member 'filled' in something not a structure or union
filled = !!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT));
^
vim +/expected_throughput +113 net//batman-adv/bat_v_elp.c
69
70 /**
71 * batadv_v_elp_get_throughput() - get the throughput towards a neighbour
72 * @neigh: the neighbour for which the throughput has to be obtained
73 *
74 * Return: The throughput towards the given neighbour in multiples of 100kpbs
75 * (a value of '1' equals to 0.1Mbps, '10' equals 1Mbps, etc).
76 */
77 static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh)
78 {
79 struct batadv_hard_iface *hard_iface = neigh->if_incoming;
80 struct ethtool_link_ksettings link_settings;
81 struct net_device *real_netdev;
82 struct station_info *sinfo;
83 u32 throughput;
84 bool filled;
85 int ret;
86
87 /* if the user specified a customised value for this interface, then
88 * return it directly
89 */
90 throughput = atomic_read(&hard_iface->bat_v.throughput_override);
91 if (throughput != 0)
92 return throughput;
93
94 /* if this is a wireless device, then ask its throughput through
95 * cfg80211 API
96 */
97 if (batadv_is_wifi_hardif(hard_iface)) {
98 if (!batadv_is_cfg80211_hardif(hard_iface))
99 /* unsupported WiFi driver version */
100 goto default_throughput;
101
102 real_netdev = batadv_get_real_netdev(hard_iface->net_dev);
103 if (!real_netdev)
104 goto default_throughput;
105
106 sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
107 if (!sinfo)
108 goto default_throughput;
109
110 ret = cfg80211_get_station(real_netdev, neigh->addr, sinfo);
111
112 /* just save these here instead of having complex free logic below */
> 113 throughput = sinfo.expected_throughput / 100;
> 114 filled = !!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT));
115
116 kfree(sinfo);
117
118 dev_put(real_netdev);
119 if (ret == -ENOENT) {
120 /* Node is not associated anymore! It would be
121 * possible to delete this neighbor. For now set
122 * the throughput metric to 0.
123 */
124 return 0;
125 }
126 if (ret || !filled)
127 goto default_throughput;
128
129 return throughput;
130 }
131
132 /* if not a wifi interface, check if this device provides data via
133 * ethtool (e.g. an Ethernet adapter)
134 */
135 memset(&link_settings, 0, sizeof(link_settings));
136 rtnl_lock();
137 ret = __ethtool_get_link_ksettings(hard_iface->net_dev, &link_settings);
138 rtnl_unlock();
139 if (ret == 0) {
140 /* link characteristics might change over time */
141 if (link_settings.base.duplex == DUPLEX_FULL)
142 hard_iface->bat_v.flags |= BATADV_FULL_DUPLEX;
143 else
144 hard_iface->bat_v.flags &= ~BATADV_FULL_DUPLEX;
145
146 throughput = link_settings.base.speed;
147 if (throughput && throughput != SPEED_UNKNOWN)
148 return throughput * 10;
149 }
150
151 default_throughput:
152 if (!(hard_iface->bat_v.flags & BATADV_WARNING_DEFAULT)) {
153 batadv_info(hard_iface->soft_iface,
154 "WiFi driver or ethtool info does not provide information about link speeds on interface %s, therefore defaulting to hardcoded throughput values of %u.%1u Mbps. Consider overriding the throughput manually or checking your driver.\n",
155 hard_iface->net_dev->name,
156 BATADV_THROUGHPUT_DEFAULT_VALUE / 10,
157 BATADV_THROUGHPUT_DEFAULT_VALUE % 10);
158 hard_iface->bat_v.flags |= BATADV_WARNING_DEFAULT;
159 }
160
161 /* if none of the above cases apply, return the base_throughput */
162 return BATADV_THROUGHPUT_DEFAULT_VALUE;
163 }
164
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c index 28687493599f..d2393ebc6af4 100644 --- a/net/batman-adv/bat_v_elp.c +++ b/net/batman-adv/bat_v_elp.c @@ -79,8 +79,9 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) struct batadv_hard_iface *hard_iface = neigh->if_incoming; struct ethtool_link_ksettings link_settings; struct net_device *real_netdev; - struct station_info sinfo; + struct station_info *sinfo; u32 throughput; + bool filled; int ret; /* if the user specified a customised value for this interface, then @@ -102,7 +103,17 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) if (!real_netdev) goto default_throughput; - ret = cfg80211_get_station(real_netdev, neigh->addr, &sinfo); + sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL); + if (!sinfo) + goto default_throughput; + + ret = cfg80211_get_station(real_netdev, neigh->addr, sinfo); + + /* just save these here instead of having complex free logic below */ + throughput = sinfo.expected_throughput / 100; + filled = !!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT)); + + kfree(sinfo); dev_put(real_netdev); if (ret == -ENOENT) { @@ -112,12 +123,10 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) */ return 0; } - if (ret) - goto default_throughput; - if (!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT))) + if (ret || !filled) goto default_throughput; - return sinfo.expected_throughput / 100; + return throughput; } /* if not a wifi interface, check if this device provides data via diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c index 9e002df0f8d8..2038e3fb25fa 100644 --- a/net/wireless/wext-compat.c +++ b/net/wireless/wext-compat.c @@ -1300,7 +1300,7 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev) struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy); /* we are under RTNL - globally locked - so can use static structs */ static struct iw_statistics wstats; - static struct station_info sinfo; + static struct station_info *sinfo; u8 bssid[ETH_ALEN]; if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_STATION) @@ -1318,17 +1318,21 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev) memcpy(bssid, wdev->current_bss->pub.bssid, ETH_ALEN); wdev_unlock(wdev); - memset(&sinfo, 0, sizeof(sinfo)); + sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL); + if (!sinfo) + return NULL; - if (rdev_get_station(rdev, dev, bssid, &sinfo)) + if (rdev_get_station(rdev, dev, bssid, sinfo)) { + kfree(sinfo); return NULL; + } memset(&wstats, 0, sizeof(wstats)); switch (rdev->wiphy.signal_type) { case CFG80211_SIGNAL_TYPE_MBM: - if (sinfo.filled & BIT(NL80211_STA_INFO_SIGNAL)) { - int sig = sinfo.signal; + if (sinfo->filled & BIT(NL80211_STA_INFO_SIGNAL)) { + int sig = sinfo->signal; wstats.qual.updated |= IW_QUAL_LEVEL_UPDATED; wstats.qual.updated |= IW_QUAL_QUAL_UPDATED; wstats.qual.updated |= IW_QUAL_DBM; @@ -1341,11 +1345,11 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev) break; } case CFG80211_SIGNAL_TYPE_UNSPEC: - if (sinfo.filled & BIT(NL80211_STA_INFO_SIGNAL)) { + if (sinfo->filled & BIT(NL80211_STA_INFO_SIGNAL)) { wstats.qual.updated |= IW_QUAL_LEVEL_UPDATED; wstats.qual.updated |= IW_QUAL_QUAL_UPDATED; - wstats.qual.level = sinfo.signal; - wstats.qual.qual = sinfo.signal; + wstats.qual.level = sinfo->signal; + wstats.qual.qual = sinfo->signal; break; } default: @@ -1354,11 +1358,12 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev) } wstats.qual.updated |= IW_QUAL_NOISE_INVALID; - if (sinfo.filled & BIT(NL80211_STA_INFO_RX_DROP_MISC)) - wstats.discard.misc = sinfo.rx_dropped_misc; - if (sinfo.filled & BIT(NL80211_STA_INFO_TX_FAILED)) - wstats.discard.retries = sinfo.tx_failed; + if (sinfo->filled & BIT(NL80211_STA_INFO_RX_DROP_MISC)) + wstats.discard.misc = sinfo->rx_dropped_misc; + if (sinfo->filled & BIT(NL80211_STA_INFO_TX_FAILED)) + wstats.discard.retries = sinfo->tx_failed; + kfree(sinfo); return &wstats; }
Since the addition of the TXQ stats to cfg80211, the station_info struct has grown to be quite large, which results in warnings when allocated on the stack. Fix the affected places to do dynamic allocations instead. WARN_ON is used where the function has no way to signal errors to the caller. This patch applies the fix to batman-adv and wext-compat, while a separate patch fixes up the drivers. Fixes: 52539ca89f36 cfg80211: Expose TXQ stats and parameters to userspace Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- net/batman-adv/bat_v_elp.c | 21 +++++++++++++++------ net/wireless/wext-compat.c | 29 +++++++++++++++++------------ 2 files changed, 32 insertions(+), 18 deletions(-)