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.
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.
Regards,
Andreas
--
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
