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

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

 



Hi,

Manu Abraham wrote:
> On Sun, Jan 10, 2010 at 4:52 PM, Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxxxxx> wrote:
> > 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.

Nak. stv090x_i2c_gate_ctrl will only be called with enable==false,
if it has previously been called successfully with enable==true.

> > 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.
> 
> It looks okay to my eyes. Any other pitfalls in the tuner operations
> can you think of ?
> 
> 
> Oliver,
> 
>  any thoughts that comes to your mind on this one ?

The purpuse of tuner_lock is to allow for 2 tuners with equal I2C
address on one I2C bus. Each one is behind its own I2C gate.

The code is a bit tricky. Lets analyse what may happen:

1. no error, enable==true:
   lock mutex -> return 0
   -> ok

2. no error, enable==false:
   unlock mutex (which has been locked by a previous call), return 0
   -> ok

3. error on enable:
   mutex lock, mutex unlock, return -1
   -> ok

4. error on disable:
   unlock mutex (which has been locked by a previous call), return -1
   -> ok

The code looks ok to me.

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
----------------------------------------------------------------
--
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

[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