Re: [PATCH] rfkill: create useful userspace interface

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

Marcel Holtmann wrote:
> Hi Alan,
>
>   
>>>>>> 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.
>>>>>       
>>>>>           
>>>> I have no problems with either approach.  As long as the information of
>>>> which devices have restored their initial state from NVS is available to
>>>> userspace, it is enough.
>>>>     
>>>>         
>>> just to get the semantic right here. We are not telling userspace if a
>>> state has been restored or not. We are telling userspace that this
>>> specific RFKILL switch is capable of storing something in a persistent
>>> state over boot. There is a difference here.
>>>
>>> If a RFKILL driver claims it is capable of persistent storage then it
>>> better work or it should not claims it. Either it does it all the time
>>> or doesn't do it at all. Otherwise we end up in policy again and that is
>>> not the job of the kernel.
>>>
>>>   
>>>       
>>>> Do note that this information also needs to be available for resume (state
>>>> should be checkpointed to NVS on sleep, and restored from NVS on resume.  I
>>>> believe tpacpi does this, but if it doesn't, I will fix it eventually).
>>>>     
>>>>         
>>> Correct. That is the job of the driver. If it is broken, that needs
>>> fixing.
>>>   
>>>       
>> The core needs fixing too, currently it restores state for all devices
>> on resume.
>>
>> This is my fault again, for coming up with scenarios you probably don't
>> care about :-).  The problem is that suspend to disk provides a
>> possibility that the state will change for _any_ driver.  It's more
>> obvious with rfkill-input and NVS, if the wireless toggle key is pressed
>> when the disk image is being written out.  But you can also contrive it
>> with no NVS and rfkilld, if rfkilld gets started in the initramfs of the
>> resume kernel.  We took the easy way out, rather than adding resume
>> handlers to all drivers, or trying to work out what the real design
>> problem was :-).
>>
>> I hope that explains the issue.  I agree with your logic, I just want to
>> be clear that it needs more work on the rfkill core.  Drivers with NVS
>> should have a resume handler to call rfkill_set_sw_state(), but for this
>> to work the core will need to stop restoring their state (for NVS
>> drivers only).  As a detail, I think this behaviour difference with NVS
>> means it should be flagged with a more explicit API, e.g.
>> rfkill_init_persistent_sw_state().
>>     
>
> I don't see any problem here. If the driver needs extra help from the
> RFKILL subsystem to express its states, that is just fine with me.
>
>   
>> rfkill-input would like another (even more intrusive) hack here to set
>> the global state on resume.  But I for one can live without it for the
>> transition.  I think NVS state change over suspend is much more of a
>> corner case.  At least on eeepc-laptop it only seems to happen if the
>> user does something relatively odd.  And the worst that will happen is
>> they have to press the wireless-toggle key a second time before it
>> starts working.
>>     
>
> So one think is NVS states and the other are the HW switches. For the
> NVS ones we need extra support for suspend/resume details, but that is
> as mentioned above just fine. Also please keep in mind that NVS states
> are per device and not global. If a system wants to treat them as global
> that is userspace policy and not our concern from the kernel side.
>
> For the HW states, I think they gonna store its state pretty obvious and
> if we need a resume callback to poll its current state, then that seems
> logical to me too.
>   

Ok.  I will work on this and post a few more patches.  I guess it's
still easiest if I post this as running code, and try to justify it to
you guys.


1/3:  The original patch removing set_global_sw_state() - this hasn't
been applied yet.

2/3:  Remove the core restore-on-resume code.  Devices with NVS will
maintain their state, and _must_ check NVS on resume.  Other devices are
expected to restore their own state.

I think this should work, because it's how the rfkill rewrite was
designed to start with.  It may take me a while to figure out _why_ it
works though.  I'm not sure about the ordering - whether my
acpi/platform driver resume callback will run before the rfkill class
resume handler, and if so then how does it work?  I don't want to submit
code unless I understand it :-).

3/3: Export the "persistent" flag in sysfs - now we have hopefully
settled what it means.


Thanks
Alan