| Message ID | 20180820073705.9683-1-johannes@sipsolutions.net (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule) | expand |
On Mon, Aug 20, 2018 at 09:37:05AM +0200, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Pointer arithmetic already adjusts by the size of the struct, > so the sizeof() calculation is wrong. This is basically the > same as Colin King's patch for similar code in the iwlwifi > driver. > > Fixes: 230ebaa189af ("cfg80211: read wmm rules from regulatory database") > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > net/wireless/reg.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/wireless/reg.c b/net/wireless/reg.c > index 4fc66a117b7d..283902974fbf 100644 > --- a/net/wireless/reg.c > +++ b/net/wireless/reg.c > @@ -452,8 +452,7 @@ reg_copy_regd(const struct ieee80211_regdomain *src_regd) > continue; > > regd->reg_rules[i].wmm_rule = d_wmm + > - (src_regd->reg_rules[i].wmm_rule - s_wmm) / > - sizeof(struct ieee80211_wmm_rule); > + (src_regd->reg_rules[i].wmm_rule - s_wmm); > } > return regd; > } As side note those pointer aritmetics related with rule->wmm_rule is really involuted in various places in reg.c . Seems would be better to just make wmm_rule part of iee80211_reg_rule structure like this: struct ieee80211_reg_rule { struct ieee80211_freq_range freq_range; struct ieee80211_power_rule power_rule; struct ieee80211_wmm_rule wmm_rule; u32 flags; u32 dfs_cac_ms; }; and use a flag to intdicate wmm_rule is valid. There should be no big memory overhead sice there are only few reg_rules for the regdomain. Or I'm wrong? Regards Stanislaw
On 20.08.2018 12:27, Stanislaw Gruszka wrote: > On Mon, Aug 20, 2018 at 09:37:05AM +0200, Johannes Berg wrote: >> From: Johannes Berg <johannes.berg@intel.com> >> >> Pointer arithmetic already adjusts by the size of the struct, >> so the sizeof() calculation is wrong. This is basically the >> same as Colin King's patch for similar code in the iwlwifi >> driver. >> >> Fixes: 230ebaa189af ("cfg80211: read wmm rules from regulatory database") >> Signed-off-by: Johannes Berg <johannes.berg@intel.com> >> --- >> net/wireless/reg.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c >> index 4fc66a117b7d..283902974fbf 100644 >> --- a/net/wireless/reg.c >> +++ b/net/wireless/reg.c >> @@ -452,8 +452,7 @@ reg_copy_regd(const struct ieee80211_regdomain *src_regd) >> continue; >> >> regd->reg_rules[i].wmm_rule = d_wmm + >> - (src_regd->reg_rules[i].wmm_rule - s_wmm) / >> - sizeof(struct ieee80211_wmm_rule); >> + (src_regd->reg_rules[i].wmm_rule - s_wmm); >> } >> return regd; >> } > As side note those pointer aritmetics related with rule->wmm_rule is > really involuted in various places in reg.c . Seems would be better to > just make wmm_rule part of iee80211_reg_rule structure like this: > > struct ieee80211_reg_rule { > struct ieee80211_freq_range freq_range; > struct ieee80211_power_rule power_rule; > struct ieee80211_wmm_rule wmm_rule; > u32 flags; > u32 dfs_cac_ms; > }; > > and use a flag to intdicate wmm_rule is valid. There should be no > big memory overhead sice there are only few reg_rules for the regdomain. > Or I'm wrong? > > Regards > Stanislaw Thank you for the patch. I've applied the patch, but it does not seem to help. Spawning second instance of hostapd causes same error as before. I don't follow why this issue would not occur with single NIC. It's not like I am an expert in this field, but those rules should be applied to both interfaces, right? Both interfaces use ath10k, that should imply same code path for setting rules.
On 21.08.2018 08:23, Grzegorz Duszyński wrote: > On 20.08.2018 12:27, Stanislaw Gruszka wrote: >> On Mon, Aug 20, 2018 at 09:37:05AM +0200, Johannes Berg wrote: >>> From: Johannes Berg <johannes.berg@intel.com> >>> >>> Pointer arithmetic already adjusts by the size of the struct, >>> so the sizeof() calculation is wrong. This is basically the >>> same as Colin King's patch for similar code in the iwlwifi >>> driver. >>> >>> Fixes: 230ebaa189af ("cfg80211: read wmm rules from regulatory >>> database") >>> Signed-off-by: Johannes Berg <johannes.berg@intel.com> >>> --- >>> net/wireless/reg.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c >>> index 4fc66a117b7d..283902974fbf 100644 >>> --- a/net/wireless/reg.c >>> +++ b/net/wireless/reg.c >>> @@ -452,8 +452,7 @@ reg_copy_regd(const struct ieee80211_regdomain >>> *src_regd) >>> continue; >>> regd->reg_rules[i].wmm_rule = d_wmm + >>> - (src_regd->reg_rules[i].wmm_rule - s_wmm) / >>> - sizeof(struct ieee80211_wmm_rule); >>> + (src_regd->reg_rules[i].wmm_rule - s_wmm); >>> } >>> return regd; >>> } >> As side note those pointer aritmetics related with rule->wmm_rule is >> really involuted in various places in reg.c . Seems would be better to >> just make wmm_rule part of iee80211_reg_rule structure like this: >> >> struct ieee80211_reg_rule { >> struct ieee80211_freq_range freq_range; >> struct ieee80211_power_rule power_rule; >> struct ieee80211_wmm_rule wmm_rule; >> u32 flags; >> u32 dfs_cac_ms; >> }; >> >> and use a flag to intdicate wmm_rule is valid. There should be no >> big memory overhead sice there are only few reg_rules for the regdomain. >> Or I'm wrong? >> >> Regards >> Stanislaw > > Thank you for the patch. > > I've applied the patch, but it does not seem to help. > Spawning second instance of hostapd causes same error as before. > > I don't follow why this issue would not occur with single NIC. > It's not like I am an expert in this field, but those rules should be > applied to both interfaces, right? > Both interfaces use ath10k, that should imply same code path for > setting rules. > Issue has also been experienced by other Arch user: https://bbs.archlinux.org/viewtopic.php?pid=1803445#p1803445 User chr0mag also reports that LTS version of Arch kernel does not exhibit this issue.
On Tue, 2018-08-21 at 08:23 +0200, Grzegorz Duszyński wrote: > > I've applied the patch, but it does not seem to help. > Spawning second instance of hostapd causes same error as before. Ok, it was worth a try. > I don't follow why this issue would not occur with single NIC. > It's not like I am an expert in this field, but those rules should be > applied to both interfaces, right? > Both interfaces use ath10k, that should imply same code path for setting > rules. Yeah that's why I thought maybe the copy code was involved ... I guess we'll need to try to reproduce. Maybe it can be made to happen with hwsim too? johannes
On 21.08.2018 11:03, Johannes Berg wrote: > On Tue, 2018-08-21 at 08:23 +0200, Grzegorz Duszyński wrote: >> I've applied the patch, but it does not seem to help. >> Spawning second instance of hostapd causes same error as before. > Ok, it was worth a try. > >> I don't follow why this issue would not occur with single NIC. >> It's not like I am an expert in this field, but those rules should be >> applied to both interfaces, right? >> Both interfaces use ath10k, that should imply same code path for setting >> rules. > Yeah that's why I thought maybe the copy code was involved ... > > I guess we'll need to try to reproduce. Maybe it can be made to happen > with hwsim too? > > johannes I can try to recreate the issue with hwsim, it will take me some time probably as I have no prior experience.
> I can try to recreate the issue with hwsim, it will take me some time > probably as I have no prior experience. No worries, I'm compiling it now. Do you have a special hostapd configuration? johannes
On 21.08.2018 11:07, Johannes Berg wrote: >> I can try to recreate the issue with hwsim, it will take me some time >> probably as I have no prior experience. > No worries, I'm compiling it now. Do you have a special hostapd > configuration? > > johannes I think they are pretty standard: Compex wle900vx: https://pastebin.com/pkThTvWH Killer 1535 - didn't have a chance to finish this one https://pastebin.com/eJ3t36ra
diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 4fc66a117b7d..283902974fbf 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -452,8 +452,7 @@ reg_copy_regd(const struct ieee80211_regdomain *src_regd) continue; regd->reg_rules[i].wmm_rule = d_wmm + - (src_regd->reg_rules[i].wmm_rule - s_wmm) / - sizeof(struct ieee80211_wmm_rule); + (src_regd->reg_rules[i].wmm_rule - s_wmm); } return regd; }