From: Johannes Berg <johannes@sipsolutions•net>
To: "Jouni Malinen" <j@w1•fi>, "João Paulo Rechi Vita" <jprvita@gmail•com>
Cc: "David S. Miller" <davem@davemloft•net>,
"Darren Hart" <dvhart@infradead•org>,
linux-wireless <linux-wireless@vger•kernel.org>,
"Network Development" <netdev@vger•kernel.org>,
platform-driver-x86@vger•kernel.org, linux-api@vger•kernel.org,
linux-doc@vger•kernel.org, LKML <linux-kernel@vger•kernel.org>,
linux@endlessm•com,
"João Paulo Rechi Vita" <jprvita@endlessm•com>
Subject: Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations
Date: Tue, 01 Mar 2016 14:43:07 +0100 [thread overview]
Message-ID: <1456839787.3926.20.camel@sipsolutions.net> (raw)
In-Reply-To: <20160229223918.GA32464@w1.fi>
On Tue, 2016-03-01 at 00:39 +0200, Jouni Malinen wrote:
> > I agree there is a difference in the logic here,
Gah. I thought I'd reviewed the logic and made sure there's no
difference ... :)
> > thanks for taking the
> > time to point it out so clearly, and sorry for missing this. But AFAIU
> > userspace should not call RFKILL_OP_CHANGE with ev.type ==
> > RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
> > block/unblock one RFKill switch, and it is not possible to create a
> > RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
> > return NULL).
> Interesting. Maybe Johannes can comment on that part since I think he
> wrote the code that interacts with kernel for the rfkill test cases.
So first of all, it seems that this argument is invalid since we can't break the ABI/API here; although perhaps if it's only a test case ...
Oh. It took me a while, but I see now. The original intent (I think)
was that with RFKILL_OP_CHANGE, the type would be ignored entirely. It
seems that the (my) original intent wouldn't have been to force
userspace to specify *both* the index and the type, but instead do
OP_CHANGE_ALL -> use type (possibly TYPE_ALL, ignoring idx)
OP_CHANGE -> use idx (ignoring type)
The original code implemented it as follows:
if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
continue;
-> check the idx only for OP_CHANGE
if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
continue;
-> check the type, allowing _ALL
Now, all userspace that I found sets the ev.type field to TYPE_ALL all
the time; and it had to given these checks.
e.g. from rfkill.py:
# idx, type, op, soft, hard
_event_struct = '@IBBBB'
[...]
def block(self):
rfk = open('/dev/rfkill', 'w')
s = struct.pack(_event_struct, self.idx, TYPE_ALL, _OP_CHANGE, 1, 0)
rfk.write(s)
rfk.close()
This check, originally, probably should've been
if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL &&
ev.op != RFKILL_OP_CHANGE)
continue;
to ignore the type entirely.
I'm fine with Jouni's change, preserving the original behaviour of
requiring TYPE_ALL or the correct type, but I'm tempted to simply
remove the type check entirely.
Thoughts?
johannes
next prev parent reply other threads:[~2016-03-01 13:43 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-22 16:36 [PATCHv2 00/10] RFKill airplane-mode indicator João Paulo Rechi Vita
2016-02-22 16:36 ` [PATCHv2 01/10] rfkill: Improve documentation language João Paulo Rechi Vita
[not found] ` <1456159001-20307-1-git-send-email-jprvita-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
2016-02-22 16:36 ` [PATCHv2 02/10] rfkill: Remove extra blank line João Paulo Rechi Vita
2016-02-22 16:36 ` [PATCHv2 03/10] rfkill: Point to the correct deprecated doc location João Paulo Rechi Vita
2016-02-22 16:36 ` [PATCHv2 04/10] rfkill: Move "state" sysfs file back to stable João Paulo Rechi Vita
2016-02-23 20:45 ` Pavel Machek
2016-02-22 16:36 ` [PATCHv2 05/10] rfkill: Factor rfkill_global_states[].cur assignments João Paulo Rechi Vita
2016-02-22 16:36 ` [PATCHv2 06/10] rfkill: Add documentation about LED triggers João Paulo Rechi Vita
2016-02-22 16:36 ` [PATCHv2 07/10] rfkill: Create "rfkill-airplane-mode" LED trigger João Paulo Rechi Vita
2016-02-22 16:36 ` [PATCHv2 08/10] rfkill: Use switch to demux userspace operations João Paulo Rechi Vita
[not found] ` <1456159001-20307-9-git-send-email-jprvita-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
2016-02-26 17:59 ` Jouni Malinen
2016-02-29 22:30 ` João Paulo Rechi Vita
2016-02-29 22:39 ` Jouni Malinen
2016-03-01 13:43 ` Johannes Berg [this message]
[not found] ` <1456839787.3926.20.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
2016-03-01 16:15 ` João Paulo Rechi Vita
2016-03-08 14:01 ` João Paulo Rechi Vita
2016-02-22 16:36 ` [PATCHv2 09/10] rfkill: Userspace control for airplane mode João Paulo Rechi Vita
2016-02-22 19:31 ` [PATCHv3] " João Paulo Rechi Vita
2016-02-23 20:45 ` [PATCHv2 09/10] " Pavel Machek
2016-02-23 20:55 ` Johannes Berg
2016-02-23 21:45 ` Pavel Machek
2016-02-24 9:01 ` Johannes Berg
2016-02-24 10:46 ` custom ioctl-based interface to control LED in networking (was Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode) Pavel Machek
2016-02-24 11:01 ` Johannes Berg
2016-02-24 12:14 ` Pavel Machek
2016-02-24 13:31 ` Johannes Berg
2016-02-25 9:06 ` Pavel Machek
2016-02-22 16:36 ` [PATCHv2 10/10] rfkill: Notify userspace of airplane-mode state changes João Paulo Rechi Vita
2016-02-22 17:00 ` [PATCHv2 00/10] RFKill airplane-mode indicator Dan Williams
2016-02-22 19:35 ` João Paulo Rechi Vita
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1456839787.3926.20.camel@sipsolutions.net \
--to=johannes@sipsolutions$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=dvhart@infradead$(echo .)org \
--cc=j@w1$(echo .)fi \
--cc=jprvita@endlessm$(echo .)com \
--cc=jprvita@gmail$(echo .)com \
--cc=linux-api@vger$(echo .)kernel.org \
--cc=linux-doc@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-wireless@vger$(echo .)kernel.org \
--cc=linux@endlessm$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=platform-driver-x86@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox