From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753941AbcCANnU (ORCPT ); Tue, 1 Mar 2016 08:43:20 -0500 Received: from s3.sipsolutions.net ([5.9.151.49]:35095 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752636AbcCANnQ (ORCPT ); Tue, 1 Mar 2016 08:43:16 -0500 Message-ID: <1456839787.3926.20.camel@sipsolutions.net> Subject: Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations From: Johannes Berg To: Jouni Malinen , =?ISO-8859-1?Q?Jo=E3o?= Paulo Rechi Vita Cc: "David S. Miller" , Darren Hart , linux-wireless , Network Development , platform-driver-x86@vger.kernel.org, linux-api@vger.kernel.org, linux-doc@vger.kernel.org, LKML , linux@endlessm.com, =?ISO-8859-1?Q?Jo=E3o?= Paulo Rechi Vita Date: Tue, 01 Mar 2016 14:43:07 +0100 In-Reply-To: <20160229223918.GA32464@w1.fi> References: <1456159001-20307-1-git-send-email-jprvita@endlessm.com> <1456159001-20307-9-git-send-email-jprvita@endlessm.com> <20160226175925.GA9331@w1.fi> <20160229223918.GA32464@w1.fi> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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