Marcel Holtmann wrote:
Hi Alan,
I really don't understand why this is needed. What benefit does it give
us compared to just sent OP_CHANGE and OP_CHANGE as an update. My X200
for example does this anyway on suspend/resume.
This is required for boot only. I have no reason for this event to be
generated on resume.
The same effect could be had by generating an OP_CHANGE on f_open,
_only_ when a platform driver has provided a value from NVS. But it
does seem clearer to make it a different type of event.
that is my whole point. If the kernel driver wants to preserve these
then it can just issue OP_ADD to notify use about the current state of
the values. The OP_ADD gets send once you open /dev/rfkill and if they
not match with our policy we have to change them anyway. I really don't
see the need for an extra operation here. Let me ask this again, how is
it different from just sending the OP_ADD and then let rfkilld decide to
either use that value or enforce its own policy.
I don't understand.
Isn't OP_ADD generated for all rfkill devices, by the core itself? How
does the driver ask to generate this event or not?
As I understand it, the difference between NVS_REPORT and ADD is that
ADD is generated for all drivers. NVS_REPORT is only generated if they
call rfkill_set_global_sw_state().
The difference is that this exposes an extra bit of information: whether
the state was restored from NVS.
If the wish is to
enforce policy you can't do anything about it anyway.
Sure.
So what is rfkilld suppose to be doing when receiving this report? What
is the expected behavior? Why do we bother with multi-OS crap here? I am
really unclear what are we trying to solve here.
In order to replicate the kernel behavior, it is expected that you set
your internal state from this event. E.g. when the user next presses
the wireless toggle key, you set the inverse of that internal state.
Since this event is generated by a platform driver, you can expect it to
be present following coldplug (the udev initscript). If the event is
not present after coldplug, you may then issue OP_CHANGE yourself, to
e.g. restore the state from a file. You would not be expected to handle
OP_NVS_REPORT after startup. (Unless the daemon is restarted).
Replicating the kernel behavior will allow us to avoid causing a couple
of niggly little regressions on at least two platforms. It preserves
the behavior when dual-booting (possibly between different linux
distros), and when the BIOS setup screen exposes the NVS state as an
option. The new behavior you suggest will annoy any users who have
become used to these scenarios "just working".
You may not use these platforms yourself. But I'm as annoyed as
Henrique is, we don't want to impose regressions just because other
platforms don't implement the feature.
Why the fuss about implementing this, it seems easy enough? Start
rfkilld after udev (like everything else). If you get NVS_REPORT, then
use those states. Fill in any other states from defaults or state files
and issue OP_CHANGE for them, just as you're already planning. Ignore
any subsequent NVS_REPORTs. That should cover it.
It's the cost for starting from a working implementation. You benefit
from having existing drivers and users, you pay by not breaking them
without good reason.
I really don't care about current behavior, because that has been just a
hack anyway.
Linux exports a stable ABI. A reasonable justification would be "this
is trivial and has practically no users who care". Hackishness is
second to that.
And it happens to work if there is proper BIOS support. We
are at the point now where we stop working around a complicated and in
some cases broken implementation. Overloading it with weird special
cases is just wrong and so far I am not buying into any of the arguments
here. The point behind the whole effort from Johannes is to finally fix
RFKILL support. If it breaks current behavior, I couldn't care less in
some cases.
I really don't understand. You say above that we don't need this
change, because drivers can already achieve the same event by
selectively generating OP_ADD. In that case it's already overloaded
with "weird special cases", and NVS_REPORT is a cosmetic difference only.
So Johannes and I talked about it a lot last week. And we will be doing
rfkilld so finally deprecated the broken idea of rfkill-input and move
the policy into userspace where it belongs.
Great. I think we can agree on one thing, that implementing
rfkill-input in the kernel without the possibility of a userspace
override is warped and twisted :-).
To make this clear, the
concept of cross-OS state keeping is broken.
Having the BIOS or a
different OS dictate policy makes no sense.
Seems to me these are two different points, and you haven't really said
why cross-OS state is broken.
It doesn't make sense to be _dictated_ to by your other OS/BIOS. That's
exactly what rfkilld would avoid, by allowing a configurable policy.
"support cross-OS state keeping" is a policy which can be applied - or
not. NVS_REPORT provides the necessary information.
So, this policy doesn't work if the other OS is un-cooperative and
soft-blocks the wireless on shutdown. That's the full extent to which
it is broken, yes? [And it's a relatively obscure problem, so the risk
is that it just rots, and the effort spent on this code is wasted].
But on eeepc-laptop, it seems safe to assume that the drivers on the
Other OS do the co-operative thing, because otherwise it would cause
confusion with the option in the BIOS setup.
Henrique (or anyone), has this ever been a problem on thinkpad-acpi? Do
you ever get a state in NVS at boot time, which the user had not requested?
Note that the linux ACPI project has the explicit goal of compatibility
with Windows. If a new platform comes along with a Windows ACPI driver
which screws up the platform state, it would be reasonable for the Linux
ACPI driver to not call set_global_sw_state().
You propose to exclude a feature that currently works, on the grounds
that it is inherently broken. But you haven't said that this has ever
caused incorrect behavior. All you have said so far is that it is a hack.
Since this was the original intent, it's a pity
rfkill_set_global_sw_state() wasn't listed in the feature removal
schedule (or a user-level description of NVS). I suggested something
like NVS_REPORT because, in the absence of a description of removal, it
looked like an oversight. In the past I've raised a couple of apparent
oversights about the rewrite, and they were treated as constructive.
But now its clear this was a policy decision, and I ended up draging out
an explanation of the policy from nitpicking details of the
implementation. I guess this is my fault for opening the discussion
based on the one loose thread of a detail, instead of asking if the
change was an oversight or intentional :-(.
Thanks
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
