Skip to content
92 changes: 61 additions & 31 deletions source/core/wifi_ctrl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1973,58 +1973,88 @@ void start_wifi_sched_timer(unsigned int index, wifi_ctrl_t *l_ctrl, wifi_schedu
}
}

void hotspot_cfg_sem_signal(bool status)
static void wifi_sem_param_init(wifi_sem_param_t *sem_param)
{
wifi_ctrl_t *ctrl = NULL;

ctrl = (wifi_ctrl_t *)get_wifictrl_obj();
if (sem_param && !sem_param->is_init) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (sem_param && !sem_param->is_init) {

move checking if (!sem_param -> is_init) to call place, in 2005 line.
checking whether sem_param exists doesnt make sense, since this is struct being part of object - it will always exist.

pthread_condattr_t cond_attr;
pthread_condattr_init(&cond_attr);
pthread_condattr_setclock(&cond_attr, CLOCK_MONOTONIC);
pthread_cond_init(&sem_param->cond, &cond_attr);
pthread_condattr_destroy(&cond_attr);
pthread_mutex_init(&sem_param->lock, NULL);
sem_param->is_init = true;
sem_param->cfg_status = false;
}
}

if (ctrl->hotspot_sem_param.is_init == true) {
pthread_mutex_lock(&ctrl->hotspot_sem_param.lock);
ctrl->hotspot_sem_param.cfg_status = status;
pthread_cond_signal(&ctrl->hotspot_sem_param.cond);
pthread_mutex_unlock(&ctrl->hotspot_sem_param.lock);
static void wifi_sem_param_destroy(wifi_sem_param_t *sem_param)
{
if (sem_param && sem_param->is_init) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above with sem_param_init please.

pthread_mutex_destroy(&sem_param->lock);
pthread_cond_destroy(&sem_param->cond);
sem_param->is_init = false;
}
}

bool hotspot_cfg_sem_wait_duration(uint32_t time_in_sec)
static bool wifi_sem_param_wait(wifi_sem_param_t *sem_param, uint32_t time_in_sec)
{
struct timespec ts;
int ret;
bool status = false;

wifi_ctrl_t *ctrl = NULL;
wifi_sem_param_init(sem_param);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wifi_sem_param_init(sem_param);
if (!sem_param->is_init) wifi_sem_param_init(sem_param);

(together with removal of the if condition from wifi_sem_param_init. )
That way, one unnecessary function call will be spared making the code a bit faster.


ctrl = (wifi_ctrl_t *)get_wifictrl_obj();
if (ctrl->hotspot_sem_param.is_init == false) {
pthread_condattr_t cond_attr;
pthread_condattr_init(&cond_attr);
pthread_condattr_setclock(&cond_attr, CLOCK_MONOTONIC);
pthread_cond_init(&ctrl->hotspot_sem_param.cond, &cond_attr);
pthread_condattr_destroy(&cond_attr);
pthread_mutex_init(&ctrl->hotspot_sem_param.lock, NULL);
ctrl->hotspot_sem_param.is_init = true;
}
clock_gettime(CLOCK_MONOTONIC, &ts);

/* Add wait duration*/
ts.tv_sec += time_in_sec;

pthread_mutex_lock(&ctrl->hotspot_sem_param.lock);
ret = pthread_cond_timedwait(&ctrl->hotspot_sem_param.cond, &ctrl->hotspot_sem_param.lock, &ts);
pthread_mutex_lock(&sem_param->lock);
ret = pthread_cond_timedwait(&sem_param->cond, &sem_param->lock, &ts);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can fail. Will extra info why it help will not be helpful in this case? or is it too rare?

if (ret == 0) {
status = ctrl->hotspot_sem_param.cfg_status;
status = sem_param->cfg_status;
}
pthread_mutex_unlock(&sem_param->lock);

pthread_mutex_unlock(&ctrl->hotspot_sem_param.lock);

ctrl->hotspot_sem_param.is_init = false;
pthread_mutex_destroy(&ctrl->hotspot_sem_param.lock);
pthread_cond_destroy(&ctrl->hotspot_sem_param.cond);
wifi_sem_param_destroy(sem_param);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in line 2005:

Suggested change
wifi_sem_param_destroy(sem_param);
if (sem_param->is_init) wifi_sem_param_destroy(sem_param);


return status;
}

static void wifi_sem_param_signal(wifi_sem_param_t *sem_param, bool status)
{
if (sem_param && sem_param->is_init) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as with _init and _destroy methods, at least remove first part of condition

pthread_mutex_lock(&sem_param->lock);
sem_param->cfg_status = status;
pthread_cond_signal(&sem_param->cond);
pthread_mutex_unlock(&sem_param->lock);
}
}

void managed_wifi_cfg_sem_signal(bool status)
{
wifi_ctrl_t *ctrl = get_wifictrl_obj();
wifi_util_info_print(WIFI_CTRL, "%s:%d - status:%d\n", __func__, __LINE__, status);
wifi_sem_param_signal(&ctrl->managed_wifi_sem_param, status);
}

bool managed_wifi_cfg_sem_wait_duration(uint32_t time_in_sec)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add declaration for this and other function used in other translation nodes to wifi_ctrl.h

{
wifi_ctrl_t *ctrl = get_wifictrl_obj();
wifi_util_info_print(WIFI_CTRL, "%s:%d - time_in_sec:%d\n", __func__, __LINE__, time_in_sec);
return wifi_sem_param_wait(&ctrl->managed_wifi_sem_param, time_in_sec);
}

bool hotspot_cfg_sem_wait_duration(uint32_t time_in_sec)
{
wifi_ctrl_t *ctrl = get_wifictrl_obj();
return wifi_sem_param_wait(&ctrl->hotspot_sem_param, time_in_sec);
}

void hotspot_cfg_sem_signal(bool status)
{
wifi_ctrl_t *ctrl = get_wifictrl_obj();
wifi_sem_param_signal(&ctrl->hotspot_sem_param, status);
}

void stop_wifi_sched_timer(unsigned int index, wifi_ctrl_t *l_ctrl, wifi_scheduler_type_t type)
{
int *handler_id;
Expand Down
11 changes: 7 additions & 4 deletions source/core/wifi_ctrl.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ extern "C" {

#define MAX_HOTSPOT_BLOB_SET_TIMEOUT 100
#define MAX_WEBCONFIG_HOTSPOT_BLOB_SET_TIMEOUT 120
#define MAX_MANAGED_WIFI_BLOB_SET_TIMEOUT 30
#define MAX_VAP_RE_CFG_APPLY_RETRY 2

//This is a dummy string if the value is not passed.
Expand Down Expand Up @@ -210,12 +211,12 @@ typedef struct {
pthread_mutex_t events_bus_lock;
} events_bus_data_t;

typedef struct hotspot_cfg_sem_param {
typedef struct {
bool is_init;
pthread_mutex_t lock;
pthread_cond_t cond;
bool cfg_status;
} hotspot_cfg_sem_param_t;
} wifi_sem_param_t;

typedef struct wifi_ctrl {
bool exit_ctrl;
Expand Down Expand Up @@ -263,10 +264,10 @@ typedef struct wifi_ctrl {
int speed_test_timeout;
int speed_test_running;
events_bus_data_t events_bus_data;
hotspot_cfg_sem_param_t hotspot_sem_param;
wifi_sem_param_t hotspot_sem_param;
wifi_sem_param_t managed_wifi_sem_param;
} wifi_ctrl_t;


typedef struct {
mac_address_t sta_mac;
int reason;
Expand Down Expand Up @@ -402,6 +403,8 @@ int webconfig_send_full_associate_status(wifi_ctrl_t *ctrl);

bool hotspot_cfg_sem_wait_duration(uint32_t time_in_sec);
void hotspot_cfg_sem_signal(bool status);
bool managed_wifi_sem_wait_duration(uint32_t time_in_sec);
void managed_wifi_sem_signal(bool status);
Comment on lines +406 to +407
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are definitions for these?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lines 2032 + 2039 in wifi_ctrl.c

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should name them the same way: managed_wifi_cfg_sem_wait_duration and managed_wifi_cfg_sem_signal


#ifdef __cplusplus
}
Expand Down
4 changes: 4 additions & 0 deletions source/core/wifi_ctrl_webconfig.c
Original file line number Diff line number Diff line change
Expand Up @@ -2370,6 +2370,10 @@ webconfig_error_t webconfig_ctrl_apply(webconfig_subdoc_t *doc, webconfig_subdoc
ctrl->webconfig_state |= ctrl_webconfig_state_vap_lnf_cfg_rsp_pending;
webconfig_analytic_event_data_to_hal_apply(data);
ret = webconfig_hal_lnf_vap_apply(ctrl, &data->u.decoded);
bool status = ((ret == RETURN_OK) ? true : false);
wifi_util_info_print(WIFI_CTRL, "%s:%d lnf blob cfg status:%d\n", __func__,
__LINE__, ret);
managed_wifi_cfg_sem_signal(status);
}
}
break;
Expand Down
63 changes: 53 additions & 10 deletions source/core/wifi_multidoc_webconfig.c
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ size_t wifi_vap_cfg_timeout_handler()
return MAX_WEBCONFIG_HOTSPOT_BLOB_SET_TIMEOUT;
}

static size_t webconf_managed_wifi_timeout_handler()
{
wifi_util_info_print(WIFI_CTRL, "%s: Enter max blob timeout value:%d\n", __func__,
MAX_MANAGED_WIFI_BLOB_SET_TIMEOUT);
return MAX_MANAGED_WIFI_BLOB_SET_TIMEOUT;
}

int wifi_vap_cfg_rollback_handler()
{
wifi_util_info_print(WIFI_CTRL, "%s: Enter\n", __func__);
Expand Down Expand Up @@ -1343,21 +1350,49 @@ static int push_blob_data(webconfig_subdoc_data_t *data, webconfig_subdoc_type_t
{
char *str;
wifi_ctrl_t *ctrl = (wifi_ctrl_t *)get_wifictrl_obj();
bool ret = true;

if (webconfig_encode(&ctrl->webconfig, data, subdoc_type) != webconfig_error_none) {
wifi_util_error_print(WIFI_CTRL, "%s:%d - Failed webconfig_encode for subdoc type %d\n", __FUNCTION__, __LINE__, subdoc_type);
wifi_util_error_print(WIFI_CTRL, "%s:%d - Failed webconfig_encode for subdoc type %d\n",
__FUNCTION__, __LINE__, subdoc_type);
return RETURN_ERR;
}

str = data->u.encoded.raw;
wifi_util_dbg_print(WIFI_CTRL, "%s:%d: Encoded blob:\n%s\n", __func__, __LINE__, str);
push_event_to_ctrl_queue(str, strlen(str), wifi_event_type_webconfig, wifi_event_webconfig_set_data_webconfig, NULL);
bool ret_value = hotspot_cfg_sem_wait_duration(MAX_HOTSPOT_BLOB_SET_TIMEOUT);
if (ret_value == false) {
wifi_util_error_print(WIFI_CTRL, "%s:%d WebConfig blob apply is failed\n", __func__,
__LINE__);
wifi_util_dbg_print(WIFI_CTRL, "%s:%d:Encoded blob:\n%s\n", __func__, __LINE__, str);
push_event_to_ctrl_queue(str, strlen(str), wifi_event_type_webconfig,
wifi_event_webconfig_set_data_webconfig, NULL);

if (subdoc_type == webconfig_subdoc_type_lnf) {
wifi_util_info_print(WIFI_CTRL,
"%s:%d: Doing a thread wait for the managed wifi blob for %d seconds\n", __func__,
__LINE__, MAX_MANAGED_WIFI_BLOB_SET_TIMEOUT);
ret = managed_wifi_cfg_sem_wait_duration(MAX_MANAGED_WIFI_BLOB_SET_TIMEOUT);

if (ret == true) {
wifi_util_info_print(WIFI_CTRL, "%s:%d Managed wifi blob applied successfully\n",
__func__, __LINE__);
webconfig_data_free(data);
return RETURN_OK;
} else {
wifi_util_error_print(WIFI_CTRL, "%s:%d Managed wifi blob failed, exiting\n", __func__,
__LINE__);
webconfig_data_free(data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as there are no special circumstances, please use one exit point from function.
setting int ret_value = return_XX at the end of the block improves readibility significantly.

return RETURN_TIMEDOUT;
}
} else if (subdoc_type == webconfig_subdoc_type_xfinity) {
wifi_util_dbg_print(WIFI_CTRL, "%s:%d Not managed wifi, proceeding with hotspot wait\n",
__func__, __LINE__);

ret = hotspot_cfg_sem_wait_duration(MAX_HOTSPOT_BLOB_SET_TIMEOUT);
if (ret == false) {
wifi_util_error_print(WIFI_CTRL, "%s:%d Hotspot blob apply failed\n", __func__,
__LINE__);
webconfig_data_free(data);
return RETURN_TIMEDOUT;
}
webconfig_data_free(data);
return RETURN_ERR;
return RETURN_OK;
}
webconfig_data_free(data);
return RETURN_OK;
Expand Down Expand Up @@ -1438,12 +1473,19 @@ static int connected_subdoc_handler(void *blob, void *amenities_blob, char *vap_
execRetVal->ErrorCode = VALIDATION_FALIED;
goto done;
}
if (push_blob_data(data, subdoc_type) != RETURN_OK) {
ret = push_blob_data(data, subdoc_type);
if (ret == RETURN_ERR) {
execRetVal->ErrorCode = WIFI_HAL_FAILURE;
strncpy(execRetVal->ErrorMsg, "push_blob_to_ctrl_queue failed", sizeof(execRetVal->ErrorMsg)-1);
wifi_util_error_print(WIFI_CTRL, "%s: failed to encode %s subdoc\n", \
__func__, (subdoc_type == webconfig_subdoc_type_lnf) ? "lnf_psk" : "xfinity");
goto done;
} else if(ret == RETURN_TIMEDOUT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code style

} else if (ret == RETURN_TIMEDOUT) {

execRetVal->ErrorCode = BLOB_EXECUTION_TIMEDOUT;
strncpy(execRetVal->ErrorMsg, "subdoc apply is failed", sizeof(execRetVal->ErrorMsg)-1);
wifi_util_error_print(WIFI_CTRL, "%s:%d WebConfig blob apply is failed:%s\n", __func__,
__LINE__, execRetVal->ErrorMsg);
goto done;
}

if (strcmp(vap_prefix,"lnf_psk")== 0) {
Expand Down Expand Up @@ -1930,13 +1972,14 @@ int register_multicomp_subdocs()
if ( strcmp(multiCompDataPointer->multi_comp_subdoc,"hotspot") == 0 )
{
multiCompDataPointer->executeBlobRequest = wifi_vap_cfg_subdoc_handler;
multiCompDataPointer->calcTimeout = wifi_vap_cfg_timeout_handler;
}
else if ( strcmp(multiCompDataPointer->multi_comp_subdoc,"connectedbuilding") == 0 )
{
multiCompDataPointer->executeBlobRequest = webconf_process_managed_subdoc;
multiCompDataPointer->calcTimeout = webconf_managed_wifi_timeout_handler;
}

multiCompDataPointer->calcTimeout = wifi_vap_cfg_timeout_handler;
multiCompDataPointer->rollbackFunc = wifi_vap_cfg_rollback_handler;
multiCompDataPointer->freeResources = NULL;
multiCompDataPointer++ ;
Expand Down
Loading