Re: [PATCH] rfkill: create useful userspace interface

[Marcel Holtmann <marcel@holtmann.org>]

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.

Regards

Marcel