Marcel Holtmann wrote:
> Hi Alan,
>
>
>>>>>>>>> We just need to fix the platform drivers then. They should not set
>>>>>>>>> global states since that is not what they are controlling. They
>>>>>>>>> control
>>>>>>>>>
>>>>>>>>>
>>>>>>>> We should change things, yes. So that the platform stores the global
>>>>>>>> state. That was half-broken on the old core (the platform stored the
>>>>>>>> state of its own device, which could be out of sync with the global
>>>>>>>> state), but the part of it setting the global state is correct.
>>>>>>>>
>>>>>>>> That needs a new in-kernel API to tie the core to platform drivers
>>>>>>>> capable of storing global states without causing problems when drivers
>>>>>>>> are unloaded, but it is not hard.
>>>>>>>>
>>>>>>>> As for NVS events, they have a clear use case: to let rfkilld know
>>>>>>>> which
>>>>>>>> global states it could leave alone the first time it loads, and which
>>>>>>>> ones have to be restored...
>>>>>>>>
>>>>>>>>
>>>>>>> show me an example of a platform device that stores the global state. I
>>>>>>> think you are confusing the word platform as in system with a platform
>>>>>>> device. The ThinkPad Bluetooth and WWAN switches are platform devices
>>>>>>> and control each one specific device. Same goes for the EeePC. They are
>>>>>>> not controlling a global state.
>>>>>>>
>>>>>>>
>>>>>> I don't know what big difference you see between the two uses of
>>>>>> "platform",
>>>>>> but I will just work around it to get something useful out of this mail.
>>>>>>
>>>>>> The laptop stores in NVS the state of its 'switches'. This is as close as
>>>>>> one gets from 'storing the global state'. When the laptop boots,
>>>>>> these devices get set by the firware to the state in NVS. It is the best
>>>>>> place to store global state, because these devices will be in their proper
>>>>>> state (i.e. matching what will be the global state when the rfkill core
>>>>>> loads) all the time. It also gives you for free multi-OS/multi-kernel
>>>>>> state
>>>>>> storage for these devices, and compatibility with BIOSes that let you
>>>>>> define
>>>>>> the initial state for the devices in the firmware configuration, etc.
>>>>>>
>>>>>>
>>>>> it stores the state of its switches and why should these be enforced as
>>>>> a global state? Who says that this is a global state? For me that sounds
>>>>> like policy.
>>>>>
>>>>>
>>>> We don't seem to be getting very far :-(. I agree that these do not
>>>> appear to be global states, just the states of individual rfkill
>>>> devices.
>>>>
>>>> So I would propose the following changes. (I'm happy to write the
>>>> code as well, but I think it's easier to read English).
>>>>
>>>> 1) remove rfkill_set_global_sw_state()
>>>> 2) rfkill devices with NVS can e.g. call rfkill_has_nvs() before
>>>> registration, setting a flag.
>>>> 3) the "has NVS" flag is reported by /dev/rfkill, (at least in ADD
>>>> events, tho it may as well be set in all events)
>>>>
>>>>
>>> you can do things like this already if you just set the states correctly
>>> between rfkill_alloc and rfkill_register. So you should make sure you
>>> register your RFKILL switch with the correct state and not toggle it
>>> later. As far as I can tell the tpacpi driver does that already.
>>>
>>>
>> Ah. I need to read the (rewritten) code again.
>>
>
> it could be still broken to some degree, but some stuff is just because
> rfkill-input interferes. So disable rfkill-input via ioctl or not
> compile it at all.
>
Hmm, this looks more like a core feature than an rfkill-input bug:
"
v4: set default global state from userspace for rfkill hotplug
(pointed out by Marcel)
...
+ if (ev.op == RFKILL_OP_CHANGE_ALL) {
+ if (ev.type == RFKILL_TYPE_ALL) {
+ enum rfkill_type i;
+ for (i = 0; i < NUM_RFKILL_TYPES; i++)
+ rfkill_global_states[i].cur = ev.soft;
+ } else {
+ rfkill_global_states[ev.type].cur = ev.soft;
+ }
+ }
It still looks like this global state will apply even if rfkill-input is
disabled and userspace has not requested OP_CHANGE_ALL; the default
state will be enforced on hotplug. If you want to keep this, I think we
still need my full scheme.
Eek! On a related note, what are we doing on resume? Johannes added it
in a response to one of my annoying eeepc-laptop scenarios, but I didn't
look at the code, only the results. It seems the individual devices are
forced into the _global_ states (indexed by device type) on resume. I
thought you were trying to neuter these global states so userspace had
full discretion? Shouldn't we just restore the individual device state?
static int rfkill_resume(struct device *dev)
{
struct rfkill *rfkill = to_rfkill(dev);
bool cur;
mutex_lock(&rfkill_global_mutex);
cur = rfkill_global_states[rfkill->type].cur;
rfkill_set_block(rfkill, cur);
mutex_unlock(&rfkill_global_mutex);
rfkill->suspended = false;
schedule_work(&rfkill->uevent_work);
rfkill_resume_polling(rfkill);
return 0;
}
>> I'm still more familiar with the old rfkill core. My understanding was
>> that the old core required drivers to say what their current state was,
>> but if that differed from the global state then it would be changed to
>> match.
>>
>>
>>>> 4) rfkill-input preserves existing behaviour - *if enabled* - by
>>>> initializing the global state from individual devices which have NVS.
>>>> (As before, each _type_ of rfkill device has its own global state).
>>>>
>>>>
>>> That still sounds horribly wrong and has been for a long time, but
>>> again, I don't care about rfkill-input since it will go away.
>>>
>>>
>>>
>>>> 5) rfkill devices with NVS will have their current state preserved,
>>>> so long as the global state has not yet been set (by userspace or by
>>>> rfkill-input). Of course userspace can change the state in response
>>>> to the device being added.
>>>>
>>>>
>>> If you register your RFKILL switch properly, they do that already. See
>>> my comment above. You start up with the proper state to begin with.
>>>
>>>
>>>
>>>> => rfkilld then has the information required to implement the same
>>>> policy as rfkill-input. Furthermore, it will have enough information
>>>> that it could implement file-based storage as a fallback, and still
>>>> support NVS where available.
>>>>
>>>> It will also allow implementation (or configuration) of completely
>>>> different policy to rfkill-input. E.g. if your internal wireless
>>>> w/NVS is broken and should be disabled, that can be done independently
>>>> of your replacement USB wireless adaptor.
>>>>
>>>>
>>> I did actually looked into this and userspace has all information
>>> available to create a proper policy if you wanna treat your NVS states
>>> (for example the tpacpi ones) as global states, you can easily do that
>>> right now. It became really simple with /dev/rfkill.
>>>
>>>
>> I still think userspace is missing an important piece of information:
>> whether the state of a certain rfkill device is persistent or not.
>>
>> The driver knows exactly whether this is the case; from what you say it
>> will call rfkill_set_state() before rfkill_register(). If it _doesn't_
>> do this, there won't be any persistent state for userspace to retrieve
>> anyway :-).
>>
>> I don't think we should expect userspace to know whether or not a device
>> has a persistent state. Yes, it _could_ maintain whitelists, but why
>> should it have to if the driver already knows?
>>
>
> If you want that, then the best approach seems an extra sysfs attribute
> for this. It is not intrusive on the event API and lets udev etc. have
> these information, too.
>
Urg. Yes, it would be nice to expose it in sysfs. I guess I can live
with readfile("/sys/class/rfkill%d/persistent"), if there is strong
objection to cluttering up struct rfkill_event with extra flags.
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
