Skip to contentSkip to conversation

Patch for stack/DMA problems in Cinergy T2 drivers (2)

PATCH [email protected] 3 participants 6 messages Jul 31, 2009 – Aug 1, 2009 Media (V4L/DVB)

lore
Share & tools
lore

[email protected]

author
#0 lore raw

Here's a patch for cinergyT2-core.c:

drivers/media/dvb/dvb-usb/cinergyT2-fe.c +111 -42
@@ -146,66 +146,103 @@
 					fe_status_t *status)
 {
 	struct cinergyt2_fe_state *state = fe->demodulator_priv;
-	struct dvbt_get_status_msg result;
-	u8 cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+	struct dvbt_get_status_msg *result;
+	static const u8 cmd0[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+        u8 *cmd;
 	int ret;
 
-	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (u8 *)&result,
-			sizeof(result), 0);
-	if (ret < 0)
+        cmd = kmalloc(sizeof(cmd0), GFP_KERNEL);
+        if (!cmd) return -ENOMEM;
+        memcpy(cmd, cmd0, sizeof(cmd0));
+        result = kmalloc(sizeof(*result), GFP_KERNEL);
+        if (!result) {
+                kfree(cmd);
+                return -ENOMEM;
+        }
+	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd0), (u8 *)result,
+			sizeof(*result), 0);
+        kfree(cmd);
+	if (ret < 0) {
+                kfree(result);
 		return ret;
+        }
 
 	*status = 0;
 
-	if (0xffff - le16_to_cpu(result.gain) > 30)
+	if (0xffff - le16_to_cpu(result->gain) > 30)
 		*status |= FE_HAS_SIGNAL;
-	if (result.lock_bits & (1 << 6))
+	if (result->lock_bits & (1 << 6))
 		*status |= FE_HAS_LOCK;
-	if (result.lock_bits & (1 << 5))
+	if (result->lock_bits & (1 << 5))
 		*status |= FE_HAS_SYNC;
-	if (result.lock_bits & (1 << 4))
+	if (result->lock_bits & (1 << 4))
 		*status |= FE_HAS_CARRIER;
-	if (result.lock_bits & (1 << 1))
+	if (result->lock_bits & (1 << 1))
 		*status |= FE_HAS_VITERBI;
 
 	if ((*status & (FE_HAS_CARRIER | FE_HAS_VITERBI | FE_HAS_SYNC)) !=
 			(FE_HAS_CARRIER | FE_HAS_VITERBI | FE_HAS_SYNC))
 		*status &= ~FE_HAS_LOCK;
 
+        kfree(result);
 	return 0;
 }
 
 static int cinergyt2_fe_read_ber(struct dvb_frontend *fe, u32 *ber)
 {
 	struct cinergyt2_fe_state *state = fe->demodulator_priv;
-	struct dvbt_get_status_msg status;
-	char cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+	struct dvbt_get_status_msg *status;
+	static const u8 cmd0[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+        u8 *cmd;
 	int ret;
 
-	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (char *)&status,
-				sizeof(status), 0);
-	if (ret < 0)
+        cmd = kmalloc(sizeof(cmd0), GFP_KERNEL);
+        if (!cmd) return -ENOMEM;
+        memcpy(cmd, cmd0, sizeof(cmd0));
+        status = kmalloc(sizeof(*status), GFP_KERNEL);
+        if (!status) {
+                kfree(cmd);
+                return -ENOMEM;
+        }
+	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd0), (char *)status,
+				sizeof(*status), 0);
+        kfree(cmd);
+	if (ret < 0) {
+                kfree(status);
 		return ret;
-
-	*ber = le32_to_cpu(status.viterbi_error_rate);
+        }
+	*ber = le32_to_cpu(status->viterbi_error_rate);
+        kfree(status);
 	return 0;
 }
 
 static int cinergyt2_fe_read_unc_blocks(struct dvb_frontend *fe, u32 *unc)
 {
 	struct cinergyt2_fe_state *state = fe->demodulator_priv;
-	struct dvbt_get_status_msg status;
-	u8 cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+	struct dvbt_get_status_msg *status;
+	static const u8 cmd0[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+        u8 *cmd;
 	int ret;
 
-	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (u8 *)&status,
-				sizeof(status), 0);
+        cmd = kmalloc(sizeof(cmd0), GFP_KERNEL);
+        if (!cmd) return -ENOMEM;
+        memcpy(cmd, cmd0, sizeof(cmd0));
+        status = kmalloc(sizeof(*status), GFP_KERNEL);
+        if (!status) {
+                kfree(cmd);
+                return -ENOMEM;
+        }
+	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd0), (u8 *)status,
+				sizeof(*status), 0);
+        kfree(cmd);
 	if (ret < 0) {
+                kfree(status);
 		err("cinergyt2_fe_read_unc_blocks() Failed! (Error=%d)\n",
 			ret);
 		return ret;
 	}
-	*unc = le32_to_cpu(status.uncorrected_block_count);
+	*unc = le32_to_cpu(status->uncorrected_block_count);
+        kfree(status);
 	return 0;
 }
 

@@ -213,35 +250,59 @@
 						u16 *strength)
 {
 	struct cinergyt2_fe_state *state = fe->demodulator_priv;
-	struct dvbt_get_status_msg status;
-	char cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+	struct dvbt_get_status_msg *status;
+	static const u8 cmd0[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+        u8 *cmd;
 	int ret;
 
-	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (char *)&status,
-				sizeof(status), 0);
+        cmd = kmalloc(sizeof(cmd0), GFP_KERNEL);
+        if (!cmd) return -ENOMEM;
+        memcpy(cmd, cmd0, sizeof(cmd0));
+        status = kmalloc(sizeof(*status), GFP_KERNEL);
+        if (!status) {
+                kfree(cmd);
+                return -ENOMEM;
+        }
+	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd0), (char *)status,
+				sizeof(*status), 0);
+        kfree(cmd);
 	if (ret < 0) {
+                kfree(status);
 		err("cinergyt2_fe_read_signal_strength() Failed!"
 			" (Error=%d)\n", ret);
 		return ret;
 	}
-	*strength = (0xffff - le16_to_cpu(status.gain));
+	*strength = (0xffff - le16_to_cpu(status->gain));
+        kfree(status);
 	return 0;
 }
 
 static int cinergyt2_fe_read_snr(struct dvb_frontend *fe, u16 *snr)
 {
 	struct cinergyt2_fe_state *state = fe->demodulator_priv;
-	struct dvbt_get_status_msg status;
-	char cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+	struct dvbt_get_status_msg *status;
+	static const u8 cmd0[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+        u8 *cmd;
 	int ret;
 
-	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (char *)&status,
-				sizeof(status), 0);
+        cmd = kmalloc(sizeof(cmd0), GFP_KERNEL);
+        if (!cmd) return -ENOMEM;
+        memcpy(cmd, cmd0, sizeof(cmd0));
+        status = kmalloc(sizeof(*status), GFP_KERNEL);
+        if (!status) {
+                kfree(cmd);
+                return -ENOMEM;
+        }
+	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd0), (char *)status,
+				sizeof(*status), 0);
+        kfree(cmd);
 	if (ret < 0) {
+                kfree(status);
 		err("cinergyt2_fe_read_snr() Failed! (Error=%d)\n", ret);
 		return ret;
 	}
-	*snr = (status.snr << 8) | status.snr;
+	*snr = (status->snr << 8) | status->snr;
+        kfree(status);
 	return 0;
 }
 

@@ -267,19 +328,27 @@
 				  struct dvb_frontend_parameters *fep)
 {
 	struct cinergyt2_fe_state *state = fe->demodulator_priv;
-	struct dvbt_set_parameters_msg param;
-	char result[2];
+        struct dvbt_set_parameters_msg *param;
+        char *result;
 	int err;
 
-	param.cmd = CINERGYT2_EP1_SET_TUNER_PARAMETERS;
-	param.tps = cpu_to_le16(compute_tps(fep));
-	param.freq = cpu_to_le32(fep->frequency / 1000);
-	param.bandwidth = 8 - fep->u.ofdm.bandwidth - BANDWIDTH_8_MHZ;
-
+        param = kmalloc(sizeof(*param), GFP_KERNEL);
+        if (!param) return -ENOMEM;
+	param->cmd = CINERGYT2_EP1_SET_TUNER_PARAMETERS;
+	param->tps = cpu_to_le16(compute_tps(fep));
+	param->freq = cpu_to_le32(fep->frequency / 1000);
+	param->bandwidth = 8 - fep->u.ofdm.bandwidth - BANDWIDTH_8_MHZ;
+        result = kmalloc(2, GFP_KERNEL);
+        if (!result) {
+                kfree(param);
+                return -ENOMEM;
+        }
 	err = dvb_usb_generic_rw(state->d,
-			(char *)&param, sizeof(param),
-			result, sizeof(result), 0);
-	if (err < 0)
+			(char *)param, sizeof(*param),
+                        result, 2, 0);
+        kfree(param);
+        kfree(result);
+        if (err < 0)
 		err("cinergyt2_fe_set_frontend() Failed! err=%d\n", err);
 
 	return (err < 0) ? err : 0;
#1 lore raw
Quoted 23 lines · On Fri, Jul 31, 2009 at 10:25:20PM +0200, [email protected] wrote:

Here's a patch for cinergyT2-core.c:

drivers/media/dvb/dvb-usb/cinergyT2-fe.c +8 -5
@@ -146,66 +146,103 @@
 					fe_status_t *status)
 {
 	struct cinergyt2_fe_state *state = fe->demodulator_priv;
-	struct dvbt_get_status_msg result;
-	u8 cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+	struct dvbt_get_status_msg *result;
+	static const u8 cmd0[] = { CINERGYT2_EP1_GET_TUNER_STATUS };
+        u8 *cmd;
 	int ret;
 
-	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (u8 *)&result,
-			sizeof(result), 0);
-	if (ret < 0)
+        cmd = kmalloc(sizeof(cmd0), GFP_KERNEL);
+        if (!cmd) return -ENOMEM;
+        memcpy(cmd, cmd0, sizeof(cmd0));
+        result = kmalloc(sizeof(*result), GFP_KERNEL);
+        if (!result) {

drivers/media/dvb/dvb-usb/cinergyT2-fe.c line 168

Quoted 10 lines
drivers/media/dvb/dvb-usb/cinergyT2-fe.c +9 -0
+                kfree(cmd);
+                return -ENOMEM;
+        }
+	ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd0), (u8 *)result,
+			sizeof(*result), 0);
+        kfree(cmd);
+	if (ret < 0) {
+                kfree(result);
 		return ret;
+        }

There is a fair amount of code duplication. A better aproach would be to allocate buffers once in cinergyt2_fe_attach() (add them to struct cinergyt2_fe_state).

And please observe http://linuxtv.org/hg/v4l-dvb/raw-file/tip/README.patches

author
#2 lore raw
Quoted 3 lines · Johannes Stezenbach

There is a fair amount of code duplication. A better aproach would be to allocate buffers once in cinergyt2_fe_attach() (add them to struct cinergyt2_fe_state).

Yes, but first I have to investigate why tuning is still quite unreliable (ie, more unreliable than in 2.6.29).

Am I really the only one who has those problems with the Cinergy T2 driver in 2.6.30?

author
#3 lore raw

I've found another bug in the Cinergy T2 driver: originally (ie, e.g. in kernel 2.6.23), the structure containing "struct dvbt_set_parameters_msg param" was allocated with kzalloc, now "struct dvbt_set_parameters_msg param" lives on the stack. However, the "flags" member of that structure is not initialized to zero (as was done by kzalloc)!

-emagick

author
#4 lore raw
In reply to Johannes Stezenbach
Quoted 3 lines · Johannes Stezenbach

There is a fair amount of code duplication. A better aproach would be to allocate buffers once in cinergyt2_fe_attach() (add them to struct cinergyt2_fe_state).

That's how it was done in the old cinergyT2 driver.

Does the code in DVB frontend code assure that the frontend ops are entered by only one thread at a time?

-emagick

#5 lore raw
In reply to [email protected]
Quoted 1 line · [email protected] wrote:

Here's a patch for cinergyT2-core.c:

More like cinergyT2-fe.c according to the patch :)

Anyway, thanks for the patch. I can confirm the problem with the current Ubuntu Karmic alpha kernel (2.6.31-4.23). If I use the modules of the current hg tree from http://linuxtv.org/hg/v4l-dvb I still have the problem but with your patch I can use the Cinergy T². However it seems to be not yet perfect because I can't use the KDE4 version of kaffeine, only the KDE3 version (from Ubuntu 9.04). According to the error message the KDE4 version doesn't find a working adapter.

Nils