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.
Regards,
Andreas
# HG changeset patch
# User Oliver Endriss <o.endriss@xxxxxx>
# Date 1263097942 -3600
# Node ID fefb0eb3c442f8ab1e446c6f275c914a99495312
# Parent b1e950fefc1ac04f3ef67d274d6a72859afd734f
stv090x: Disable I2C gate on error
From: Oliver Endriss <o.endriss@xxxxxx>
The I2C gate must also be disabled, if a tuner command failed.
Otherwise the tuner mutex would be locked forever.
Priority: normal
Signed-off-by: Oliver Endriss <o.endriss@xxxxxx>
diff -r b1e950fefc1a -r fefb0eb3c442 linux/drivers/media/dvb/frontends/stv090x.c
--- a/linux/drivers/media/dvb/frontends/stv090x.c Wed Jan 06 02:24:56 2010 +0400
+++ b/linux/drivers/media/dvb/frontends/stv090x.c Sun Jan 10 05:32:22 2010 +0100
@@ -2514,12 +2514,12 @@ static u32 stv090x_srate_srch_coarse(str
if (state->config->tuner_set_frequency) {
if (state->config->tuner_set_frequency(fe, freq) < 0)
- goto err;
+ goto err_gateoff;
}
if (state->config->tuner_set_bandwidth) {
if (state->config->tuner_set_bandwidth(fe, state->tuner_bw) < 0)
- goto err;
+ goto err_gateoff;
}
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
@@ -2532,7 +2532,7 @@ static u32 stv090x_srate_srch_coarse(str
if (state->config->tuner_get_status) {
if (state->config->tuner_get_status(fe, ®) < 0)
- goto err;
+ goto err_gateoff;
}
if (reg)
@@ -2551,6 +2551,9 @@ static u32 stv090x_srate_srch_coarse(str
srate_coarse = stv090x_get_srate(state, state->internal->mclk);
return srate_coarse;
+
+err_gateoff:
+ stv090x_i2c_gate_ctrl(fe, 0);
err:
dprintk(FE_ERROR, 1, "I/O error");
return -1;
@@ -2899,12 +2902,12 @@ static int stv090x_get_coldlock(struct s
if (state->config->tuner_set_frequency) {
if (state->config->tuner_set_frequency(fe, freq) < 0)
- goto err;
+ goto err_gateoff;
}
if (state->config->tuner_set_bandwidth) {
if (state->config->tuner_set_bandwidth(fe, state->tuner_bw) < 0)
- goto err;
+ goto err_gateoff;
}
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
@@ -2917,7 +2920,7 @@ static int stv090x_get_coldlock(struct s
if (state->config->tuner_get_status) {
if (state->config->tuner_get_status(fe, ®) < 0)
- goto err;
+ goto err_gateoff;
}
if (reg)
@@ -2948,6 +2951,8 @@ static int stv090x_get_coldlock(struct s
return lock;
+err_gateoff:
+ stv090x_i2c_gate_ctrl(fe, 0);
err:
dprintk(FE_ERROR, 1, "I/O error");
return -1;
@@ -3321,7 +3326,7 @@ static enum stv090x_signal_state stv090x
if (state->config->tuner_get_frequency) {
if (state->config->tuner_get_frequency(fe, &state->frequency) < 0)
- goto err;
+ goto err_gateoff;
}
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
@@ -3349,7 +3354,7 @@ static enum stv090x_signal_state stv090x
if (state->config->tuner_get_frequency) {
if (state->config->tuner_get_frequency(fe, &state->frequency) < 0)
- goto err;
+ goto err_gateoff;
}
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
@@ -3369,6 +3374,9 @@ static enum stv090x_signal_state stv090x
}
return STV090x_OUTOFRANGE;
+
+err_gateoff:
+ stv090x_i2c_gate_ctrl(fe, 0);
err:
dprintk(FE_ERROR, 1, "I/O error");
return -1;
@@ -3735,7 +3743,7 @@ static int stv090x_optimize_track(struct
if (state->config->tuner_set_bandwidth) {
if (state->config->tuner_set_bandwidth(fe, state->tuner_bw) < 0)
- goto err;
+ goto err_gateoff;
}
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
@@ -3787,6 +3795,9 @@ static int stv090x_optimize_track(struct
stv090x_set_vit_thtracq(state);
return 0;
+
+err_gateoff:
+ stv090x_i2c_gate_ctrl(fe, 0);
err:
dprintk(FE_ERROR, 1, "I/O error");
return -1;
@@ -4042,17 +4053,17 @@ static enum stv090x_signal_state stv090x
if (state->config->tuner_set_bbgain) {
if (state->config->tuner_set_bbgain(fe, 10) < 0) /* 10dB */
- goto err;
+ goto err_gateoff;
}
if (state->config->tuner_set_frequency) {
if (state->config->tuner_set_frequency(fe, state->frequency) < 0)
- goto err;
+ goto err_gateoff;
}
if (state->config->tuner_set_bandwidth) {
if (state->config->tuner_set_bandwidth(fe, state->tuner_bw) < 0)
- goto err;
+ goto err_gateoff;
}
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
@@ -4065,7 +4076,7 @@ static enum stv090x_signal_state stv090x
if (state->config->tuner_get_status) {
if (state->config->tuner_get_status(fe, ®) < 0)
- goto err;
+ goto err_gateoff;
}
if (reg)
@@ -4198,6 +4209,8 @@ static enum stv090x_signal_state stv090x
}
return signal_state;
+err_gateoff:
+ stv090x_i2c_gate_ctrl(fe, 0);
err:
dprintk(FE_ERROR, 1, "I/O error");
return -1;
@@ -5138,12 +5151,12 @@ static int stv090x_init(struct dvb_front
if (config->tuner_set_mode) {
if (config->tuner_set_mode(fe, TUNER_WAKE) < 0)
- goto err;
+ goto err_gateoff;
}
if (config->tuner_init) {
if (config->tuner_init(fe) < 0)
- goto err;
+ goto err_gateoff;
}
if (stv090x_i2c_gate_ctrl(fe, 0) < 0)
@@ -5153,6 +5166,9 @@ static int stv090x_init(struct dvb_front
goto err;
return 0;
+
+err_gateoff:
+ stv090x_i2c_gate_ctrl(fe, 0);
err:
dprintk(FE_ERROR, 1, "I/O error");
return -1;
