Re: [PATCH v2 01/12] [media] dvb-frontends/stv0367: add flag to make i2c_gatectrl optional

[Matthias Schwarzott]

Am 26.03.2017 um 12:34 schrieb Daniel Scheller:
> Am Sun, 26 Mar 2017 10:03:33 +0200
> schrieb Matthias Schwarzott :
> 
>> Am 24.03.2017 um 19:23 schrieb Daniel Scheller:
>>> From: Daniel Scheller 
>>>
>>> Some hardware and bridges (namely ddbridge) require that tuner
>>> access is limited to one concurrent access and wrap i2c gate
>>> control with a mutex_lock when attaching frontends. According to
>>> vendor information, this is required as concurrent tuner
>>> reconfiguration can interfere each other and at worst cause tuning
>>> fails or bad reception quality.
>>>
>>> If the demod driver does gate_ctrl before setting up tuner
>>> parameters, and the tuner does another I2C enable, it will deadlock
>>> forever when gate_ctrl is wrapped into the mutex_lock. This adds a
>>> flag and a conditional before triggering gate_ctrl in the
>>> demodulator driver. 
>>
>> If I get this right, the complete call to i2c_gate_ctrl should be
>> disabled. Why not just overwrite the function-pointer i2c_gate_ctrl
>> with NULL in the relevant attach function (stv0367ddb_attach) or not
>> define it in stv0367ddb_ops?
> 
> This will make communication with the TDA18212 tuner chip impossible.
> We need to open stv0367's I2C gate, thus need the function. But for the
> overall hardware case, concurrent tuner reconfiguration must be avoided
> due to the mentioned issues, thus after _attach the bridge driver
> remaps the function pointer to wrap i2c_gate_ctrl with a lock to
> accomplish this - see [1] and [2]. As the demod AND the tuner driver
> both open the I2C gate, this will lead to a dead lock. To not change or
> break existing behaviour with other cards and tuner drivers, this
> (flag, conditional) appears to be the best option.

Ok, I understand: The real problem is that both demod driver (around
tuner access) and tuner driver care about the i2c_gate.

Regards
Matthias

Re: [PATCH v2 01/12] [media] dvb-frontends/stv0367: add flag to make i2c_gatectrl optional

[Daniel Scheller]

Am Sun, 26 Mar 2017 10:03:33 +0200
schrieb Matthias Schwarzott :

> Am 24.03.2017 um 19:23 schrieb Daniel Scheller:
> > From: Daniel Scheller 
> > 
> > Some hardware and bridges (namely ddbridge) require that tuner
> > access is limited to one concurrent access and wrap i2c gate
> > control with a mutex_lock when attaching frontends. According to
> > vendor information, this is required as concurrent tuner
> > reconfiguration can interfere each other and at worst cause tuning
> > fails or bad reception quality.
> > 
> > If the demod driver does gate_ctrl before setting up tuner
> > parameters, and the tuner does another I2C enable, it will deadlock
> > forever when gate_ctrl is wrapped into the mutex_lock. This adds a
> > flag and a conditional before triggering gate_ctrl in the
> > demodulator driver. 
> 
> If I get this right, the complete call to i2c_gate_ctrl should be
> disabled. Why not just overwrite the function-pointer i2c_gate_ctrl
> with NULL in the relevant attach function (stv0367ddb_attach) or not
> define it in stv0367ddb_ops?

This will make communication with the TDA18212 tuner chip impossible.
We need to open stv0367's I2C gate, thus need the function. But for the
overall hardware case, concurrent tuner reconfiguration must be avoided
due to the mentioned issues, thus after _attach the bridge driver
remaps the function pointer to wrap i2c_gate_ctrl with a lock to
accomplish this - see [1] and [2]. As the demod AND the tuner driver
both open the I2C gate, this will lead to a dead lock. To not change or
break existing behaviour with other cards and tuner drivers, this
(flag, conditional) appears to be the best option.

Thanks,
Daniel

[1]
https://github.com/herrnst/dddvb-linux-kernel/blob/cfefdc71b25d8c135889971dc5735414d22003cf/drivers/media/pci/ddbridge/ddbridge-core.c#L646
[2]
https://github.com/herrnst/dddvb-linux-kernel/blob/cfefdc71b25d8c135889971dc5735414d22003cf/drivers/media/pci/ddbridge/ddbridge-core.c#L556

Re: [PATCH v2 01/12] [media] dvb-frontends/stv0367: add flag to make i2c_gatectrl optional

[Matthias Schwarzott]

Am 24.03.2017 um 19:23 schrieb Daniel Scheller:
> From: Daniel Scheller 
> 
> Some hardware and bridges (namely ddbridge) require that tuner access is
> limited to one concurrent access and wrap i2c gate control with a
> mutex_lock when attaching frontends. According to vendor information, this
> is required as concurrent tuner reconfiguration can interfere each other
> and at worst cause tuning fails or bad reception quality.
> 
> If the demod driver does gate_ctrl before setting up tuner parameters, and
> the tuner does another I2C enable, it will deadlock forever when gate_ctrl
> is wrapped into the mutex_lock. This adds a flag and a conditional before
> triggering gate_ctrl in the demodulator driver.
> 

If I get this right, the complete call to i2c_gate_ctrl should be disabled.
Why not just overwrite the function-pointer i2c_gate_ctrl with NULL in
the relevant attach function (stv0367ddb_attach) or not define it in
stv0367ddb_ops?

That should have exactly the same effect.

Regards
Matthias

[PATCH v2 01/12] [media] dvb-frontends/stv0367: add flag to make i2c_gatectrl optional

[Daniel Scheller]

From: Daniel Scheller 

Some hardware and bridges (namely ddbridge) require that tuner access is
limited to one concurrent access and wrap i2c gate control with a
mutex_lock when attaching frontends. According to vendor information, this
is required as concurrent tuner reconfiguration can interfere each other
and at worst cause tuning fails or bad reception quality.

If the demod driver does gate_ctrl before setting up tuner parameters, and
the tuner does another I2C enable, it will deadlock forever when gate_ctrl
is wrapped into the mutex_lock. This adds a flag and a conditional before
triggering gate_ctrl in the demodulator driver.

Signed-off-by: Daniel Scheller 
---
 drivers/media/dvb-frontends/stv0367.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-frontends/stv0367.c 
b/drivers/media/dvb-frontends/stv0367.c
index fd49c43..fc80934 100644
--- a/drivers/media/dvb-frontends/stv0367.c
+++ b/drivers/media/dvb-frontends/stv0367.c
@@ -89,6 +89,8 @@ struct stv0367_state {
struct stv0367cab_state *cab_state;
/* DVB-T */
struct stv0367ter_state *ter_state;
+   /* flags for operation control */
+   u8 use_i2c_gatectrl;
 };
 
 struct st_register {
@@ -1827,10 +1829,10 @@ static int stv0367ter_set_frontend(struct dvb_frontend 
*fe)
stv0367ter_init(fe);
 
if (fe->ops.tuner_ops.set_params) {
-   if (fe->ops.i2c_gate_ctrl)
+   if (state->use_i2c_gatectrl && fe->ops.i2c_gate_ctrl)
fe->ops.i2c_gate_ctrl(fe, 1);
fe->ops.tuner_ops.set_params(fe);
-   if (fe->ops.i2c_gate_ctrl)
+   if (state->use_i2c_gatectrl && fe->ops.i2c_gate_ctrl)
fe->ops.i2c_gate_ctrl(fe, 0);
}
 
@@ -2321,6 +2323,9 @@ struct dvb_frontend *stv0367ter_attach(const struct 
stv0367_config *config,
state->fe.demodulator_priv = state;
state->chip_id = stv0367_readreg(state, 0xf000);
 
+   /* demod operation options */
+   state->use_i2c_gatectrl = 1;
+
dprintk("%s: chip_id = 0x%x\n", __func__, state->chip_id);
 
/* check if the demod is there */
@@ -3120,10 +3125,10 @@ static int stv0367cab_set_frontend(struct dvb_frontend 
*fe)
 
/* Tuner Frequency Setting */
if (fe->ops.tuner_ops.set_params) {
-   if (fe->ops.i2c_gate_ctrl)
+   if (state->use_i2c_gatectrl && fe->ops.i2c_gate_ctrl)
fe->ops.i2c_gate_ctrl(fe, 1);
fe->ops.tuner_ops.set_params(fe);
-   if (fe->ops.i2c_gate_ctrl)
+   if (state->use_i2c_gatectrl && fe->ops.i2c_gate_ctrl)
fe->ops.i2c_gate_ctrl(fe, 0);
}
 
@@ -3437,6 +3442,9 @@ struct dvb_frontend *stv0367cab_attach(const struct 
stv0367_config *config,
state->fe.demodulator_priv = state;
state->chip_id = stv0367_readreg(state, 0xf000);
 
+   /* demod operation options */
+   state->use_i2c_gatectrl = 1;
+
dprintk("%s: chip_id = 0x%x\n", __func__, state->chip_id);
 
/* check if the demod is there */
-- 
2.10.2