Re: PULL http://jusst.de/hg/stv090x

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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, &reg) < 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, &reg) < 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, &reg) < 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;

[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux