Skip to contentSkip to conversation

drxk: Switch the delivery system on FE_SET_PROPERTY

PATCH Superseded Mauro Carvalho Chehab 6 participants 21 messages Dec 9, 2011 – Dec 10, 2011 Media (V4L/DVB)

lore
Share & tools
lore

Mauro Carvalho Chehab

author
#0 lore raw

PATCH 1/1 drxk: Switch the delivery system on FE_SET_PROPERTY

The DRX-K doesn't change the delivery system at set_properties, but do it at frontend init. This causes problems on programs like w_scan that, by default, opens both frontends.

Instead, explicitly set the format when set_parameters callback is called.

Signed-off-by Mauro Carvalho Chehab <[email protected]>
---
 drivers/media/dvb/frontends/drxk_hard.c |   32 ++++++++++++++++++++++--------
 drivers/media/dvb/frontends/drxk_hard.h |    2 +
 2 files changed, 25 insertions(+), 9 deletions(-)
drivers/media/dvb/frontends/drxk_hard.c +23 -9
index 95cbc98..c8e0921 100644
@@ -1847,6 +1847,7 @@ static int SetOperationMode(struct drxk_state *state,
 		*/
 	switch (oMode) {
 	case OM_DVBT:
+		dprintk(1, ": DVB-T\n");
 		state->m_OperationMode = oMode;
 		status = SetDVBTStandard(state, oMode);
 		if (status < 0)

@@ -1854,6 +1855,8 @@ static int SetOperationMode(struct drxk_state *state,
 		break;
 	case OM_QAM_ITU_A:	/* fallthrough */
 	case OM_QAM_ITU_C:
+		dprintk(1, ": DVB-C Annex %c\n",
+			(state->m_OperationMode == OM_QAM_ITU_A) ? 'A' : 'C');
 		state->m_OperationMode = oMode;
 		status = SetQAMStandard(state, oMode);
 		if (status < 0)

@@ -6183,7 +6186,10 @@ static int drxk_c_init(struct dvb_frontend *fe)
 	dprintk(1, "\n");
 	if (mutex_trylock(&state->ctlock) == 0)
 		return -EBUSY;
-	SetOperationMode(state, OM_QAM_ITU_A);
+	if (state->m_itut_annex_c)
+		SetOperationMode(state, OM_QAM_ITU_C);
+	else
+		SetOperationMode(state, OM_QAM_ITU_A);
 	return 0;
 }
 

@@ -6219,14 +6225,6 @@ static int drxk_set_parameters(struct dvb_frontend *fe,
 		return -EINVAL;
 	}
 
-	if (state->m_OperationMode == OM_QAM_ITU_A ||
-	    state->m_OperationMode == OM_QAM_ITU_C) {
-		if (fe->dtv_property_cache.rolloff == ROLLOFF_13)
-			state->m_OperationMode = OM_QAM_ITU_C;
-		else
-			state->m_OperationMode = OM_QAM_ITU_A;
-	}
-
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 1);
 	if (fe->ops.tuner_ops.set_params)

@@ -6235,6 +6233,22 @@ static int drxk_set_parameters(struct dvb_frontend *fe,
 		fe->ops.i2c_gate_ctrl(fe, 0);
 	state->param = *p;
 	fe->ops.tuner_ops.get_if_frequency(fe, &IF);
+
+	/*
+	 * Make sure that the frontend is on the right state
+	 */
+
+	if (fe->ops.info.type == FE_QAM) {
+		if (fe->dtv_property_cache.rolloff == ROLLOFF_13) {
+			state->m_itut_annex_c = true;
+			SetOperationMode(state, OM_QAM_ITU_C);
+		} else {
+			state->m_itut_annex_c = false;
+			SetOperationMode(state, OM_QAM_ITU_A);
+		}
+	} else
+		SetOperationMode(state, OM_DVBT);
+
 	Start(state, 0, IF);
 
 	/* printk(KERN_DEBUG "drxk: %s IF=%d done\n", __func__, IF); */
drivers/media/dvb/frontends/drxk_hard.h +2 -0
index a05c32e..85a423f 100644
@@ -263,6 +263,8 @@ struct drxk_state {
 	u8     m_TSDataStrength;
 	u8     m_TSClockkStrength;
 
+	bool   m_itut_annex_c;      /* If true, uses ITU-T DVB-C Annex C, instead of Annex A */
+
 	enum DRXMPEGStrWidth_t  m_widthSTR;    /**< MPEG start width */
 	u32    m_mpegTsStaticBitrate;          /**< Maximum bitrate in b/s in case
 						    static clockrate is selected */
Signature · 1 line
1.7.8
#1 lore raw
Quoted 6 lines · Mauro Carvalho Chehab

The DRX-K doesn't change the delivery system at set_properties, but do it at frontend init. This causes problems on programs like w_scan that, by default, opens both frontends.

Instead, explicitly set the format when set_parameters callback is called.

May I ask why you don't use mfe_shared flag instead?

regards
Antti

Signature · 1 line
http://palosaari.fi/
author
#2 lore raw
Quoted 9 lines · Antti Palosaari
Quoted 6 lines · Mauro Carvalho Chehab

The DRX-K doesn't change the delivery system at set_properties, but do it at frontend init. This causes problems on programs like w_scan that, by default, opens both frontends.

Instead, explicitly set the format when set_parameters callback is called.

May I ask why you don't use mfe_shared flag instead?

Tested with it. Works. It takes a little more time to switch, but the solution will be cleaner. version 2 will follow.

Regards,
Mauro

#3 lore raw
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 5 lines · Antti Palosaari
Quoted 4 lines · Mauro Carvalho Chehab

The DRX-K doesn't change the delivery system at set_properties, but do it at frontend init. This causes problems on programs like w_scan that, by default, opens both frontends.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Instead, explicitly set the format when set_parameters callback is called.

May I ask why you don't use mfe_shared flag instead?

Tested with it. Works. It takes a little more time to switch, but the solution will be cleaner. version 2 will follow.

Yes, there is kind of loop timer which tries to take FE and swithing takes second or two because it waits FE is freed. I looked it when I did MFE and I did not understood why it was done like that.

cxd2820r has earlier simple lock (inside of demod driver) that just returned error immediately when busy. It is a little bit mystery for me why mfe_shared has that kind of waiting mechanism. Could someone explain reason for that?

regards
Antti

Signature · 1 line
http://palosaari.fi/
author
#4 lore raw
Quoted 15 lines · Antti Palosaari
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 5 lines · Antti Palosaari
Quoted 4 lines · Mauro Carvalho Chehab

The DRX-K doesn't change the delivery system at set_properties, but do it at frontend init. This causes problems on programs like w_scan that, by default, opens both frontends.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Instead, explicitly set the format when set_parameters callback is called.

May I ask why you don't use mfe_shared flag instead?

Tested with it. Works. It takes a little more time to switch, but the solution will be cleaner. version 2 will follow.

Quoted 7 lines

Yes, there is kind of loop timer which tries to take FE and swithing takes second or two because it waits FE is freed. I looked it when I did MFE and I did not understood why it was done like that.

cxd2820r has earlier simple lock (inside of demod driver) that just returned error immediately when busy. It is a little bit mystery for me why mfe_shared has that kind of waiting mechanism.

Still, it doesn't make much sense, at least on w_scan, as the only thing that is called with each adapter is FE_GET_INFO:

open("/dev/dvb/adapter0/frontend0", O_RDWR|O_NONBLOCK) = 3
ioctl(3, FE_GET_INFO, 0x635120) = 0
write(2, "\t/dev/dvb/adapter0/frontend0 -> "..., 92 /dev/dvb/adapter0/frontend0 -> DVB-C "DRXK DVB-C": specified was DVB-T -> SEARCH NEXT ONE.
) = 92
close(3) = 0
open("/dev/dvb/adapter0/frontend1", O_RDWR|O_NONBLOCK) = 3
ioctl(3, FE_GET_INFO, 0x635120) = 0
write(2, "\t/dev/dvb/adapter0/frontend1 -> "..., 52 /dev/dvb/adapter0/frontend1 -> DVB-T "DRXK DVB-T": ) = 52
write(2, "good :-)\n", 9good :-)
) = 9
close(3) = 0

Still, the second open takes about 4 seconds to complete, _even_ with O_NONBLOCK.

There's something bad there, as it is violating POSIX.

Quoted 1 line

Could someone explain reason for that?

I dunno, but I think this needs to be fixed, at least when the frontend is opened with O_NONBLOCK.

Regards,
Mauro.

#5 lore raw
Quoted 5 lines · Mauro Carvalho Chehab
Quoted 1 line

Could someone explain reason for that?

I dunno, but I think this needs to be fixed, at least when the frontend is opened with O_NONBLOCK.

Are you doing the drx-k firmware load on dvb_init()? That could easily take 4 seconds.

Devin

Signature · 2 lines
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
author
#6 lore raw
Quoted 10 lines · Devin Heitmueller
Quoted 5 lines · Mauro Carvalho Chehab
Quoted 1 line

Could someone explain reason for that?

I dunno, but I think this needs to be fixed, at least when the frontend is opened with O_NONBLOCK.

Are you doing the drx-k firmware load on dvb_init()? That could easily take 4 seconds.

No. The firmware were opened previously.

This is what happens at the driver:

(frontend0 open - DVB-C)

[ 5177.932326] drxk: drxk_c_init
[ 5177.932330] drxk: SetOperationMode
[ 5177.932691] drxk: drxk_gate_ctrlenable
[ 5177.932695] drxk: ConfigureI2CBridge
[ 5177.932697] xc5000: xc5000_init()
[ 5177.936565] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5177.943306] xc5000: xc_initialize()
[ 5178.187199] xc5000: *** ADC envelope (0-1023) = 4
[ 5178.192569] xc5000: *** Frequency error = 0 Hz
[ 5178.197566] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5178.205291] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5178.210662] xc5000: *** Horizontal sync frequency = 15473 Hz
[ 5178.216909] xc5000: *** Frame lines = 789
[ 5178.221659] xc5000: *** Quality (0:<8dB, 7:>56dB) = 9
[ 5178.226753] drxk: drxk_gate_ctrldisable
[ 5178.226755] drxk: ConfigureI2CBridge
	(frontend1 open - DVB-T)
[ 5181.224873] drxk: drxk_gate_ctrlenable
[ 5181.224877] drxk: ConfigureI2CBridge
[ 5181.224880] xc5000: xc5000_sleep()
[ 5181.228327] xc5000: xc5000_TunerReset()
[ 5181.232204] drxk: drxk_gate_ctrldisable
[ 5181.232205] drxk: ConfigureI2CBridge
[ 5181.232207] drxk: drxk_c_sleep
[ 5181.232209] drxk: ShutDown
[ 5181.232211] drxk: MPEGTSStop
[ 5181.731673] drxk: drxk_t_init
[ 5181.731677] drxk: SetOperationMode
[ 5181.732101] drxk: MPEGTSStop
[ 5181.734075] drxk: PowerDownQAM
[ 5181.735075] drxk: scu_command
[ 5181.737970] drxk: SetIqmAf
[ 5181.738948] drxk: SetOperationMode: DVB-T
[ 5181.738950] drxk: SetDVBTStandard
[ 5181.738952] drxk: PowerUpDVBT
[ 5181.738954] drxk: CtrlPowerMode
[ 5181.738956] drxk: PowerUpDevice
[ 5181.740321] drxk: DVBTEnableOFDMTokenRing
[ 5181.741947] drxk: SwitchAntennaToDVBT
[ 5181.741949] drxk: scu_command
[ 5181.744718] drxk: scu_command
[ 5181.750317] drxk: SetIqmAf
[ 5181.755439] drxk: BLChainCmd
[ 5181.760710] drxk: ADCSynchronization
[ 5181.760713] drxk: ADCSyncMeasurement
[ 5181.763596] drxk: SetPreSaw
[ 5181.764309] drxk: SetAgcRf
[ 5181.766433] drxk: SetAgcIf
[ 5181.773183] drxk: MPEGTSDtoSetup
[ 5181.777948] drxk: DVBTActivatePresets
[ 5181.777951] drxk: DVBTCtrlSetIncEnable
[ 5181.778301] drxk: DVBTCtrlSetFrEnable
[ 5181.778703] drxk: DVBTCtrlSetEchoThreshold
[ 5181.779697] drxk: DVBTCtrlSetEchoThreshold
[ 5181.781053] drxk: drxk_gate_ctrlenable
[ 5181.781056] drxk: ConfigureI2CBridge
[ 5181.781058] xc5000: xc5000_init()
[ 5181.785050] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5181.791790] xc5000: xc_initialize()
[ 5182.041187] xc5000: *** ADC envelope (0-1023) = 4
[ 5182.046559] xc5000: *** Frequency error = 0 Hz
[ 5182.051557] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5182.059448] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5182.065154] xc5000: *** Horizontal sync frequency = 14817 Hz
[ 5182.071424] xc5000: *** Frame lines = 1283
[ 5182.076273] xc5000: *** Quality (0:<8dB, 7:>56dB) = 9
[ 5182.081366] drxk: drxk_gate_ctrldisable
[ 5182.081368] drxk: ConfigureI2CBridge
[ 5185.079823] drxk: drxk_gate_ctrlenable
[ 5185.079827] drxk: ConfigureI2CBridge
[ 5185.079830] xc5000: xc5000_sleep()
[ 5185.083276] xc5000: xc5000_TunerReset()
[ 5185.087104] drxk: drxk_gate_ctrldisable
[ 5185.087107] drxk: ConfigureI2CBridge
[ 5185.087111] drxk: drxk_t_sleep
[ 5185.087323] drxk: drxk_c_init
[ 5185.087326] drxk: SetOperationMode
[ 5185.087778] drxk: MPEGTSStop
[ 5185.089993] drxk: PowerDownDVBT
[ 5185.090780] drxk: scu_command
[ 5185.094100] drxk: scu_command
[ 5185.098219] drxk: SetIqmAf
[ 5185.099221] drxk: CtrlPowerMode
[ 5185.099223] drxk: MPEGTSStop
[ 5185.101218] drxk: PowerDownDVBT
[ 5185.101854] drxk: scu_command
[ 5185.105090] drxk: scu_command
[ 5185.109215] drxk: SetIqmAf
[ 5185.110215] drxk: DVBTEnableOFDMTokenRing
[ 5185.112566] drxk: SetOperationMode: DVB-C Annex C
[ 5185.112568] drxk: SetQAMStandard
[ 5185.112570] drxk: SwitchAntennaToQAM
[ 5185.112572] drxk: PowerUpQAM
[ 5185.112574] drxk: CtrlPowerMode
[ 5185.112575] drxk: QAMResetQAM
[ 5185.112962] drxk: scu_command
[ 5185.116838] drxk: BLChainCmd
[ 5185.127954] drxk: SetIqmAf
[ 5185.129306] drxk: ADCSynchronization
[ 5185.129308] drxk: ADCSyncMeasurement
[ 5185.132949] drxk: InitAGC
[ 5185.149315] drxk: SetPreSaw
[ 5185.149721] drxk: SetAgcRf
[ 5185.151720] drxk: SetAgcIf
[ 5185.155817] drxk: drxk_gate_ctrlenable
[ 5185.155820] drxk: ConfigureI2CBridge
[ 5185.155822] xc5000: xc5000_init()
[ 5185.159694] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5185.166432] xc5000: xc_initialize()
[ 5185.416305] xc5000: *** ADC envelope (0-1023) = 4
[ 5185.421593] xc5000: *** Frequency error = 0 Hz
[ 5185.426676] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5185.434622] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5185.440270] xc5000: *** Horizontal sync frequency = 14374 Hz
[ 5185.446579] xc5000: *** Frame lines = 1283
[ 5185.451389] xc5000: *** Quality (0:<8dB, 7:>56dB) = 9
[ 5185.456475] drxk: drxk_gate_ctrldisable
[ 5185.456477] drxk: ConfigureI2CBridge
[ 5185.456614] drxk: drxk_c_get_tune_settings
[ 5185.456773] drxk: drxk_set_parameters
[ 5185.456776] drxk: drxk_gate_ctrlenable
[ 5185.456778] drxk: ConfigureI2CBridge
[ 5185.457576] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5185.464311] xc5000: xc5000_set_params() frequency=57000000 (Hz)
[ 5185.470268] xc5000: xc5000_set_params() QAM modulation
[ 5185.475439] xc5000: xc5000_set_params() Bandwidth 6MHz (5999550)
[ 5185.481473] xc5000: xc5000_set_params() frequency=55250000 (compensated)
[ 5185.488202] xc5000: xc_SetSignalSource(1) Source = CABLE
[ 5185.494524] xc5000: xc_SetTVStandard(0x8002,0x00c0)
[ 5185.499435] xc5000: xc_SetTVStandard() Standard = DTV6
[ 5185.513360] xc5000: xc_set_IF_frequency(freq_khz = 4000) freq_code = 0x1000
[ 5185.528244] xc5000: xc_tune_channel(55250000)
[ 5185.532643] xc5000: xc_set_RF_frequency(55250000)
[ 5185.729144] xc5000: *** ADC envelope (0-1023) = 744
[ 5185.734762] xc5000: *** Frequency error = 0 Hz
[ 5185.739739] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5185.747612] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5185.753107] xc5000: *** Horizontal sync frequency = 15038 Hz
[ 5185.759410] xc5000: *** Frame lines = 65535
[ 5185.764408] xc5000: *** Quality (0:<8dB, 7:>56dB) = 5
[ 5185.769503] drxk: drxk_gate_ctrldisable
[ 5185.769505] drxk: ConfigureI2CBridge
[ 5185.769507] xc5000: xc5000_get_if_frequency()
[ 5185.773902] drxk: Start
[ 5185.773904] drxk: SetQAM
[ 5185.774845] drxk: QAMResetQAM
[ 5185.775218] drxk: scu_command
[ 5185.778738] drxk: QAMSetSymbolrate
[ 5185.779969] drxk: scu_command
[ 5185.783737] drxk: SCU_RESULT_INVPAR while sending cmd 0x0203 with params:
[ 5185.790388] drxk: 02 00 00 00 10 00 05 00 03 02                    ..........
[ 5185.790581] drxk: scu_command
[ 5185.793857] drxk: scu_command
[ 5185.797729] drxk: SetFrequencyShifter
[ 5185.798209] drxk: SetQAMMeasurement
[ 5185.808601] drxk: SetQAM64
[ 5185.829359] drxk: MPEGTSDtoSetup
[ 5185.836588] drxk: scu_command
	(I've aborted w_scan to avoid generating a too big dump)
[ 5188.837828] drxk: drxk_gate_ctrlenable
[ 5188.837833] drxk: ConfigureI2CBridge
[ 5188.837835] xc5000: xc5000_sleep()
[ 5188.841280] xc5000: xc5000_TunerReset()
[ 5188.845155] drxk: drxk_gate_ctrldisable
[ 5188.845157] drxk: ConfigureI2CBridge
[ 5188.845159] drxk: drxk_c_sleep
[ 5188.845160] drxk: ShutDown
[ 5188.845162] drxk: MPEGTSStop
author
#7 lore raw
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 5 lines · Devin Heitmueller
Quoted 3 lines · Mauro Carvalho Chehab
Quoted 1 line

Could someone explain reason for that?

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I dunno, but I think this needs to be fixed, at least when the frontend is opened with O_NONBLOCK.

Are you doing the drx-k firmware load on dvb_init()? That could easily take 4 seconds.

No. The firmware were opened previously.

Maybe the delay is due to this part of dvb_frontend.c:

static int dvb_mfe_wait_time = 5;
...

                        int mferetry = (dvb_mfe_wait_time << 1);

                        mutex_unlock (&adapter->mfe_lock);
                        while (mferetry-- && (mfedev->users != -1 ||
                                        mfepriv->thread != NULL)) {
                                if(msleep_interruptible(500)) {
                                        if(signal_pending(current))
                                                return -EINTR;
                                }
                        }

If I set this modprobe parameter to 1, the delay reduces drastically:

[ 5975.865162] drxk: ConfigureI2CBridge
[ 5975.865164] xc5000: xc5000_init()
[ 5975.869257] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5975.876009] xc5000: xc_initialize()
[ 5976.120891] xc5000: *** ADC envelope (0-1023) = 4
[ 5976.126260] xc5000: *** Frequency error = 0 Hz
[ 5976.131260] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5976.139111] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5976.144733] xc5000: *** Horizontal sync frequency = 11292 Hz
[ 5976.150976] xc5000: *** Frame lines = 1442
[ 5976.155850] xc5000: *** Quality (0:<8dB, 7:>56dB) = 9
[ 5976.160937] drxk: drxk_gate_ctrldisable
[ 5976.160939] drxk: ConfigureI2CBridge
[ 5977.161897] drxk: drxk_c_get_tune_settings
[ 5977.162085] drxk: drxk_c_init
[ 5977.162089] drxk: drxk_gate_ctrlenable
[ 5977.162091] drxk: ConfigureI2CBridge
[ 5977.162094] xc5000: xc5000_init()
[ 5977.166095] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5977.172836] xc5000: xc_initialize()
[ 5977.422213] xc5000: *** ADC envelope (0-1023) = 4
[ 5977.427706] xc5000: *** Frequency error = 0 Hz
[ 5977.432832] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5977.440682] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5977.446177] xc5000: *** Horizontal sync frequency = 10460 Hz
[ 5977.452482] xc5000: *** Frame lines = 1442
[ 5977.457296] xc5000: *** Quality (0:<8dB, 7:>56dB) = 9
[ 5977.462385] drxk: drxk_gate_ctrldisable
[ 5977.462388] drxk: ConfigureI2CBridge
[ 5977.462390] drxk: drxk_set_parameters
[ 5977.462392] drxk: drxk_gate_ctrlenable
[ 5977.462394] drxk: ConfigureI2CBridge
[ 5977.463043] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5977.469781] xc5000: xc5000_set_params() frequency=57000000 (Hz)
[ 5977.475740] xc5000: xc5000_set_params() QAM modulation
[ 5977.480912] xc5000: xc5000_set_params() Bandwidth 6MHz (5999550)
[ 5977.486948] xc5000: xc5000_set_params() frequency=55250000 (compensated)
[ 5977.493677] xc5000: xc_SetSignalSource(1) Source = CABLE
[ 5977.500024] xc5000: xc_SetTVStandard(0x8002,0x00c0)
[ 5977.504930] xc5000: xc_SetTVStandard() Standard = DTV6
[ 5977.518267] xc5000: xc_set_IF_frequency(freq_khz = 4000) freq_code = 0x1000
[ 5977.527135] xc5000: xc_tune_channel(55250000)
[ 5977.531530] xc5000: xc_set_RF_frequency(55250000)
[ 5977.728050] xc5000: *** ADC envelope (0-1023) = 768
[ 5977.733671] xc5000: *** Frequency error = 0 Hz
[ 5977.738649] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5977.746523] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5977.752017] xc5000: *** Horizontal sync frequency = 14970 Hz
[ 5977.758288] xc5000: *** Frame lines = 65535
[ 5977.763137] xc5000: *** Quality (0:<8dB, 7:>56dB) = 5
[ 5977.768224] drxk: drxk_gate_ctrldisable
[ 5977.768226] drxk: ConfigureI2CBridge
[ 5977.768228] xc5000: xc5000_get_if_frequency()
[ 5977.772624] drxk: Start
[ 5977.772626] drxk: SetQAM
[ 5977.773530] drxk: QAMResetQAM
[ 5977.773880] drxk: scu_command
[ 5977.777653] drxk: QAMSetSymbolrate
[ 5977.778880] drxk: scu_command
[ 5977.782647] drxk: SCU_RESULT_INVPAR while sending cmd 0x0203 with params:
[ 5977.789298] drxk: 02 00 00 00 10 00 05 00 03 02                    ..........
[ 5977.789490] drxk: scu_command
[ 5977.792644] drxk: scu_command
[ 5977.795641] drxk: SetFrequencyShifter
[ 5977.796119] drxk: SetQAMMeasurement
[ 5977.806489] drxk: SetQAM64
[ 5977.827502] drxk: MPEGTSDtoSetup
[ 5977.834621] drxk: scu_command
[ 5978.161550] drxk: drxk_read_status
[ 5978.161554] drxk: GetLockStatus
[ 5978.161556] drxk: GetQAMLockStatus
[ 5978.161558] drxk: scu_command
[ 5978.315220] drxk: drxk_read_status
[ 5978.315223] drxk: GetLockStatus
[ 5978.315225] drxk: GetQAMLockStatus
[ 5978.315227] drxk: scu_command
[ 5978.469137] drxk: drxk_read_status
[ 5978.469141] drxk: GetLockStatus
[ 5978.469143] drxk: GetQAMLockStatus
[ 5978.469144] drxk: scu_command
[ 5978.623043] drxk: drxk_read_status
[ 5978.623046] drxk: GetLockStatus
[ 5978.623048] drxk: GetQAMLockStatus
[ 5978.623050] drxk: scu_command
[ 5978.776974] drxk: drxk_read_status
[ 5978.776977] drxk: GetLockStatus
[ 5978.776979] drxk: GetQAMLockStatus
[ 5978.776981] drxk: scu_command
[ 5978.930891] drxk: drxk_read_status
[ 5978.930894] drxk: GetLockStatus
[ 5978.930896] drxk: GetQAMLockStatus
[ 5978.930898] drxk: scu_command
[ 5979.084814] drxk: drxk_read_status
[ 5979.084817] drxk: GetLockStatus
[ 5979.084819] drxk: GetQAMLockStatus
[ 5979.084821] drxk: scu_command
[ 5979.238727] drxk: drxk_read_status
[ 5979.238730] drxk: GetLockStatus
[ 5979.238732] drxk: GetQAMLockStatus
[ 5979.238734] drxk: scu_command
[ 5979.392643] drxk: drxk_read_status
[ 5979.392646] drxk: GetLockStatus
[ 5979.392648] drxk: GetQAMLockStatus
[ 5979.392650] drxk: scu_command
[ 5979.546595] drxk: drxk_read_status
[ 5979.546598] drxk: GetLockStatus
[ 5979.546601] drxk: GetQAMLockStatus
[ 5979.546602] drxk: scu_command
[ 5979.700506] drxk: drxk_c_get_tune_settings
[ 5979.700683] drxk: drxk_set_parameters
[ 5979.700687] drxk: drxk_gate_ctrlenable
[ 5979.700689] drxk: ConfigureI2CBridge
[ 5979.701382] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5979.708099] xc5000: xc5000_set_params() frequency=57000000 (Hz)
[ 5979.714055] xc5000: xc5000_set_params() QAM modulation
[ 5979.719230] xc5000: xc5000_set_params() Bandwidth 6MHz (5929400)
[ 5979.725267] xc5000: xc5000_set_params() frequency=55250000 (compensated)
[ 5979.731996] xc5000: xc_SetSignalSource(1) Source = CABLE
[ 5979.738262] xc5000: xc_SetTVStandard(0x8002,0x00c0)
[ 5979.743170] xc5000: xc_SetTVStandard() Standard = DTV6
[ 5979.757100] xc5000: xc_set_IF_frequency(freq_khz = 4000) freq_code = 0x1000
[ 5979.765968] xc5000: xc_tune_channel(55250000)
[ 5979.770364] xc5000: xc_set_RF_frequency(55250000)
[ 5979.966886] xc5000: *** ADC envelope (0-1023) = 816
[ 5979.972506] xc5000: *** Frequency error = 0 Hz
[ 5979.977483] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5979.985482] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5979.990975] xc5000: *** Horizontal sync frequency = 15023 Hz
[ 5979.997496] xc5000: *** Frame lines = 65535
[ 5980.002347] xc5000: *** Quality (0:<8dB, 7:>56dB) = 0
[ 5980.007428] drxk: drxk_gate_ctrldisable
[ 5980.007430] drxk: ConfigureI2CBridge
[ 5980.007432] xc5000: xc5000_get_if_frequency()
[ 5980.011820] drxk: Start
[ 5980.011821] drxk: SetQAM
[ 5980.012714] drxk: QAMResetQAM
[ 5980.013111] drxk: scu_command
[ 5980.016483] drxk: QAMSetSymbolrate
[ 5980.017713] drxk: scu_command
[ 5980.021482] drxk: SCU_RESULT_INVPAR while sending cmd 0x0203 with params:
[ 5980.028126] drxk: 02 00 00 00 10 00 05 00 03 02                    ..........
[ 5980.028316] drxk: scu_command
[ 5980.031475] drxk: scu_command
[ 5980.034475] drxk: SetFrequencyShifter
[ 5980.034953] drxk: SetQAMMeasurement
[ 5980.045136] drxk: SetQAM64
[ 5980.066075] drxk: MPEGTSDtoSetup
[ 5980.073329] drxk: scu_command
[ 5980.700153] drxk: drxk_read_status
[ 5980.700157] drxk: GetLockStatus
[ 5980.700159] drxk: GetQAMLockStatus
[ 5980.700161] drxk: scu_command
[ 5980.853772] drxk: drxk_read_status
[ 5980.853775] drxk: GetLockStatus
[ 5980.853777] drxk: GetQAMLockStatus
[ 5980.853779] drxk: scu_command
[ 5981.007797] drxk: drxk_read_status
[ 5981.007801] drxk: GetLockStatus
[ 5981.007803] drxk: GetQAMLockStatus
[ 5981.007805] drxk: scu_command
[ 5981.161681] drxk: drxk_read_status
[ 5981.161684] drxk: GetLockStatus
[ 5981.161686] drxk: GetQAMLockStatus
[ 5981.161688] drxk: scu_command
[ 5981.315635] drxk: drxk_read_status
[ 5981.315638] drxk: GetLockStatus
[ 5981.315640] drxk: GetQAMLockStatus
[ 5981.315642] drxk: scu_command
[ 5981.469555] drxk: drxk_read_status
[ 5981.469558] drxk: GetLockStatus
[ 5981.469561] drxk: GetQAMLockStatus
[ 5981.469562] drxk: scu_command
[ 5981.623468] drxk: drxk_read_status
[ 5981.623472] drxk: GetLockStatus
[ 5981.623474] drxk: GetQAMLockStatus
[ 5981.623476] drxk: scu_command
[ 5981.777388] drxk: drxk_read_status
[ 5981.777391] drxk: GetLockStatus
[ 5981.777393] drxk: GetQAMLockStatus
[ 5981.777395] drxk: scu_command
[ 5981.931307] drxk: drxk_read_status
[ 5981.931311] drxk: GetLockStatus
[ 5981.931313] drxk: GetQAMLockStatus
[ 5981.931314] drxk: scu_command
[ 5982.085228] drxk: drxk_read_status
[ 5982.085232] drxk: GetLockStatus
[ 5982.085234] drxk: GetQAMLockStatus
[ 5982.085236] drxk: scu_command
[ 5982.239174] drxk: drxk_c_get_tune_settings
[ 5982.239356] drxk: drxk_set_parameters
[ 5982.239360] drxk: drxk_gate_ctrlenable
[ 5982.239362] drxk: ConfigureI2CBridge
[ 5982.240066] xc5000: xc5000_is_firmware_loaded() returns True id = 0x1388
[ 5982.246785] xc5000: xc5000_set_params() frequency=57000000 (Hz)
[ 5982.252740] xc5000: xc5000_set_params() QAM modulation
[ 5982.257912] xc5000: xc5000_set_params() Bandwidth 6MHz (5750000)
[ 5982.263949] xc5000: xc5000_set_params() frequency=55250000 (compensated)
[ 5982.270679] xc5000: xc_SetSignalSource(1) Source = CABLE
[ 5982.276941] xc5000: xc_SetTVStandard(0x8002,0x00c0)
[ 5982.281849] xc5000: xc_SetTVStandard() Standard = DTV6
[ 5982.295776] xc5000: xc_set_IF_frequency(freq_khz = 4000) freq_code = 0x1000
[ 5982.304771] xc5000: xc_tune_channel(55250000)
[ 5982.309169] xc5000: xc_set_RF_frequency(55250000)
[ 5982.506561] xc5000: *** ADC envelope (0-1023) = 724
[ 5982.512186] xc5000: *** Frequency error = 0 Hz
[ 5982.517408] xc5000: *** Lock status (0-Wait, 1-Locked, 2-No-signal) = 1
[ 5982.525178] xc5000: *** HW: V03.02, FW: V01.06.0072
[ 5982.530799] xc5000: *** Horizontal sync frequency = 14954 Hz
[ 5982.537149] xc5000: *** Frame lines = 65535
[ 5982.542022] xc5000: *** Quality (0:<8dB, 7:>56dB) = 5
[ 5982.547109] drxk: drxk_gate_ctrldisable
[ 5982.547111] drxk: ConfigureI2CBridge
[ 5982.547113] xc5000: xc5000_get_if_frequency()
[ 5982.551512] drxk: Start
[ 5982.551514] drxk: SetQAM
[ 5982.552390] drxk: QAMResetQAM
[ 5982.552769] drxk: scu_command
[ 5982.556160] drxk: QAMSetSymbolrate
[ 5982.557390] drxk: scu_command
[ 5982.561160] drxk: SCU_RESULT_INVPAR while sending cmd 0x0203 with params:
[ 5982.567809] drxk: 02 00 00 00 10 00 05 00 03 02                    ..........
[ 5982.568001] drxk: scu_command
[ 5982.571154] drxk: scu_command
[ 5982.574150] drxk: SetFrequencyShifter
[ 5982.574630] drxk: SetQAMMeasurement
[ 5982.584376] drxk: SetQAM64
[ 5982.604258] drxk: MPEGTSDtoSetup
[ 5982.611361] drxk: scu_command
[ 5982.615007] drxk: drxk_gate_ctrlenable
[ 5982.615010] drxk: ConfigureI2CBridge
[ 5982.615012] xc5000: xc5000_sleep()
[ 5982.618436] xc5000: xc5000_TunerReset()
[ 5982.622313] drxk: drxk_gate_ctrldisable
[ 5982.622315] drxk: ConfigureI2CBridge
[ 5982.622317] drxk: drxk_c_sleep
[ 5982.622319] drxk: ShutDown
[ 5982.622321] drxk: MPEGTSStop

Regards,
Mauro.

#8 lore raw

PATCH 1/1 DVB: dvb_frontend: fix delayed thread exit

Quoted 19 lines · Mauro Carvalho Chehab
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 5 lines · Devin Heitmueller
Quoted 3 lines · Mauro Carvalho Chehab
Quoted 1 line

Could someone explain reason for that?

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I dunno, but I think this needs to be fixed, at least when the frontend is opened with O_NONBLOCK.

Are you doing the drx-k firmware load on dvb_init()? That could easily take 4 seconds.

No. The firmware were opened previously.

Maybe the delay is due to this part of dvb_frontend.c:

static int dvb_mfe_wait_time = 5;
...

Quoted 10 lines
                        int mferetry = (dvb_mfe_wait_time << 1);

                        mutex_unlock (&adapter->mfe_lock);
                        while (mferetry-- && (mfedev->users != -1 ||
                                        mfepriv->thread != NULL)) {
                                if(msleep_interruptible(500)) {
                                        if(signal_pending(current))
                                                return -EINTR;
                                }
                        }

I haven't looked at the mfe code, but in case it's waiting for the frontend thread to exit, there's a problem that causes the thread not to exit immediately. Here's a patch that's been sitting in my queue for a while:

---

Signed-off-by Andreas Oberritter <[email protected]>
linux/drivers/media/dvb/dvb-core/dvb_frontend.c +5 -2
index 7784d74..6823c2b 100644
@@ -514,7 +514,7 @@
 		return 1;
 
 	if (fepriv->dvbdev->writers == 1)
-		if (time_after(jiffies, fepriv->release_jiffies +
+		if (time_after_eq(jiffies, fepriv->release_jiffies +
 				  dvb_shutdown_timeout * HZ))
 			return 1;
 

@@ -2070,12 +2070,15 @@
 
 	dprintk ("%s\n", __func__);
 
-	if ((file->f_flags & O_ACCMODE) != O_RDONLY)
+	if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
 		fepriv->release_jiffies = jiffies;
+		mb();
+	}
 
 	ret = dvb_generic_release (inode, file);
 
 	if (dvbdev->users == -1) {
+		wake_up(&fepriv->wait_queue);
 		if (fepriv->exit != DVB_FE_NO_EXIT) {
 			fops_put(file->f_op);
 			file->f_op = NULL;
#9 lore raw

Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

Quoted 60 lines · Andreas Oberritter
Quoted 21 lines · Mauro Carvalho Chehab
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 5 lines · Devin Heitmueller
Quoted 3 lines · Mauro Carvalho Chehab
Quoted 1 line

Could someone explain reason for that?

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I dunno, but I think this needs to be fixed, at least when the frontend is opened with O_NONBLOCK.

Are you doing the drx-k firmware load on dvb_init()? That could easily take 4 seconds.

No. The firmware were opened previously.

Maybe the delay is due to this part of dvb_frontend.c:

static int dvb_mfe_wait_time = 5;
...
                        int mferetry = (dvb_mfe_wait_time << 1);

Quoted 8 lines

                        mutex_unlock (&adapter->mfe_lock); while (mferetry-- && (mfedev->users != -1 || mfepriv->thread != NULL)) { if(msleep_interruptible(500)) { if(signal_pending(current)) return -EINTR; }
                        }

I haven't looked at the mfe code, but in case it's waiting for the frontend thread to exit, there's a problem that causes the thread not to exit immediately. Here's a patch that's been sitting in my queue for a while:

---

Signed-off-by Andreas Oberritter <[email protected]>
linux/drivers/media/dvb/dvb-core/dvb_frontend.c   2011-09-13 15:55:48.865742791 +0200 renamed from linux/drivers/media/dvb/dvb-core/dvb_frontend.c   2011-09-07 12:32:24.000000000 +0200 +2 -2
index 7784d74..6823c2b 100644
@@ -514,7 +514,7 @@
                return 1;
 
        if (fepriv->dvbdev->writers == 1)
-               if (time_after(jiffies, fepriv->release_jiffies +
+               if (time_after_eq(jiffies, fepriv->release_jiffies +
                                  dvb_shutdown_timeout * HZ))
                        return 1;
 

@@ -2070,12 +2070,15 @@
 
        dprintk ("%s\n", __func__);
 
-       if ((file->f_flags & O_ACCMODE) != O_RDONLY)
+       if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
                fepriv->release_jiffies = jiffies;

linux/drivers/media/dvb/dvb-core/dvb_frontend.c   2011-09-13 15:55:48.865742791 +0200 line 2084

Quoted 10 lines
linux/drivers/media/dvb/dvb-core/dvb_frontend.c   2011-09-13 15:55:48.865742791 +0200 renamed from linux/drivers/media/dvb/dvb-core/dvb_frontend.c   2011-09-07 12:32:24.000000000 +0200 +3 -0
+               mb();
+       }
 
        ret = dvb_generic_release (inode, file);
 
        if (dvbdev->users == -1) {
+               wake_up(&fepriv->wait_queue);
                if (fepriv->exit != DVB_FE_NO_EXIT) {
                        fops_put(file->f_op);
                        file->f_op = NULL;

This patch needs to have a much better explanation of exactly what it does and what problem it solves. We have a history of race conditions in dvb_frontend.c, and it's patches like this with virtually no details just makes it worse.

I'm not arguing the actual merits of the code change - it *may* be correct. But without the appropriate background there is no real way of knowing...

Mauro, this patch should be NACK'd and resubmitted with a detailed explanation of the current behavior, what the problem is, and how the code changes proposed solve that problem.

Devin

Signature · 2 lines
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
#10 lore raw

Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

Quoted 77 lines · Devin Heitmueller
Quoted 60 lines · Andreas Oberritter
Quoted 19 lines · Mauro Carvalho Chehab
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 5 lines · Devin Heitmueller
Quoted 3 lines · Mauro Carvalho Chehab
Quoted 1 line

Could someone explain reason for that?

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I dunno, but I think this needs to be fixed, at least when the frontend is opened with O_NONBLOCK.

Are you doing the drx-k firmware load on dvb_init()? That could easily take 4 seconds.

No. The firmware were opened previously.

Maybe the delay is due to this part of dvb_frontend.c:

static int dvb_mfe_wait_time = 5;
...

Quoted 10 lines
                        int mferetry = (dvb_mfe_wait_time << 1);

                        mutex_unlock (&adapter->mfe_lock);
                        while (mferetry-- && (mfedev->users != -1 ||
                                        mfepriv->thread != NULL)) {
                                if(msleep_interruptible(500)) {
                                        if(signal_pending(current))
                                                return -EINTR;
                                }
                        }

I haven't looked at the mfe code, but in case it's waiting for the frontend thread to exit, there's a problem that causes the thread not to exit immediately. Here's a patch that's been sitting in my queue for a while:

---

Signed-off-by Andreas Oberritter <[email protected]>
linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13 15:55:48.865742791 +0200 renamed from linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07 12:32:24.000000000 +0200 +2 -2
index 7784d74..6823c2b 100644
@@ -514,7 +514,7 @@
               return 1;
 
       if (fepriv->dvbdev->writers == 1)
-               if (time_after(jiffies, fepriv->release_jiffies +
+               if (time_after_eq(jiffies, fepriv->release_jiffies +
                                 dvb_shutdown_timeout * HZ))
                       return 1;
 

@@ -2070,12 +2070,15 @@
 
       dprintk ("%s\n", __func__);
 
-       if ((file->f_flags & O_ACCMODE) != O_RDONLY)
+       if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
               fepriv->release_jiffies = jiffies;

linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13 15:55:48.865742791 +0200 line 2084

Quoted 10 lines
linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13 15:55:48.865742791 +0200 renamed from linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07 12:32:24.000000000 +0200 +3 -0
+               mb();
+       }
 
       ret = dvb_generic_release (inode, file);
 
       if (dvbdev->users == -1) {
+               wake_up(&fepriv->wait_queue);
               if (fepriv->exit != DVB_FE_NO_EXIT) {
                       fops_put(file->f_op);
                       file->f_op = NULL;

This patch needs to have a much better explanation of exactly what it does and what problem it solves. We have a history of race conditions in dvb_frontend.c, and it's patches like this with virtually no details just makes it worse.

Quoted 7 lines

I'm not arguing the actual merits of the code change - it *may* be correct. But without the appropriate background there is no real way of knowing...

Mauro, this patch should be NACK'd and resubmitted with a detailed explanation of the current behavior, what the problem is, and how the code changes proposed solve that problem.

WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free to analyze the code and resubmit it.

#11 lore raw

Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

Hello Andreas,

Quoted 2 lines · Andreas Oberritter

WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free to analyze the code and resubmit it.

1.  It's marked with a subject line that starts with "[PATCH]"
2.  It has your SIgned-Off-By line.
3.  it was sent to the mailing list.
4.  It doesn't have any keywords like "RFC" or "proposed".

If you don't intend for it to go upstream then don't do all of the above.

I'm not sure if your "WTF, Devin, you again?" is some indication that I'm annoying you. The last patch you submitted that touches the threading in dvb_frontend.c had a host of problems and was clearly not well researched (i.e. "DVB: dvb_frontend: convert semaphore to mutex"). As in this case, there is no background indicating that this patch has been fully thought out and due diligence has been done.

Maybe you have thoroughly researched the change, taken the time to fully understand its effects, and tested it with a variety of boards and scenarios. Without a good patch description, there is no way to know.

I apologize if you're inconvenienced if I'm making an active effort to prevent poorly documented changes from getting merged (which often result in regressions). Oh wait, I'm not sorry at all. Nevermind.

Devin

Signature · 2 lines
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
author
#12 lore raw

Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

Quoted 10 lines · Devin Heitmueller

Hello Andreas,

Quoted 2 lines · Andreas Oberritter

WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free to analyze the code and resubmit it.

1.  It's marked with a subject line that starts with "[PATCH]"
2.  It has your SIgned-Off-By line.
3.  it was sent to the mailing list.
4.  It doesn't have any keywords like "RFC" or "proposed".

Devin,

You're over-reacting. This patch were a reply from Andreas to a thread, and not a separate patch submission.

Patches like are generally handled as RFC, especially since it doesn't contain a description.

Quoted 14 lines

If you don't intend for it to go upstream then don't do all of the above.

I'm not sure if your "WTF, Devin, you again?" is some indication that I'm annoying you. The last patch you submitted that touches the threading in dvb_frontend.c had a host of problems and was clearly not well researched (i.e. "DVB: dvb_frontend: convert semaphore to mutex"). As in this case, there is no background indicating that this patch has been fully thought out and due diligence has been done.

Maybe you have thoroughly researched the change, taken the time to fully understand its effects, and tested it with a variety of boards and scenarios. Without a good patch description, there is no way to know.

Quoted 6 lines

I apologize if you're inconvenienced if I'm making an active effort to prevent poorly documented changes from getting merged (which often result in regressions). Oh wait, I'm not sorry at all. Nevermind.

Devin

Regards,
Mauro

#13 lore raw

Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

Hello Mauro,

Quoted 7 lines · Mauro Carvalho Chehab

Devin,

You're over-reacting. This patch were a reply from Andreas to a thread, and not a separate patch submission.

Patches like are generally handled as RFC, especially since it doesn't contain a description.

Any email that starts with "WTF, Devin, you again?" will probably not get a very polite response.

I agree there's been some overreaction, but it hasn't been on my part.
He took the time to split it onto a new thread, add the subject line
"PATCH", as well as adding an SOB. Even if his intent was only to get it reviewed, why should I waste half an hour of my time analyzing his patch to try to figure out his intent if he isn't willing to simply document it?

You have a history of blindly accepting such patches without review. My only intent was to flag this patch to ensure that this didn't happen here. I've spent way more time than I should have to fixing obscure race conditions in dvb core. If the author of a patch cannot take the time to document his findings to provide context then the patch should be rejected without review until he does so.

And why isn't this broken into a patch series? Even after you analyzed the patch you still don't understand what the changes do and why there are being made. Your explanation for why he added the "mb()" call was because "Probably Andreas added it because he noticed some race condition". What is the race condition? Did he find multiple race conditions? Is this patch multiple fixes for a race condition and some other crap at the same time?

If a developer wants a patch reviewed (as Andreas suggested was the case here after-the-fact), then here's my feedback: break this into a series of small incremental patches which *in detail* describe the problem that was found and how each patch addresses the issue. Once we have that, the maintainer can do a more in-depth analysis of whether the patch should be accepted. Code whose function cannot be explicitly justified but simply 'looks better' should not be mixed in with real functional changes.

Devin

Signature · 2 lines
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
author
#14 lore raw

Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

Quoted 15 lines · Devin Heitmueller

Hello Mauro,

Quoted 7 lines · Mauro Carvalho Chehab

Devin,

You're over-reacting. This patch were a reply from Andreas to a thread, and not a separate patch submission.

Patches like are generally handled as RFC, especially since it doesn't contain a description.

Any email that starts with "WTF, Devin, you again?" will probably not get a very polite response.

Quoted 6 lines

I agree there's been some overreaction, but it hasn't been on my part.
He took the time to split it onto a new thread, add the subject line
"PATCH", as well as adding an SOB. Even if his intent was only to get it reviewed, why should I waste half an hour of my time analyzing his patch to try to figure out his intent if he isn't willing to simply document it?

Both overacted, but this doesn't bring anything good.

Quoted 1 line

You have a history of blindly accepting such patches without review.

No. I always review all patches I receive. Yeah, I have to confess:
I'm not a robot, I'm not infallible ;) (well, even a robot would hardly be infallible, anyway).

Quoted 14 lines

My only intent was to flag this patch to ensure that this didn't happen here. I've spent way more time than I should have to fixing obscure race conditions in dvb core. If the author of a patch cannot take the time to document his findings to provide context then the patch should be rejected without review until he does so.

And why isn't this broken into a patch series? Even after you analyzed the patch you still don't understand what the changes do and why there are being made. Your explanation for why he added the "mb()" call was because "Probably Andreas added it because he noticed some race condition". What is the race condition? Did he find multiple race conditions? Is this patch multiple fixes for a race condition and some other crap at the same time?

Quoted 8 lines

If a developer wants a patch reviewed (as Andreas suggested was the case here after-the-fact), then here's my feedback: break this into a series of small incremental patches which *in detail* describe the problem that was found and how each patch addresses the issue. Once we have that, the maintainer can do a more in-depth analysis of whether the patch should be accepted. Code whose function cannot be explicitly justified but simply 'looks better' should not be mixed in with real functional changes.

I understand that you want patches better documented, so do I, and it would be great if this patch had a better description since the beginning.

Yet, I don't agree that this patch should be split. It does just one
thing: it fixes the timeout handling for the dvb core frontend thread.

Regards,
Mauro

author
#15 lore raw

Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

In reply to Andreas Oberritter
Quoted 31 lines · Andreas Oberritter
Quoted 19 lines · Mauro Carvalho Chehab
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 5 lines · Devin Heitmueller
Quoted 3 lines · Mauro Carvalho Chehab
Quoted 1 line

Could someone explain reason for that?

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I dunno, but I think this needs to be fixed, at least when the frontend is opened with O_NONBLOCK.

Are you doing the drx-k firmware load on dvb_init()? That could easily take 4 seconds.

No. The firmware were opened previously.

Maybe the delay is due to this part of dvb_frontend.c:

static int dvb_mfe_wait_time = 5;
...

Quoted 10 lines
                         int mferetry = (dvb_mfe_wait_time<<  1);

                         mutex_unlock (&adapter->mfe_lock);
                         while (mferetry--&&  (mfedev->users != -1 ||
                                         mfepriv->thread != NULL)) {
                                 if(msleep_interruptible(500)) {
                                         if(signal_pending(current))
                                                 return -EINTR;
                                 }
                         }
Quoted 8 lines

I haven't looked at the mfe code, but in case it's waiting for the frontend thread to exit, there's a problem that causes the thread not to exit immediately. Here's a patch that's been sitting in my queue for a while:

---

Signed-off-by Andreas Oberritter<[email protected]>

Andreas,

Thanks for the patch!

Devin,

linux/drivers/media/dvb/dvb-core/dvb_frontend.c line 518

Quoted 11 lines
linux/drivers/media/dvb/dvb-core/dvb_frontend.c +1 -1
index 7784d74..6823c2b 100644
@@ -514,7 +514,7 @@
  		return 1;
 
  	if (fepriv->dvbdev->writers == 1)
-		if (time_after(jiffies, fepriv->release_jiffies +
+		if (time_after_eq(jiffies, fepriv->release_jiffies +
  				  dvb_shutdown_timeout * HZ))

The only change here is that it will now use dvb_shutdown_timeout instead of (dvb_shutdown_timeout * HZ + 1).

This makes sense.

linux/drivers/media/dvb/dvb-core/dvb_frontend.c line 2075

Quoted 10 lines
linux/drivers/media/dvb/dvb-core/dvb_frontend.c +2 -1
  			return 1;
 

@@ -2070,12 +2070,15 @@
 
  	dprintk ("%s\n", __func__);
 
-	if ((file->f_flags&  O_ACCMODE) != O_RDONLY)
+	if ((file->f_flags&  O_ACCMODE) != O_RDONLY) {
  		fepriv->release_jiffies = jiffies;
+		mb();

This is just a memory barrier to warrant that all CPU's will consider the new value for release_jiffies. Probably Andreas added it because he noticed some race condition.

In any case, this won't cause any regressions.

linux/drivers/media/dvb/dvb-core/dvb_frontend.c line 2081

Quoted 6 lines
linux/drivers/media/dvb/dvb-core/dvb_frontend.c +2 -0
+	}
 
  	ret = dvb_generic_release (inode, file);
 
  	if (dvbdev->users == -1) {
+		wake_up(&fepriv->wait_queue);

This is the only hook that changes the core behavior.

linux/drivers/media/dvb/dvb-core/dvb_frontend.c line 2084

Quoted 3 lines
linux/drivers/media/dvb/dvb-core/dvb_frontend.c +0 -0
  		if (fepriv->exit != DVB_FE_NO_EXIT) {
  			fops_put(file->f_op);
  			file->f_op = NULL;

With this change, the current code at dvb_frontend_release() wil; be:

        ret = dvb_generic_release (inode, file);

     	if (dvbdev->users == -1) {
		wake_up(&fepriv->wait_queue);
                if (fepriv->exit != DVB_FE_NO_EXIT) {
                       	fops_put(file->f_op);
                        file->f_op = NULL;
                        wake_up(&dvbdev->wait_queue);
                }
               	if (fe->ops.ts_bus_ctrl)
                        fe->ops.ts_bus_ctrl(fe, 0);
        }

The addition of a wake_up there is that the wake_up thread will be called also when fepriv->exit == DVB_FE_NO_EXIT. This seems to make sense, as dvb_frontend_thread() explicitly tests for it at:

                wait_event_interruptible_timeout(fepriv->wait_queue,
                        dvb_frontend_should_wakeup(fe) || kthread_should_stop()
                                || freezing(current),
                        fepriv->delay);

as dvb_frontend_should_wakeup(fe) is defined (after applying this patch) as:

static int dvb_frontend_is_exiting(struct dvb_frontend *fe)

{
        struct dvb_frontend_private *fepriv = fe->frontend_priv;

        if (fepriv->exit != DVB_FE_NO_EXIT)
                return 1;

        if (fepriv->dvbdev->writers == 1)
                if (time_after_eq(jiffies, fepriv->release_jiffies +
                                  dvb_shutdown_timeout * HZ))
                       	return 1;

       	return 0;
}

static int dvb_frontend_should_wakeup(struct dvb_frontend *fe)

{
        struct dvb_frontend_private *fepriv = fe->frontend_priv;

        if (fepriv->wakeup) {
                fepriv->wakeup = 0;
                return 1;
        }
	return dvb_frontend_is_exiting(fe);
}

So, this code makes sense to me. Btw, a wait queue can wait even without an explicit call, since it is just something like [1]:

	do schedule() while (!condition);

So, all this patch would hurt would be to increase the chance for us to detect a bug that it is already there.

Devin,

I'll do some tests here with a few devices, but, in principle, I don't see any reason why not applying this patch. So, except if I detect something wrong on my tests, of if you you point us for a regression caused by this change, I'll apply it.

Of course, it would be nice if Andreas could add some comments, but if he doesn't, I can write something. It won't be the first patch that the maintainer would need to insert some description.

Regards,
Mauro.

[1] The actual implementation is a more complex than that loop. In this specific case, as it uses the interruptible version, any signal would also wake this thread.

author
#16 lore raw

PATCH v2 1/1 drxk: Switch the delivery system on FE_SET_PROPERTY

In reply to Antti Palosaari

The DRX-K doesn't change the delivery system at set_properties, but do it at frontend init. This causes problems on programs like w_scan that, by default, opens both frontends.

Use adap->mfe_shared in order to prevent this, and be sure that Annex A or C are properly selected.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

v2: Use mfe_shared

 drivers/media/dvb/frontends/drxk_hard.c |   16 ++++++++++------
 drivers/media/dvb/frontends/drxk_hard.h |    2 ++
 drivers/media/video/em28xx/em28xx-dvb.c |    4 ++++
 3 files changed, 16 insertions(+), 6 deletions(-)
drivers/media/dvb/frontends/drxk_hard.c +10 -6
index 95cbc98..388b815 100644
@@ -1847,6 +1847,7 @@ static int SetOperationMode(struct drxk_state *state,
 		*/
 	switch (oMode) {
 	case OM_DVBT:
+		dprintk(1, ": DVB-T\n");
 		state->m_OperationMode = oMode;
 		status = SetDVBTStandard(state, oMode);
 		if (status < 0)

@@ -1854,6 +1855,8 @@ static int SetOperationMode(struct drxk_state *state,
 		break;
 	case OM_QAM_ITU_A:	/* fallthrough */
 	case OM_QAM_ITU_C:
+		dprintk(1, ": DVB-C Annex %c\n",
+			(state->m_OperationMode == OM_QAM_ITU_A) ? 'A' : 'C');
 		state->m_OperationMode = oMode;
 		status = SetQAMStandard(state, oMode);
 		if (status < 0)

@@ -6183,7 +6186,10 @@ static int drxk_c_init(struct dvb_frontend *fe)
 	dprintk(1, "\n");
 	if (mutex_trylock(&state->ctlock) == 0)
 		return -EBUSY;
-	SetOperationMode(state, OM_QAM_ITU_A);
+	if (state->m_itut_annex_c)
+		SetOperationMode(state, OM_QAM_ITU_C);
+	else
+		SetOperationMode(state, OM_QAM_ITU_A);
 	return 0;
 }
 

@@ -6219,13 +6225,11 @@ static int drxk_set_parameters(struct dvb_frontend *fe,
 		return -EINVAL;
 	}
 
-	if (state->m_OperationMode == OM_QAM_ITU_A ||
-	    state->m_OperationMode == OM_QAM_ITU_C) {
+	if (fe->ops.info.type == FE_QAM) {
 		if (fe->dtv_property_cache.rolloff == ROLLOFF_13)
-			state->m_OperationMode = OM_QAM_ITU_C;
+			state->m_itut_annex_c = true;
 		else
-			state->m_OperationMode = OM_QAM_ITU_A;
-	}
+			state->m_itut_annex_c = false;
 
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 1);
drivers/media/dvb/frontends/drxk_hard.h +2 -0
index a05c32e..85a423f 100644
@@ -263,6 +263,8 @@ struct drxk_state {
 	u8     m_TSDataStrength;
 	u8     m_TSClockkStrength;
 
+	bool   m_itut_annex_c;      /* If true, uses ITU-T DVB-C Annex C, instead of Annex A */
+
 	enum DRXMPEGStrWidth_t  m_widthSTR;    /**< MPEG start width */
 	u32    m_mpegTsStaticBitrate;          /**< Maximum bitrate in b/s in case
 						    static clockrate is selected */
drivers/media/video/em28xx/em28xx-dvb.c +4 -0
index 7f0592c..3868c1e 100644
@@ -899,6 +899,8 @@ static int em28xx_dvb_init(struct em28xx *dev)
 		       &dvb->fe[0]->ops.tuner_ops,
 		       sizeof(dvb->fe[0]->ops.tuner_ops));
 
+		mfe_shared = 1;
+
 		break;
 	}
 	case EM2884_BOARD_TERRATEC_H5:

@@ -935,6 +937,8 @@ static int em28xx_dvb_init(struct em28xx *dev)
 		       &dvb->fe[0]->ops.tuner_ops,
 		       sizeof(dvb->fe[0]->ops.tuner_ops));
 
+		mfe_shared = 1;
+
 		break;
 	case EM28174_BOARD_PCTV_460E:
 		/* attach demod */
Signature · 1 line
1.7.8
#17 lore raw

Hi,

Quoted 1 line

v2: Use mfe_shared

on hvr930c this patch solve the commutation issue of frontend.

still persist same issue like Fredrik on w_scan. scan still works perfectly...

root@depieri1lnx:~# w_scan -f t -c IT
w_scan version 20110616 (compiled for DVB API 5.3)
using settings for ITALY
DVB aerial
DVB-T Europe
frontend_type DVB-T, channellist 4
output format vdr-1.6
output charset 'UTF-8', use -C <charset> to override
Info: using DVB adapter auto detection.
	/dev/dvb/adapter0/frontend0 -> DVB-C "DRXK DVB-C": specified was
DVB-T -> SEARCH NEXT ONE.
	/dev/dvb/adapter0/frontend1 -> DVB-T "DRXK DVB-T": good :-)
Using DVB-T frontend (adapter /dev/dvb/adapter0/frontend1)
-_-_-_-_ Getting frontend capabilities-_-_-_-_
Using DVB API 5.4
frontend 'DRXK DVB-T' supports
INVERSION_AUTO
QAM_AUTO
TRANSMISSION_MODE_AUTO
GUARD_INTERVAL_AUTO
HIERARCHY_AUTO
FEC_AUTO
FREQ (47.12MHz ... 865.00MHz)
-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_
Scanning 7MHz frequencies...
[...]
858000: (time: 03:10) (time: 03:13)

ERROR: Sorry - i couldn't get any working frequency/transponder
Nothing to scan!!

this verbose mode seems interesting but I don't figure out why i get timeout.

177500: (time: 00:04) set_frontend: using DVB API 5.4 (time: 00:06) set_frontend: using DVB API 5.4 signal ok:

	QAM_AUTO f = 177500 kHz I999B7C999D999T999G999Y999
NIT (actual TS)
	new transponder:
	   (QAM_64   f = 1600000 kHz I999B8C34D0T8G16Y0)

Info: NIT(other) filter timeout

author
#18 lore raw
Quoted 5 lines · Eddi De Pieri

Hi,

Quoted 1 line

v2: Use mfe_shared

on hvr930c this patch solve the commutation issue of frontend.

Ok, good. One issue solved.

Quoted 40 lines

still persist same issue like Fredrik on w_scan. scan still works perfectly...

root@depieri1lnx:~# w_scan -f t -c IT
w_scan version 20110616 (compiled for DVB API 5.3)
using settings for ITALY
DVB aerial
DVB-T Europe
frontend_type DVB-T, channellist 4
output format vdr-1.6
output charset 'UTF-8', use -C<charset>  to override
Info: using DVB adapter auto detection.
	/dev/dvb/adapter0/frontend0 ->  DVB-C "DRXK DVB-C": specified was
DVB-T ->  SEARCH NEXT ONE.
	/dev/dvb/adapter0/frontend1 ->  DVB-T "DRXK DVB-T": good :-)
Using DVB-T frontend (adapter /dev/dvb/adapter0/frontend1)
-_-_-_-_ Getting frontend capabilities-_-_-_-_
Using DVB API 5.4
frontend 'DRXK DVB-T' supports
INVERSION_AUTO
QAM_AUTO
TRANSMISSION_MODE_AUTO
GUARD_INTERVAL_AUTO
HIERARCHY_AUTO
FEC_AUTO
FREQ (47.12MHz ... 865.00MHz)
-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_
Scanning 7MHz frequencies...
[...]
858000: (time: 03:10) (time: 03:13)

ERROR: Sorry - i couldn't get any working frequency/transponder
Nothing to scan!!

this verbose mode seems interesting but I don't figure out why i get timeout.

177500: (time: 00:04) set_frontend: using DVB API 5.4 (time: 00:06) set_frontend: using DVB API 5.4 signal ok:

Quoted 7 lines
	QAM_AUTO f = 177500 kHz I999B7C999D999T999G999Y999
NIT (actual TS)
	new transponder:
	   (QAM_64   f = 1600000 kHz I999B8C34D0T8G16Y0)

Info: NIT(other) filter timeout

Try to use w_scan with "-F" parameter, to increase the timeout. Maybe this issue is due to a smaller timeout on w_scan, when compared with scan.

If this doesn't help, then you'll need to figure out what w_scan is doing different than scan.

Regards,
Mauro.

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#19 lore raw
In reply to Mauro Carvalho Chehab
Quoted 8 lines · On Friday 09 December 2011 20:00:12 Mauro Carvalho Chehab wrote:

The DRX-K doesn't change the delivery system at set_properties, but do it at frontend init. This causes problems on programs like w_scan that, by default, opens both frontends.

Use adap->mfe_shared in order to prevent this, and be sure that Annex A or C are properly selected.

Signed-off-by Mauro Carvalho Chehab <[email protected]>
Quoted 8 lines

---

v2: Use mfe_shared

 drivers/media/dvb/frontends/drxk_hard.c |   16 ++++++++++------
 drivers/media/dvb/frontends/drxk_hard.h |    2 ++
 drivers/media/video/em28xx/em28xx-dvb.c |    4 ++++
 3 files changed, 16 insertions(+), 6 deletions(-)

...

Please commit Manu's patch to 'Query DVB frontend delivery capabilities'. Then you will no longer have to struggle with multi-frontend problems.

We could finally get rid of having 2 mutual-exclusive frontends, which is just an ugly workaround, barely covered by the API spec...

CU
Oliver

Signature · 10 lines
----------------------------------------------------------------
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/
----------------------------------------------------------------
Oliver Endriss                         ESCAPE GmbH
e-mail:  [email protected]       EDV-Loesungen
phone:   +49 (0)7722 21504             Birkenweg 9
fax:     +49 (0)7722 21510             D-78098 Triberg
----------------------------------------------------------------
author
#20 lore raw
Quoted 13 lines · Oliver Endriss
Quoted 12 lines · On Friday 09 December 2011 20:00:12 Mauro Carvalho Chehab wrote:

The DRX-K doesn't change the delivery system at set_properties, but do it at frontend init. This causes problems on programs like w_scan that, by default, opens both frontends.

Use adap->mfe_shared in order to prevent this, and be sure that Annex A or C are properly selected.

Signed-off-by Mauro Carvalho Chehab<[email protected]>

---

v2: Use mfe_shared

Quoted 8 lines
Quoted 4 lines
  drivers/media/dvb/frontends/drxk_hard.c |   16 ++++++++++------
  drivers/media/dvb/frontends/drxk_hard.h |    2 ++
  drivers/media/video/em28xx/em28xx-dvb.c |    4 ++++
  3 files changed, 16 insertions(+), 6 deletions(-)

...

Please commit Manu's patch to 'Query DVB frontend delivery capabilities'. Then you will no longer have to struggle with multi-frontend problems.

I was waiting for him to submit the new version, as there were several comments on the last series. Just checked that he submitted the new version today. That will be a great improvement!

Quoted 2 lines

We could finally get rid of having 2 mutual-exclusive frontends, which is just an ugly workaround, barely covered by the API spec...

Agreed.

Regards,
Mauro.