Re: [PATCH] rfkill: create useful userspace interface

[Alan Jenkins <alan-jenkins@tuffmail.co.uk>]

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