[PATCH] [media] dib0700: get rid of on-stack dma buffers

[PATCH] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

This should fix warnings seen by some:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
Signed-off-by: Florian Mickler <florian@mickler.org>
CC: Mauro Carvalho Chehab <mchehab@infradead.org>
CC: linux-media@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Greg Kroah-Hartman <greg@kroah.com>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Maciej Rutecki <maciej.rutecki@gmail.com>
---

Please take a look at it, as I do not do that much kernel hacking
and don't wanna brake anybodys computer... :)

>From my point of view this should _not_ go to stable even though it would
be applicable. But if someone feels strongly about that and can
take responsibility for that change...


drivers/media/dvb/dvb-usb/dib0700_core.c |  121 +++++++++++++++++++++++-------
 1 files changed, 94 insertions(+), 27 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..1a12182 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
 			u32 *romversion, u32 *ramversion, u32 *fwtype)
 {
-	u8 b[16];
-	int ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
+	int ret;
+	u8 *b;
+
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
+	ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
 				  REQUEST_GET_VERSION,
 				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+				  b, 16, USB_CTRL_GET_TIMEOUT);
 	if (hwversion != NULL)
 		*hwversion  = (b[0] << 24)  | (b[1] << 16)  | (b[2] << 8)  | b[3];
 	if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
 		*ramversion = (b[8] << 24)  | (b[9] << 16)  | (b[10] << 8) | b[11];
 	if (fwtype != NULL)
 		*fwtype     = (b[12] << 24) | (b[13] << 16) | (b[14] << 8) | b[15];
+
+	kfree(b);
 	return ret;
 }
 
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 txlen, u8 *rx, u8 rxlen
 
 int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 gpio_dir, u8 gpio_val)
 {
-	u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6) };
-	return dib0700_ctrl_wr(d, buf, sizeof(buf));
+	s16 ret;
+	u8 *buf = kmalloc(3, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	buf[0] = REQUEST_SET_GPIO;
+	buf[1] = gpio;
+	buf[2] = ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6);
+
+	ret = dib0700_ctrl_wr(d, buf, sizeof(buf));
+
+	kfree(buf);
+	return ret;
 }
 
 static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 nb_ts_packets)
@@ -141,13 +160,20 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
 	uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
 	uint8_t en_start = 0;
 	uint8_t en_stop = 0;
-	uint8_t buf[255]; /* TBV: malloc ? */
+	uint8_t *buf;
 	int result, i;
 
+	buf = kmalloc(255, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
 	/* Ensure nobody else hits the i2c bus while we're sending our
 	   sequence of messages, (such as the remote control thread) */
-	if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
-		return -EAGAIN;
+	if (mutex_lock_interruptible(&d->i2c_mutex) < 0) {
+		result = -EAGAIN;
+		goto out;
+	}
+
 
 	for (i = 0; i < num; i++) {
 		if (i == 0) {
@@ -220,8 +246,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
 			}
 		}
 	}
+	result = i;
 	mutex_unlock(&d->i2c_mutex);
-	return i;
+
+out:
+	kfree(buf);
+	return result;
 }
 
 /*
@@ -232,10 +262,17 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 {
 	struct dvb_usb_device *d = i2c_get_adapdata(adap);
 	int i,len;
-	u8 buf[255];
+	s16 ret;
+	u8 *buf;
 
-	if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
-		return -EAGAIN;
+	buf = kmalloc(255, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	if (mutex_lock_interruptible(&d->i2c_mutex) < 0) {
+		ret = -EAGAIN;
+		goto out;
+	}
 
 	for (i = 0; i < num; i++) {
 		/* fill in the address */
@@ -264,9 +301,11 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 				break;
 		}
 	}
-
+	ret = i;
 	mutex_unlock(&d->i2c_mutex);
-	return i;
+out:
+	kfree(buf);
+	return ret;
 }
 
 static int dib0700_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
@@ -297,15 +336,23 @@ struct i2c_algorithm dib0700_i2c_algo = {
 int dib0700_identify_state(struct usb_device *udev, struct dvb_usb_device_properties *props,
 			struct dvb_usb_device_description **desc, int *cold)
 {
-	u8 b[16];
-	s16 ret = usb_control_msg(udev, usb_rcvctrlpipe(udev,0),
+	s16 ret;
+	u8 *b;
+
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return	-ENOMEM;
+
+
+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 		REQUEST_GET_VERSION, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, b, 16, USB_CTRL_GET_TIMEOUT);
 
 	deb_info("FW GET_VERSION length: %d\n",ret);
 
 	*cold = ret <= 0;
-
 	deb_info("cold: %d\n", *cold);
+
+	kfree(b);
 	return 0;
 }
 
@@ -313,7 +360,13 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
 	u8 pll_src, u8 pll_range, u8 clock_gpio3, u16 pll_prediv,
 	u16 pll_loopdiv, u16 free_div, u16 dsuScaler)
 {
-	u8 b[10];
+	s16 ret;
+	u8 *b;
+
+	b = kmalloc(10, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	b[0] = REQUEST_SET_CLOCK;
 	b[1] = (en_pll << 7) | (pll_src << 6) | (pll_range << 5) | (clock_gpio3 << 4);
 	b[2] = (pll_prediv >> 8)  & 0xff; // MSB
@@ -325,7 +378,10 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
 	b[8] = (dsuScaler >> 8)   & 0xff; // MSB
 	b[9] =  dsuScaler         & 0xff; // LSB
 
-	return dib0700_ctrl_wr(d, b, 10);
+	ret = dib0700_ctrl_wr(d, b, 10);
+
+	kfree(b);
+	return ret;
 }
 
 int dib0700_ctrl_clock(struct dvb_usb_device *d, u32 clk_MHz, u8 clock_out_gp3)
@@ -361,11 +417,14 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 {
 	struct hexline hx;
 	int pos = 0, ret, act_len, i, adap_num;
-	u8 b[16];
+	u8 *b;
 	u32 fw_version;
-
 	u8 buf[260];
 
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	while ((ret = dvb_usb_get_hexline(fw, &hx, &pos)) > 0) {
 		deb_fwdata("writing to address 0x%08x (buffer: 0x%02x %02x)\n",
 				hx.addr, hx.len, hx.chk);
@@ -386,7 +445,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 
 		if (ret < 0) {
 			err("firmware download failed at %d with %d",pos,ret);
-			return ret;
+			goto out;
 		}
 	}
 
@@ -407,7 +466,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 	usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 				  REQUEST_GET_VERSION,
 				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+				  b, 16, USB_CTRL_GET_TIMEOUT);
 	fw_version = (b[8] << 24) | (b[9] << 16) | (b[10] << 8) | b[11];
 
 	/* set the buffer size - DVB-USB is allocating URB buffers
@@ -426,16 +485,21 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 			}
 		}
 	}
-
+out:
+	kfree(b);
 	return ret;
 }
 
 int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 {
 	struct dib0700_state *st = adap->dev->priv;
-	u8 b[4];
+	u8 *b;
 	int ret;
 
+	b = kmalloc(4, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	if ((onoff != 0) && (st->fw_version >= 0x10201)) {
 		/* for firmware later than 1.20.1,
 		 * the USB xfer length can be set  */
@@ -443,7 +507,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 			st->nb_packet_buffer_size);
 		if (ret < 0) {
 			deb_info("can not set the USB xfer len\n");
-			return ret;
+			goto out;
 		}
 	}
 
@@ -468,7 +532,10 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 
 	deb_info("data for streaming: %x %x\n", b[1], b[2]);
 
-	return dib0700_ctrl_wr(adap->dev, b, 4);
+	ret = dib0700_ctrl_wr(adap->dev, b, 4);
+out:
+	kfree(b);
+	return ret;
 }
 
 int dib0700_change_protocol(struct rc_dev *rc, u64 rc_type)
-- 
1.7.4.rc3

Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers

[Oliver Neukum]

Am Sonntag, 6. März 2011, 12:16:52 schrieb Florian Mickler:
> This should fix warnings seen by some:
> 	WARNING: at lib/dma-debug.c:866 check_for_stack
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
> Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
> Signed-off-by: Florian Mickler <florian@mickler.org>
> CC: Mauro Carvalho Chehab <mchehab@infradead.org>
> CC: linux-media@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: Greg Kroah-Hartman <greg@kroah.com>
> CC: Rafael J. Wysocki <rjw@sisk.pl>
> CC: Maciej Rutecki <maciej.rutecki@gmail.com>
> ---
> 
> Please take a look at it, as I do not do that much kernel hacking
> and don't wanna brake anybodys computer... :)
> 
> From my point of view this should _not_ go to stable even though it would
> be applicable. But if someone feels strongly about that and can
> take responsibility for that change...

The patch looks good and is needed in stable.
It could be improved by using a buffer allocated once in the places
you hold a mutex anyway.

	Regards
		Oliver

Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

On Sun, 6 Mar 2011 13:06:09 +0100
Oliver Neukum <oliver@neukum.org> wrote:

> Am Sonntag, 6. März 2011, 12:16:52 schrieb Florian Mickler:
> > This should fix warnings seen by some:
> > 	WARNING: at lib/dma-debug.c:866 check_for_stack
> > 
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
> > Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
> > Signed-off-by: Florian Mickler <florian@mickler.org>
> > CC: Mauro Carvalho Chehab <mchehab@infradead.org>
> > CC: linux-media@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
> > CC: Greg Kroah-Hartman <greg@kroah.com>
> > CC: Rafael J. Wysocki <rjw@sisk.pl>
> > CC: Maciej Rutecki <maciej.rutecki@gmail.com>
> > ---
> > 
> > Please take a look at it, as I do not do that much kernel hacking
> > and don't wanna brake anybodys computer... :)
> > 
> > From my point of view this should _not_ go to stable even though it would
> > be applicable. But if someone feels strongly about that and can
> > take responsibility for that change...
> 
> The patch looks good and is needed in stable.
> It could be improved by using a buffer allocated once in the places
> you hold a mutex anyway.
> 
> 	Regards
> 		Oliver

Ok, I now put a buffer member in the priv dib0700_state which gets
allocated on the heap. 

My patch introduces a new error condition in the dib0700_identify_state
callback which gets not checked for in dvb_usb_find_device... 
Should we worry?

Same for dib0700_get_version in the probe callback...
But there, there was already the possibility of usb_control_msg
returning an error...

Regards,
Flo

[PATCH 1/3 v2] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

This should fix warnings seen by some:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
Signed-off-by: Florian Mickler <florian@mickler.org>
CC: Mauro Carvalho Chehab <mchehab@infradead.org>
CC: linux-media@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Greg Kroah-Hartman <greg@kroah.com>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Maciej Rutecki <maciej.rutecki@gmail.com>
CC: Oliver Neukum <oliver@neukum.org>
CC: Jack Stone <jwjstone@fastmail.fm>

[v2: use preallocated buffer where the mutex is held; fix sizeof in one case]
---
 drivers/media/dvb/dvb-usb/dib0700.h      |    5 +-
 drivers/media/dvb/dvb-usb/dib0700_core.c |   92 +++++++++++++++++++++++-------
 2 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700.h b/drivers/media/dvb/dvb-usb/dib0700.h
index 3537d65..1401e7d 100644
--- a/drivers/media/dvb/dvb-usb/dib0700.h
+++ b/drivers/media/dvb/dvb-usb/dib0700.h
@@ -45,8 +45,9 @@ struct dib0700_state {
 	u8 is_dib7000pc;
 	u8 fw_use_new_i2c_api;
 	u8 disable_streaming_master_mode;
-    u32 fw_version;
-    u32 nb_packet_buffer_size;
+	u32 fw_version;
+	u32 nb_packet_buffer_size;
+	u8 buf[255];
 };
 
 extern int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..028ed87 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
 			u32 *romversion, u32 *ramversion, u32 *fwtype)
 {
-	u8 b[16];
-	int ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
+	int ret;
+	u8 *b;
+
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
+	ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
 				  REQUEST_GET_VERSION,
 				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+				  b, 16, USB_CTRL_GET_TIMEOUT);
 	if (hwversion != NULL)
 		*hwversion  = (b[0] << 24)  | (b[1] << 16)  | (b[2] << 8)  | b[3];
 	if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
 		*ramversion = (b[8] << 24)  | (b[9] << 16)  | (b[10] << 8) | b[11];
 	if (fwtype != NULL)
 		*fwtype     = (b[12] << 24) | (b[13] << 16) | (b[14] << 8) | b[15];
+
+	kfree(b);
 	return ret;
 }
 
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 txlen, u8 *rx, u8 rxlen
 
 int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 gpio_dir, u8 gpio_val)
 {
-	u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6) };
-	return dib0700_ctrl_wr(d, buf, sizeof(buf));
+	s16 ret;
+	u8 *buf = kmalloc(3, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	buf[0] = REQUEST_SET_GPIO;
+	buf[1] = gpio;
+	buf[2] = ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6);
+
+	ret = dib0700_ctrl_wr(d, buf, 3);
+
+	kfree(buf);
+	return ret;
 }
 
 static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 nb_ts_packets)
@@ -137,11 +156,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
 	   properly support i2c read calls not preceded by a write */
 
 	struct dvb_usb_device *d = i2c_get_adapdata(adap);
+	struct dib0700_state *st = d->priv;
 	uint8_t bus_mode = 1;  /* 0=eeprom bus, 1=frontend bus */
 	uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
 	uint8_t en_start = 0;
 	uint8_t en_stop = 0;
-	uint8_t buf[255]; /* TBV: malloc ? */
+	uint8_t *buf = st->buf;
 	int result, i;
 
 	/* Ensure nobody else hits the i2c bus while we're sending our
@@ -221,6 +241,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
 		}
 	}
 	mutex_unlock(&d->i2c_mutex);
+
 	return i;
 }
 
@@ -231,8 +252,9 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 				   struct i2c_msg *msg, int num)
 {
 	struct dvb_usb_device *d = i2c_get_adapdata(adap);
+	struct dib0700_state *st = d->priv;
 	int i,len;
-	u8 buf[255];
+	u8 *buf = st->buf;
 
 	if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
 		return -EAGAIN;
@@ -264,8 +286,8 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 				break;
 		}
 	}
-
 	mutex_unlock(&d->i2c_mutex);
+
 	return i;
 }
 
@@ -297,15 +319,23 @@ struct i2c_algorithm dib0700_i2c_algo = {
 int dib0700_identify_state(struct usb_device *udev, struct dvb_usb_device_properties *props,
 			struct dvb_usb_device_description **desc, int *cold)
 {
-	u8 b[16];
-	s16 ret = usb_control_msg(udev, usb_rcvctrlpipe(udev,0),
+	s16 ret;
+	u8 *b;
+
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return	-ENOMEM;
+
+
+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 		REQUEST_GET_VERSION, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, b, 16, USB_CTRL_GET_TIMEOUT);
 
 	deb_info("FW GET_VERSION length: %d\n",ret);
 
 	*cold = ret <= 0;
-
 	deb_info("cold: %d\n", *cold);
+
+	kfree(b);
 	return 0;
 }
 
@@ -313,7 +343,13 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
 	u8 pll_src, u8 pll_range, u8 clock_gpio3, u16 pll_prediv,
 	u16 pll_loopdiv, u16 free_div, u16 dsuScaler)
 {
-	u8 b[10];
+	s16 ret;
+	u8 *b;
+
+	b = kmalloc(10, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	b[0] = REQUEST_SET_CLOCK;
 	b[1] = (en_pll << 7) | (pll_src << 6) | (pll_range << 5) | (clock_gpio3 << 4);
 	b[2] = (pll_prediv >> 8)  & 0xff; // MSB
@@ -325,7 +361,10 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
 	b[8] = (dsuScaler >> 8)   & 0xff; // MSB
 	b[9] =  dsuScaler         & 0xff; // LSB
 
-	return dib0700_ctrl_wr(d, b, 10);
+	ret = dib0700_ctrl_wr(d, b, 10);
+
+	kfree(b);
+	return ret;
 }
 
 int dib0700_ctrl_clock(struct dvb_usb_device *d, u32 clk_MHz, u8 clock_out_gp3)
@@ -361,11 +400,14 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 {
 	struct hexline hx;
 	int pos = 0, ret, act_len, i, adap_num;
-	u8 b[16];
+	u8 *b;
 	u32 fw_version;
-
 	u8 buf[260];
 
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	while ((ret = dvb_usb_get_hexline(fw, &hx, &pos)) > 0) {
 		deb_fwdata("writing to address 0x%08x (buffer: 0x%02x %02x)\n",
 				hx.addr, hx.len, hx.chk);
@@ -386,7 +428,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 
 		if (ret < 0) {
 			err("firmware download failed at %d with %d",pos,ret);
-			return ret;
+			goto out;
 		}
 	}
 
@@ -407,7 +449,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 	usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 				  REQUEST_GET_VERSION,
 				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+				  b, 16, USB_CTRL_GET_TIMEOUT);
 	fw_version = (b[8] << 24) | (b[9] << 16) | (b[10] << 8) | b[11];
 
 	/* set the buffer size - DVB-USB is allocating URB buffers
@@ -426,16 +468,21 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 			}
 		}
 	}
-
+out:
+	kfree(b);
 	return ret;
 }
 
 int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 {
 	struct dib0700_state *st = adap->dev->priv;
-	u8 b[4];
+	u8 *b;
 	int ret;
 
+	b = kmalloc(4, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	if ((onoff != 0) && (st->fw_version >= 0x10201)) {
 		/* for firmware later than 1.20.1,
 		 * the USB xfer length can be set  */
@@ -443,7 +490,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 			st->nb_packet_buffer_size);
 		if (ret < 0) {
 			deb_info("can not set the USB xfer len\n");
-			return ret;
+			goto out;
 		}
 	}
 
@@ -468,7 +515,10 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 
 	deb_info("data for streaming: %x %x\n", b[1], b[2]);
 
-	return dib0700_ctrl_wr(adap->dev, b, 4);
+	ret = dib0700_ctrl_wr(adap->dev, b, 4);
+out:
+	kfree(b);
+	return ret;
 }
 
 int dib0700_change_protocol(struct rc_dev *rc, u64 rc_type)
-- 
1.7.4.rc3

[PATCH 2/3] [media] dib0700: remove unused variable

[Florian Mickler]

Signed-off-by: Florian Mickler <florian@mickler.org>
CC: Mauro Carvalho Chehab <mchehab@infradead.org>
CC: linux-media@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Greg Kroah-Hartman <greg@kroah.com>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Maciej Rutecki <maciej.rutecki@gmail.com>
CC: Oliver Neukum <oliver@neukum.org>
CC: Jack Stone <jwjstone@fastmail.fm>
---
 drivers/media/dvb/dvb-usb/dib0700_core.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 028ed87..77a3060 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -576,7 +576,6 @@ struct dib0700_rc_response {
 static void dib0700_rc_urb_completion(struct urb *purb)
 {
 	struct dvb_usb_device *d = purb->context;
-	struct dib0700_state *st;
 	struct dib0700_rc_response *poll_reply;
 	u32 uninitialized_var(keycode);
 	u8 toggle;
@@ -591,7 +590,6 @@ static void dib0700_rc_urb_completion(struct urb *purb)
 		return;
 	}
 
-	st = d->priv;
 	poll_reply = purb->transfer_buffer;
 
 	if (purb->status < 0) {
-- 
1.7.4.rc3

[PATCH 3/3] [media] dib0700: don't ignore errors in driver probe

[Florian Mickler]

Signed-off-by: Florian Mickler <florian@mickler.org>
CC: Mauro Carvalho Chehab <mchehab@infradead.org>
CC: linux-media@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Greg Kroah-Hartman <greg@kroah.com>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Maciej Rutecki <maciej.rutecki@gmail.com>
CC: Oliver Neukum <oliver@neukum.org>
CC: Jack Stone <jwjstone@fastmail.fm>
---
 drivers/media/dvb/dvb-usb/dib0700_core.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 77a3060..3ad18ce 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -698,6 +698,7 @@ static int dib0700_probe(struct usb_interface *intf,
 		const struct usb_device_id *id)
 {
 	int i;
+	int ret;
 	struct dvb_usb_device *dev;
 
 	for (i = 0; i < dib0700_device_count; i++)
@@ -706,8 +707,10 @@ static int dib0700_probe(struct usb_interface *intf,
 			struct dib0700_state *st = dev->priv;
 			u32 hwversion, romversion, fw_version, fwtype;
 
-			dib0700_get_version(dev, &hwversion, &romversion,
+			ret = dib0700_get_version(dev, &hwversion, &romversion,
 				&fw_version, &fwtype);
+			if (ret < 0)
+				goto out;
 
 			deb_info("Firmware version: %x, %d, 0x%x, %d\n",
 				hwversion, romversion, fw_version, fwtype);
@@ -721,11 +724,15 @@ static int dib0700_probe(struct usb_interface *intf,
 			else
 				dev->props.rc.core.bulk_mode = false;
 
-			dib0700_rc_setup(dev);
+			ret = dib0700_rc_setup(dev);
+			if (ret)
+				goto out;
 
 			return 0;
+out:
+			dvb_usb_device_exit();
+			return ret;
 		}
-
 	return -ENODEV;
 }
 
-- 
1.7.4.rc3

Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers

[Oliver Neukum]

Am Sonntag, 6. März 2011, 15:38:05 schrieb Florian Mickler:
> On Sun, 6 Mar 2011 13:06:09 +0100
> Oliver Neukum <oliver@neukum.org> wrote:
> 
> > Am Sonntag, 6. März 2011, 12:16:52 schrieb Florian Mickler:

> > > Please take a look at it, as I do not do that much kernel hacking
> > > and don't wanna brake anybodys computer... :)
> > > 
> > > From my point of view this should _not_ go to stable even though it would
> > > be applicable. But if someone feels strongly about that and can
> > > take responsibility for that change...
> > 
> > The patch looks good and is needed in stable.
> > It could be improved by using a buffer allocated once in the places
> > you hold a mutex anyway.
> > 
> > 	Regards
> > 		Oliver
> 
> Ok, I now put a buffer member in the priv dib0700_state which gets
> allocated on the heap. 

This however is wrong. Just like DMA on the stack this breaks
coherency rules. You may do DMA to the heap in the sense that
you can do DMA to buffers allocated on the heap, but you cannot
do DMA to a part of another structure allocated on the heap.
You need a separate kmalloc for each buffer.
You can reuse the buffer with proper locking, but you must allocate
it seperately once.

	Regards
		Oliver

Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

On Sun, 6 Mar 2011 16:06:38 +0100
Oliver Neukum <oliver@neukum.org> wrote:

> Am Sonntag, 6. März 2011, 15:38:05 schrieb Florian Mickler:
> > On Sun, 6 Mar 2011 13:06:09 +0100
> > Oliver Neukum <oliver@neukum.org> wrote:
> > 
> > > Am Sonntag, 6. März 2011, 12:16:52 schrieb Florian Mickler:
> 
> > > > Please take a look at it, as I do not do that much kernel hacking
> > > > and don't wanna brake anybodys computer... :)
> > > > 
> > > > From my point of view this should _not_ go to stable even though it would
> > > > be applicable. But if someone feels strongly about that and can
> > > > take responsibility for that change...
> > > 
> > > The patch looks good and is needed in stable.
> > > It could be improved by using a buffer allocated once in the places
> > > you hold a mutex anyway.
> > > 
> > > 	Regards
> > > 		Oliver
> > 
> > Ok, I now put a buffer member in the priv dib0700_state which gets
> > allocated on the heap. 
> 
> This however is wrong. Just like DMA on the stack this breaks
> coherency rules. You may do DMA to the heap in the sense that
> you can do DMA to buffers allocated on the heap, but you cannot
> do DMA to a part of another structure allocated on the heap.
> You need a separate kmalloc for each buffer.
> You can reuse the buffer with proper locking, but you must allocate
> it seperately once.
> 
> 	Regards
> 		Oliver

Hm.. allocating the buffer
in the probe routine and deallocating it in the usb_driver disconnect
callback should work?

How come that it must be a seperate kmalloc buffer? Is it some aligning
that kmalloc garantees? 

Regards,
Flo

Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers

[Oliver Neukum]

Am Sonntag, 6. März 2011, 16:45:21 schrieb Florian Mickler:

> Hm.. allocating the buffer
> in the probe routine and deallocating it in the usb_driver disconnect
> callback should work?

Yes.
 
> How come that it must be a seperate kmalloc buffer? Is it some aligning
> that kmalloc garantees? 

On some CPUs DMA affects on main CPU, not the CPU caches. You
need to synchronize the cache before you start DMA and must not touch
the buffer until DMA is finished. This applies with a certain granularity
that kmalloc respects. The ugly details are in Documentation.

	Regards
		Oliver

[PATCH 1/2 v3] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

This should fix warnings seen by some:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
Signed-off-by: Florian Mickler <florian@mickler.org>
CC: Mauro Carvalho Chehab <mchehab@infradead.org>
CC: linux-media@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Greg Kroah-Hartman <greg@kroah.com>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Maciej Rutecki <maciej.rutecki@gmail.com>
CC: Oliver Neukum <oliver@neukum.org>
CC: Jack Stone <jwjstone@fastmail.fm>

---
[v2: use preallocated buffer; fix sizeof in one case]
[v3: use seperate kmalloc mapping for the preallocation,
     dont ignore errors in probe codepaths  ]

 drivers/media/dvb/dvb-usb/dib0700.h      |    5 +-
 drivers/media/dvb/dvb-usb/dib0700_core.c |  119 ++++++++++++++++++++++++------
 2 files changed, 98 insertions(+), 26 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700.h b/drivers/media/dvb/dvb-usb/dib0700.h
index 3537d65..99a1485 100644
--- a/drivers/media/dvb/dvb-usb/dib0700.h
+++ b/drivers/media/dvb/dvb-usb/dib0700.h
@@ -45,8 +45,9 @@ struct dib0700_state {
 	u8 is_dib7000pc;
 	u8 fw_use_new_i2c_api;
 	u8 disable_streaming_master_mode;
-    u32 fw_version;
-    u32 nb_packet_buffer_size;
+	u32 fw_version;
+	u32 nb_packet_buffer_size;
+	u8 *buf;
 };
 
 extern int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..0b04cb6 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
 			u32 *romversion, u32 *ramversion, u32 *fwtype)
 {
-	u8 b[16];
-	int ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
+	int ret;
+	u8 *b;
+
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
+	ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
 				  REQUEST_GET_VERSION,
 				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+				  b, 16, USB_CTRL_GET_TIMEOUT);
 	if (hwversion != NULL)
 		*hwversion  = (b[0] << 24)  | (b[1] << 16)  | (b[2] << 8)  | b[3];
 	if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
 		*ramversion = (b[8] << 24)  | (b[9] << 16)  | (b[10] << 8) | b[11];
 	if (fwtype != NULL)
 		*fwtype     = (b[12] << 24) | (b[13] << 16) | (b[14] << 8) | b[15];
+
+	kfree(b);
 	return ret;
 }
 
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 txlen, u8 *rx, u8 rxlen
 
 int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 gpio_dir, u8 gpio_val)
 {
-	u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6) };
-	return dib0700_ctrl_wr(d, buf, sizeof(buf));
+	s16 ret;
+	u8 *buf = kmalloc(3, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	buf[0] = REQUEST_SET_GPIO;
+	buf[1] = gpio;
+	buf[2] = ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6);
+
+	ret = dib0700_ctrl_wr(d, buf, 3);
+
+	kfree(buf);
+	return ret;
 }
 
 static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 nb_ts_packets)
@@ -137,11 +156,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
 	   properly support i2c read calls not preceded by a write */
 
 	struct dvb_usb_device *d = i2c_get_adapdata(adap);
+	struct dib0700_state *st = d->priv;
 	uint8_t bus_mode = 1;  /* 0=eeprom bus, 1=frontend bus */
 	uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
 	uint8_t en_start = 0;
 	uint8_t en_stop = 0;
-	uint8_t buf[255]; /* TBV: malloc ? */
+	uint8_t *buf = st->buf;
 	int result, i;
 
 	/* Ensure nobody else hits the i2c bus while we're sending our
@@ -221,6 +241,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
 		}
 	}
 	mutex_unlock(&d->i2c_mutex);
+
 	return i;
 }
 
@@ -231,8 +252,9 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 				   struct i2c_msg *msg, int num)
 {
 	struct dvb_usb_device *d = i2c_get_adapdata(adap);
+	struct dib0700_state *st = d->priv;
 	int i,len;
-	u8 buf[255];
+	u8 *buf = st->buf;
 
 	if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
 		return -EAGAIN;
@@ -264,8 +286,8 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 				break;
 		}
 	}
-
 	mutex_unlock(&d->i2c_mutex);
+
 	return i;
 }
 
@@ -297,15 +319,23 @@ struct i2c_algorithm dib0700_i2c_algo = {
 int dib0700_identify_state(struct usb_device *udev, struct dvb_usb_device_properties *props,
 			struct dvb_usb_device_description **desc, int *cold)
 {
-	u8 b[16];
-	s16 ret = usb_control_msg(udev, usb_rcvctrlpipe(udev,0),
+	s16 ret;
+	u8 *b;
+
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return	-ENOMEM;
+
+
+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 		REQUEST_GET_VERSION, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, b, 16, USB_CTRL_GET_TIMEOUT);
 
 	deb_info("FW GET_VERSION length: %d\n",ret);
 
 	*cold = ret <= 0;
-
 	deb_info("cold: %d\n", *cold);
+
+	kfree(b);
 	return 0;
 }
 
@@ -313,7 +343,13 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
 	u8 pll_src, u8 pll_range, u8 clock_gpio3, u16 pll_prediv,
 	u16 pll_loopdiv, u16 free_div, u16 dsuScaler)
 {
-	u8 b[10];
+	s16 ret;
+	u8 *b;
+
+	b = kmalloc(10, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	b[0] = REQUEST_SET_CLOCK;
 	b[1] = (en_pll << 7) | (pll_src << 6) | (pll_range << 5) | (clock_gpio3 << 4);
 	b[2] = (pll_prediv >> 8)  & 0xff; // MSB
@@ -325,7 +361,10 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
 	b[8] = (dsuScaler >> 8)   & 0xff; // MSB
 	b[9] =  dsuScaler         & 0xff; // LSB
 
-	return dib0700_ctrl_wr(d, b, 10);
+	ret = dib0700_ctrl_wr(d, b, 10);
+
+	kfree(b);
+	return ret;
 }
 
 int dib0700_ctrl_clock(struct dvb_usb_device *d, u32 clk_MHz, u8 clock_out_gp3)
@@ -361,11 +400,14 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 {
 	struct hexline hx;
 	int pos = 0, ret, act_len, i, adap_num;
-	u8 b[16];
+	u8 *b;
 	u32 fw_version;
-
 	u8 buf[260];
 
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	while ((ret = dvb_usb_get_hexline(fw, &hx, &pos)) > 0) {
 		deb_fwdata("writing to address 0x%08x (buffer: 0x%02x %02x)\n",
 				hx.addr, hx.len, hx.chk);
@@ -386,7 +428,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 
 		if (ret < 0) {
 			err("firmware download failed at %d with %d",pos,ret);
-			return ret;
+			goto out;
 		}
 	}
 
@@ -407,7 +449,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 	usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 				  REQUEST_GET_VERSION,
 				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+				  b, 16, USB_CTRL_GET_TIMEOUT);
 	fw_version = (b[8] << 24) | (b[9] << 16) | (b[10] << 8) | b[11];
 
 	/* set the buffer size - DVB-USB is allocating URB buffers
@@ -426,16 +468,21 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 			}
 		}
 	}
-
+out:
+	kfree(b);
 	return ret;
 }
 
 int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 {
 	struct dib0700_state *st = adap->dev->priv;
-	u8 b[4];
+	u8 *b;
 	int ret;
 
+	b = kmalloc(4, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	if ((onoff != 0) && (st->fw_version >= 0x10201)) {
 		/* for firmware later than 1.20.1,
 		 * the USB xfer length can be set  */
@@ -443,7 +490,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 			st->nb_packet_buffer_size);
 		if (ret < 0) {
 			deb_info("can not set the USB xfer len\n");
-			return ret;
+			goto out;
 		}
 	}
 
@@ -468,7 +515,10 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 
 	deb_info("data for streaming: %x %x\n", b[1], b[2]);
 
-	return dib0700_ctrl_wr(adap->dev, b, 4);
+	ret = dib0700_ctrl_wr(adap->dev, b, 4);
+out:
+	kfree(b);
+	return ret;
 }
 
 int dib0700_change_protocol(struct rc_dev *rc, u64 rc_type)
@@ -650,6 +700,7 @@ static int dib0700_probe(struct usb_interface *intf,
 		const struct usb_device_id *id)
 {
 	int i;
+	int ret;
 	struct dvb_usb_device *dev;
 
 	for (i = 0; i < dib0700_device_count; i++)
@@ -658,8 +709,10 @@ static int dib0700_probe(struct usb_interface *intf,
 			struct dib0700_state *st = dev->priv;
 			u32 hwversion, romversion, fw_version, fwtype;
 
-			dib0700_get_version(dev, &hwversion, &romversion,
+			ret = dib0700_get_version(dev, &hwversion, &romversion,
 				&fw_version, &fwtype);
+			if (ret < 0)
+				goto out;
 
 			deb_info("Firmware version: %x, %d, 0x%x, %d\n",
 				hwversion, romversion, fw_version, fwtype);
@@ -673,18 +726,36 @@ static int dib0700_probe(struct usb_interface *intf,
 			else
 				dev->props.rc.core.bulk_mode = false;
 
-			dib0700_rc_setup(dev);
+			ret = dib0700_rc_setup(dev);
+			if (ret)
+				goto out;
+
+			st->buf = kmalloc(255, GFP_KERNEL);
+			if (!st->buf) {
+				ret = -ENOMEM;
+				goto out;
+			}
 
 			return 0;
+out:
+			dvb_usb_device_exit(intf);
+			return ret;
 		}
 
 	return -ENODEV;
 }
 
+static void dib0700_disconnect(struct usb_interface *intf) {
+	struct dvb_usb_device *d = usb_get_intfdata(intf);
+	struct dib0700_state *st = d->priv;
+	kfree(st->buf);
+	dvb_usb_device_exit(intf);
+}
+
 static struct usb_driver dib0700_driver = {
 	.name       = "dvb_usb_dib0700",
 	.probe      = dib0700_probe,
-	.disconnect = dvb_usb_device_exit,
+	.disconnect = dib0700_disconnect,
 	.id_table   = dib0700_usb_id_table,
 };
 
-- 
1.7.4.rc3

[PATCH 2/2] [media] dib0700: remove unused variable

[Florian Mickler]

Signed-off-by: Florian Mickler <florian@mickler.org>
CC: linux-media@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Greg Kroah-Hartman <greg@kroah.com>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Maciej Rutecki <maciej.rutecki@gmail.com>
CC: Oliver Neukum <oliver@neukum.org>
---
 drivers/media/dvb/dvb-usb/dib0700_core.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 0b04cb6..5770265 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -576,7 +576,6 @@ struct dib0700_rc_response {
 static void dib0700_rc_urb_completion(struct urb *purb)
 {
 	struct dvb_usb_device *d = purb->context;
-	struct dib0700_state *st;
 	struct dib0700_rc_response *poll_reply;
 	u32 uninitialized_var(keycode);
 	u8 toggle;
@@ -591,7 +590,6 @@ static void dib0700_rc_urb_completion(struct urb *purb)
 		return;
 	}
 
-	st = d->priv;
 	poll_reply = purb->transfer_buffer;
 
 	if (purb->status < 0) {
-- 
1.7.4.rc3

Re: [PATCH 1/2 v3] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

On Sun,  6 Mar 2011 18:47:56 +0100
Florian Mickler <florian@mickler.org> wrote:


> +static void dib0700_disconnect(struct usb_interface *intf) {


That { should go on its own line... sorry ;-)

If that patch is acceptable, I can resend with that fixed. 

Regards,
Flo

Re: [PATCH 1/2 v3] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

On Sun, 6 Mar 2011 18:57:13 +0100
Florian Mickler <florian@mickler.org> wrote:

> On Sun,  6 Mar 2011 18:47:56 +0100
> Florian Mickler <florian@mickler.org> wrote:
> 
> 
> > +static void dib0700_disconnect(struct usb_interface *intf) {
> 
> 
> That { should go on its own line... sorry ;-)
> 
> If that patch is acceptable, I can resend with that fixed. 
> 
> Regards,
> Flo


Hi Mauro, 

what about this patch? I have similar patches for the same problem in
the other dvb-usb drivers in need of beeing fixed. Are you
maintaining these drivers or should I find someone else to pick these
up? 

Regards,
Flo

[PATCH 01/16] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
Signed-off-by: Florian Mickler <florian@mickler.org>

[v2: use preallocated buffer; fix sizeof in one case]
[v3: use seperate kmalloc mapping for the preallocation,
     dont ignore errors in probe codepaths  ]
[v4: minor style nit: functions opening brace goes onto it's own line ]
---
 drivers/media/dvb/dvb-usb/dib0700.h      |    5 +-
 drivers/media/dvb/dvb-usb/dib0700_core.c |  120 ++++++++++++++++++++++++------
 2 files changed, 99 insertions(+), 26 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700.h b/drivers/media/dvb/dvb-usb/dib0700.h
index 3537d65..99a1485 100644
--- a/drivers/media/dvb/dvb-usb/dib0700.h
+++ b/drivers/media/dvb/dvb-usb/dib0700.h
@@ -45,8 +45,9 @@ struct dib0700_state {
 	u8 is_dib7000pc;
 	u8 fw_use_new_i2c_api;
 	u8 disable_streaming_master_mode;
-    u32 fw_version;
-    u32 nb_packet_buffer_size;
+	u32 fw_version;
+	u32 nb_packet_buffer_size;
+	u8 *buf;
 };
 
 extern int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..1c19b73 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
 			u32 *romversion, u32 *ramversion, u32 *fwtype)
 {
-	u8 b[16];
-	int ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
+	int ret;
+	u8 *b;
+
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
+	ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
 				  REQUEST_GET_VERSION,
 				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+				  b, 16, USB_CTRL_GET_TIMEOUT);
 	if (hwversion != NULL)
 		*hwversion  = (b[0] << 24)  | (b[1] << 16)  | (b[2] << 8)  | b[3];
 	if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
 		*ramversion = (b[8] << 24)  | (b[9] << 16)  | (b[10] << 8) | b[11];
 	if (fwtype != NULL)
 		*fwtype     = (b[12] << 24) | (b[13] << 16) | (b[14] << 8) | b[15];
+
+	kfree(b);
 	return ret;
 }
 
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 txlen, u8 *rx, u8 rxlen
 
 int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 gpio_dir, u8 gpio_val)
 {
-	u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6) };
-	return dib0700_ctrl_wr(d, buf, sizeof(buf));
+	s16 ret;
+	u8 *buf = kmalloc(3, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	buf[0] = REQUEST_SET_GPIO;
+	buf[1] = gpio;
+	buf[2] = ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6);
+
+	ret = dib0700_ctrl_wr(d, buf, 3);
+
+	kfree(buf);
+	return ret;
 }
 
 static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 nb_ts_packets)
@@ -137,11 +156,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
 	   properly support i2c read calls not preceded by a write */
 
 	struct dvb_usb_device *d = i2c_get_adapdata(adap);
+	struct dib0700_state *st = d->priv;
 	uint8_t bus_mode = 1;  /* 0=eeprom bus, 1=frontend bus */
 	uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
 	uint8_t en_start = 0;
 	uint8_t en_stop = 0;
-	uint8_t buf[255]; /* TBV: malloc ? */
+	uint8_t *buf = st->buf;
 	int result, i;
 
 	/* Ensure nobody else hits the i2c bus while we're sending our
@@ -221,6 +241,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
 		}
 	}
 	mutex_unlock(&d->i2c_mutex);
+
 	return i;
 }
 
@@ -231,8 +252,9 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 				   struct i2c_msg *msg, int num)
 {
 	struct dvb_usb_device *d = i2c_get_adapdata(adap);
+	struct dib0700_state *st = d->priv;
 	int i,len;
-	u8 buf[255];
+	u8 *buf = st->buf;
 
 	if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
 		return -EAGAIN;
@@ -264,8 +286,8 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 				break;
 		}
 	}
-
 	mutex_unlock(&d->i2c_mutex);
+
 	return i;
 }
 
@@ -297,15 +319,23 @@ struct i2c_algorithm dib0700_i2c_algo = {
 int dib0700_identify_state(struct usb_device *udev, struct dvb_usb_device_properties *props,
 			struct dvb_usb_device_description **desc, int *cold)
 {
-	u8 b[16];
-	s16 ret = usb_control_msg(udev, usb_rcvctrlpipe(udev,0),
+	s16 ret;
+	u8 *b;
+
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return	-ENOMEM;
+
+
+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 		REQUEST_GET_VERSION, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, b, 16, USB_CTRL_GET_TIMEOUT);
 
 	deb_info("FW GET_VERSION length: %d\n",ret);
 
 	*cold = ret <= 0;
-
 	deb_info("cold: %d\n", *cold);
+
+	kfree(b);
 	return 0;
 }
 
@@ -313,7 +343,13 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
 	u8 pll_src, u8 pll_range, u8 clock_gpio3, u16 pll_prediv,
 	u16 pll_loopdiv, u16 free_div, u16 dsuScaler)
 {
-	u8 b[10];
+	s16 ret;
+	u8 *b;
+
+	b = kmalloc(10, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	b[0] = REQUEST_SET_CLOCK;
 	b[1] = (en_pll << 7) | (pll_src << 6) | (pll_range << 5) | (clock_gpio3 << 4);
 	b[2] = (pll_prediv >> 8)  & 0xff; // MSB
@@ -325,7 +361,10 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
 	b[8] = (dsuScaler >> 8)   & 0xff; // MSB
 	b[9] =  dsuScaler         & 0xff; // LSB
 
-	return dib0700_ctrl_wr(d, b, 10);
+	ret = dib0700_ctrl_wr(d, b, 10);
+
+	kfree(b);
+	return ret;
 }
 
 int dib0700_ctrl_clock(struct dvb_usb_device *d, u32 clk_MHz, u8 clock_out_gp3)
@@ -361,11 +400,14 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 {
 	struct hexline hx;
 	int pos = 0, ret, act_len, i, adap_num;
-	u8 b[16];
+	u8 *b;
 	u32 fw_version;
-
 	u8 buf[260];
 
+	b = kmalloc(16, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	while ((ret = dvb_usb_get_hexline(fw, &hx, &pos)) > 0) {
 		deb_fwdata("writing to address 0x%08x (buffer: 0x%02x %02x)\n",
 				hx.addr, hx.len, hx.chk);
@@ -386,7 +428,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 
 		if (ret < 0) {
 			err("firmware download failed at %d with %d",pos,ret);
-			return ret;
+			goto out;
 		}
 	}
 
@@ -407,7 +449,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 	usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 				  REQUEST_GET_VERSION,
 				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+				  b, 16, USB_CTRL_GET_TIMEOUT);
 	fw_version = (b[8] << 24) | (b[9] << 16) | (b[10] << 8) | b[11];
 
 	/* set the buffer size - DVB-USB is allocating URB buffers
@@ -426,16 +468,21 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
 			}
 		}
 	}
-
+out:
+	kfree(b);
 	return ret;
 }
 
 int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 {
 	struct dib0700_state *st = adap->dev->priv;
-	u8 b[4];
+	u8 *b;
 	int ret;
 
+	b = kmalloc(4, GFP_KERNEL);
+	if (!b)
+		return -ENOMEM;
+
 	if ((onoff != 0) && (st->fw_version >= 0x10201)) {
 		/* for firmware later than 1.20.1,
 		 * the USB xfer length can be set  */
@@ -443,7 +490,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 			st->nb_packet_buffer_size);
 		if (ret < 0) {
 			deb_info("can not set the USB xfer len\n");
-			return ret;
+			goto out;
 		}
 	}
 
@@ -468,7 +515,10 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
 
 	deb_info("data for streaming: %x %x\n", b[1], b[2]);
 
-	return dib0700_ctrl_wr(adap->dev, b, 4);
+	ret = dib0700_ctrl_wr(adap->dev, b, 4);
+out:
+	kfree(b);
+	return ret;
 }
 
 int dib0700_change_protocol(struct rc_dev *rc, u64 rc_type)
@@ -650,6 +700,7 @@ static int dib0700_probe(struct usb_interface *intf,
 		const struct usb_device_id *id)
 {
 	int i;
+	int ret;
 	struct dvb_usb_device *dev;
 
 	for (i = 0; i < dib0700_device_count; i++)
@@ -658,8 +709,10 @@ static int dib0700_probe(struct usb_interface *intf,
 			struct dib0700_state *st = dev->priv;
 			u32 hwversion, romversion, fw_version, fwtype;
 
-			dib0700_get_version(dev, &hwversion, &romversion,
+			ret = dib0700_get_version(dev, &hwversion, &romversion,
 				&fw_version, &fwtype);
+			if (ret < 0)
+				goto out;
 
 			deb_info("Firmware version: %x, %d, 0x%x, %d\n",
 				hwversion, romversion, fw_version, fwtype);
@@ -673,18 +726,37 @@ static int dib0700_probe(struct usb_interface *intf,
 			else
 				dev->props.rc.core.bulk_mode = false;
 
-			dib0700_rc_setup(dev);
+			ret = dib0700_rc_setup(dev);
+			if (ret)
+				goto out;
+
+			st->buf = kmalloc(255, GFP_KERNEL);
+			if (!st->buf) {
+				ret = -ENOMEM;
+				goto out;
+			}
 
 			return 0;
+out:
+			dvb_usb_device_exit(intf);
+			return ret;
 		}
 
 	return -ENODEV;
 }
 
+static void dib0700_disconnect(struct usb_interface *intf)
+{
+	struct dvb_usb_device *d = usb_get_intfdata(intf);
+	struct dib0700_state *st = d->priv;
+	kfree(st->buf);
+	dvb_usb_device_exit(intf);
+}
+
 static struct usb_driver dib0700_driver = {
 	.name       = "dvb_usb_dib0700",
 	.probe      = dib0700_probe,
-	.disconnect = dvb_usb_device_exit,
+	.disconnect = dib0700_disconnect,
 	.id_table   = dib0700_usb_id_table,
 };
 
-- 
1.7.4.rc3

[PATCH 02/16] [media] dib0700: remove unused variable

[Florian Mickler]

This variable is never used.

Signed-off-by: Florian Mickler <florian@mickler.org>
--
 drivers/media/dvb/dvb-usb/dib0700_core.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
---
 drivers/media/dvb/dvb-usb/dib0700_core.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 1c19b73..c705ea4 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -576,7 +576,6 @@ struct dib0700_rc_response {
 static void dib0700_rc_urb_completion(struct urb *purb)
 {
 	struct dvb_usb_device *d = purb->context;
-	struct dib0700_state *st;
 	struct dib0700_rc_response *poll_reply;
 	u32 uninitialized_var(keycode);
 	u8 toggle;
@@ -591,7 +590,6 @@ static void dib0700_rc_urb_completion(struct urb *purb)
 		return;
 	}
 
-	st = d->priv;
 	poll_reply = purb->transfer_buffer;
 
 	if (purb->status < 0) {
-- 
1.7.4.rc3

[PATCH 03/16] [media] a800: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/a800.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/a800.c b/drivers/media/dvb/dvb-usb/a800.c
index 53b93a4..fe2366b 100644
--- a/drivers/media/dvb/dvb-usb/a800.c
+++ b/drivers/media/dvb/dvb-usb/a800.c
@@ -78,17 +78,26 @@ static struct rc_map_table rc_map_a800_table[] = {
 
 static int a800_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 {
-	u8 key[5];
+	int ret;
+	u8 *key = kmalloc(5, GFP_KERNEL);
+	if (!key)
+		return -ENOMEM;
+
 	if (usb_control_msg(d->udev,usb_rcvctrlpipe(d->udev,0),
 				0x04, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, key, 5,
-				2000) != 5)
-		return -ENODEV;
+				2000) != 5) {
+		ret = -ENODEV;
+		goto out;
+	}
 
 	/* call the universal NEC remote processor, to find out the key's state and event */
 	dvb_usb_nec_rc_key_to_event(d,key,event,state);
 	if (key[0] != 0)
 		deb_rc("key: %x %x %x %x %x\n",key[0],key[1],key[2],key[3],key[4]);
-	return 0;
+	ret = 0;
+out:
+	kfree(key);
+	return ret;
 }
 
 /* USB Driver stuff */
-- 
1.7.4.rc3

[PATCH 04/16] [media] vp7045: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/vp7045.c |   41 ++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/vp7045.c b/drivers/media/dvb/dvb-usb/vp7045.c
index ab0ab3c..17478ec 100644
--- a/drivers/media/dvb/dvb-usb/vp7045.c
+++ b/drivers/media/dvb/dvb-usb/vp7045.c
@@ -28,9 +28,9 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 int vp7045_usb_op(struct dvb_usb_device *d, u8 cmd, u8 *out, int outlen, u8 *in, int inlen, int msec)
 {
 	int ret = 0;
-	u8 inbuf[12] = { 0 }, outbuf[20] = { 0 };
+	u8 *buf = d->priv;
 
-	outbuf[0] = cmd;
+	buf[0] = cmd;
 
 	if (outlen > 19)
 		outlen = 19;
@@ -39,10 +39,10 @@ int vp7045_usb_op(struct dvb_usb_device *d, u8 cmd, u8 *out, int outlen, u8 *in,
 		inlen = 11;
 
 	if (out != NULL && outlen > 0)
-		memcpy(&outbuf[1], out, outlen);
+		memcpy(&buf[1], out, outlen);
 
 	deb_xfer("out buffer: ");
-	debug_dump(outbuf,outlen+1,deb_xfer);
+	debug_dump(buf, outlen+1, deb_xfer);
 
 	if ((ret = mutex_lock_interruptible(&d->usb_mutex)))
 		return ret;
@@ -50,7 +50,7 @@ int vp7045_usb_op(struct dvb_usb_device *d, u8 cmd, u8 *out, int outlen, u8 *in,
 	if (usb_control_msg(d->udev,
 			usb_sndctrlpipe(d->udev,0),
 			TH_COMMAND_OUT, USB_TYPE_VENDOR | USB_DIR_OUT, 0, 0,
-			outbuf, 20, 2000) != 20) {
+			buf, 20, 2000) != 20) {
 		err("USB control message 'out' went wrong.");
 		ret = -EIO;
 		goto unlock;
@@ -61,17 +61,17 @@ int vp7045_usb_op(struct dvb_usb_device *d, u8 cmd, u8 *out, int outlen, u8 *in,
 	if (usb_control_msg(d->udev,
 			usb_rcvctrlpipe(d->udev,0),
 			TH_COMMAND_IN, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-			inbuf, 12, 2000) != 12) {
+			buf, 12, 2000) != 12) {
 		err("USB control message 'in' went wrong.");
 		ret = -EIO;
 		goto unlock;
 	}
 
 	deb_xfer("in buffer: ");
-	debug_dump(inbuf,12,deb_xfer);
+	debug_dump(buf, 12, deb_xfer);
 
 	if (in != NULL && inlen > 0)
-		memcpy(in,&inbuf[1],inlen);
+		memcpy(in, &buf[1], inlen);
 
 unlock:
 	mutex_unlock(&d->usb_mutex);
@@ -222,8 +222,26 @@ static struct dvb_usb_device_properties vp7045_properties;
 static int vp7045_usb_probe(struct usb_interface *intf,
 		const struct usb_device_id *id)
 {
-	return dvb_usb_device_init(intf, &vp7045_properties,
-				   THIS_MODULE, NULL, adapter_nr);
+	struct dvb_usb_device *d;
+	int ret = dvb_usb_device_init(intf, &vp7045_properties,
+				   THIS_MODULE, &d, adapter_nr);
+	if (ret)
+		return ret;
+
+	d->priv = kmalloc(20, GFP_KERNEL);
+	if (!d->priv) {
+		dvb_usb_device_exit(intf);
+		return -ENOMEM;
+	}
+
+	return ret;
+}
+
+static void vp7045_usb_disconnect(struct usb_interface *intf)
+{
+	struct dvb_usb_device *d = usb_get_intfdata(intf);
+	kfree(d->priv);
+	dvb_usb_device_exit(intf);
 }
 
 static struct usb_device_id vp7045_usb_table [] = {
@@ -238,6 +256,7 @@ MODULE_DEVICE_TABLE(usb, vp7045_usb_table);
 static struct dvb_usb_device_properties vp7045_properties = {
 	.usb_ctrl = CYPRESS_FX2,
 	.firmware = "dvb-usb-vp7045-01.fw",
+	.size_of_priv = sizeof(u8 *),
 
 	.num_adapters = 1,
 	.adapter = {
@@ -284,7 +303,7 @@ static struct dvb_usb_device_properties vp7045_properties = {
 static struct usb_driver vp7045_usb_driver = {
 	.name		= "dvb_usb_vp7045",
 	.probe		= vp7045_usb_probe,
-	.disconnect = dvb_usb_device_exit,
+	.disconnect	= vp7045_usb_disconnect,
 	.id_table	= vp7045_usb_table,
 };
 
-- 
1.7.4.rc3

[PATCH 05/16] [media] ec168: get rid of on stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/ec168.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/ec168.c b/drivers/media/dvb/dvb-usb/ec168.c
index 52f5d4f..1ba3e5d 100644
--- a/drivers/media/dvb/dvb-usb/ec168.c
+++ b/drivers/media/dvb/dvb-usb/ec168.c
@@ -36,7 +36,9 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
 	int ret;
 	unsigned int pipe;
 	u8 request, requesttype;
-	u8 buf[req->size];
+	u8 *buf;
+
+
 
 	switch (req->cmd) {
 	case DOWNLOAD_FIRMWARE:
@@ -72,6 +74,12 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
 		goto error;
 	}
 
+	buf = kmalloc(req->size, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
 	if (requesttype == (USB_TYPE_VENDOR | USB_DIR_OUT)) {
 		/* write */
 		memcpy(buf, req->data, req->size);
@@ -84,13 +92,13 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
 	msleep(1); /* avoid I2C errors */
 
 	ret = usb_control_msg(udev, pipe, request, requesttype, req->value,
-		req->index, buf, sizeof(buf), EC168_USB_TIMEOUT);
+		req->index, buf, req->size, EC168_USB_TIMEOUT);
 
 	ec168_debug_dump(request, requesttype, req->value, req->index, buf,
 		req->size, deb_xfer);
 
 	if (ret < 0)
-		goto error;
+		goto err_dealloc;
 	else
 		ret = 0;
 
@@ -98,7 +106,11 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
 	if (!ret && requesttype == (USB_TYPE_VENDOR | USB_DIR_IN))
 		memcpy(req->data, buf, req->size);
 
+	kfree(buf);
 	return ret;
+
+err_dealloc:
+	kfree(buf);
 error:
 	deb_info("%s: failed:%d\n", __func__, ret);
 	return ret;
-- 
1.7.4.rc3

Re: [PATCH 05/16] [media] ec168: get rid of on stack dma buffers

[Antti Palosaari]

On 03/15/2011 10:43 AM, Florian Mickler wrote:
> usb_control_msg initiates (and waits for completion of) a dma transfer using
> the supplied buffer. That buffer thus has to be seperately allocated on
> the heap.
>
> In lib/dma_debug.c the function check_for_stack even warns about it:
> 	WARNING: at lib/dma-debug.c:866 check_for_stack
>
> Note: This change is tested to compile only, as I don't have the hardware.
>
> Signed-off-by: Florian Mickler<florian@mickler.org>

Acked-by: Antti Palosaari <crope@iki.fi>
Reviewed-by: Antti Palosaari <crope@iki.fi>
Tested-by: Antti Palosaari <crope@iki.fi>

t. Antti

> ---
>   drivers/media/dvb/dvb-usb/ec168.c |   18 +++++++++++++++---
>   1 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/dvb/dvb-usb/ec168.c b/drivers/media/dvb/dvb-usb/ec168.c
> index 52f5d4f..1ba3e5d 100644
> --- a/drivers/media/dvb/dvb-usb/ec168.c
> +++ b/drivers/media/dvb/dvb-usb/ec168.c
> @@ -36,7 +36,9 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
>   	int ret;
>   	unsigned int pipe;
>   	u8 request, requesttype;
> -	u8 buf[req->size];
> +	u8 *buf;
> +
> +
>
>   	switch (req->cmd) {
>   	case DOWNLOAD_FIRMWARE:
> @@ -72,6 +74,12 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
>   		goto error;
>   	}
>
> +	buf = kmalloc(req->size, GFP_KERNEL);
> +	if (!buf) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
>   	if (requesttype == (USB_TYPE_VENDOR | USB_DIR_OUT)) {
>   		/* write */
>   		memcpy(buf, req->data, req->size);
> @@ -84,13 +92,13 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
>   	msleep(1); /* avoid I2C errors */
>
>   	ret = usb_control_msg(udev, pipe, request, requesttype, req->value,
> -		req->index, buf, sizeof(buf), EC168_USB_TIMEOUT);
> +		req->index, buf, req->size, EC168_USB_TIMEOUT);
>
>   	ec168_debug_dump(request, requesttype, req->value, req->index, buf,
>   		req->size, deb_xfer);
>
>   	if (ret<  0)
> -		goto error;
> +		goto err_dealloc;
>   	else
>   		ret = 0;
>
> @@ -98,7 +106,11 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
>   	if (!ret&&  requesttype == (USB_TYPE_VENDOR | USB_DIR_IN))
>   		memcpy(req->data, buf, req->size);
>
> +	kfree(buf);
>   	return ret;
> +
> +err_dealloc:
> +	kfree(buf);
>   error:
>   	deb_info("%s: failed:%d\n", __func__, ret);
>   	return ret;


-- 
http://palosaari.fi/

Re: [PATCH 05/16] [media] ec168: get rid of on stack dma buffers

[Florian Mickler]

On Fri, 18 Mar 2011 18:36:53 +0200
Antti Palosaari <crope@iki.fi> wrote:

> On 03/15/2011 10:43 AM, Florian Mickler wrote:
> > usb_control_msg initiates (and waits for completion of) a dma transfer using
> > the supplied buffer. That buffer thus has to be seperately allocated on
> > the heap.
> >
> > In lib/dma_debug.c the function check_for_stack even warns about it:
> > 	WARNING: at lib/dma-debug.c:866 check_for_stack
> >
> > Note: This change is tested to compile only, as I don't have the hardware.
> >
> > Signed-off-by: Florian Mickler<florian@mickler.org>
> 
> Acked-by: Antti Palosaari <crope@iki.fi>
> Reviewed-by: Antti Palosaari <crope@iki.fi>
> Tested-by: Antti Palosaari <crope@iki.fi>
> 
> t. Antti
> 

Thanks for the feedback!

Regards,
Flo

[PATCH 06/16] [media] ce6230: get rid of on-stack dma buffer

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/ce6230.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/ce6230.c b/drivers/media/dvb/dvb-usb/ce6230.c
index 3df2045..6d1a304 100644
--- a/drivers/media/dvb/dvb-usb/ce6230.c
+++ b/drivers/media/dvb/dvb-usb/ce6230.c
@@ -39,7 +39,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
 	u8 requesttype;
 	u16 value;
 	u16 index;
-	u8 buf[req->data_len];
+	u8 *buf;
 
 	request = req->cmd;
 	value = req->value;
@@ -62,6 +62,12 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
 		goto error;
 	}
 
+	buf = kmalloc(req->data_len, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
 	if (requesttype == (USB_TYPE_VENDOR | USB_DIR_OUT)) {
 		/* write */
 		memcpy(buf, req->data, req->data_len);
@@ -74,7 +80,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
 	msleep(1); /* avoid I2C errors */
 
 	ret = usb_control_msg(udev, pipe, request, requesttype, value, index,
-				buf, sizeof(buf), CE6230_USB_TIMEOUT);
+				buf, req->data_len, CE6230_USB_TIMEOUT);
 
 	ce6230_debug_dump(request, requesttype, value, index, buf,
 		req->data_len, deb_xfer);
@@ -88,6 +94,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
 	if (!ret && requesttype == (USB_TYPE_VENDOR | USB_DIR_IN))
 		memcpy(req->data, buf, req->data_len);
 
+	kfree(buf);
 error:
 	return ret;
 }
-- 
1.7.4.rc3

Re: [PATCH 06/16] [media] ce6230: get rid of on-stack dma buffer

[Antti Palosaari]

On 03/15/2011 10:43 AM, Florian Mickler wrote:
> usb_control_msg initiates (and waits for completion of) a dma transfer using
> the supplied buffer. That buffer thus has to be seperately allocated on
> the heap.
>
> In lib/dma_debug.c the function check_for_stack even warns about it:
> 	WARNING: at lib/dma-debug.c:866 check_for_stack
>
> Note: This change is tested to compile only, as I don't have the hardware.
>
> Signed-off-by: Florian Mickler<florian@mickler.org>

Acked-by: Antti Palosaari <crope@iki.fi>
Reviewed-by: Antti Palosaari <crope@iki.fi>
Tested-by: Antti Palosaari <crope@iki.fi>


t. Antti

> ---
>   drivers/media/dvb/dvb-usb/ce6230.c |   11 +++++++++--
>   1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/dvb/dvb-usb/ce6230.c b/drivers/media/dvb/dvb-usb/ce6230.c
> index 3df2045..6d1a304 100644
> --- a/drivers/media/dvb/dvb-usb/ce6230.c
> +++ b/drivers/media/dvb/dvb-usb/ce6230.c
> @@ -39,7 +39,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
>   	u8 requesttype;
>   	u16 value;
>   	u16 index;
> -	u8 buf[req->data_len];
> +	u8 *buf;
>
>   	request = req->cmd;
>   	value = req->value;
> @@ -62,6 +62,12 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
>   		goto error;
>   	}
>
> +	buf = kmalloc(req->data_len, GFP_KERNEL);
> +	if (!buf) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
>   	if (requesttype == (USB_TYPE_VENDOR | USB_DIR_OUT)) {
>   		/* write */
>   		memcpy(buf, req->data, req->data_len);
> @@ -74,7 +80,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
>   	msleep(1); /* avoid I2C errors */
>
>   	ret = usb_control_msg(udev, pipe, request, requesttype, value, index,
> -				buf, sizeof(buf), CE6230_USB_TIMEOUT);
> +				buf, req->data_len, CE6230_USB_TIMEOUT);
>
>   	ce6230_debug_dump(request, requesttype, value, index, buf,
>   		req->data_len, deb_xfer);
> @@ -88,6 +94,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
>   	if (!ret&&  requesttype == (USB_TYPE_VENDOR | USB_DIR_IN))
>   		memcpy(req->data, buf, req->data_len);
>
> +	kfree(buf);
>   error:
>   	return ret;
>   }


-- 
http://palosaari.fi/

Re: [PATCH 06/16] [media] ce6230: get rid of on-stack dma buffer

[Florian Mickler]

On Fri, 18 Mar 2011 18:36:11 +0200
Antti Palosaari <crope@iki.fi> wrote:

> On 03/15/2011 10:43 AM, Florian Mickler wrote:
> > usb_control_msg initiates (and waits for completion of) a dma transfer using
> > the supplied buffer. That buffer thus has to be seperately allocated on
> > the heap.
> >
> > In lib/dma_debug.c the function check_for_stack even warns about it:
> > 	WARNING: at lib/dma-debug.c:866 check_for_stack
> >
> > Note: This change is tested to compile only, as I don't have the hardware.
> >
> > Signed-off-by: Florian Mickler<florian@mickler.org>
> 
> Acked-by: Antti Palosaari <crope@iki.fi>
> Reviewed-by: Antti Palosaari <crope@iki.fi>
> Tested-by: Antti Palosaari <crope@iki.fi>
> 
> 
> t. Antti
> 

Thanks for your feedback!

Regards,
Flo

[PATCH 07/16] [media] friio: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/friio.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/friio.c b/drivers/media/dvb/dvb-usb/friio.c
index 14a65b4..76159ae 100644
--- a/drivers/media/dvb/dvb-usb/friio.c
+++ b/drivers/media/dvb/dvb-usb/friio.c
@@ -142,17 +142,20 @@ static u32 gl861_i2c_func(struct i2c_adapter *adapter)
 	return I2C_FUNC_I2C;
 }
 
-
 static int friio_ext_ctl(struct dvb_usb_adapter *adap,
 			 u32 sat_color, int lnb_on)
 {
 	int i;
 	int ret;
 	struct i2c_msg msg;
-	u8 buf[2];
+	u8 *buf;
 	u32 mask;
 	u8 lnb = (lnb_on) ? FRIIO_CTL_LNB : 0;
 
+	buf = kmalloc(2, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
 	msg.addr = 0x00;
 	msg.flags = 0;
 	msg.len = 2;
@@ -189,6 +192,7 @@ static int friio_ext_ctl(struct dvb_usb_adapter *adap,
 	buf[1] |= FRIIO_CTL_CLK;
 	ret += gl861_i2c_xfer(&adap->dev->i2c_adap, &msg, 1);
 
+	kfree(buf);
 	return (ret == 70);
 }
 
@@ -219,11 +223,20 @@ static int friio_initialize(struct dvb_usb_device *d)
 	int ret;
 	int i;
 	int retry = 0;
-	u8 rbuf[2];
-	u8 wbuf[3];
+	u8 *rbuf, *wbuf;
 
 	deb_info("%s called.\n", __func__);
 
+	wbuf = kmalloc(3, GFP_KERNEL);
+	if (!wbuf)
+		return -ENOMEM;
+
+	rbuf = kmalloc(2, GFP_KERNEL);
+	if (!rbuf) {
+		kfree(wbuf);
+		return -ENOMEM;
+	}
+
 	/* use gl861_i2c_msg instead of gl861_i2c_xfer(), */
 	/* because the i2c device is not set up yet. */
 	wbuf[0] = 0x11;
@@ -358,6 +371,8 @@ restart:
 	return 0;
 
 error:
+	kfree(wbuf);
+	kfree(rbuf);
 	deb_info("%s:ret == %d\n", __func__, ret);
 	return -EIO;
 }
-- 
1.7.4.rc3

[PATCH 11/16] [media] lmedm04: correct indentation

[Florian Mickler]

This should not change anything except whitespace.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/lmedm04.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/lmedm04.c b/drivers/media/dvb/dvb-usb/lmedm04.c
index 9eea418..0a3e88f 100644
--- a/drivers/media/dvb/dvb-usb/lmedm04.c
+++ b/drivers/media/dvb/dvb-usb/lmedm04.c
@@ -626,15 +626,15 @@ static int lme2510_download_firmware(struct usb_device *dev,
 				data[0] = i | 0x80;
 				dlen = (u8)(end - j)-1;
 			}
-		data[1] = dlen;
-		memcpy(&data[2], fw_data, dlen+1);
-		wlen = (u8) dlen + 4;
-		data[wlen-1] = check_sum(fw_data, dlen+1);
-		deb_info(1, "Data S=%02x:E=%02x CS= %02x", data[3],
+			data[1] = dlen;
+			memcpy(&data[2], fw_data, dlen+1);
+			wlen = (u8) dlen + 4;
+			data[wlen-1] = check_sum(fw_data, dlen+1);
+			deb_info(1, "Data S=%02x:E=%02x CS= %02x", data[3],
 				data[dlen+2], data[dlen+3]);
-		ret |= lme2510_bulk_write(dev, data,  wlen, 1);
-		ret |= lme2510_bulk_read(dev, data, len_in , 1);
-		ret |= (data[0] == 0x88) ? 0 : -1;
+			ret |= lme2510_bulk_write(dev, data,  wlen, 1);
+			ret |= lme2510_bulk_read(dev, data, len_in , 1);
+			ret |= (data[0] == 0x88) ? 0 : -1;
 		}
 	}
 
-- 
1.7.4.rc3

[PATCH 12/16] [media] lmedm04: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/lmedm04.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/lmedm04.c b/drivers/media/dvb/dvb-usb/lmedm04.c
index 0a3e88f..bec5439 100644
--- a/drivers/media/dvb/dvb-usb/lmedm04.c
+++ b/drivers/media/dvb/dvb-usb/lmedm04.c
@@ -314,12 +314,17 @@ static int lme2510_int_read(struct dvb_usb_adapter *adap)
 static int lme2510_return_status(struct usb_device *dev)
 {
 	int ret = 0;
-	u8 data[10] = {0};
+	u8 *data;
+
+	data = kzalloc(10, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
 	ret |= usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
 			0x06, 0x80, 0x0302, 0x00, data, 0x0006, 200);
 	info("Firmware Status: %x (%x)", ret , data[2]);
 
+	kfree(data);
 	return (ret < 0) ? -ENODEV : data[2];
 }
 
@@ -603,7 +608,7 @@ static int lme2510_download_firmware(struct usb_device *dev,
 					const struct firmware *fw)
 {
 	int ret = 0;
-	u8 data[512] = {0};
+	u8 *data;
 	u16 j, wlen, len_in, start, end;
 	u8 packet_size, dlen, i;
 	u8 *fw_data;
@@ -611,6 +616,11 @@ static int lme2510_download_firmware(struct usb_device *dev,
 	packet_size = 0x31;
 	len_in = 1;
 
+	data = kzalloc(512, GFP_KERNEL);
+	if (!data) {
+		info("FRM Could not start Firmware Download (Buffer allocation failed)");
+		return -ENOMEM;
+	}
 
 	info("FRM Starting Firmware Download");
 
@@ -654,7 +664,7 @@ static int lme2510_download_firmware(struct usb_device *dev,
 	else
 		info("FRM Firmware Download Completed - Resetting Device");
 
-
+	kfree(data);
 	return (ret < 0) ? -ENODEV : 0;
 }
 
-- 
1.7.4.rc3

Re: [PATCH 12/16] [media] lmedm04: get rid of on-stack dma buffers

[Malcolm Priestley]

The patch failed for the following reason.

On Tue, 2011-03-15 at 09:43 +0100, Florian Mickler wrote:
> usb_control_msg initiates (and waits for completion of) a dma transfer using
> the supplied buffer. That buffer thus has to be seperately allocated on
> the heap.
> 
> In lib/dma_debug.c the function check_for_stack even warns about it:
> 	WARNING: at lib/dma-debug.c:866 check_for_stack
> 
> Note: This change is tested to compile only, as I don't have the hardware.
> 
> Signed-off-by: Florian Mickler <florian@mickler.org>
> ---
>  drivers/media/dvb/dvb-usb/lmedm04.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/dvb/dvb-usb/lmedm04.c b/drivers/media/dvb/dvb-usb/lmedm04.c
> index 0a3e88f..bec5439 100644
> --- a/drivers/media/dvb/dvb-usb/lmedm04.c
> +++ b/drivers/media/dvb/dvb-usb/lmedm04.c
> @@ -314,12 +314,17 @@ static int lme2510_int_read(struct dvb_usb_adapter *adap)
>  static int lme2510_return_status(struct usb_device *dev)
>  {
>  	int ret = 0;
> -	u8 data[10] = {0};
> +	u8 *data;
> +
> +	data = kzalloc(10, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
>  
>  	ret |= usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
>  			0x06, 0x80, 0x0302, 0x00, data, 0x0006, 200);
>  	info("Firmware Status: %x (%x)", ret , data[2]);
>  
> +	kfree(data);
>  	return (ret < 0) ? -ENODEV : data[2];

data has been killed off when we need the buffer contents.
changing to the following fixed.

	ret = (ret < 0) ? -ENODEV : data[2];

	kfree(data);

	return ret;


> @@ -603,7 +608,7 @@ static int lme2510_download_firmware(struct usb_device *dev,
>  					const struct firmware *fw)
>  {
>  	int ret = 0;
> -	u8 data[512] = {0};
> +	u8 *data;
>  	u16 j, wlen, len_in, start, end;
>  	u8 packet_size, dlen, i;
>  	u8 *fw_data;
> @@ -611,6 +616,11 @@ static int lme2510_download_firmware(struct usb_device *dev,
>  	packet_size = 0x31;
>  	len_in = 1;
>  
> +	data = kzalloc(512, GFP_KERNEL);
> +	if (!data) {
> +		info("FRM Could not start Firmware Download (Buffer allocation failed)");

Longer than 80 characters, 

> +		return -ENOMEM;
> +	}
>  
>  	info("FRM Starting Firmware Download");
>  
> @@ -654,7 +664,7 @@ static int lme2510_download_firmware(struct usb_device *dev,
>  	else
>  		info("FRM Firmware Download Completed - Resetting Device");
>  
> -
> +	kfree(data);
>  	return (ret < 0) ? -ENODEV : 0;
>  }
>  

Otherwise the patch as corrected has been put on test. No initial
problems have been encountered.

Regards 

Malcolm

Re: [PATCH 12/16] [media] lmedm04: get rid of on-stack dma buffers

[Florian Mickler]

On Tue, 15 Mar 2011 20:54:43 +0000
Malcolm Priestley <tvboxspy@gmail.com> wrote:

> The patch failed for the following reason.
> 
> On Tue, 2011-03-15 at 09:43 +0100, Florian Mickler wrote:
> > usb_control_msg initiates (and waits for completion of) a dma transfer using
> > the supplied buffer. That buffer thus has to be seperately allocated on
> > the heap.
> > 
> > In lib/dma_debug.c the function check_for_stack even warns about it:
> > 	WARNING: at lib/dma-debug.c:866 check_for_stack
> > 
> > Note: This change is tested to compile only, as I don't have the hardware.
> > 
> > Signed-off-by: Florian Mickler <florian@mickler.org>
> > ---
> >  drivers/media/dvb/dvb-usb/lmedm04.c |   16 +++++++++++++---
> >  1 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/dvb/dvb-usb/lmedm04.c b/drivers/media/dvb/dvb-usb/lmedm04.c
> > index 0a3e88f..bec5439 100644
> > --- a/drivers/media/dvb/dvb-usb/lmedm04.c
> > +++ b/drivers/media/dvb/dvb-usb/lmedm04.c
> > @@ -314,12 +314,17 @@ static int lme2510_int_read(struct dvb_usb_adapter *adap)
> >  static int lme2510_return_status(struct usb_device *dev)
> >  {
> >  	int ret = 0;
> > -	u8 data[10] = {0};
> > +	u8 *data;
> > +
> > +	data = kzalloc(10, GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> >  
> >  	ret |= usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
> >  			0x06, 0x80, 0x0302, 0x00, data, 0x0006, 200);
> >  	info("Firmware Status: %x (%x)", ret , data[2]);
> >  
> > +	kfree(data);
> >  	return (ret < 0) ? -ENODEV : data[2];
> 
> data has been killed off when we need the buffer contents.
> changing to the following fixed.
> 
> 	ret = (ret < 0) ? -ENODEV : data[2];
> 
> 	kfree(data);
> 
> 	return ret;

Yes. Thanks. I updated the patch locally. 

> 
> > @@ -603,7 +608,7 @@ static int lme2510_download_firmware(struct usb_device *dev,
> >  					const struct firmware *fw)
> >  {
> >  	int ret = 0;
> > -	u8 data[512] = {0};
> > +	u8 *data;
> >  	u16 j, wlen, len_in, start, end;
> >  	u8 packet_size, dlen, i;
> >  	u8 *fw_data;
> > @@ -611,6 +616,11 @@ static int lme2510_download_firmware(struct usb_device *dev,
> >  	packet_size = 0x31;
> >  	len_in = 1;
> >  
> > +	data = kzalloc(512, GFP_KERNEL);
> > +	if (!data) {
> > +		info("FRM Could not start Firmware Download (Buffer allocation failed)");
> 
> Longer than 80 characters, 

I choose to ignore this warning to keep the string on one line (for
grep and co).

> > +		return -ENOMEM;
> > +	}
> >  
> >  	info("FRM Starting Firmware Download");
> >  
> > @@ -654,7 +664,7 @@ static int lme2510_download_firmware(struct usb_device *dev,
> >  	else
> >  		info("FRM Firmware Download Completed - Resetting Device");
> >  
> > -
> > +	kfree(data);
> >  	return (ret < 0) ? -ENODEV : 0;
> >  }
> >  
> 
> Otherwise the patch as corrected has been put on test. No initial
> problems have been encountered.
> 
> Regards 
> 
> Malcolm
> 

Thanks. I added a 
Tested-by: Malcolm Priestley <tvboxspy@gmail.com>,
if that is ok?

Do you know how often/when is the .identify_state callback called during
normal operation? I.e. is this memory allocation/deallocation
in lme2510_return_status happening often and should use a preallocated
buffer?

Regards,
Flo

Re: [PATCH 01/16] [media] dib0700: get rid of on-stack dma buffers

[Mauro Carvalho Chehab]

Em 15-03-2011 05:43, Florian Mickler escreveu:
> usb_control_msg initiates (and waits for completion of) a dma transfer using
> the supplied buffer. That buffer thus has to be seperately allocated on
> the heap.
> 
> In lib/dma_debug.c the function check_for_stack even warns about it:
> 	WARNING: at lib/dma-debug.c:866 check_for_stack
> 
> Note: This change is tested to compile only, as I don't have the hardware.
> 
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
> Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
> Signed-off-by: Florian Mickler <florian@mickler.org>
> 
> [v2: use preallocated buffer; fix sizeof in one case]
> [v3: use seperate kmalloc mapping for the preallocation,
>      dont ignore errors in probe codepaths  ]
> [v4: minor style nit: functions opening brace goes onto it's own line ]


I'm c/c Patrick, as he is the driver maintainer for those Dibcom drivers.

> ---
>  drivers/media/dvb/dvb-usb/dib0700.h      |    5 +-
>  drivers/media/dvb/dvb-usb/dib0700_core.c |  120 ++++++++++++++++++++++++------
>  2 files changed, 99 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/dvb/dvb-usb/dib0700.h b/drivers/media/dvb/dvb-usb/dib0700.h
> index 3537d65..99a1485 100644
> --- a/drivers/media/dvb/dvb-usb/dib0700.h
> +++ b/drivers/media/dvb/dvb-usb/dib0700.h
> @@ -45,8 +45,9 @@ struct dib0700_state {
>  	u8 is_dib7000pc;
>  	u8 fw_use_new_i2c_api;
>  	u8 disable_streaming_master_mode;
> -    u32 fw_version;
> -    u32 nb_packet_buffer_size;
> +	u32 fw_version;
> +	u32 nb_packet_buffer_size;
> +	u8 *buf;
>  };
>  
>  extern int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
> diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
> index 98ffb40..1c19b73 100644
> --- a/drivers/media/dvb/dvb-usb/dib0700_core.c
> +++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
> @@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>  int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
>  			u32 *romversion, u32 *ramversion, u32 *fwtype)
>  {
> -	u8 b[16];
> -	int ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
> +	int ret;
> +	u8 *b;
> +
> +	b = kmalloc(16, GFP_KERNEL);
> +	if (!b)
> +		return -ENOMEM;
> +
> +	ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0),
>  				  REQUEST_GET_VERSION,
>  				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
> -				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
> +				  b, 16, USB_CTRL_GET_TIMEOUT);
>  	if (hwversion != NULL)
>  		*hwversion  = (b[0] << 24)  | (b[1] << 16)  | (b[2] << 8)  | b[3];
>  	if (romversion != NULL)
> @@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
>  		*ramversion = (b[8] << 24)  | (b[9] << 16)  | (b[10] << 8) | b[11];
>  	if (fwtype != NULL)
>  		*fwtype     = (b[12] << 24) | (b[13] << 16) | (b[14] << 8) | b[15];
> +
> +	kfree(b);
>  	return ret;
>  }
>  
> @@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 txlen, u8 *rx, u8 rxlen
>  
>  int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 gpio_dir, u8 gpio_val)
>  {
> -	u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6) };
> -	return dib0700_ctrl_wr(d, buf, sizeof(buf));
> +	s16 ret;
> +	u8 *buf = kmalloc(3, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	buf[0] = REQUEST_SET_GPIO;
> +	buf[1] = gpio;
> +	buf[2] = ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6);
> +
> +	ret = dib0700_ctrl_wr(d, buf, 3);
> +
> +	kfree(buf);
> +	return ret;
>  }
>  
>  static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 nb_ts_packets)
> @@ -137,11 +156,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
>  	   properly support i2c read calls not preceded by a write */
>  
>  	struct dvb_usb_device *d = i2c_get_adapdata(adap);
> +	struct dib0700_state *st = d->priv;
>  	uint8_t bus_mode = 1;  /* 0=eeprom bus, 1=frontend bus */
>  	uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
>  	uint8_t en_start = 0;
>  	uint8_t en_stop = 0;
> -	uint8_t buf[255]; /* TBV: malloc ? */
> +	uint8_t *buf = st->buf;
>  	int result, i;
>  
>  	/* Ensure nobody else hits the i2c bus while we're sending our
> @@ -221,6 +241,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
>  		}
>  	}
>  	mutex_unlock(&d->i2c_mutex);
> +
>  	return i;
>  }
>  
> @@ -231,8 +252,9 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
>  				   struct i2c_msg *msg, int num)
>  {
>  	struct dvb_usb_device *d = i2c_get_adapdata(adap);
> +	struct dib0700_state *st = d->priv;
>  	int i,len;
> -	u8 buf[255];
> +	u8 *buf = st->buf;
>  
>  	if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
>  		return -EAGAIN;
> @@ -264,8 +286,8 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
>  				break;
>  		}
>  	}
> -
>  	mutex_unlock(&d->i2c_mutex);
> +
>  	return i;
>  }
>  
> @@ -297,15 +319,23 @@ struct i2c_algorithm dib0700_i2c_algo = {
>  int dib0700_identify_state(struct usb_device *udev, struct dvb_usb_device_properties *props,
>  			struct dvb_usb_device_description **desc, int *cold)
>  {
> -	u8 b[16];
> -	s16 ret = usb_control_msg(udev, usb_rcvctrlpipe(udev,0),
> +	s16 ret;
> +	u8 *b;
> +
> +	b = kmalloc(16, GFP_KERNEL);
> +	if (!b)
> +		return	-ENOMEM;
> +
> +
> +	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>  		REQUEST_GET_VERSION, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, b, 16, USB_CTRL_GET_TIMEOUT);
>  
>  	deb_info("FW GET_VERSION length: %d\n",ret);
>  
>  	*cold = ret <= 0;
> -
>  	deb_info("cold: %d\n", *cold);
> +
> +	kfree(b);
>  	return 0;
>  }
>  
> @@ -313,7 +343,13 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
>  	u8 pll_src, u8 pll_range, u8 clock_gpio3, u16 pll_prediv,
>  	u16 pll_loopdiv, u16 free_div, u16 dsuScaler)
>  {
> -	u8 b[10];
> +	s16 ret;
> +	u8 *b;
> +
> +	b = kmalloc(10, GFP_KERNEL);
> +	if (!b)
> +		return -ENOMEM;
> +
>  	b[0] = REQUEST_SET_CLOCK;
>  	b[1] = (en_pll << 7) | (pll_src << 6) | (pll_range << 5) | (clock_gpio3 << 4);
>  	b[2] = (pll_prediv >> 8)  & 0xff; // MSB
> @@ -325,7 +361,10 @@ static int dib0700_set_clock(struct dvb_usb_device *d, u8 en_pll,
>  	b[8] = (dsuScaler >> 8)   & 0xff; // MSB
>  	b[9] =  dsuScaler         & 0xff; // LSB
>  
> -	return dib0700_ctrl_wr(d, b, 10);
> +	ret = dib0700_ctrl_wr(d, b, 10);
> +
> +	kfree(b);
> +	return ret;
>  }
>  
>  int dib0700_ctrl_clock(struct dvb_usb_device *d, u32 clk_MHz, u8 clock_out_gp3)
> @@ -361,11 +400,14 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
>  {
>  	struct hexline hx;
>  	int pos = 0, ret, act_len, i, adap_num;
> -	u8 b[16];
> +	u8 *b;
>  	u32 fw_version;
> -
>  	u8 buf[260];
>  
> +	b = kmalloc(16, GFP_KERNEL);
> +	if (!b)
> +		return -ENOMEM;
> +
>  	while ((ret = dvb_usb_get_hexline(fw, &hx, &pos)) > 0) {
>  		deb_fwdata("writing to address 0x%08x (buffer: 0x%02x %02x)\n",
>  				hx.addr, hx.len, hx.chk);
> @@ -386,7 +428,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
>  
>  		if (ret < 0) {
>  			err("firmware download failed at %d with %d",pos,ret);
> -			return ret;
> +			goto out;
>  		}
>  	}
>  
> @@ -407,7 +449,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
>  	usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>  				  REQUEST_GET_VERSION,
>  				  USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
> -				  b, sizeof(b), USB_CTRL_GET_TIMEOUT);
> +				  b, 16, USB_CTRL_GET_TIMEOUT);
>  	fw_version = (b[8] << 24) | (b[9] << 16) | (b[10] << 8) | b[11];
>  
>  	/* set the buffer size - DVB-USB is allocating URB buffers
> @@ -426,16 +468,21 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
>  			}
>  		}
>  	}
> -
> +out:
> +	kfree(b);
>  	return ret;
>  }
>  
>  int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
>  {
>  	struct dib0700_state *st = adap->dev->priv;
> -	u8 b[4];
> +	u8 *b;
>  	int ret;
>  
> +	b = kmalloc(4, GFP_KERNEL);
> +	if (!b)
> +		return -ENOMEM;
> +
>  	if ((onoff != 0) && (st->fw_version >= 0x10201)) {
>  		/* for firmware later than 1.20.1,
>  		 * the USB xfer length can be set  */
> @@ -443,7 +490,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
>  			st->nb_packet_buffer_size);
>  		if (ret < 0) {
>  			deb_info("can not set the USB xfer len\n");
> -			return ret;
> +			goto out;
>  		}
>  	}
>  
> @@ -468,7 +515,10 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
>  
>  	deb_info("data for streaming: %x %x\n", b[1], b[2]);
>  
> -	return dib0700_ctrl_wr(adap->dev, b, 4);
> +	ret = dib0700_ctrl_wr(adap->dev, b, 4);
> +out:
> +	kfree(b);
> +	return ret;
>  }
>  
>  int dib0700_change_protocol(struct rc_dev *rc, u64 rc_type)
> @@ -650,6 +700,7 @@ static int dib0700_probe(struct usb_interface *intf,
>  		const struct usb_device_id *id)
>  {
>  	int i;
> +	int ret;
>  	struct dvb_usb_device *dev;
>  
>  	for (i = 0; i < dib0700_device_count; i++)
> @@ -658,8 +709,10 @@ static int dib0700_probe(struct usb_interface *intf,
>  			struct dib0700_state *st = dev->priv;
>  			u32 hwversion, romversion, fw_version, fwtype;
>  
> -			dib0700_get_version(dev, &hwversion, &romversion,
> +			ret = dib0700_get_version(dev, &hwversion, &romversion,
>  				&fw_version, &fwtype);
> +			if (ret < 0)
> +				goto out;
>  
>  			deb_info("Firmware version: %x, %d, 0x%x, %d\n",
>  				hwversion, romversion, fw_version, fwtype);
> @@ -673,18 +726,37 @@ static int dib0700_probe(struct usb_interface *intf,
>  			else
>  				dev->props.rc.core.bulk_mode = false;
>  
> -			dib0700_rc_setup(dev);
> +			ret = dib0700_rc_setup(dev);
> +			if (ret)
> +				goto out;
> +
> +			st->buf = kmalloc(255, GFP_KERNEL);
> +			if (!st->buf) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}

You're allocating a buffer for URB control messages. IMO, this is a good idea, as
this way, allocating/freeing on each urb call is avoided. However, on most places,
you're not using it. The better would be to just use this buffer on all
the above places.

You should just take care of protecting such buffer with a mutex, to avoid concurrency.
Such kind of protection is generally ok, as dvb applications generally serialize
the calls anyway.



>  
>  			return 0;
> +out:
> +			dvb_usb_device_exit(intf);
> +			return ret;
>  		}
>  
>  	return -ENODEV;
>  }
>  
> +static void dib0700_disconnect(struct usb_interface *intf)
> +{
> +	struct dvb_usb_device *d = usb_get_intfdata(intf);
> +	struct dib0700_state *st = d->priv;
> +	kfree(st->buf);
> +	dvb_usb_device_exit(intf);
> +}
> +
>  static struct usb_driver dib0700_driver = {
>  	.name       = "dvb_usb_dib0700",
>  	.probe      = dib0700_probe,
> -	.disconnect = dvb_usb_device_exit,
> +	.disconnect = dib0700_disconnect,
>  	.id_table   = dib0700_usb_id_table,
>  };
>  

Re: [PATCH 01/16] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

On Tue, 15 Mar 2011 09:02:00 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> 
> You're allocating a buffer for URB control messages. IMO, this is a good idea, as
> this way, allocating/freeing on each urb call is avoided. However, on most places,
> you're not using it. The better would be to just use this buffer on all
> the above places.
> 
> You should just take care of protecting such buffer with a mutex, to avoid concurrency.
> Such kind of protection is generally ok, as dvb applications generally serialize
> the calls anyway.
> 

I didn't do so already, because I had/have no overview over the big
picture operation of the dvb framework and thus feared to introduce
deadlocks or massive serializations where concurrency is needed. But if
you suggest it, I guess it should be benign. I'm wondering about the
purpose of the usb_mutex and the i2c_mutex ... 

Should I introduce new driver specific mutexes to protect the buffer or
is it possible to reuse one of the 2? 

vp702x_usb_inout_op takes the usb_mutex, 
but vp702x_usb_out_op and vp702x_usb_in_op get called without that
mutex hold. That makes me wonder what that mutex purpose is in that
driver?

Other drivers like the az6027 introduce a driver specific mutex and
also use the usb_mutex. That make conceptually (to my inexperienced
mind) more sense. (each layer does it's own locking) but the idea of
operation is not yet clear in my mind...

Can you perhaps shed some light on what the purpose of those locks is
and if it is sufficient to use the usb_mutex to serialize all
usb_control_msg calls (which would probably protect the buffer
sufficiently but may be too much if the dvb-usb framework uses that
mutex for different purposes). 

In the meantime I will respin this series using preallocated buffers and
will hopefully work out stuff that is unclear to me yet ...

Regards,
Flo

Re: [PATCH 09/16] [media] au6610: get rid of on-stack dma buffer

[Antti Palosaari]

On 03/15/2011 10:43 AM, Florian Mickler wrote:
> usb_control_msg initiates (and waits for completion of) a dma transfer using
> the supplied buffer. That buffer thus has to be seperately allocated on
> the heap.
>
> In lib/dma_debug.c the function check_for_stack even warns about it:
> 	WARNING: at lib/dma-debug.c:866 check_for_stack
>
> Note: This change is tested to compile only, as I don't have the hardware.
>
> Signed-off-by: Florian Mickler<florian@mickler.org>


This patch did not found from patchwork! Probably skipped due to broken 
Cc at my contact. Please resend.

Anyhow, I tested and reviewed it.

Acked-by: Antti Palosaari <crope@iki.fi>
Reviewed-by: Antti Palosaari <crope@iki.fi>
Tested-by: Antti Palosaari <crope@iki.fi>

[1] https://patchwork.kernel.org/project/linux-media/list/

Antti



> ---
>   drivers/media/dvb/dvb-usb/au6610.c |   22 ++++++++++++++++------
>   1 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/dvb/dvb-usb/au6610.c b/drivers/media/dvb/dvb-usb/au6610.c
> index eb34cc3..2351077 100644
> --- a/drivers/media/dvb/dvb-usb/au6610.c
> +++ b/drivers/media/dvb/dvb-usb/au6610.c
> @@ -33,8 +33,16 @@ static int au6610_usb_msg(struct dvb_usb_device *d, u8 operation, u8 addr,
>   {
>   	int ret;
>   	u16 index;
> -	u8 usb_buf[6]; /* enough for all known requests,
> -			  read returns 5 and write 6 bytes */
> +	u8 *usb_buf;
> +
> +	/*
> +	 * allocate enough for all known requests,
> +	 * read returns 5 and write 6 bytes
> +	 */
> +	usb_buf = kmalloc(6, GFP_KERNEL);
> +	if (!usb_buf)
> +		return -ENOMEM;
> +
>   	switch (wlen) {
>   	case 1:
>   		index = wbuf[0]<<  8;
> @@ -45,14 +53,15 @@ static int au6610_usb_msg(struct dvb_usb_device *d, u8 operation, u8 addr,
>   		break;
>   	default:
>   		warn("wlen = %x, aborting.", wlen);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto error;
>   	}
>
>   	ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0), operation,
>   			      USB_TYPE_VENDOR|USB_DIR_IN, addr<<  1, index,
> -			      usb_buf, sizeof(usb_buf), AU6610_USB_TIMEOUT);
> +			      usb_buf, 6, AU6610_USB_TIMEOUT);
>   	if (ret<  0)
> -		return ret;
> +		goto error;
>
>   	switch (operation) {
>   	case AU6610_REQ_I2C_READ:
> @@ -60,7 +69,8 @@ static int au6610_usb_msg(struct dvb_usb_device *d, u8 operation, u8 addr,
>   		/* requested value is always 5th byte in buffer */
>   		rbuf[0] = usb_buf[4];
>   	}
> -
> +error:
> +	kfree(usb_buf);
>   	return ret;
>   }
>


-- 
http://palosaari.fi/

Re: [PATCH 09/16] [media] au6610: get rid of on-stack dma buffer

[Florian Mickler]

On Fri, 18 Mar 2011 18:34:58 +0200
Antti Palosaari <crope@iki.fi> wrote:

> On 03/15/2011 10:43 AM, Florian Mickler wrote:
> > usb_control_msg initiates (and waits for completion of) a dma transfer using
> > the supplied buffer. That buffer thus has to be seperately allocated on
> > the heap.
> >
> > In lib/dma_debug.c the function check_for_stack even warns about it:
> > 	WARNING: at lib/dma-debug.c:866 check_for_stack
> >
> > Note: This change is tested to compile only, as I don't have the hardware.
> >
> > Signed-off-by: Florian Mickler<florian@mickler.org>
> 
> 
> This patch did not found from patchwork! Probably skipped due to broken 
> Cc at my contact. Please resend.
> 
> Anyhow, I tested and reviewed it.
> 
> Acked-by: Antti Palosaari <crope@iki.fi>
> Reviewed-by: Antti Palosaari <crope@iki.fi>
> Tested-by: Antti Palosaari <crope@iki.fi>
> 
> [1] https://patchwork.kernel.org/project/linux-media/list/
> 
> Antti
> 

Yes, there was some broken adressing on my side. Sorry. 

Thanks for review && test!  I will resend (hopefully this weekend) the
series when I reviewed some of the other patches if it is
feasible/better to use prealocated memory as suggested by Mauro. 

How often does au6610_usb_msg get called in normal operation? Should it
use preallocated memory?

Regards,
Flo

Re: [PATCH 09/16] [media] au6610: get rid of on-stack dma buffer

[Antti Palosaari]

On 03/18/2011 11:27 PM, Florian Mickler wrote:
> On Fri, 18 Mar 2011 18:34:58 +0200
> Antti Palosaari<crope@iki.fi>  wrote:
>
>> On 03/15/2011 10:43 AM, Florian Mickler wrote:
>>> usb_control_msg initiates (and waits for completion of) a dma transfer using
>>> the supplied buffer. That buffer thus has to be seperately allocated on
>>> the heap.
>>>
>>> In lib/dma_debug.c the function check_for_stack even warns about it:
>>> 	WARNING: at lib/dma-debug.c:866 check_for_stack
>>>
>>> Note: This change is tested to compile only, as I don't have the hardware.
>>>
>>> Signed-off-by: Florian Mickler<florian@mickler.org>
>>
>>
>> This patch did not found from patchwork! Probably skipped due to broken
>> Cc at my contact. Please resend.
>>
>> Anyhow, I tested and reviewed it.
>>
>> Acked-by: Antti Palosaari<crope@iki.fi>
>> Reviewed-by: Antti Palosaari<crope@iki.fi>
>> Tested-by: Antti Palosaari<crope@iki.fi>
>>
>> [1] https://patchwork.kernel.org/project/linux-media/list/
>>
>> Antti
>>
>
> Yes, there was some broken adressing on my side. Sorry.
>
> Thanks for review&&  test!  I will resend (hopefully this weekend) the
> series when I reviewed some of the other patches if it is
> feasible/better to use prealocated memory as suggested by Mauro.
>
> How often does au6610_usb_msg get called in normal operation? Should it
> use preallocated memory?

It is called by demodulator and tuner drivers via I2C. One call per one 
register access. Tuner driver is qt1010 and demod driver is zl10353. 
When you perform tune to channel and device is in sleep there is maybe 
100 or more calls. After channel is tuned as OK, application starts 
calling only some signalling statistics from demod, it is usually only 
few calls per sec.

On my experience I cannot say if it is wise to preallocate or not. 
Anyhow, this same apply for all DVB USB drivers.

Antti
-- 
http://palosaari.fi/

Re: [PATCH 1/2 v3] [media] dib0700: get rid of on-stack dma buffers

[Mauro Carvalho Chehab]

Em 15-03-2011 05:36, Florian Mickler escreveu:
> On Sun, 6 Mar 2011 18:57:13 +0100
> Florian Mickler <florian@mickler.org> wrote:
> 
>> On Sun,  6 Mar 2011 18:47:56 +0100
>> Florian Mickler <florian@mickler.org> wrote:
>>
>>
>>> +static void dib0700_disconnect(struct usb_interface *intf) {
>>
>>
>> That { should go on its own line... sorry ;-)
>>
>> If that patch is acceptable, I can resend with that fixed. 
>>
>> Regards,
>> Flo
> 
> 
> Hi Mauro, 
> 
> what about this patch? I have similar patches for the same problem in
> the other dvb-usb drivers in need of beeing fixed. Are you
> maintaining these drivers or should I find someone else to pick these
> up? 

I generally wait for the driver maintainer to review and test on their
hardware before applying on my tree.

I'll send you a few comments over the first patch on a separate email,
that also applies to the other patches.

Thanks,
Mauro.

Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers

[Jack Stone]

On 06/03/2011 11:16, Florian Mickler wrote:
> This should fix warnings seen by some:
> 	WARNING: at lib/dma-debug.c:866 check_for_stack
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
> Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
> Signed-off-by: Florian Mickler <florian@mickler.org>
> CC: Mauro Carvalho Chehab <mchehab@infradead.org>
> CC: linux-media@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: Greg Kroah-Hartman <greg@kroah.com>
> CC: Rafael J. Wysocki <rjw@sisk.pl>
> CC: Maciej Rutecki <maciej.rutecki@gmail.com>
> ---
> @@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 txlen, u8 *rx, u8 rxlen
>  
>  int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 gpio_dir, u8 gpio_val)
>  {
> -	u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6) };
> -	return dib0700_ctrl_wr(d, buf, sizeof(buf));
> +	s16 ret;
> +	u8 *buf = kmalloc(3, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	buf[0] = REQUEST_SET_GPIO;
> +	buf[1] = gpio;
> +	buf[2] = ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6);
> +
> +	ret = dib0700_ctrl_wr(d, buf, sizeof(buf));

Shouldn't this sizeof be changed as well?

Thanks,

Jack

Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers

[Florian Mickler]

On Sun, 06 Mar 2011 13:49:56 +0000
Jack Stone <jwjstone@fastmail.fm> wrote:

> On 06/03/2011 11:16, Florian Mickler wrote:
> > This should fix warnings seen by some:
> > 	WARNING: at lib/dma-debug.c:866 check_for_stack
> > 
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
> > Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
> > Signed-off-by: Florian Mickler <florian@mickler.org>
> > CC: Mauro Carvalho Chehab <mchehab@infradead.org>
> > CC: linux-media@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
> > CC: Greg Kroah-Hartman <greg@kroah.com>
> > CC: Rafael J. Wysocki <rjw@sisk.pl>
> > CC: Maciej Rutecki <maciej.rutecki@gmail.com>
> > ---
> > @@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 txlen, u8 *rx, u8 rxlen
> >  
> >  int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 gpio_dir, u8 gpio_val)
> >  {
> > -	u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6) };
> > -	return dib0700_ctrl_wr(d, buf, sizeof(buf));
> > +	s16 ret;
> > +	u8 *buf = kmalloc(3, GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	buf[0] = REQUEST_SET_GPIO;
> > +	buf[1] = gpio;
> > +	buf[2] = ((gpio_dir & 0x01) << 7) | ((gpio_val & 0x01) << 6);
> > +
> > +	ret = dib0700_ctrl_wr(d, buf, sizeof(buf));
> 
> Shouldn't this sizeof be changed as well?
> 
> Thanks,
> 
> Jack

argh... indeed.