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 <[email protected]>
Signed-off-by: Florian Mickler <[email protected]>
CC: Mauro Carvalho Chehab <[email protected]>
CC: [email protected]
CC: [email protected]
CC: Greg Kroah-Hartman <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Maciej Rutecki <[email protected]>
---
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
Signed-off-by: Florian Mickler <[email protected]>
CC: Mauro Carvalho Chehab <[email protected]>
CC: [email protected]
CC: [email protected]
CC: Greg Kroah-Hartman <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Maciej Rutecki <[email protected]>
CC: Oliver Neukum <[email protected]>
CC: Jack Stone <[email protected]>
---
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
Am Sonntag, 6. M?rz 2011, 15:38:05 schrieb Florian Mickler:
> On Sun, 6 Mar 2011 13:06:09 +0100
> Oliver Neukum <[email protected]> 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
On Sun, 6 Mar 2011 16:06:38 +0100
Oliver Neukum <[email protected]> wrote:
> Am Sonntag, 6. M?rz 2011, 15:38:05 schrieb Florian Mickler:
> > On Sun, 6 Mar 2011 13:06:09 +0100
> > Oliver Neukum <[email protected]> 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
Signed-off-by: Florian Mickler <[email protected]>
CC: Mauro Carvalho Chehab <[email protected]>
CC: [email protected]
CC: [email protected]
CC: Greg Kroah-Hartman <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Maciej Rutecki <[email protected]>
CC: Oliver Neukum <[email protected]>
CC: Jack Stone <[email protected]>
---
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
Signed-off-by: Florian Mickler <[email protected]>
CC: [email protected]
CC: [email protected]
CC: Greg Kroah-Hartman <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Maciej Rutecki <[email protected]>
CC: Oliver Neukum <[email protected]>
---
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
On Sun, 6 Mar 2011 13:06:09 +0100
Oliver Neukum <[email protected]> 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 <[email protected]>
> > Signed-off-by: Florian Mickler <[email protected]>
> > CC: Mauro Carvalho Chehab <[email protected]>
> > CC: [email protected]
> > CC: [email protected]
> > CC: Greg Kroah-Hartman <[email protected]>
> > CC: Rafael J. Wysocki <[email protected]>
> > CC: Maciej Rutecki <[email protected]>
> > ---
> >
> > 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
On Sun, 06 Mar 2011 13:49:56 +0000
Jack Stone <[email protected]> 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 <[email protected]>
> > Signed-off-by: Florian Mickler <[email protected]>
> > CC: Mauro Carvalho Chehab <[email protected]>
> > CC: [email protected]
> > CC: [email protected]
> > CC: Greg Kroah-Hartman <[email protected]>
> > CC: Rafael J. Wysocki <[email protected]>
> > CC: Maciej Rutecki <[email protected]>
> > ---
> > @@ -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.
On Sun, 6 Mar 2011 18:47:56 +0100
Florian Mickler <[email protected]> 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
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 <[email protected]>
Signed-off-by: Florian Mickler <[email protected]>
CC: Mauro Carvalho Chehab <[email protected]>
CC: [email protected]
CC: [email protected]
CC: Greg Kroah-Hartman <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Maciej Rutecki <[email protected]>
CC: Oliver Neukum <[email protected]>
CC: Jack Stone <[email protected]>
[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
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
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 <[email protected]>
Signed-off-by: Florian Mickler <[email protected]>
CC: Mauro Carvalho Chehab <[email protected]>
CC: [email protected]
CC: [email protected]
CC: Greg Kroah-Hartman <[email protected]>
CC: Rafael J. Wysocki <[email protected]>
CC: Maciej Rutecki <[email protected]>
CC: Oliver Neukum <[email protected]>
CC: Jack Stone <[email protected]>
---
[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
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 <[email protected]>
> Signed-off-by: Florian Mickler <[email protected]>
> CC: Mauro Carvalho Chehab <[email protected]>
> CC: [email protected]
> CC: [email protected]
> CC: Greg Kroah-Hartman <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Maciej Rutecki <[email protected]>
> ---
> @@ -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
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 <[email protected]>
> Signed-off-by: Florian Mickler <[email protected]>
> CC: Mauro Carvalho Chehab <[email protected]>
> CC: [email protected]
> CC: [email protected]
> CC: Greg Kroah-Hartman <[email protected]>
> CC: Rafael J. Wysocki <[email protected]>
> CC: Maciej Rutecki <[email protected]>
> ---
>
> 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
On Sun, 6 Mar 2011 18:57:13 +0100
Florian Mickler <[email protected]> wrote:
> On Sun, 6 Mar 2011 18:47:56 +0100
> Florian Mickler <[email protected]> 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
This should not change anything except whitespace.
Signed-off-by: Florian Mickler <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Florian Mickler <[email protected]>
[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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
This variable is never used.
Signed-off-by: Florian Mickler <[email protected]>
--
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
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 <[email protected]>
---
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
Em 15-03-2011 05:36, Florian Mickler escreveu:
> On Sun, 6 Mar 2011 18:57:13 +0100
> Florian Mickler <[email protected]> wrote:
>
>> On Sun, 6 Mar 2011 18:47:56 +0100
>> Florian Mickler <[email protected]> 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.
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 <[email protected]>
> Signed-off-by: Florian Mickler <[email protected]>
>
> [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,
> };
>
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 <[email protected]>
> ---
> 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
On Tue, 15 Mar 2011 09:02:00 -0300
Mauro Carvalho Chehab <[email protected]> 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
On Tue, 15 Mar 2011 20:54:43 +0000
Malcolm Priestley <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>,
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
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<[email protected]>
Acked-by: Antti Palosaari <[email protected]>
Reviewed-by: Antti Palosaari <[email protected]>
Tested-by: Antti Palosaari <[email protected]>
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/
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<[email protected]>
Acked-by: Antti Palosaari <[email protected]>
Reviewed-by: Antti Palosaari <[email protected]>
Tested-by: Antti Palosaari <[email protected]>
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/
On Fri, 18 Mar 2011 18:36:11 +0200
Antti Palosaari <[email protected]> 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<[email protected]>
>
> Acked-by: Antti Palosaari <[email protected]>
> Reviewed-by: Antti Palosaari <[email protected]>
> Tested-by: Antti Palosaari <[email protected]>
>
>
> t. Antti
>
Thanks for your feedback!
Regards,
Flo
On Fri, 18 Mar 2011 18:36:53 +0200
Antti Palosaari <[email protected]> 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<[email protected]>
>
> Acked-by: Antti Palosaari <[email protected]>
> Reviewed-by: Antti Palosaari <[email protected]>
> Tested-by: Antti Palosaari <[email protected]>
>
> t. Antti
>
Thanks for the feedback!
Regards,
Flo