Which is exactly what should be done, yes. So far, I understood the
code correctly.
Where in the code do we change the *global* state for a switch type,
based on the state of a particular switch of that type? That's the step
I am missing.
Here's the timeline of how I think (so far, anyway) these whole thing
should work:
1. rfkill class registers with kernel
- all initial global switch states (there are NO switches registered
yet) are set to ON (current rfkill) or to either ON or OFF based on
rfkill module parameters (with the patch).
These are the "desired radio states for each radio type", and
are not related to whatever *real* state the hardware is in. Let's
call them "global switch state (per switch type)", or "global
switch states" for short.
These global switch states are usually manipulated by rfkill-input
when it traps input events that affect a certain type of switch
(and we should probably have a way to manipulate these global
states from userspace as well when rfkill-input is not loaded).
[user interaction to change these switch states MAY or MAY NOT happen
here, it doesn't matter. If it happens, the switch states CAN be
changed]
2. driver with rfkill support is loaded (coldplug, hotplug, etc)
2.1 driver allocates an rfkill structure
2.2 driver sets rfkill->state to the REAL state of the radio in
the hardware [optional for write-only devices] -- i'd prefer
the API made this mandatory by requesting this state as a
parameter of the rfkill_register() call.
2.3 driver registers rfkill interface with the rfkill class
2.3.1 rfkill class tries to set the radio state
(using toggle_radio() and updating rfkill->state if that
succeeds) to the CURRENT global switch state.
If we have something different than this, it is a bug. And I am not
clear what we should do in 2.3.1 WHEN the switch is a read-only switch,
but my guess is that we just ignore the failure, as trying to match all
other switches would break badly if there are more than one read-only
switches of the same type, and their states don't match.
Why should it? See below for my comments on centralizing this policy on
the "initial state of radios" on rfkill.
Which we don't have :(
We better make sure not to confuse the layers here.
We have a layer that deals with the switches themselves (rfkill), which
has NOTHING to do with keys at all. It lets one access the current
switch state, and modify it. It should also define the policy for what
the initial state of the switch should be, IMO.
Drivers like b43 and thinkpad-acpi are connected to the rfkill layer.
We have a layer that should be OPTIONAL, and that listen to input events
and manipulates the rfkill switches in semanthic groups ("switches for
the same type of hardware"). This is rfkill-input.
No drivers are connected to the rfkill-input layer directly. When a
driver can report something like EV_KEY KEY_RADIO or EV_SW SW_RADIO (and
the type-specific ones, like KEY_WLAN), it connects directly to the
input layer.
key X is registered to the INPUT LAYER... so this doesn't make sense,
or I am getting completely confused.
IMO, global state should NEVER be undefined. Setting an initial policy
for radios is a damn good idea. And if it is to be hardcoded, we
hardcode it to "radios OFF" which is always safe (but I'd just make it a
module parameter).
For write-only switches, you ALWAYS have to set them initially. If you
mean the driver needs to do it to whatever it has done to rfkill->state
BEFORE calling rfkill_register, I am fine with it, but we *NEED* to
document that and revise all drivers to make sure it is correct.
I don't mind adding a new UNDEFINED state, it would be very useful for
write-only switches which can be messed with by something other than the
kernel.
But I don't think it is a good idea at all to have "unknown" be allowed
into the GLOBAL state.
If we have an *initial* global state of "unknown", it will confuse
users, as that will make the effective initial state of an entire radio
type change during coldplug depending on the initial state of the first
device of that type registered, which is NOT deterministic at all.
Please let's go with either an OFF *initial* global state (safe), or a
user-seletable *initial* global state (module parameter, and I'd say the
best default would be OFF here too, but AFAIK, it is ON right now).
And there is absolutely no reason at all (as far as I can see, anyway)
to let anything set the global state to "unknown". If some radio
switch can't follow the global state (e.g. it is read-only), it simply
won't. If someone overrides the state of the switch (write to state
attribute) to be different from the global state, it will be brought
back in sync the next time the global state changes.
I just thought of a way to let userspace mess with the global states
when it wants to control an entire set of switches (and not just one).
I will send in a patch if you like the idea: basically, the rfkill
class module also registers a platform driver, and platform devices for
every type of switch. THOSE devices are used to control the global
state for each type of switch. This makes rfkill-input functionality
implementable in userspace, which is good.
Or, if we forbit an undefined global state to begin with (see above), we
just set it to the global state (which will be ON or OFF), have less
complex code, AND reduce user confusion...
Sure it is, if that module is the class that controls all the radio
switches (which rfkill is). Unless we now make each rfkill radio type
a module of its own, which is quite doable, but IMHO a lot of code
complexity and memory waste for very little gain.
How about we have a *single* initial global state parameter, but
disallow an "unknown" global state parameter? Less user confusion, less
module parameters, AND less code complexity.
Ok, I will put my brains and fingers into high gear and come up with a
new patchset, but first I have to look at input-pooldev to see if/how I
can use that for thinkpad-acpi, in order to re-think the read-write
support.
BTW: isn't it high time to add yourself as the RFKILL maintainer to
MAINTAINERS (couldn't find you in there, nor RFKILL for that matter) and
also to document the rfkill class in Documentation/ ?
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
--
