Subject: Re: [RFC PATCH v3 03/12] mac80211: IEEE 802.11 Extended Key ID
 support
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
References: <20190210210620.31181-1-alexander@wetzel-home.de>
 <20190210210620.31181-4-alexander@wetzel-home.de>
 <f4d8db23bc3804fdc98f2ce7a702215bd82fd4e3.camel@sipsolutions.net>
 <e27bffe8-4c75-34d9-b326-39ffb05b73eb@wetzel-home.de>
 <171d2db4-7639-4738-2a6d-c899f0247aee@wetzel-home.de>
 <767ada740d984e180d0ac799487722293a50367c.camel@sipsolutions.net>
From:   Alexander Wetzel <alexander@wetzel-home.de>
Message-ID: <95ba87e8-d7d2-53ae-f2c8-d4656dc1d29a@wetzel-home.de>
Date:   Fri, 1 Mar 2019 21:43:43 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <767ada740d984e180d0ac799487722293a50367c.camel@sipsolutions.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

>>> So here the reasoning for why I named them as they are and why I  prefer
>>> the names used in the patch.
>>> First, many drivers will handle SET_KEY and SET_KEY_RX_ONLY with the
>>> same code and not differentiate between those at all. Using EXT_as a
>>> prefix for the "normal" command is therefore a nice way to imply the
>>> command can only be used with Extended Key ID and still link it to the
>>> original command.
>>> But more important for me was the clash between what the command spells
>>> and what it does in the COMPAT mode: SET_KEY_RX_ONLY would then be used
>>> to install a TX only key which never can be used by the card for Rx.
>>> So I decided to rename it to EXT_SET_KEY, just indicating that this
>>> command adds a new key to the card for Extended Key ID and drop the
>>> confusing reference to Rx.
> Wait, what? In compat mode SET_KEY_RX_ONLY installs a TX-only key? Ah,
> you mean before you changed this.
> 
> Why don't we split out compat mode then?
> 
> But I see where you're coming from with the EXT_ now. I need to think of
> it less as an "extension" now, but as "extended key ID". I'm not really
> entirely sure that makes sense - even what we think of as "extended key
> ID" now might be the new normal soon? But then again the spec does the
> same thing.
> 
>>> Long story short: Using SET_KEY_RXONLY and SET_KEY_TX is not wrong, but
>>> I would rate them more confusing.
> Fair enough.

I've had second (or more like tenths..) thoughts about this API.
If you like any of the solutions below more than the others I'll use 
that in the next patch, of course...

In a nutshell, the point of EXT_SET_KEY is to install a key, prepare it 
for Tx but NOT use it for Tx. The driver can use it for Rx (NATIVE) or 
just do nothing with the (in COMPAT mode) Tx only key till mac80211 
starts using it for Tx later.

1) Assuming we keep the RX_ONLY key flag we could just drop EXT_SET_KEY 
and  use the "normal" SET_KEY with the flag moved to ieee80211_key_conf.
We really just want to install the key to the driver here, only drivers 
like ath10k need a way to figure out it must not be used for Tx.

2) We also can drop the Rx only flag/extra command and simply use 
SET_KEY. ath10k will then have to honor the keyID mac80211 prepared when 
it want to support Extended Key ID. (ath10k seems to need a firmware 
update either way, so that could be acceptable.)

That said I've now decided to simply name the calls in the next patch:
SET_KEY	
	-> no RX only flag!
SET_KEY_RX
	-> SET_KEY, only key must not be used for Tx
DISABLE_KEY,
	-> unchanged
ACTIVATE_KEY
	-> so far named EXT_KEY_RX_TX
DISABLE_KEY_RX
	-> so far named EXT_DISABLE_KEY_RX

We can then continue the discussion with a new patch and the other fixes 
in it when needed.
To really clean that up we would have to change the existing names to 
something like ADD_KEY, REMOVE_KEY. We could then name the new calls 
ADD_KEY_RX, ACTIVATE_KEY and DISABLE_KEY_RX. But that seems to be overkill.

Alexander

Alexander