Re: [PATCH 8/9] rfkill: Userspace control for airplane mode

On 8 February 2016 at 17:53, Julian Calaby [off-list ref] wrote:

quoted

+       if (ev.op == RFKILL_OP_AIRPLANE_MODE_RELEASE) {
+               if (rfkill_apm_owned && !data->is_apm_owner) {

Are you sure this is correct?

In the case that airplane mode isn't owned, the
rfkill_apm_led_trigger_event() call will be a noop, so we should
arguably not be calling it.

Ok, I'm changing the check to be consistent with _CHANGE, so the call
only succeeds if (rfkill_apm_owned && data->is_apm_owner), and return
an error otherwise.

Also, should we just fail silently if we're not the owner? I.e. what
does userspace learn from this op failing and is that useful?

I think it is better to return an error every time userspace is trying
to call an operation that it was not supposed to call at a certain
state (without acquiring control of the airplane-mode indicator). If a
program has a logic error that makes it call _RELEASE without calling
_ACQUIRE first, it's easier for the programmer to spot the problem if
we return an error here.

quoted

+                       count = -EACCES;
+               } else {
+                       bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur;
+
+                       rfkill_apm_owned = false;
+                       data->is_apm_owner = false;
+                       rfkill_apm_led_trigger_event(state);
+               }
+       }
+
+       if (ev.op == RFKILL_OP_AIRPLANE_MODE_CHANGE) {
+               if (rfkill_apm_owned && data->is_apm_owner)
+                       rfkill_apm_led_trigger_event(ev.soft);
+               else
+                       count = -EACCES;
+       }
+
        if (ev.op == RFKILL_OP_CHANGE_ALL)
                rfkill_update_global_state(ev.type, ev.soft);

@@ -1230,7 +1261,17 @@ static int rfkill_fop_release(struct inode *inode, struct file *file)
        struct rfkill_int_event *ev, *tmp;

        mutex_lock(&rfkill_global_mutex);
+
+       if (data->is_apm_owner) {
+               bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur;
+
+               rfkill_apm_owned = false;
+               data->is_apm_owner = false;
+               rfkill_apm_led_trigger_event(state);

Also, this code is duplicated from the _RELEASE op above. Would it
make sense to factor it out into a separate function?

Yes, makes sense. This also made me notice I was assigning a negative
value to a size_t variable (count).

quoted

+       }
+
        list_del(&data->list);
+

(extra line)

After factoring out the _RELEASE code it looks better without this
additional line.

quoted

        mutex_unlock(&rfkill_global_mutex);

        mutex_destroy(&data->mtx);

Thanks,

Thanks for the review, Julian. I'm sending an updated version.

--
João Paulo Rechi Vita
http://about.me/jprvita