Mauro, 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. > > 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 ? Regards, Manu -- 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
