On Sun, Jan 10, 2010 at 6:43 PM, Andreas Regel <andreas.regel@xxxxxx> wrote: > Hi Mauro, > > Am 10.01.2010 15:15, schrieb Mauro Carvalho Chehab: >> >> 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. >> > > you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver > contains code like this: > > if (stv090x_i2c_gate_ctrl(fe, 1) < 0) > goto err; > > tuner access > > if (stv090x_i2c_gate_ctrl(fe, 0) < 0) > goto err; > > Intention of the patch is to make the tuner access exclusive. Thats the > reason why the mutex is locked when gate is opened and unlocked when the > gate is closed. > > In case the opening fails the close call is not made und thus mutex is not > unlocked twice. > > There one problem pending with: when there is an error during "tuner access" > the gate will not be closed and thus the mutex will not be unlocked. For > this case a patch from Oliver Endriss is attached. Mauro, Andreas, Ok, I will queue up the other patch as well and issue another pull request. Mauro, Ignore the pull request for the moment. 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
