[LEDE-DEV] [PATCH 1/1 V3] netifd: track when wdev setup fails

[Eduardo Abinader]

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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
>