diff mbox

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

Message ID 4A735330.1000406@magic.ms (mailing list archive)
State Rejected
Headers show

Commit Message

emagick@magic.ms July 31, 2009, 8:25 p.m. UTC 
Here's a patch for cinergyT2-core.c:


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

Comments

Johannes Stezenbach July 31, 2009, 9:40 p.m. UTC | #1 
On Fri, Jul 31, 2009 at 10:25:20PM +0200, emagick@magic.ms wrote:
> Here's a patch for cinergyT2-core.c:
> 
> --- a/drivers/media/dvb/dvb-usb/cinergyT2-fe.c	2009-06-10 05:05:27.000000000 +0200
> +++ b/drivers/media/dvb/dvb-usb/cinergyT2-fe.c	2009-07-31 22:02:48.000000000 +0200
> @@ -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;
> +        }

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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
emagick@magic.ms Aug. 1, 2009, 8:27 a.m. UTC | #2 
Johannes Stezenbach wrote:

> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
emagick@magic.ms Aug. 1, 2009, 1:23 p.m. UTC | #3 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
emagick@magic.ms Aug. 1, 2009, 1:34 p.m. UTC | #4 
Johannes Stezenbach wrote:

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nils Kassube Aug. 1, 2009, 4 p.m. UTC | #5 
emagick@magic.ms 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

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

Patch

--- a/drivers/media/dvb/dvb-usb/cinergyT2-fe.c	2009-06-10 05:05:27.000000000 +0200
+++ b/drivers/media/dvb/dvb-usb/cinergyT2-fe.c	2009-07-31 22:02:48.000000000 +0200
@@ -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;