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 <eduardoabina...@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
>
_______________________________________________
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
