Hi Samuel,
Thanks for your comments. My inline response.
On Tue, Mar 1, 2011 at 2:12 AM, Samuel Ortiz <sa...@linux.intel.com> wrote:
> 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 ?
Yes.
>
>> 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.
Yup. Ill fix this by resetting device->powered_pending. Thanks for
catching this.
> 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.
Yup. I'll fix this too by invoking connman_device_set_powered()
accordingly. Thanks for catching this one too.
>
>> @@ -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 ?
yeah, this is wrong. will fix this one as well.
>
>> @@ -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 ?
I am not. So we are left in a unrecoverable situation. How about we
don't check "powered"
against device->powered_pending? and just set the device state to
powered? This would mean
we always respect what the plugin tells us.
>
>
>> @@ -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.
Actually it isn't. I don't really remember why i put this one here.
its a valid one though.
__connman_notifier_enable() incremented enabled[type] unconditionally.
It should be
done just once. i remember having some trouble with this. Should i put
this one in
another patch ?
>
> Cheers,
> Samuel.
>
Thanks!
Cheers,
Alok.
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
>
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
