Hi,
since I got no reaction[1] on the vp702x driver, I proceed with the
dib0700.
There are multiple drivers in drivers/media/dvb/dvb-usb/ which use
usb_control_msg to perform dma to stack-allocated buffers. This is a bad idea
because of cache-coherency issues and on some platforms the stack is mapped
virtually and also lib/dma-debug.c warn's about it at runtime.
Patches to ec168, ce6230, au6610 and lmedm04 were already tested and reviewed
and submitted for inclusion [2]. Patches to a800, vp7045, friio, dw2102, m920x
and opera1 are still waiting for for review and testing [3].
This patch to dib0700 is a fix for a warning seen and reported by Zdenek
Kabalec in Bug #15977 [4].
Florian Mickler (2):
[media] dib0700: get rid of on-stack dma buffers
[media] dib0700: remove unused variable
Regards,
Flo
References:
[1]: http://www.spinics.net/lists/linux-media/msg30448.html
[2]: http://comments.gmane.org/gmane.linux.kernel/1115404
[3]: http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/e169edc121b91181/f1498cd026a59fe2
[4]: https://bugzilla.kernel.org/show_bug.cgi?id=15977
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 ]
[v5: use preallocated buffer whereever we have a dvb_usb_device
available even if it means acquiring i2c_mutex where we did previously
not + found some more on-stack buffers that escaped my first
review somehow...]
drivers/media/dvb/dvb-usb/dib0700.h | 5 +-
drivers/media/dvb/dvb-usb/dib0700_core.c | 142 +++++++++++++++++++++-----
drivers/media/dvb/dvb-usb/dib0700_devices.c | 9 ++-
3 files changed, 125 insertions(+), 31 deletions(-)
diff --git a/drivers/media/dvb/dvb-usb/dib0700.h b/drivers/media/dvb/dvb-usb/dib0700.h
index b2a87f2..368fbcf 100644
--- a/drivers/media/dvb/dvb-usb/dib0700.h
+++ b/drivers/media/dvb/dvb-usb/dib0700.h
@@ -46,8 +46,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; /* protected by dvb_usb_devices i2c_mutex */
};
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 b79af68..ca80520 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,16 @@ 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),
+ struct dib0700_state *st = d->priv;
+ int ret;
+ u8 *b = st->buf;
+
+ mutex_lock(&d->i2c_mutex);
+
+ 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 +45,9 @@ 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];
+
+ mutex_unlock(&d->i2c_mutex);
+
return ret;
}
@@ -101,16 +109,30 @@ 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;
+ struct dib0700_state *st = d->priv;
+ u8 *buf = st->buf;
+
+ mutex_lock(&d->i2c_mutex);
+
+ 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);
+
+ mutex_unlock(&d->i2c_mutex);
+
+ return ret;
}
static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 nb_ts_packets)
{
struct dib0700_state *st = d->priv;
- u8 b[3];
+ u8 *b = st->buf;
int ret;
+ mutex_lock(&d->i2c_mutex);
+
if (st->fw_version >= 0x10201) {
b[0] = REQUEST_SET_USB_XFER_LEN;
b[1] = (nb_ts_packets >> 8) & 0xff;
@@ -118,12 +140,14 @@ static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 nb_ts_packets)
deb_info("set the USB xfer len to %i Ts packet\n", nb_ts_packets);
- ret = dib0700_ctrl_wr(d, b, sizeof(b));
+ ret = dib0700_ctrl_wr(d, b, 3);
} else {
deb_info("this firmware does not allow to change the USB xfer len\n");
ret = -EIO;
}
+ mutex_unlock(&d->i2c_mutex);
+
return ret;
}
@@ -137,11 +161,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 +246,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg,
}
}
mutex_unlock(&d->i2c_mutex);
+
return i;
}
@@ -231,8 +257,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 +291,8 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
break;
}
}
-
mutex_unlock(&d->i2c_mutex);
+
return i;
}
@@ -297,15 +324,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 +348,14 @@ 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];
+ struct dib0700_state *st = d->priv;
+ s16 ret;
+ u8 *b;
+
+ b = st->buf;
+
+ mutex_lock(&d->i2c_mutex);
+
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 +367,11 @@ 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);
+
+ mutex_unlock(&d->i2c_mutex);
+
+ return ret;
}
int dib0700_set_i2c_speed(struct dvb_usb_device *d, u16 scl_kHz)
@@ -386,11 +432,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);
@@ -411,7 +460,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;
}
}
@@ -432,7 +481,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
@@ -451,14 +500,15 @@ 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 = st->buf;
int ret;
if ((onoff != 0) && (st->fw_version >= 0x10201)) {
@@ -468,10 +518,12 @@ 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;
}
}
+ mutex_lock(&adap->dev->i2c_mutex);
+
b[0] = REQUEST_ENABLE_VIDEO;
b[1] = (onoff << 4) | 0x00; /* this bit gives a kind of command, rather than enabling something or not */
@@ -503,16 +555,21 @@ 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);
+
+ mutex_unlock(&adap->dev->i2c_mutex);
+out:
+ return ret;
}
int dib0700_change_protocol(struct rc_dev *rc, u64 rc_type)
{
struct dvb_usb_device *d = rc->priv;
struct dib0700_state *st = d->priv;
- u8 rc_setup[3] = { REQUEST_SET_RC, 0, 0 };
+ u8 *rc_setup = st->buf;
int new_proto, ret;
+
/* Set the IR mode */
if (rc_type == RC_TYPE_RC5)
new_proto = 1;
@@ -526,9 +583,16 @@ int dib0700_change_protocol(struct rc_dev *rc, u64 rc_type)
} else
return -EINVAL;
+ mutex_lock(&d->i2c_mutex);
+
+ rc_setup[0] = REQUEST_SET_RC;
rc_setup[1] = new_proto;
+ rc_setup[2] = 0;
+
+ ret = dib0700_ctrl_wr(d, rc_setup, 3);
+
+ mutex_unlock(&d->i2c_mutex);
- ret = dib0700_ctrl_wr(d, rc_setup, sizeof(rc_setup));
if (ret < 0) {
err("ir protocol setup failed");
return ret;
@@ -685,6 +749,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++)
@@ -693,8 +758,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);
@@ -708,18 +775,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,
};
diff --git a/drivers/media/dvb/dvb-usb/dib0700_devices.c b/drivers/media/dvb/dvb-usb/dib0700_devices.c
index 97af266..e9891af 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_devices.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_devices.c
@@ -487,6 +487,7 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d)
u8 toggle;
int i;
struct dib0700_state *st = d->priv;
+ u8 *buf = st->buf;
if (st->fw_version >= 0x10200) {
/* For 1.20 firmware , We need to keep the RC polling
@@ -496,7 +497,13 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d)
return 0;
}
- i = dib0700_ctrl_rd(d, rc_request, 2, key, 4);
+ mutex_lock(&d->i2c_mutex);
+
+ i = dib0700_ctrl_rd(d, rc_request, 2, buf, 4);
+ memcpy(key, buf, 4);
+
+ mutex_unlock(&d->i2c_mutex);
+
if (i <= 0) {
err("RC Query Failed");
return -1;
--
1.7.4.1
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(-)
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c b/drivers/media/dvb/dvb-usb/dib0700_core.c
index ca80520..7f6f023 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -625,7 +625,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;
@@ -640,7 +639,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.1
Hi Florian,
On Sun, 3 Apr 2011, Florian Mickler wrote:
> Hi,
>
> since I got no reaction[1] on the vp702x driver, I proceed with the
> dib0700.
>
> There are multiple drivers in drivers/media/dvb/dvb-usb/ which use
> usb_control_msg to perform dma to stack-allocated buffers. This is a bad idea
> because of cache-coherency issues and on some platforms the stack is mapped
> virtually and also lib/dma-debug.c warn's about it at runtime.
>
> Patches to ec168, ce6230, au6610 and lmedm04 were already tested and reviewed
> and submitted for inclusion [2]. Patches to a800, vp7045, friio, dw2102, m920x
> and opera1 are still waiting for for review and testing [3].
>
> This patch to dib0700 is a fix for a warning seen and reported by Zdenek
> Kabalec in Bug #15977 [4].
>
> Florian Mickler (2):
> [media] dib0700: get rid of on-stack dma buffers
For this one we implemented an alternative. See here:
http://git.linuxtv.org/pb/media_tree.git?a=commit;h=16b54de2d8b46e48c5c8bdf9b350eac04e8f6b46
which I pushed, but obviously forgot to send the pull-request.
This is done now.
For the second patch I will incorperate it as soon as I find the time.
best regards,
--
Patrick
On Mon, 4 Apr 2011 09:42:04 +0200 (CEST)
Patrick Boettcher <[email protected]> wrote:
> For this one we implemented an alternative. See here:
>
> http://git.linuxtv.org/pb/media_tree.git?a=commit;h=16b54de2d8b46e48c5c8bdf9b350eac04e8f6b46
>
> which I pushed, but obviously forgot to send the pull-request.
>
> This is done now.
Thanks for the information. I see there is a CC: Florian Mickler in
there, but I didn't get any email... maybe something wrong on your
side?
It helps a lot with closing bug reports in the bugzilla, if people add a
reference to the bugreport - if there is one . I.e. a line:
"This should address bug XXXXX. "
Or even a link (preferred).
Regards,
Flo
>
> For the second patch I will incorperate it as soon as I find the time.
no probs.
>
> best regards,
> --
>
> Patrick
On Mon, 4 Apr 2011 09:42:04 +0200 (CEST)
Patrick Boettcher <[email protected]> wrote:
> Hi Florian,
>
> For this one we implemented an alternative. See here:
>
> http://git.linuxtv.org/pb/media_tree.git?a=commit;h=16b54de2d8b46e48c5c8bdf9b350eac04e8f6b46
>
> which I pushed, but obviously forgot to send the pull-request.
>
OK, I just looked over it. What about dib0700_rc_query_old_firmware,
that would also need to be fixed.
I don't have an overview over the media framework, so I wonder what
arbitrates concurrent access to the buffer? Functions which are only
called from the initialization and probe routines are probably properly
arbitrated by the driver core. But I would expect (perhaps
that is me being naive) stuff like dib0700_change_protocol to need some
sort of mutex ?
It seems to be called from some /sys/class/*/ file while for example
legacy_dvb_usb_read_remote_control, which calls dib0700_rc_query_old_firmware, is
described as being a polling function, i.e. periodically executed...
or the streaming_ctrl function, that looks like it is executed at
times...
Thanks,
Flo
p.s.:
can you add yourself to the
MAINTAINERS file please?
Em 04-04-2011 04:42, Patrick Boettcher escreveu:
> Hi Florian,
>
> On Sun, 3 Apr 2011, Florian Mickler wrote:
>
>> Hi,
>>
>> since I got no reaction[1] on the vp702x driver, I proceed with the
>> dib0700.
>>
>> There are multiple drivers in drivers/media/dvb/dvb-usb/ which use
>> usb_control_msg to perform dma to stack-allocated buffers. This is a bad idea
>> because of cache-coherency issues and on some platforms the stack is mapped
>> virtually and also lib/dma-debug.c warn's about it at runtime.
>>
>> Patches to ec168, ce6230, au6610 and lmedm04 were already tested and reviewed
>> and submitted for inclusion [2]. Patches to a800, vp7045, friio, dw2102, m920x
>> and opera1 are still waiting for for review and testing [3].
>>
>> This patch to dib0700 is a fix for a warning seen and reported by Zdenek
>> Kabalec in Bug #15977 [4].
>>
>> Florian Mickler (2):
>> [media] dib0700: get rid of on-stack dma buffers
>
> For this one we implemented an alternative. See here:
>
> http://git.linuxtv.org/pb/media_tree.git?a=commit;h=16b54de2d8b46e48c5c8bdf9b350eac04e8f6b46
>
> which I pushed, but obviously forgot to send the pull-request.
>
> This is done now.
And I obviously forgot to pick ;) Ok, I'm applying Oliver Grenie's version and
marking Florian's version as superseded at patchwork.
> For the second patch I will incorperate it as soon as I find the time.
As it is a trivial fix, I'll be picking it directly.
>
> best regards,
> --
>
> Patrick
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 29 Apr 2011 18:32:34 -0300
Mauro Carvalho Chehab <[email protected]> wrote:
> As it is a trivial fix, I'll be picking it directly.
Zdenek reported in the bug that it doesn't fix all instances of the
warning.
Regards,
Flo
Patrick,
Em 30-04-2011 06:46, Florian Mickler escreveu:
> On Fri, 29 Apr 2011 18:32:34 -0300
> Mauro Carvalho Chehab <[email protected]> wrote:
>
>> As it is a trivial fix, I'll be picking it directly.
>
> Zdenek reported in the bug that it doesn't fix all instances of the
> warning.
Could you please take a look on it?
I'll apply the patch anyway, as at least it will reduce the stack size
and partially fix the issue, but, as Florian and Zdenek pointed, there
are still some cases were stack pointers are used to pass data to URB's,
and this is forbidden (and don't even work on some architectures).
Mauro.