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

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

 



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

[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