Hi, On Thu, 2009-04-30 at 00:19 -0300, Henrique de Moraes Holschuh wrote: > I am halfway on the thinkpad-acpi conversion. It is going slow because > I have to cleanup the entire thing to have the code look even remotely > sane now that HW and SW states are to be tracked separately by the > backend driver, and I lost the RFKILL_STATE_* stuff. Hmm. I didn't think it was that much of a problem for the other drivers at least. > Anyway, in the process (using v7 of the patch, taken from the URL you > posted sometime ago), I noticed this: > > The return values for rfkill_alloc() are not the same on the stub and > real functions. The stub function uses ERR_PTR, while the real > function uses NULL for any errors. Please make them do the same > thing, and it is probably a damn good idea to document in the > kerneldoc what kind of return values one should expect when you return > a pointer and the function can fail (i.e. NULL or ERR_PTR). Oops, yes, will fix that. I think I intended to use ERR_PTR but then thought it was too much of an API change. Don't really remember though. > Also, I have a question: are hooks ever called in atomic/interrupt > context? Just about every ACPI and I2C driver needs to sleep to do > their magic, so I sure hope the answer is "no", otherwise life > suddenly got a whole lot more complicated for thinkpad-acpi.. The answer is "no" unless there's a bug, this didn't really change, they're called only from the input handler. > Also, due to races, it is _impossible_ to assure the backend driver > that the set_block hook will never be called when the radio is > hard-blocked. Best to change the description of that hook to make it > clear that it could happen, but that the rfkill core will try to > optimize away such changes. Here's the race: > > A B > rfkill_set_block > saves state in "prev" > ... rfkill_set_hw_state(true) > call set_block hook > but radio IS hard-blocked > and the core WAS informed > of this Good point. I wonder what's more important -- it would be almost trivial to change this by using a spinlock instead of atomic bit operations. > Also, it looks like the SW state rollback done by rfkill_set_block can > sw-unblock a radio when it shouldn't on races very similar as the one > above, but involving sw-blocking. > > I don't see a way to do safe state rollback in a lockless rfkill core > if set_sw_state can happen at any time. Please enlighten me if I am > wrong (and I hope I am) :-( > > If I am not wrong, one could change rfkill_set_sw_state and > rfkill_set_states to return an error if rfkill_set_block is on the way > (that can be done lockless). For rfkill_set_states, document that the > hw state WILL be updated regardless, but that the sw state might not > be. > > Or one could kick the problem to the backend driver (which will have > any required locking anyway), by giving the set_block hook the duty of > calling set_sw_state when it succeeds. The return status might be > kept should we ever start passing it down to other layers like > userspace, or you can remove it. Thanks for your analysis. It does seem like a problem, and even locking in rfkill can't really avoid the problem [1]. However, why would we require the driver to postpone that? Your idea is sound, but I think we can do it in the core. It might require a spinlock there, but that's acceptable, I didn't use atomic operations to make it faster but to make it simpler. * rfkill_set_block can set a flag "I'm in an update" * a concurrent rfkill_set_sw_state will check that flag, and update the "prev" variable instead of the real variable * on errors, rfkill_set_block will then revert to whatever the last set_sw_state said That way, if the driver returns an error from ops->set_block, we will really revert to what the driver last told us with set_sw_state. It would seem to get this prev/in-update handling correct we need to add a spinlock, but concurrent rfkill_set_block()s are already prevented by the mutex so no additional locking would be needed there. Does that sound correct/acceptable? johannes [1] unless we want to use a mutex or require ops->set_block to be atomic but then we can't really do either
Attachment:
signature.asc
Description: This is a digitally signed message part
