| Message ID | 1429606392-7776-1-git-send-email-andrei.otcheretianski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
On Tue, 2015-04-21 at 11:53 +0300, andrei.otc@gmail.com wrote: > From: Andrei Otcheretianski <andrei.otcheretianski@intel.com> > > When a vif is assigned to a reserved channel context (during CSA, for example) This .. doesn't make much sense. vifs aren't assigned to channel contexts, it's the other way around. I guess I'll rewrite that to "When a vif starts using a reserved channel context (...)" > the width of this chanctx should be adjusted to be the maximum between the > reserved chandef and all the chandefs of other assigned vifs. This is not what you do - you don't take anything in the chanctx into account. ieee80211_chanctx_non_reserved_chandef() just recalculates the required chanctx, and you're fixing the code to actually apply the recalculated value. > Not doing so would result in using chanctx with narrower width than actually > required. Fix this by calling ieee80211_change_chanctx with the widest common > chandef. This both changes the chanctx's width and recalcs min_def. This seems possible, yeah. > chandef = ieee80211_chanctx_non_reserved_chandef(local, new_ctx, > &sdata->reserved_chandef); > if (WARN_ON(!chandef)) > return -EINVAL; > > + ieee80211_change_chanctx(local, new_ctx, chandef); > + > vif_chsw[0].vif = &sdata->vif; > vif_chsw[0].old_ctx = &old_ctx->conf; > vif_chsw[0].new_ctx = &new_ctx->conf; [...] > ieee80211_vif_update_chandef(sdata, &sdata->reserved_chandef); Hmm. (added more context) The code here seems to be wrong though. It shouldn't overwrite reserved_chandef, or it shouldn't call ieee80211_vif_update_chandef() with it, as the vif's bss_conf.chandef should represent what *this* vif wants/needs, while you're now putting there what the *combination* requires in the chandef. That's clearly wrong - please submit a new patch that fixes all the issues in these functions. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 24, 2015 at 1:41 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Tue, 2015-04-21 at 11:53 +0300, andrei.otc@gmail.com wrote: > > From: Andrei Otcheretianski <andrei.otcheretianski@intel.com> > > > > When a vif is assigned to a reserved channel context (during CSA, for example) > > This .. doesn't make much sense. vifs aren't assigned to channel > contexts, it's the other way around. I guess I'll rewrite that to "When > a vif starts using a reserved channel context (...)" > OK, thanks for clarification. > > the width of this chanctx should be adjusted to be the maximum between the > > reserved chandef and all the chandefs of other assigned vifs. > > This is not what you do - you don't take anything in the chanctx into > account. ieee80211_chanctx_non_reserved_chandef() just recalculates the > required chanctx, and you're fixing the code to actually apply the > recalculated value. > sdata->reserved_chandef is passed to ieee80211_chanctx_non_reserved_chandef, so it takes the reservation into account when the required chandef is recalculated. This is what I tried to say here. I'll rephrase to make it more clear. > > Not doing so would result in using chanctx with narrower width than actually > > required. Fix this by calling ieee80211_change_chanctx with the widest common > > chandef. This both changes the chanctx's width and recalcs min_def. > > This seems possible, yeah. > > > > chandef = ieee80211_chanctx_non_reserved_chandef(local, new_ctx, > > &sdata->reserved_chandef); > > > if (WARN_ON(!chandef)) > > return -EINVAL; > > > > + ieee80211_change_chanctx(local, new_ctx, chandef); > > + > > vif_chsw[0].vif = &sdata->vif; > > vif_chsw[0].old_ctx = &old_ctx->conf; > > vif_chsw[0].new_ctx = &new_ctx->conf; > [...] > > ieee80211_vif_update_chandef(sdata, &sdata->reserved_chandef); > > Hmm. (added more context) > > The code here seems to be wrong though. It shouldn't overwrite > reserved_chandef, or it shouldn't call ieee80211_vif_update_chandef() > with it, as the vif's bss_conf.chandef should represent what *this* vif > wants/needs, while you're now putting there what the *combination* > requires in the chandef. > > That's clearly wrong - please submit a new patch that fixes all the > issues in these functions. I don't see any issue here. sdata->reserved_chandef isn't overwritten and it is what sdata really needs. > > johannes > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2015-04-26 at 11:13 +0300, Andrei Otcheretianski wrote: > sdata->reserved_chandef is passed to > ieee80211_chanctx_non_reserved_chandef, so it takes the reservation > into account when the required chandef is recalculated. > This is what I tried to say here. I'll rephrase to make it more clear. Ok, thanks. > > > Not doing so would result in using chanctx with narrower width than actually > > > required. Fix this by calling ieee80211_change_chanctx with the widest common > > > chandef. This both changes the chanctx's width and recalcs min_def. > > > > This seems possible, yeah. > > > > > > > chandef = ieee80211_chanctx_non_reserved_chandef(local, new_ctx, > > > &sdata->reserved_chandef); > > > > > if (WARN_ON(!chandef)) > > > return -EINVAL; > > > > > > + ieee80211_change_chanctx(local, new_ctx, chandef); > > > + > > > vif_chsw[0].vif = &sdata->vif; > > > vif_chsw[0].old_ctx = &old_ctx->conf; > > > vif_chsw[0].new_ctx = &new_ctx->conf; > > [...] > > > ieee80211_vif_update_chandef(sdata, &sdata->reserved_chandef); > > > > Hmm. (added more context) > > > > The code here seems to be wrong though. It shouldn't overwrite > > reserved_chandef, or it shouldn't call ieee80211_vif_update_chandef() > > with it, as the vif's bss_conf.chandef should represent what *this* vif > > wants/needs, while you're now putting there what the *combination* > > requires in the chandef. > > > > That's clearly wrong - please submit a new patch that fixes all the > > issues in these functions. > > I don't see any issue here. sdata->reserved_chandef isn't overwritten > and it is what sdata really needs. Yes, good point, sorry. I confused the pointer assignments with struct assignments. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c index 5bcd4e5..0fd9274 100644 --- a/net/mac80211/chan.c +++ b/net/mac80211/chan.c @@ -1008,6 +1008,8 @@ ieee80211_vif_use_reserved_reassign(struct ieee80211_sub_if_data *sdata) if (WARN_ON(!chandef)) return -EINVAL; + ieee80211_change_chanctx(local, new_ctx, chandef); + vif_chsw[0].vif = &sdata->vif; vif_chsw[0].old_ctx = &old_ctx->conf; vif_chsw[0].new_ctx = &new_ctx->conf; @@ -1079,6 +1081,8 @@ ieee80211_vif_use_reserved_assign(struct ieee80211_sub_if_data *sdata) if (WARN_ON(!chandef)) return -EINVAL; + ieee80211_change_chanctx(local, new_ctx, chandef); + list_del(&sdata->reserved_chanctx_list); sdata->reserved_chanctx = NULL;