Hi Alok,
On Sat, Feb 19, 2011 at 01:07:37AM +0200, Alok Barsode wrote:
> From: Alok Barsode <alok.bars...@nokia.com>
>
> There is no need to cache rfkill events, since Connman cannot keep track of
> individual devices anyway. A Technology's init state depends on global
> offline mode rather then the 1st detected rfkill switch.
> Connman's rfkill mechanism tries to keep all the devices in a technology
> in either OFFLINE or in AVAILABLE state. Also accidently blocking or
> unblocking a device, is safefuarded by trapping the rfkill update event
> and then taking the right action.
That's actually not what we intend to do. A user unblocking a previously
rfkilled device should be interpreted as turning a technology on.
This is precisely why we are keeping track of rfkill ids and devices
associated with a technology.
> diff --git a/src/rfkill.c b/src/rfkill.c
> index 2a4458b..3b55cc3 100644
> --- a/src/rfkill.c
> +++ b/src/rfkill.c
> @@ -41,6 +41,7 @@ enum rfkill_type {
> RFKILL_TYPE_WWAN,
> RFKILL_TYPE_GPS,
> RFKILL_TYPE_FM,
> + NUM_RFKILL_TYPES,
Let's call it RFKILL_TYPE_MAX.
> +/* Block a device by index */
> +int __connman_rfkill_block_id(unsigned int index, gboolean block)
__connman_rfkill_block_device(), please.
> +int __connman_rfkill_block_technology(enum connman_service_type type,
> gboolean block)
>
over 80 chars.
> +{
> + struct rfkill_event event;
> + GIOStatus status;
> + gsize len;
> + uint8_t rfkill_type;
> +
> + if (channel == NULL) {
> + connman_error("No RFKILL control device");
> + return -EIO;
> + }
> +
> + rfkill_type = get_type(type);
> + if (rfkill_type == NUM_RFKILL_TYPES)
> + return -EIO;
> +
> + memset(&event, 0, sizeof(event));
> +
> + event.op = RFKILL_OP_CHANGE_ALL;
> + event.type = rfkill_type;
> + event.soft = block;
> +
> + status = g_io_channel_write_chars(channel, (gchar *) &event,
> + sizeof(struct rfkill_event), &len, NULL);
> +
> + if (status != G_IO_STATUS_NORMAL ||
> + len != sizeof(struct rfkill_event)) {
> + connman_error("Failed to change RFKILL state");
> + return FALSE;
> + }
> +
> + return TRUE;
You either fix your prototype to return a bool, or you do return an int. I'd
prefer the latter.
> int __connman_technology_update_rfkill(unsigned int index,
> + enum connman_service_type type,
> connman_bool_t softblock,
> connman_bool_t hardblock)
> {
> struct connman_technology *technology;
> - struct connman_rfkill *rfkill;
> - connman_bool_t blocked, old_blocked;
> + connman_bool_t blocked;
> + connman_bool_t offlinemode = __connman_profile_get_offlinemode();
>
> - DBG("index %u soft %u hard %u", index, softblock, hardblock);
> + DBG("rfkill update: index %u type %d soft %u hard %u", index, type,
> + softblock, hardblock);
>
> - technology = g_hash_table_lookup(rfkill_table, &index);
> + technology = technology_get(type);
> if (technology == NULL)
> return -ENXIO;
>
> - rfkill = g_hash_table_lookup(technology->rfkill_list, &index);
> - if (rfkill == NULL)
> - return -ENXIO;
> -
> - old_blocked = (rfkill->softblock || rfkill->hardblock) ? TRUE : FALSE;
> blocked = (softblock || hardblock) ? TRUE : FALSE;
>
> - rfkill->softblock = softblock;
> - rfkill->hardblock = hardblock;
> -
> - if (blocked == old_blocked)
> + if (offlinemode == blocked)
> return 0;
>
> - if (blocked) {
> - guint n_blocked;
> + if (offlinemode && !blocked)
> + __connman_rfkill_block_id(index, TRUE);
>
> - n_blocked =
> - g_atomic_int_exchange_and_add(&technology->blocked, 1);
> - if (n_blocked != g_hash_table_size(technology->rfkill_list) - 1)
> - return 0;
> -
> - technology_blocked(technology, blocked);
> - technology->state = CONNMAN_TECHNOLOGY_STATE_OFFLINE;
> - state_changed(technology);
> - } else {
> - if (g_atomic_int_dec_and_test(&technology->blocked) == FALSE)
> - return 0;
> -
> - technology_blocked(technology, blocked);
> - technology->state = CONNMAN_TECHNOLOGY_STATE_AVAILABLE;
> - state_changed(technology);
> - }
> + if (!offlinemode && blocked)
> + __connman_rfkill_block_id(index, FALSE);
So the logic here is incorrect:
- if (offlinemode && !blocked) should enable the technology.
- if (!offlinemode && blocked) should disable the technology if that's the
only device associated with it.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
