Devin Heitmueller wrote: > On Fri, Jan 22, 2010 at 1:23 PM, Manu Abraham <abraham.manu@xxxxxxxxx> wrote: >> The point there is that it is a dual demod which implements a FSM for >> it's tuner flow control and hence. The demodulators in the tree are >> generally single/standalone and don't need such a FSM to handle that. >> Even if it's a dual frontend, most of them just do locking for the >> register access, while here it is not register access locking, while >> it is controlling state, similar to Finite State Machine (FSM) > > Ok, I see what is going on now. I didn't notice before that the gate > enable was keeping the "state->internal->tuner_lock" mutex set even > after the call returned. So you proofed my point: the code "quietly" changes the state of the mutex. As all other code that do that has a _lock/_unlock, it is a matter of documenting things using the standard kernel way for it, to avoid people to bad interpret the code. > My only concern here then is that I've seen cases where the > i2c_gate_ctrl() function is called in a non-symmetric fashion (where > the enable/disable calls are not necessarily properly balanced), > depending on the driver. While I am relatively confident that you > have balanced the instances where it is called within your driver, I > would be concerned about how it could possibly be called from other > places such as from the tuner driver or the dvb frontend core. I've checked the core and it does it on a balanced way. Yet, it is risky to assume that this will always happen, and having a bad, non-interrupt mutex there can lead to machine hangups. Even the patch that introduced this locking schema had this bug, showing that it is simple for someone to forget to ballance. The usage of the _lock/_unlock suffix helps to document that those calls need to be balanced. > Today, there is no real problem if a particular call path attempts to > enable the gate if it is already open (or disable if it is already > closed). With your proposed change, you will result in a deadlock. Precisely. Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
