Hi Alok,
On Sat, Feb 19, 2011 at 01:07:33AM +0200, Alok Barsode wrote:
> From: Alok Barsode <alok.bars...@nokia.com>
>
> Use __connman_device_[enable/disable] to store the peristent states
> of device instead.
With the current behaviour, we intentionally store the device persistent state
only when called from manager.EnableTechnology. Does this patch follow this
requirement ?
> connman_device_set_powered is the asyc func called by the plugin
> on successfully changing the power state. We also change technology state
> after successfully changing the power state.
I like the idea of keeping those 2 separated, but I'm not sure that the new
code handles all the use cases properly:
> @@ -188,7 +188,6 @@ enum connman_service_type
> __connman_device_get_service_type(struct connman_devic
> int __connman_device_enable(struct connman_device *device)
> {
> int err;
> - enum connman_service_type type;
>
> DBG("device %p %d", device, device->blocked);
>
> @@ -201,28 +200,17 @@ int __connman_device_enable(struct connman_device
> *device)
> if (device->blocked == TRUE)
> return -ENOLINK;
>
> + device->powered_persistent = TRUE;
> + __connman_storage_save_device(device);
> +
> connman_device_set_disconnected(device, FALSE);
> device->scanning = FALSE;
>
> - err = device->driver->enable(device);
> - if (err < 0 && err != -EALREADY) {
> - if (err == -EINPROGRESS) {
> - device->powered_pending = TRUE;
> - device->offlinemode = FALSE;
> - if (__connman_profile_get_offlinemode() == TRUE)
> - __connman_profile_set_offlinemode(FALSE, FALSE);
> - }
> - return err;
> - }
> -
> device->powered_pending = TRUE;
> - device->powered = TRUE;
> - device->offlinemode = FALSE;
> - if (__connman_profile_get_offlinemode() == TRUE)
> - __connman_profile_set_offlinemode(FALSE, FALSE);
>
> - type = __connman_device_get_service_type(device);
> - __connman_technology_enable(type);
> + err = device->driver->enable(device);
> + if (err < 0 && err != -EINPROGRESS)
> + return err;
So, if device->driver->enable() failed, then you'll still have
device->powered_pending set to TRUE.
Also if device->driver->enable() returned 0, then you know that the device is
actually on so you can safely set device->powered to TRUE.
> @@ -249,20 +236,14 @@ int __connman_device_disable(struct connman_device
> *device)
>
> g_hash_table_remove_all(device->networks);
>
> - err = device->driver->disable(device);
> - if (err < 0 && err != -EALREADY) {
> - if (err == -EINPROGRESS)
> - device->powered_pending = FALSE;
> - return err;
> - }
> -
> - device->connections = 0;
> + device->powered_persistent = FALSE;
> + __connman_storage_save_device(device);
>
> device->powered_pending = FALSE;
> - device->powered = FALSE;
>
> - type = __connman_device_get_service_type(device);
> - __connman_technology_disable(type);
> + err = device->driver->disable(device);
> + if (err < 0 && err != -EINPROGRESS)
> + return err;
>
> return 0;
> }
> @@ -283,8 +264,7 @@ static int setup_device(struct connman_device *device)
>
> __connman_technology_add_device(device);
>
> - if (device->offlinemode == FALSE &&
> - device->powered_persistent == TRUE)
> + if (device->powered_persistent == TRUE)
> __connman_device_enable(device);
We will actually power the device up even though device->offlinemode is TRUE.
Why ?
> @@ -636,49 +616,44 @@ const char *connman_device_get_ident(struct
> connman_device *device)
> *
> * Change power state of device
> */
> -int connman_device_set_powered(struct connman_device *device,
> +void connman_device_set_powered(struct connman_device *device,
> connman_bool_t powered)
> {
> - int err;
> enum connman_service_type type;
> + connman_bool_t offlinemode = __connman_profile_get_offlinemode();
>
> DBG("driver %p powered %d", device, powered);
>
> - if (device->powered == powered) {
> - device->powered_pending = powered;
> - return -EALREADY;
> - }
> + /* This is to safeguard against power toggles.
> + We always process only the last request */
> + if (device->powered_pending != powered)
> + return;
Here if powered_pending is TRUE, but the technology plugin actually tells you
that the device is off, how are you handling it ?
> @@ -685,9 +685,9 @@ int __connman_technology_enable(enum connman_service_type
> type)
> if (g_atomic_int_get(&technology->blocked))
> return -ERFKILL;
>
> - __connman_notifier_enable(type);
> -
> if (g_atomic_int_exchange_and_add(&technology->enabled, 1) == 0) {
> + __connman_notifier_enable(type);
> +
> technology->state = CONNMAN_TECHNOLOGY_STATE_ENABLED;
> state_changed(technology);
> }
I don't see how this chunk is related to this patch.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
