| Message ID | 20231012211203.529730-1-gasmibal@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | CSA MBSS: CHAN_SWITCH is always failing. | expand |
Hi, On Thu, 2023-10-12 at 23:12 +0200, Baligh Gasmi wrote: > When asking hostapd to switch channel in the multi-BSS configuration, > only the first (associated to the first BSS) request is executed > correctly by some driver, other BSS's request are failing. > This is because multi-BSS configuration is using same Radio and one > request seems to be suffisant to driver to switch on. > > Solution in here is to return SUCCESS if there was at least one BSS > switched OK. I think this solution may confuse the internal state a bit as hostapd_switch_channel resets some state on failure and also sets csa_in_progress on success. I am thinking right now that two things might make sense: 1. Either splitting channel switch into three steps, or always switching the BSSs together. 2. Only doing a single call to the driver interface with the information for all BSSs. After all, the switch will need to happen at the same point in time. But, I don't really know right now what underlying drivers are expecting, so maybe the suggestion is just wrong. Another solution could be that the driver detects that the switch is already in progress and ensures a success return if appropriate. Benjamin > > Signed-off-by: Baligh Gasmi <gasmibal@gmail.com> > --- > hostapd/ctrl_iface.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c > index f90eb22c4..cf3d65172 100644 > --- a/hostapd/ctrl_iface.c > +++ b/hostapd/ctrl_iface.c > @@ -2694,6 +2694,8 @@ static int > hostapd_ctrl_iface_chan_switch(struct hostapd_iface *iface, > return 0; > } > > + int one_success = 0; > + int last_fail = ret; > for (i = 0; i < iface->num_bss; i++) { > > /* Save CHAN_SWITCH VHT, HE, and EHT config */ > @@ -2704,11 +2706,13 @@ static int > hostapd_ctrl_iface_chan_switch(struct hostapd_iface *iface, > if (ret) { > /* FIX: What do we do if CSA fails in the > middle of > * submitting multi-BSS CSA requests? */ > - return ret; > + last_fail = ret; > + } else { > + one_success = 1; > } > } > > - return 0; > + return one_success ? 0 : last_fail; > #else /* NEED_AP_MLME */ > return -1; > #endif /* NEED_AP_MLME */
> > Hi, > > On Thu, 2023-10-12 at 23:12 +0200, Baligh Gasmi wrote: > > When asking hostapd to switch channel in the multi-BSS configuration, > > only the first (associated to the first BSS) request is executed > > correctly by some driver, other BSS's request are failing. > > This is because multi-BSS configuration is using same Radio and one > > request seems to be suffisant to driver to switch on. > > > > Solution in here is to return SUCCESS if there was at least one BSS > > switched OK. > > I think this solution may confuse the internal state a bit as > hostapd_switch_channel resets some state on failure and also sets > csa_in_progress on success. > > I am thinking right now that two things might make sense: > 1. Either splitting channel switch into three steps, or always > switching the BSSs together. > 2. Only doing a single call to the driver interface with the > information for all BSSs. After all, the switch will need to > happen at the same point in time. > > But, I don't really know right now what underlying drivers are > expecting, so maybe the suggestion is just wrong. Another solution > could be that the driver detects that the switch is already in progress > and ensures a success return if appropriate. > > Benjamin > Hello, Sorry, but I do not get it! I'm not saying that the patch is correct or to be used as it, trying just to understand. With the patch, when hostapd_switch_channel() fails, that means that no switch on any BSS has succussed, so may the reset of the state is coherent. Other way, if it successed with one BSS (some or all), that means that the CSA is ongoing, and having csa_in_progress is coherent too. I can think about adding CHAN_SWITCH argument, according to which we use all BSSes or only the main BSS, but still not global solution I think, since we will have to detect driver behaviour to add/remove new arg. Baligh
On Fri, 2023-10-13 at 12:07 +0200, Baligh GASMI wrote: > > > > Hi, > > > > On Thu, 2023-10-12 at 23:12 +0200, Baligh Gasmi wrote: > > > When asking hostapd to switch channel in the multi-BSS > > > configuration, > > > only the first (associated to the first BSS) request is executed > > > correctly by some driver, other BSS's request are failing. > > > This is because multi-BSS configuration is using same Radio and > > > one > > > request seems to be suffisant to driver to switch on. > > > > > > Solution in here is to return SUCCESS if there was at least one > > > BSS > > > switched OK. > > > > I think this solution may confuse the internal state a bit as > > hostapd_switch_channel resets some state on failure and also sets > > csa_in_progress on success. > > > > I am thinking right now that two things might make sense: > > 1. Either splitting channel switch into three steps, or always > > switching the BSSs together. > > 2. Only doing a single call to the driver interface with the > > information for all BSSs. After all, the switch will need to > > happen at the same point in time. > > > > But, I don't really know right now what underlying drivers are > > expecting, so maybe the suggestion is just wrong. Another solution > > could be that the driver detects that the switch is already in > > progress > > and ensures a success return if appropriate. > > > > Benjamin > > > > Hello, > > Sorry, but I do not get it! > I'm not saying that the patch is correct or to be used as it, trying > just to understand. > > With the patch, when hostapd_switch_channel() fails, that means that > no switch on any BSS has succussed, so may the reset of the state is > coherent. > Other way, if it successed with one BSS (some or all), that means that > the CSA is ongoing, and having csa_in_progress is coherent too. I was talking about the internal hostapd state. i.e.: int hostapd_switch_channel(struct hostapd_data *hapd, struct csa_settings *settings) { int ret; ... ret = hostapd_drv_switch_channel(hapd, settings); free_beacon_data(&settings->beacon_csa); free_beacon_data(&settings->beacon_after); if (ret) { /* if we failed, clean cs parameters */ hostapd_cleanup_cs_params(hapd); return ret; } hapd->csa_in_progress = 1; return 0; } If hostapd_drv_switch_channel fails (which you expect to happen), then csa_in_progress is not set to 1 and hostapd_cleanup_cs_params is called. Both of this seem like an undesired side-effect. Benjamin > > I can think about adding CHAN_SWITCH argument, according to which we > use all BSSes or only the main BSS, but still not global solution I > think, since we will have to detect driver behaviour to add/remove > new > arg. > > Baligh >
> > On Fri, 2023-10-13 at 12:07 +0200, Baligh GASMI wrote: > > > > > > Hi, > > > > > > On Thu, 2023-10-12 at 23:12 +0200, Baligh Gasmi wrote: > > > > When asking hostapd to switch channel in the multi-BSS > > > > configuration, > > > > only the first (associated to the first BSS) request is executed > > > > correctly by some driver, other BSS's request are failing. > > > > This is because multi-BSS configuration is using same Radio and > > > > one > > > > request seems to be suffisant to driver to switch on. > > > > > > > > Solution in here is to return SUCCESS if there was at least one > > > > BSS > > > > switched OK. > > > > > > I think this solution may confuse the internal state a bit as > > > hostapd_switch_channel resets some state on failure and also sets > > > csa_in_progress on success. > > > > > > I am thinking right now that two things might make sense: > > > 1. Either splitting channel switch into three steps, or always > > > switching the BSSs together. > > > 2. Only doing a single call to the driver interface with the > > > information for all BSSs. After all, the switch will need to > > > happen at the same point in time. > > > > > > But, I don't really know right now what underlying drivers are > > > expecting, so maybe the suggestion is just wrong. Another solution > > > could be that the driver detects that the switch is already in > > > progress > > > and ensures a success return if appropriate. > > > > > > Benjamin > > > > > > > Hello, > > > > Sorry, but I do not get it! > > I'm not saying that the patch is correct or to be used as it, trying > > just to understand. > > > > With the patch, when hostapd_switch_channel() fails, that means that > > no switch on any BSS has succussed, so may the reset of the state is > > coherent. > > Other way, if it successed with one BSS (some or all), that means that > > the CSA is ongoing, and having csa_in_progress is coherent too. > > I was talking about the internal hostapd state. i.e.: > > int hostapd_switch_channel(struct hostapd_data *hapd, > struct csa_settings *settings) > { > int ret; > ... > ret = hostapd_drv_switch_channel(hapd, settings); > free_beacon_data(&settings->beacon_csa); > free_beacon_data(&settings->beacon_after); > > if (ret) { > /* if we failed, clean cs parameters */ > hostapd_cleanup_cs_params(hapd); > return ret; > } > > hapd->csa_in_progress = 1; > return 0; > } > > If hostapd_drv_switch_channel fails (which you expect to happen), then > csa_in_progress is not set to 1 and hostapd_cleanup_cs_params is > called. csa_in_progress is per BSS, so if the driver fails on one BSS, it will not impact other. right? > > > Both of this seem like an undesired side-effect. > > Benjamin > > > > > I can think about adding CHAN_SWITCH argument, according to which we > > use all BSSes or only the main BSS, but still not global solution I > > think, since we will have to detect driver behaviour to add/remove > > new > > arg. > > > > Baligh > > >
Hi, On Fri, 2023-10-13 at 12:33 +0200, Baligh GASMI wrote: > > > [SNIP] > > > > I was talking about the internal hostapd state. i.e.: > > > > int hostapd_switch_channel(struct hostapd_data *hapd, > > struct csa_settings *settings) > > { > > int ret; > > ... > > ret = hostapd_drv_switch_channel(hapd, settings); > > free_beacon_data(&settings->beacon_csa); > > free_beacon_data(&settings->beacon_after); > > > > if (ret) { > > /* if we failed, clean cs parameters */ > > hostapd_cleanup_cs_params(hapd); > > return ret; > > } > > > > hapd->csa_in_progress = 1; > > return 0; > > } > > > > If hostapd_drv_switch_channel fails (which you expect to happen), then > > csa_in_progress is not set to 1 and hostapd_cleanup_cs_params is > > called. > > csa_in_progress is per BSS, so if the driver fails on one BSS, it will > not impact other. > right? Yes, csa_in_progress would be set for the first BSS and not the others. I guess hapd_csa_in_progress actually checks all BSSs, so things might work fine for the most part? It may be acceptable to ignore the error on the other BSSs (at least if they are non-transmitting ones, otherwise we might need to update the beacon?). I don't want to comment on whether that is right or not. It just doesn't sit quite right to me to ignore the error in this way. If simply ignoring it is what you want to do, then wouldn't it be possible to do so inside the driver code? Benjamin > > > > > > Both of this seem like an undesired side-effect. > > > > Benjamin > > > > > > > > I can think about adding CHAN_SWITCH argument, according to which > > > we > > > use all BSSes or only the main BSS, but still not global solution > > > I > > > think, since we will have to detect driver behaviour to > > > add/remove > > > new > > > arg. > > > > > > Baligh > > > > > >
Hello, It appears that a similar patch has already been applied." Thanks. Le ven. 13 oct. 2023 à 15:00, Benjamin Berg <benjamin@sipsolutions.net> a écrit : > > Hi, > > On Fri, 2023-10-13 at 12:33 +0200, Baligh GASMI wrote: > > > > [SNIP] > > > > > > I was talking about the internal hostapd state. i.e.: > > > > > > int hostapd_switch_channel(struct hostapd_data *hapd, > > > struct csa_settings *settings) > > > { > > > int ret; > > > ... > > > ret = hostapd_drv_switch_channel(hapd, settings); > > > free_beacon_data(&settings->beacon_csa); > > > free_beacon_data(&settings->beacon_after); > > > > > > if (ret) { > > > /* if we failed, clean cs parameters */ > > > hostapd_cleanup_cs_params(hapd); > > > return ret; > > > } > > > > > > hapd->csa_in_progress = 1; > > > return 0; > > > } > > > > > > If hostapd_drv_switch_channel fails (which you expect to happen), then > > > csa_in_progress is not set to 1 and hostapd_cleanup_cs_params is > > > called. > > > > csa_in_progress is per BSS, so if the driver fails on one BSS, it will > > not impact other. > > right? > > Yes, csa_in_progress would be set for the first BSS and not the others. > I guess hapd_csa_in_progress actually checks all BSSs, so things might > work fine for the most part? > > It may be acceptable to ignore the error on the other BSSs (at least if > they are non-transmitting ones, otherwise we might need to update the > beacon?). I don't want to comment on whether that is right or not. > > It just doesn't sit quite right to me to ignore the error in this way. > If simply ignoring it is what you want to do, then wouldn't it be > possible to do so inside the driver code? > > Benjamin > > > > > > > > > > Both of this seem like an undesired side-effect. > > > > > > Benjamin > > > > > > > > > > > I can think about adding CHAN_SWITCH argument, according to which > > > > we > > > > use all BSSes or only the main BSS, but still not global solution > > > > I > > > > think, since we will have to detect driver behaviour to > > > > add/remove > > > > new > > > > arg. > > > > > > > > Baligh > > > > > > > > > >
diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c index f90eb22c4..cf3d65172 100644 --- a/hostapd/ctrl_iface.c +++ b/hostapd/ctrl_iface.c @@ -2694,6 +2694,8 @@ static int hostapd_ctrl_iface_chan_switch(struct hostapd_iface *iface, return 0; } + int one_success = 0; + int last_fail = ret; for (i = 0; i < iface->num_bss; i++) { /* Save CHAN_SWITCH VHT, HE, and EHT config */ @@ -2704,11 +2706,13 @@ static int hostapd_ctrl_iface_chan_switch(struct hostapd_iface *iface, if (ret) { /* FIX: What do we do if CSA fails in the middle of * submitting multi-BSS CSA requests? */ - return ret; + last_fail = ret; + } else { + one_success = 1; } } - return 0; + return one_success ? 0 : last_fail; #else /* NEED_AP_MLME */ return -1; #endif /* NEED_AP_MLME */
When asking hostapd to switch channel in the multi-BSS configuration, only the first (associated to the first BSS) request is executed correctly by some driver, other BSS's request are failing. This is because multi-BSS configuration is using same Radio and one request seems to be suffisant to driver to switch on. Solution in here is to return SUCCESS if there was at least one BSS switched OK. Signed-off-by: Baligh Gasmi <gasmibal@gmail.com> --- hostapd/ctrl_iface.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)