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 -- 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
