Hi, Manu Abraham wrote: > On Sun, Jan 10, 2010 at 4:52 PM, Mauro Carvalho Chehab > <mchehab@xxxxxxxxxxxxx> wrote: > > Manu Abraham wrote: > >> Mauro, > >> > >> Please pull http://jusst.de/hg/stv090x changesets: from 13880 - 13891 > >> inclusive of both. > > > > The third changeset has some locking troubles: > > > > # Node ID 4dd17a6a5ecc320eab58590a8567e5ba03b9d53a > > [STV090x] Added mutex protection around tuner I2C access. > > > > After the patch, the code will look like: > > > > static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable) > > { > > struct stv090x_state *state = fe->demodulator_priv; > > u32 reg; > > > > if (enable) > > mutex_lock(&state->internal->tuner_lock); > > > > reg = STV090x_READ_DEMOD(state, I2CRPT); > > if (enable) { > > dprintk(FE_DEBUG, 1, "Enable Gate"); > > STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1); > > if (STV090x_WRITE_DEMOD(state, I2CRPT, reg) < 0) > > goto err; > > > > } else { > > > > dprintk(FE_DEBUG, 1, "Disable Gate"); > > STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0); > > if ((STV090x_WRITE_DEMOD(state, I2CRPT, reg)) < 0) > > goto err; > > } > > > > if (!enable) > > mutex_unlock(&state->internal->tuner_lock); > > > > return 0; > > err: > > dprintk(FE_ERROR, 1, "I/O error"); > > mutex_unlock(&state->internal->tuner_lock); > > return -1; > > } > > > > As the err: is called on both enable and disable conditions, the error > > code will try to unlock an already unlocked mutex, if !enable. Nak. stv090x_i2c_gate_ctrl will only be called with enable==false, if it has previously been called successfully with enable==true. > > Also, it doesn't make sense to lock only if the gate is enabled, > > since it seems that the lock is meant to protect the i2c gate bus and > > both conditions will call STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, enable ? 1 : 0); > > > > The better would be simply to remove the if (enable)/if (!enable) tests > > on the above code. > > It looks okay to my eyes. Any other pitfalls in the tuner operations > can you think of ? > > > Oliver, > > any thoughts that comes to your mind on this one ? The purpuse of tuner_lock is to allow for 2 tuners with equal I2C address on one I2C bus. Each one is behind its own I2C gate. The code is a bit tricky. Lets analyse what may happen: 1. no error, enable==true: lock mutex -> return 0 -> ok 2. no error, enable==false: unlock mutex (which has been locked by a previous call), return 0 -> ok 3. error on enable: mutex lock, mutex unlock, return -1 -> ok 4. error on disable: unlock mutex (which has been locked by a previous call), return -1 -> ok The code looks ok to me. CU Oliver -- ---------------------------------------------------------------- VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/ 4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/ Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/ ---------------------------------------------------------------- -- 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
