| Message ID | 1470916940-28035-1-git-send-email-eduardoabinader@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 2016-08-11 14:02, Eduardo Abinader wrote: > When netifd failed to load a valid configuration, after an invalid one, > it was not possible to setup the wireless device. This patch > aims to track this situation and behave acordingly, by keeping > track of failed setup without affecting autostart behavior. Also > block the restart of the wdev, when not applied. > > Signed-off-by: Eduardo Abinader <eduardoabinader@gmail.com> > --- > wireless.c | 26 ++++++++++++++++++++------ > wireless.h | 1 + > 2 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/wireless.c b/wireless.c > index 34dd328..1212a77 100644 > --- a/wireless.c > +++ b/wireless.c > @@ -287,7 +287,7 @@ __wireless_device_set_up(struct wireless_device *wdev) > if (wdev->disabled) > return; > > - if (wdev->state != IFS_DOWN || config_init) > + if ((wdev->config_state != IFC_RELOAD) && (wdev->state != IFS_DOWN || config_init)) > return; > > free(wdev->prev_config); This part looks like it should be dropped now. > @@ -313,11 +313,15 @@ wdev_handle_config_change(struct wireless_device *wdev) > > switch(state) { > case IFC_NORMAL: > - case IFC_RELOAD: > - wdev->config_state = IFC_NORMAL; > if (wdev->autostart) > __wireless_device_set_up(wdev); > break; > + case IFC_RELOAD: > + if (wdev->autostart || wdev->retry_setup_failed) > + __wireless_device_set_up(wdev); You can move the wdev->retry_setup_failed (and maybe even autostart) check to __wireless_device_set_up to avoid having to repeat it. > + > + wdev->config_state = IFC_NORMAL; > + break; > case IFC_REMOVE: > wireless_device_free(wdev); > break; > @@ -388,6 +392,13 @@ wireless_device_mark_up(struct wireless_device *wdev) > > D(WIRELESS, "Wireless device '%s' is now up\n", wdev->name); > wdev->state = IFS_UP; > + > + if (wdev->retry_setup_failed) { > + wdev->retry_setup_failed = false; > + > + /* a new chance is given, if a previous setup failed */ > + wdev->autostart = true; > + } > vlist_for_each_element(&wdev->interfaces, vif, node) > wireless_interface_handle_link(vif, true); > } > @@ -398,9 +409,10 @@ wireless_device_retry_setup(struct wireless_device *wdev) > if (wdev->state == IFS_TEARDOWN || wdev->state == IFS_DOWN || wdev->cancel) > return; > > - if (--wdev->retry < 0) > + if (--wdev->retry < 0) { > wdev->autostart = false; > - > + wdev->retry_setup_failed = true; Please remove the wdev->autostart = false assignment here, and also the wdev->autostart = true line in the chunk above. > @@ -681,6 +693,7 @@ wireless_device_create(struct wireless_driver *drv, const char *name, struct blo > wdev->config_state = IFC_NORMAL; > wdev->name = strcpy(name_buf, name); > wdev->config = data; > + wdev->retry_setup_failed = false; > wdev->config_autostart = true; > wdev->autostart = wdev->config_autostart; > INIT_LIST_HEAD(&wdev->script_proc); > @@ -991,6 +1004,7 @@ wireless_start_pending(void) > struct wireless_device *wdev; > > vlist_for_each_element(&wireless_devices, wdev, node) > - if (wdev->autostart) > + if (wdev->autostart && wdev->state == IFS_DOWN) Why did you make this change? - Felix
On 11.08.2016 14:16, Felix Fietkau wrote: > On 2016-08-11 14:02, Eduardo Abinader wrote: >> When netifd failed to load a valid configuration, after an invalid one, >> it was not possible to setup the wireless device. This patch >> aims to track this situation and behave acordingly, by keeping >> track of failed setup without affecting autostart behavior. Also >> block the restart of the wdev, when not applied. >> >> Signed-off-by: Eduardo Abinader <eduardoabinader@gmail.com> >> --- >> wireless.c | 26 ++++++++++++++++++++------ >> wireless.h | 1 + >> 2 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/wireless.c b/wireless.c >> index 34dd328..1212a77 100644 >> --- a/wireless.c >> +++ b/wireless.c >> @@ -287,7 +287,7 @@ __wireless_device_set_up(struct wireless_device *wdev) >> if (wdev->disabled) >> return; >> >> - if (wdev->state != IFS_DOWN || config_init) >> + if ((wdev->config_state != IFC_RELOAD) && (wdev->state != IFS_DOWN || config_init)) >> return; >> >> free(wdev->prev_config); > This part looks like it should be dropped now. > Based on your next comment, right? I mean, replace by the proper placement of earlier return of __wireless_device_set_up on reload/autostart/retry_setup, ok? >> @@ -313,11 +313,15 @@ wdev_handle_config_change(struct wireless_device *wdev) >> >> switch(state) { >> case IFC_NORMAL: >> - case IFC_RELOAD: >> - wdev->config_state = IFC_NORMAL; >> if (wdev->autostart) >> __wireless_device_set_up(wdev); >> break; >> + case IFC_RELOAD: >> + if (wdev->autostart || wdev->retry_setup_failed) >> + __wireless_device_set_up(wdev); > You can move the wdev->retry_setup_failed (and maybe even autostart) > check to __wireless_device_set_up to avoid having to repeat it. > >> + >> + wdev->config_state = IFC_NORMAL; >> + break; >> case IFC_REMOVE: >> wireless_device_free(wdev); >> break; >> @@ -388,6 +392,13 @@ wireless_device_mark_up(struct wireless_device *wdev) >> >> D(WIRELESS, "Wireless device '%s' is now up\n", wdev->name); >> wdev->state = IFS_UP; >> + >> + if (wdev->retry_setup_failed) { >> + wdev->retry_setup_failed = false; >> + >> + /* a new chance is given, if a previous setup failed */ >> + wdev->autostart = true; >> + } >> vlist_for_each_element(&wdev->interfaces, vif, node) >> wireless_interface_handle_link(vif, true); >> } > >> @@ -398,9 +409,10 @@ wireless_device_retry_setup(struct wireless_device *wdev) >> if (wdev->state == IFS_TEARDOWN || wdev->state == IFS_DOWN || wdev->cancel) >> return; >> >> - if (--wdev->retry < 0) >> + if (--wdev->retry < 0) { >> wdev->autostart = false; >> - >> + wdev->retry_setup_failed = true; > Please remove the wdev->autostart = false assignment here, and also the > wdev->autostart = true line in the chunk above. > >> @@ -681,6 +693,7 @@ wireless_device_create(struct wireless_driver *drv, const char *name, struct blo >> wdev->config_state = IFC_NORMAL; >> wdev->name = strcpy(name_buf, name); >> wdev->config = data; >> + wdev->retry_setup_failed = false; >> wdev->config_autostart = true; >> wdev->autostart = wdev->config_autostart; >> INIT_LIST_HEAD(&wdev->script_proc); >> @@ -991,6 +1004,7 @@ wireless_start_pending(void) >> struct wireless_device *wdev; >> >> vlist_for_each_element(&wireless_devices, wdev, node) >> - if (wdev->autostart) >> + if (wdev->autostart && wdev->state == IFS_DOWN) > Why did you make this change? > To avoid settinp up wdev while a teardown is still in progress, I noticed the previous hostapd instance was still running, so it seems more appropriate to defer it to wireless_device_check_script_tasks. Should it be a separate patch? > - Felix > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev >
On 2016-08-11 15:02, Eduardo Abinader wrote: > On 11.08.2016 14:16, Felix Fietkau wrote: >> On 2016-08-11 14:02, Eduardo Abinader wrote: >>> When netifd failed to load a valid configuration, after an invalid one, >>> it was not possible to setup the wireless device. This patch >>> aims to track this situation and behave acordingly, by keeping >>> track of failed setup without affecting autostart behavior. Also >>> block the restart of the wdev, when not applied. >>> >>> Signed-off-by: Eduardo Abinader <eduardoabinader@gmail.com> >>> --- >>> wireless.c | 26 ++++++++++++++++++++------ >>> wireless.h | 1 + >>> 2 files changed, 21 insertions(+), 6 deletions(-) >>> >>> diff --git a/wireless.c b/wireless.c >>> index 34dd328..1212a77 100644 >>> --- a/wireless.c >>> +++ b/wireless.c >>> @@ -287,7 +287,7 @@ __wireless_device_set_up(struct wireless_device *wdev) >>> if (wdev->disabled) >>> return; >>> >>> - if (wdev->state != IFS_DOWN || config_init) >>> + if ((wdev->config_state != IFC_RELOAD) && (wdev->state != IFS_DOWN || config_init)) >>> return; >>> >>> free(wdev->prev_config); >> This part looks like it should be dropped now. >> > > Based on your next comment, right? I mean, replace by the proper > placement of earlier return of __wireless_device_set_up on > reload/autostart/retry_setup, ok? Yes. >>> @@ -681,6 +693,7 @@ wireless_device_create(struct wireless_driver *drv, const char *name, struct blo >>> wdev->config_state = IFC_NORMAL; >>> wdev->name = strcpy(name_buf, name); >>> wdev->config = data; >>> + wdev->retry_setup_failed = false; >>> wdev->config_autostart = true; >>> wdev->autostart = wdev->config_autostart; >>> INIT_LIST_HEAD(&wdev->script_proc); >>> @@ -991,6 +1004,7 @@ wireless_start_pending(void) >>> struct wireless_device *wdev; >>> >>> vlist_for_each_element(&wireless_devices, wdev, node) >>> - if (wdev->autostart) >>> + if (wdev->autostart && wdev->state == IFS_DOWN) >> Why did you make this change? >> > > To avoid settinp up wdev while a teardown is still in progress, I > noticed the previous hostapd instance was still running, so it seems > more appropriate to defer it to wireless_device_check_script_tasks. > Should it be a separate patch? I think the issue you're trying to work around here is actually introduced by that other change above that I suggested removing. If that's the case, you can drop this one as well. - Felix
diff --git a/wireless.c b/wireless.c index 34dd328..1212a77 100644 --- a/wireless.c +++ b/wireless.c @@ -287,7 +287,7 @@ __wireless_device_set_up(struct wireless_device *wdev) if (wdev->disabled) return; - if (wdev->state != IFS_DOWN || config_init) + if ((wdev->config_state != IFC_RELOAD) && (wdev->state != IFS_DOWN || config_init)) return; free(wdev->prev_config); @@ -313,11 +313,15 @@ wdev_handle_config_change(struct wireless_device *wdev) switch(state) { case IFC_NORMAL: - case IFC_RELOAD: - wdev->config_state = IFC_NORMAL; if (wdev->autostart) __wireless_device_set_up(wdev); break; + case IFC_RELOAD: + if (wdev->autostart || wdev->retry_setup_failed) + __wireless_device_set_up(wdev); + + wdev->config_state = IFC_NORMAL; + break; case IFC_REMOVE: wireless_device_free(wdev); break; @@ -388,6 +392,13 @@ wireless_device_mark_up(struct wireless_device *wdev) D(WIRELESS, "Wireless device '%s' is now up\n", wdev->name); wdev->state = IFS_UP; + + if (wdev->retry_setup_failed) { + wdev->retry_setup_failed = false; + + /* a new chance is given, if a previous setup failed */ + wdev->autostart = true; + } vlist_for_each_element(&wdev->interfaces, vif, node) wireless_interface_handle_link(vif, true); } @@ -398,9 +409,10 @@ wireless_device_retry_setup(struct wireless_device *wdev) if (wdev->state == IFS_TEARDOWN || wdev->state == IFS_DOWN || wdev->cancel) return; - if (--wdev->retry < 0) + if (--wdev->retry < 0) { wdev->autostart = false; - + wdev->retry_setup_failed = true; + } __wireless_device_set_down(wdev); } @@ -681,6 +693,7 @@ wireless_device_create(struct wireless_driver *drv, const char *name, struct blo wdev->config_state = IFC_NORMAL; wdev->name = strcpy(name_buf, name); wdev->config = data; + wdev->retry_setup_failed = false; wdev->config_autostart = true; wdev->autostart = wdev->config_autostart; INIT_LIST_HEAD(&wdev->script_proc); @@ -991,6 +1004,7 @@ wireless_start_pending(void) struct wireless_device *wdev; vlist_for_each_element(&wireless_devices, wdev, node) - if (wdev->autostart) + if (wdev->autostart && wdev->state == IFS_DOWN) __wireless_device_set_up(wdev); + } diff --git a/wireless.h b/wireless.h index 665cdb7..403cc86 100644 --- a/wireless.h +++ b/wireless.h @@ -56,6 +56,7 @@ struct wireless_device { bool config_autostart; bool autostart; bool disabled; + bool retry_setup_failed; enum interface_state state; enum interface_config_state config_state;
When netifd failed to load a valid configuration, after an invalid one, it was not possible to setup the wireless device. This patch aims to track this situation and behave acordingly, by keeping track of failed setup without affecting autostart behavior. Also block the restart of the wdev, when not applied. Signed-off-by: Eduardo Abinader <eduardoabinader@gmail.com> --- wireless.c | 26 ++++++++++++++++++++------ wireless.h | 1 + 2 files changed, 21 insertions(+), 6 deletions(-)