Andreas Regel wrote: > Hi Mauro, > > Am 10.01.2010 13:52, schrieb Mauro Carvalho Chehab: >> 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. > > The lock shall protect the access to the tuners and not to the registers > itself, so it is locked when enable is set and unlocked when enable isn't Ok. > > The code between a call to stv090x_i2c_gate_ctrl(..., 1) > and stv090x_i2c_gate_ctrl(..., 0) shall be exclusive in case both tuners > have the same I2C address. By just looking at the i2c_gate_ctrl, it is not clear how do you expect that the locking will work. You should add a comment better explaining it. Also, as I pointed, if STV090x_WRITE_DEMOD fails, the unlock code will be called even if the gate is not enabled. As the lock should be used only if gate is enabled, it would be cleaner if you code the routine as: static int stv090x_i2c_gate_ctrl(struct dvb_frontend *fe, int enable) { struct stv090x_state *state = fe->demodulator_priv; u32 reg; int rc = -EINVAL; if (enable) { /* * (please better explain the lock - something like: * Tuner access should be locked to avoid * concurrent access when reading/writing to I2CRPT, * since those calls do tuner access) */ mutex_lock(&state->internal->tuner_lock); reg = STV090x_READ_DEMOD(state, I2CRPT); dprintk(FE_DEBUG, 1, "Enable Gate"); STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 1); rc = STV090x_WRITE_DEMOD(state, I2CRPT, reg) mutex_unlock(&state->internal->tuner_lock); if (rc < 0) goto err; } else { dprintk(FE_DEBUG, 1, "Disable Gate"); reg = STV090x_READ_DEMOD(state, I2CRPT); STV090x_SETFIELD_Px(reg, I2CT_ON_FIELD, 0); rc = STV090x_WRITE_DEMOD(state, I2CRPT, reg); if (rc < 0) goto err; } return 0; err: dprintk(FE_ERROR, 1, "I/O error"); return -rc; } This way, you avoid the risks of having lock troubles. A side effect of the above code is that the routine will properly propagate the error code produced at STV090x_WRITE_DEMOD(), instead of returning -1. We should really avoid using -1 for errors, using, instead an error code that it is defined on errors.h headers. 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
