Re: [media] dib0700: get rid of on-stack dma buffers
On Fri, 29 Apr 2011 18:32:34 -0300 Mauro Carvalho Chehab mche...@infradead.org 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media] dib0700: get rid of on-stack dma buffers
Patrick, Em 30-04-2011 06:46, Florian Mickler escreveu: On Fri, 29 Apr 2011 18:32:34 -0300 Mauro Carvalho Chehab mche...@infradead.org 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. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media] dib0700: get rid of on-stack dma buffers
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media] dib0700: get rid of on-stack dma buffers
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media] dib0700: get rid of on-stack dma buffers
On Mon, 4 Apr 2011 09:42:04 +0200 (CEST) Patrick Boettcher pboettc...@kernellabs.com 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 X. 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media] dib0700: get rid of on-stack dma buffers
On Mon, 4 Apr 2011 09:42:04 +0200 (CEST) Patrick Boettcher pboettc...@kernellabs.com 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? -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[media] dib0700: get rid of on-stack dma buffers
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] [media] dib0700: get rid of on-stack dma buffers
usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.
In lib/dma_debug.c the function check_for_stack even warns about it:
WARNING: at lib/dma-debug.c:866 check_for_stack
Note: This change is tested to compile only, as I don't have the hardware.
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Signed-off-by: Florian Mickler flor...@mickler.org
---
[v2: use preallocated buffer; fix sizeof in one case]
[v3: use seperate kmalloc mapping for the preallocation,
dont ignore errors in probe codepaths ]
[v4: minor style nit: functions opening brace goes onto it's own line ]
[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 */
Re: [PATCH 1/2 v3] [media] dib0700: get rid of on-stack dma buffers
On Sun, 6 Mar 2011 18:57:13 +0100
Florian Mickler flor...@mickler.org wrote:
On Sun, 6 Mar 2011 18:47:56 +0100
Florian Mickler flor...@mickler.org wrote:
+static void dib0700_disconnect(struct usb_interface *intf) {
That { should go on its own line... sorry ;-)
If that patch is acceptable, I can resend with that fixed.
Regards,
Flo
Hi Mauro,
what about this patch? I have similar patches for the same problem in
the other dvb-usb drivers in need of beeing fixed. Are you
maintaining these drivers or should I find someone else to pick these
up?
Regards,
Flo
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/16] [media] dib0700: get rid of on-stack dma buffers
usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.
In lib/dma_debug.c the function check_for_stack even warns about it:
WARNING: at lib/dma-debug.c:866 check_for_stack
Note: This change is tested to compile only, as I don't have the hardware.
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Signed-off-by: Florian Mickler flor...@mickler.org
[v2: use preallocated buffer; fix sizeof in one case]
[v3: use seperate kmalloc mapping for the preallocation,
dont ignore errors in probe codepaths ]
[v4: minor style nit: functions opening brace goes onto it's own line ]
---
drivers/media/dvb/dvb-usb/dib0700.h |5 +-
drivers/media/dvb/dvb-usb/dib0700_core.c | 120 --
2 files changed, 99 insertions(+), 26 deletions(-)
diff --git a/drivers/media/dvb/dvb-usb/dib0700.h
b/drivers/media/dvb/dvb-usb/dib0700.h
index 3537d65..99a1485 100644
--- a/drivers/media/dvb/dvb-usb/dib0700.h
+++ b/drivers/media/dvb/dvb-usb/dib0700.h
@@ -45,8 +45,9 @@ struct dib0700_state {
u8 is_dib7000pc;
u8 fw_use_new_i2c_api;
u8 disable_streaming_master_mode;
-u32 fw_version;
-u32 nb_packet_buffer_size;
+ u32 fw_version;
+ u32 nb_packet_buffer_size;
+ u8 *buf;
};
extern int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..1c19b73 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
u32 *romversion, u32 *ramversion, u32 *fwtype)
{
- u8 b[16];
- int ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
+ int ret;
+ u8 *b;
+
+ b = kmalloc(16, GFP_KERNEL);
+ if (!b)
+ return -ENOMEM;
+
+ ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
REQUEST_GET_VERSION,
USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
- b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+ b, 16, USB_CTRL_GET_TIMEOUT);
if (hwversion != NULL)
*hwversion = (b[0] 24) | (b[1] 16) | (b[2] 8) |
b[3];
if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32
*hwversion,
*ramversion = (b[8] 24) | (b[9] 16) | (b[10] 8) |
b[11];
if (fwtype != NULL)
*fwtype = (b[12] 24) | (b[13] 16) | (b[14] 8) |
b[15];
+
+ kfree(b);
return ret;
}
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8
txlen, u8 *rx, u8 rxlen
int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8
gpio_dir, u8 gpio_val)
{
- u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir 0x01) 7) |
((gpio_val 0x01) 6) };
- return dib0700_ctrl_wr(d, buf, sizeof(buf));
+ s16 ret;
+ u8 *buf = kmalloc(3, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ buf[0] = REQUEST_SET_GPIO;
+ buf[1] = gpio;
+ buf[2] = ((gpio_dir 0x01) 7) | ((gpio_val 0x01) 6);
+
+ ret = dib0700_ctrl_wr(d, buf, 3);
+
+ kfree(buf);
+ return ret;
}
static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16
nb_ts_packets)
@@ -137,11 +156,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
properly support i2c read calls not preceded by a write */
struct dvb_usb_device *d = i2c_get_adapdata(adap);
+ struct dib0700_state *st = d-priv;
uint8_t bus_mode = 1; /* 0=eeprom bus, 1=frontend bus */
uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
uint8_t en_start = 0;
uint8_t en_stop = 0;
- uint8_t buf[255]; /* TBV: malloc ? */
+ uint8_t *buf = st-buf;
int result, i;
/* Ensure nobody else hits the i2c bus while we're sending our
@@ -221,6 +241,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
}
}
mutex_unlock(d-i2c_mutex);
+
return i;
}
@@ -231,8 +252,9 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
struct i2c_msg *msg, int num)
{
struct dvb_usb_device *d = i2c_get_adapdata(adap);
+ struct dib0700_state *st = d-priv;
int i,len;
- u8 buf[255];
+ u8 *buf = st-buf;
if (mutex_lock_interruptible(d-i2c_mutex) 0)
return -EAGAIN;
@@ -264,8 +286,8 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
Re: [PATCH 1/2 v3] [media] dib0700: get rid of on-stack dma buffers
Em 15-03-2011 05:36, Florian Mickler escreveu:
On Sun, 6 Mar 2011 18:57:13 +0100
Florian Mickler flor...@mickler.org wrote:
On Sun, 6 Mar 2011 18:47:56 +0100
Florian Mickler flor...@mickler.org wrote:
+static void dib0700_disconnect(struct usb_interface *intf) {
That { should go on its own line... sorry ;-)
If that patch is acceptable, I can resend with that fixed.
Regards,
Flo
Hi Mauro,
what about this patch? I have similar patches for the same problem in
the other dvb-usb drivers in need of beeing fixed. Are you
maintaining these drivers or should I find someone else to pick these
up?
I generally wait for the driver maintainer to review and test on their
hardware before applying on my tree.
I'll send you a few comments over the first patch on a separate email,
that also applies to the other patches.
Thanks,
Mauro.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/16] [media] dib0700: get rid of on-stack dma buffers
Em 15-03-2011 05:43, Florian Mickler escreveu:
usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.
In lib/dma_debug.c the function check_for_stack even warns about it:
WARNING: at lib/dma-debug.c:866 check_for_stack
Note: This change is tested to compile only, as I don't have the hardware.
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Signed-off-by: Florian Mickler flor...@mickler.org
[v2: use preallocated buffer; fix sizeof in one case]
[v3: use seperate kmalloc mapping for the preallocation,
dont ignore errors in probe codepaths ]
[v4: minor style nit: functions opening brace goes onto it's own line ]
I'm c/c Patrick, as he is the driver maintainer for those Dibcom drivers.
---
drivers/media/dvb/dvb-usb/dib0700.h |5 +-
drivers/media/dvb/dvb-usb/dib0700_core.c | 120
--
2 files changed, 99 insertions(+), 26 deletions(-)
diff --git a/drivers/media/dvb/dvb-usb/dib0700.h
b/drivers/media/dvb/dvb-usb/dib0700.h
index 3537d65..99a1485 100644
--- a/drivers/media/dvb/dvb-usb/dib0700.h
+++ b/drivers/media/dvb/dvb-usb/dib0700.h
@@ -45,8 +45,9 @@ struct dib0700_state {
u8 is_dib7000pc;
u8 fw_use_new_i2c_api;
u8 disable_streaming_master_mode;
-u32 fw_version;
-u32 nb_packet_buffer_size;
+ u32 fw_version;
+ u32 nb_packet_buffer_size;
+ u8 *buf;
};
extern int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..1c19b73 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
u32 *romversion, u32 *ramversion, u32 *fwtype)
{
- u8 b[16];
- int ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
+ int ret;
+ u8 *b;
+
+ b = kmalloc(16, GFP_KERNEL);
+ if (!b)
+ return -ENOMEM;
+
+ ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
REQUEST_GET_VERSION,
USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
- b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+ b, 16, USB_CTRL_GET_TIMEOUT);
if (hwversion != NULL)
*hwversion = (b[0] 24) | (b[1] 16) | (b[2] 8) |
b[3];
if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32
*hwversion,
*ramversion = (b[8] 24) | (b[9] 16) | (b[10] 8) |
b[11];
if (fwtype != NULL)
*fwtype = (b[12] 24) | (b[13] 16) | (b[14] 8) |
b[15];
+
+ kfree(b);
return ret;
}
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8
txlen, u8 *rx, u8 rxlen
int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8
gpio_dir, u8 gpio_val)
{
- u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir 0x01) 7) |
((gpio_val 0x01) 6) };
- return dib0700_ctrl_wr(d, buf, sizeof(buf));
+ s16 ret;
+ u8 *buf = kmalloc(3, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ buf[0] = REQUEST_SET_GPIO;
+ buf[1] = gpio;
+ buf[2] = ((gpio_dir 0x01) 7) | ((gpio_val 0x01) 6);
+
+ ret = dib0700_ctrl_wr(d, buf, 3);
+
+ kfree(buf);
+ return ret;
}
static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16
nb_ts_packets)
@@ -137,11 +156,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter
*adap, struct i2c_msg *msg,
properly support i2c read calls not preceded by a write */
struct dvb_usb_device *d = i2c_get_adapdata(adap);
+ struct dib0700_state *st = d-priv;
uint8_t bus_mode = 1; /* 0=eeprom bus, 1=frontend bus */
uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
uint8_t en_start = 0;
uint8_t en_stop = 0;
- uint8_t buf[255]; /* TBV: malloc ? */
+ uint8_t *buf = st-buf;
int result, i;
/* Ensure nobody else hits the i2c bus while we're sending our
@@ -221,6 +241,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
}
}
mutex_unlock(d-i2c_mutex);
+
return i;
}
@@ -231,8 +252,9 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter
*adap,
struct i2c_msg *msg, int num)
{
struct dvb_usb_device *d = i2c_get_adapdata(adap);
+ struct dib0700_state *st = d-priv;
int i,len;
- u8 buf[255];
+ u8 *buf = st-buf;
if
Re: [PATCH 01/16] [media] dib0700: get rid of on-stack dma buffers
On Tue, 15 Mar 2011 09:02:00 -0300 Mauro Carvalho Chehab mche...@infradead.org wrote: You're allocating a buffer for URB control messages. IMO, this is a good idea, as this way, allocating/freeing on each urb call is avoided. However, on most places, you're not using it. The better would be to just use this buffer on all the above places. You should just take care of protecting such buffer with a mutex, to avoid concurrency. Such kind of protection is generally ok, as dvb applications generally serialize the calls anyway. I didn't do so already, because I had/have no overview over the big picture operation of the dvb framework and thus feared to introduce deadlocks or massive serializations where concurrency is needed. But if you suggest it, I guess it should be benign. I'm wondering about the purpose of the usb_mutex and the i2c_mutex ... Should I introduce new driver specific mutexes to protect the buffer or is it possible to reuse one of the 2? vp702x_usb_inout_op takes the usb_mutex, but vp702x_usb_out_op and vp702x_usb_in_op get called without that mutex hold. That makes me wonder what that mutex purpose is in that driver? Other drivers like the az6027 introduce a driver specific mutex and also use the usb_mutex. That make conceptually (to my inexperienced mind) more sense. (each layer does it's own locking) but the idea of operation is not yet clear in my mind... Can you perhaps shed some light on what the purpose of those locks is and if it is sufficient to use the usb_mutex to serialize all usb_control_msg calls (which would probably protect the buffer sufficiently but may be too much if the dvb-usb framework uses that mutex for different purposes). In the meantime I will respin this series using preallocated buffers and will hopefully work out stuff that is unclear to me yet ... Regards, Flo -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] dib0700: get rid of on-stack dma buffers
This should fix warnings seen by some:
WARNING: at lib/dma-debug.c:866 check_for_stack
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Signed-off-by: Florian Mickler flor...@mickler.org
CC: Mauro Carvalho Chehab mche...@infradead.org
CC: linux-media@vger.kernel.org
CC: linux-ker...@vger.kernel.org
CC: Greg Kroah-Hartman g...@kroah.com
CC: Rafael J. Wysocki r...@sisk.pl
CC: Maciej Rutecki maciej.rute...@gmail.com
---
Please take a look at it, as I do not do that much kernel hacking
and don't wanna brake anybodys computer... :)
From my point of view this should _not_ go to stable even though it would
be applicable. But if someone feels strongly about that and can
take responsibility for that change...
drivers/media/dvb/dvb-usb/dib0700_core.c | 121 +++---
1 files changed, 94 insertions(+), 27 deletions(-)
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..1a12182 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
u32 *romversion, u32 *ramversion, u32 *fwtype)
{
- u8 b[16];
- int ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
+ int ret;
+ u8 *b;
+
+ b = kmalloc(16, GFP_KERNEL);
+ if (!b)
+ return -ENOMEM;
+
+ ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
REQUEST_GET_VERSION,
USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
- b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+ b, 16, USB_CTRL_GET_TIMEOUT);
if (hwversion != NULL)
*hwversion = (b[0] 24) | (b[1] 16) | (b[2] 8) |
b[3];
if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32
*hwversion,
*ramversion = (b[8] 24) | (b[9] 16) | (b[10] 8) |
b[11];
if (fwtype != NULL)
*fwtype = (b[12] 24) | (b[13] 16) | (b[14] 8) |
b[15];
+
+ kfree(b);
return ret;
}
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8
txlen, u8 *rx, u8 rxlen
int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8
gpio_dir, u8 gpio_val)
{
- u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir 0x01) 7) |
((gpio_val 0x01) 6) };
- return dib0700_ctrl_wr(d, buf, sizeof(buf));
+ s16 ret;
+ u8 *buf = kmalloc(3, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ buf[0] = REQUEST_SET_GPIO;
+ buf[1] = gpio;
+ buf[2] = ((gpio_dir 0x01) 7) | ((gpio_val 0x01) 6);
+
+ ret = dib0700_ctrl_wr(d, buf, sizeof(buf));
+
+ kfree(buf);
+ return ret;
}
static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16
nb_ts_packets)
@@ -141,13 +160,20 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
uint8_t en_start = 0;
uint8_t en_stop = 0;
- uint8_t buf[255]; /* TBV: malloc ? */
+ uint8_t *buf;
int result, i;
+ buf = kmalloc(255, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
/* Ensure nobody else hits the i2c bus while we're sending our
sequence of messages, (such as the remote control thread) */
- if (mutex_lock_interruptible(d-i2c_mutex) 0)
- return -EAGAIN;
+ if (mutex_lock_interruptible(d-i2c_mutex) 0) {
+ result = -EAGAIN;
+ goto out;
+ }
+
for (i = 0; i num; i++) {
if (i == 0) {
@@ -220,8 +246,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
}
}
}
+ result = i;
mutex_unlock(d-i2c_mutex);
- return i;
+
+out:
+ kfree(buf);
+ return result;
}
/*
@@ -232,10 +262,17 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter
*adap,
{
struct dvb_usb_device *d = i2c_get_adapdata(adap);
int i,len;
- u8 buf[255];
+ s16 ret;
+ u8 *buf;
- if (mutex_lock_interruptible(d-i2c_mutex) 0)
- return -EAGAIN;
+ buf = kmalloc(255, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ if (mutex_lock_interruptible(d-i2c_mutex) 0) {
+ ret = -EAGAIN;
+ goto out;
+ }
for (i = 0; i num; i++) {
/* fill in the address */
@@ -264,9 +301,11 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter
*adap,
break;
}
}
-
+ ret = i;
Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers
Am Sonntag, 6. März 2011, 12:16:52 schrieb Florian Mickler: This should fix warnings seen by some: WARNING: at lib/dma-debug.c:866 check_for_stack Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977. Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com Signed-off-by: Florian Mickler flor...@mickler.org CC: Mauro Carvalho Chehab mche...@infradead.org CC: linux-media@vger.kernel.org CC: linux-ker...@vger.kernel.org CC: Greg Kroah-Hartman g...@kroah.com CC: Rafael J. Wysocki r...@sisk.pl CC: Maciej Rutecki maciej.rute...@gmail.com --- Please take a look at it, as I do not do that much kernel hacking and don't wanna brake anybodys computer... :) From my point of view this should _not_ go to stable even though it would be applicable. But if someone feels strongly about that and can take responsibility for that change... The patch looks good and is needed in stable. It could be improved by using a buffer allocated once in the places you hold a mutex anyway. Regards Oliver -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers
On 06/03/2011 11:16, Florian Mickler wrote:
This should fix warnings seen by some:
WARNING: at lib/dma-debug.c:866 check_for_stack
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Signed-off-by: Florian Mickler flor...@mickler.org
CC: Mauro Carvalho Chehab mche...@infradead.org
CC: linux-media@vger.kernel.org
CC: linux-ker...@vger.kernel.org
CC: Greg Kroah-Hartman g...@kroah.com
CC: Rafael J. Wysocki r...@sisk.pl
CC: Maciej Rutecki maciej.rute...@gmail.com
---
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8
txlen, u8 *rx, u8 rxlen
int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8
gpio_dir, u8 gpio_val)
{
- u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir 0x01) 7) |
((gpio_val 0x01) 6) };
- return dib0700_ctrl_wr(d, buf, sizeof(buf));
+ s16 ret;
+ u8 *buf = kmalloc(3, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ buf[0] = REQUEST_SET_GPIO;
+ buf[1] = gpio;
+ buf[2] = ((gpio_dir 0x01) 7) | ((gpio_val 0x01) 6);
+
+ ret = dib0700_ctrl_wr(d, buf, sizeof(buf));
Shouldn't this sizeof be changed as well?
Thanks,
Jack
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers
On Sun, 06 Mar 2011 13:49:56 +
Jack Stone jwjst...@fastmail.fm wrote:
On 06/03/2011 11:16, Florian Mickler wrote:
This should fix warnings seen by some:
WARNING: at lib/dma-debug.c:866 check_for_stack
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Signed-off-by: Florian Mickler flor...@mickler.org
CC: Mauro Carvalho Chehab mche...@infradead.org
CC: linux-media@vger.kernel.org
CC: linux-ker...@vger.kernel.org
CC: Greg Kroah-Hartman g...@kroah.com
CC: Rafael J. Wysocki r...@sisk.pl
CC: Maciej Rutecki maciej.rute...@gmail.com
---
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx,
u8 txlen, u8 *rx, u8 rxlen
int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8
gpio_dir, u8 gpio_val)
{
- u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir 0x01) 7) |
((gpio_val 0x01) 6) };
- return dib0700_ctrl_wr(d, buf, sizeof(buf));
+ s16 ret;
+ u8 *buf = kmalloc(3, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ buf[0] = REQUEST_SET_GPIO;
+ buf[1] = gpio;
+ buf[2] = ((gpio_dir 0x01) 7) | ((gpio_val 0x01) 6);
+
+ ret = dib0700_ctrl_wr(d, buf, sizeof(buf));
Shouldn't this sizeof be changed as well?
Thanks,
Jack
argh... indeed.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers
On Sun, 6 Mar 2011 13:06:09 +0100 Oliver Neukum oli...@neukum.org wrote: Am Sonntag, 6. März 2011, 12:16:52 schrieb Florian Mickler: This should fix warnings seen by some: WARNING: at lib/dma-debug.c:866 check_for_stack Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977. Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com Signed-off-by: Florian Mickler flor...@mickler.org CC: Mauro Carvalho Chehab mche...@infradead.org CC: linux-media@vger.kernel.org CC: linux-ker...@vger.kernel.org CC: Greg Kroah-Hartman g...@kroah.com CC: Rafael J. Wysocki r...@sisk.pl CC: Maciej Rutecki maciej.rute...@gmail.com --- Please take a look at it, as I do not do that much kernel hacking and don't wanna brake anybodys computer... :) From my point of view this should _not_ go to stable even though it would be applicable. But if someone feels strongly about that and can take responsibility for that change... The patch looks good and is needed in stable. It could be improved by using a buffer allocated once in the places you hold a mutex anyway. Regards Oliver Ok, I now put a buffer member in the priv dib0700_state which gets allocated on the heap. My patch introduces a new error condition in the dib0700_identify_state callback which gets not checked for in dvb_usb_find_device... Should we worry? Same for dib0700_get_version in the probe callback... But there, there was already the possibility of usb_control_msg returning an error... Regards, Flo -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3 v2] [media] dib0700: get rid of on-stack dma buffers
This should fix warnings seen by some:
WARNING: at lib/dma-debug.c:866 check_for_stack
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Signed-off-by: Florian Mickler flor...@mickler.org
CC: Mauro Carvalho Chehab mche...@infradead.org
CC: linux-media@vger.kernel.org
CC: linux-ker...@vger.kernel.org
CC: Greg Kroah-Hartman g...@kroah.com
CC: Rafael J. Wysocki r...@sisk.pl
CC: Maciej Rutecki maciej.rute...@gmail.com
CC: Oliver Neukum oli...@neukum.org
CC: Jack Stone jwjst...@fastmail.fm
[v2: use preallocated buffer where the mutex is held; fix sizeof in one case]
---
drivers/media/dvb/dvb-usb/dib0700.h |5 +-
drivers/media/dvb/dvb-usb/dib0700_core.c | 92 +++---
2 files changed, 74 insertions(+), 23 deletions(-)
diff --git a/drivers/media/dvb/dvb-usb/dib0700.h
b/drivers/media/dvb/dvb-usb/dib0700.h
index 3537d65..1401e7d 100644
--- a/drivers/media/dvb/dvb-usb/dib0700.h
+++ b/drivers/media/dvb/dvb-usb/dib0700.h
@@ -45,8 +45,9 @@ struct dib0700_state {
u8 is_dib7000pc;
u8 fw_use_new_i2c_api;
u8 disable_streaming_master_mode;
-u32 fw_version;
-u32 nb_packet_buffer_size;
+ u32 fw_version;
+ u32 nb_packet_buffer_size;
+ u8 buf[255];
};
extern int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..028ed87 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
u32 *romversion, u32 *ramversion, u32 *fwtype)
{
- u8 b[16];
- int ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
+ int ret;
+ u8 *b;
+
+ b = kmalloc(16, GFP_KERNEL);
+ if (!b)
+ return -ENOMEM;
+
+ ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
REQUEST_GET_VERSION,
USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
- b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+ b, 16, USB_CTRL_GET_TIMEOUT);
if (hwversion != NULL)
*hwversion = (b[0] 24) | (b[1] 16) | (b[2] 8) |
b[3];
if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32
*hwversion,
*ramversion = (b[8] 24) | (b[9] 16) | (b[10] 8) |
b[11];
if (fwtype != NULL)
*fwtype = (b[12] 24) | (b[13] 16) | (b[14] 8) |
b[15];
+
+ kfree(b);
return ret;
}
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8
txlen, u8 *rx, u8 rxlen
int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8
gpio_dir, u8 gpio_val)
{
- u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir 0x01) 7) |
((gpio_val 0x01) 6) };
- return dib0700_ctrl_wr(d, buf, sizeof(buf));
+ s16 ret;
+ u8 *buf = kmalloc(3, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ buf[0] = REQUEST_SET_GPIO;
+ buf[1] = gpio;
+ buf[2] = ((gpio_dir 0x01) 7) | ((gpio_val 0x01) 6);
+
+ ret = dib0700_ctrl_wr(d, buf, 3);
+
+ kfree(buf);
+ return ret;
}
static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16
nb_ts_packets)
@@ -137,11 +156,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
properly support i2c read calls not preceded by a write */
struct dvb_usb_device *d = i2c_get_adapdata(adap);
+ struct dib0700_state *st = d-priv;
uint8_t bus_mode = 1; /* 0=eeprom bus, 1=frontend bus */
uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
uint8_t en_start = 0;
uint8_t en_stop = 0;
- uint8_t buf[255]; /* TBV: malloc ? */
+ uint8_t *buf = st-buf;
int result, i;
/* Ensure nobody else hits the i2c bus while we're sending our
@@ -221,6 +241,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
}
}
mutex_unlock(d-i2c_mutex);
+
return i;
}
@@ -231,8 +252,9 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
struct i2c_msg *msg, int num)
{
struct dvb_usb_device *d = i2c_get_adapdata(adap);
+ struct dib0700_state *st = d-priv;
int i,len;
- u8 buf[255];
+ u8 *buf = st-buf;
if (mutex_lock_interruptible(d-i2c_mutex) 0)
return -EAGAIN;
@@ -264,8 +286,8 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
break;
}
}
-
mutex_unlock(d-i2c_mutex);
+
return i;
Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers
On Sun, 6 Mar 2011 16:06:38 +0100 Oliver Neukum oli...@neukum.org wrote: Am Sonntag, 6. März 2011, 15:38:05 schrieb Florian Mickler: On Sun, 6 Mar 2011 13:06:09 +0100 Oliver Neukum oli...@neukum.org wrote: Am Sonntag, 6. März 2011, 12:16:52 schrieb Florian Mickler: Please take a look at it, as I do not do that much kernel hacking and don't wanna brake anybodys computer... :) From my point of view this should _not_ go to stable even though it would be applicable. But if someone feels strongly about that and can take responsibility for that change... The patch looks good and is needed in stable. It could be improved by using a buffer allocated once in the places you hold a mutex anyway. Regards Oliver Ok, I now put a buffer member in the priv dib0700_state which gets allocated on the heap. This however is wrong. Just like DMA on the stack this breaks coherency rules. You may do DMA to the heap in the sense that you can do DMA to buffers allocated on the heap, but you cannot do DMA to a part of another structure allocated on the heap. You need a separate kmalloc for each buffer. You can reuse the buffer with proper locking, but you must allocate it seperately once. Regards Oliver Hm.. allocating the buffer in the probe routine and deallocating it in the usb_driver disconnect callback should work? How come that it must be a seperate kmalloc buffer? Is it some aligning that kmalloc garantees? Regards, Flo -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] dib0700: get rid of on-stack dma buffers
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2 v3] [media] dib0700: get rid of on-stack dma buffers
This should fix warnings seen by some:
WARNING: at lib/dma-debug.c:866 check_for_stack
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=15977.
Reported-by: Zdenek Kabelac zdenek.kabe...@gmail.com
Signed-off-by: Florian Mickler flor...@mickler.org
CC: Mauro Carvalho Chehab mche...@infradead.org
CC: linux-media@vger.kernel.org
CC: linux-ker...@vger.kernel.org
CC: Greg Kroah-Hartman g...@kroah.com
CC: Rafael J. Wysocki r...@sisk.pl
CC: Maciej Rutecki maciej.rute...@gmail.com
CC: Oliver Neukum oli...@neukum.org
CC: Jack Stone jwjst...@fastmail.fm
---
[v2: use preallocated buffer; fix sizeof in one case]
[v3: use seperate kmalloc mapping for the preallocation,
dont ignore errors in probe codepaths ]
drivers/media/dvb/dvb-usb/dib0700.h |5 +-
drivers/media/dvb/dvb-usb/dib0700_core.c | 119 --
2 files changed, 98 insertions(+), 26 deletions(-)
diff --git a/drivers/media/dvb/dvb-usb/dib0700.h
b/drivers/media/dvb/dvb-usb/dib0700.h
index 3537d65..99a1485 100644
--- a/drivers/media/dvb/dvb-usb/dib0700.h
+++ b/drivers/media/dvb/dvb-usb/dib0700.h
@@ -45,8 +45,9 @@ struct dib0700_state {
u8 is_dib7000pc;
u8 fw_use_new_i2c_api;
u8 disable_streaming_master_mode;
-u32 fw_version;
-u32 nb_packet_buffer_size;
+ u32 fw_version;
+ u32 nb_packet_buffer_size;
+ u8 *buf;
};
extern int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 98ffb40..0b04cb6 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -27,11 +27,17 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
int dib0700_get_version(struct dvb_usb_device *d, u32 *hwversion,
u32 *romversion, u32 *ramversion, u32 *fwtype)
{
- u8 b[16];
- int ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
+ int ret;
+ u8 *b;
+
+ b = kmalloc(16, GFP_KERNEL);
+ if (!b)
+ return -ENOMEM;
+
+ ret = usb_control_msg(d-udev, usb_rcvctrlpipe(d-udev, 0),
REQUEST_GET_VERSION,
USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
- b, sizeof(b), USB_CTRL_GET_TIMEOUT);
+ b, 16, USB_CTRL_GET_TIMEOUT);
if (hwversion != NULL)
*hwversion = (b[0] 24) | (b[1] 16) | (b[2] 8) |
b[3];
if (romversion != NULL)
@@ -40,6 +46,8 @@ int dib0700_get_version(struct dvb_usb_device *d, u32
*hwversion,
*ramversion = (b[8] 24) | (b[9] 16) | (b[10] 8) |
b[11];
if (fwtype != NULL)
*fwtype = (b[12] 24) | (b[13] 16) | (b[14] 8) |
b[15];
+
+ kfree(b);
return ret;
}
@@ -101,8 +109,19 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8
txlen, u8 *rx, u8 rxlen
int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8
gpio_dir, u8 gpio_val)
{
- u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir 0x01) 7) |
((gpio_val 0x01) 6) };
- return dib0700_ctrl_wr(d, buf, sizeof(buf));
+ s16 ret;
+ u8 *buf = kmalloc(3, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ buf[0] = REQUEST_SET_GPIO;
+ buf[1] = gpio;
+ buf[2] = ((gpio_dir 0x01) 7) | ((gpio_val 0x01) 6);
+
+ ret = dib0700_ctrl_wr(d, buf, 3);
+
+ kfree(buf);
+ return ret;
}
static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16
nb_ts_packets)
@@ -137,11 +156,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
properly support i2c read calls not preceded by a write */
struct dvb_usb_device *d = i2c_get_adapdata(adap);
+ struct dib0700_state *st = d-priv;
uint8_t bus_mode = 1; /* 0=eeprom bus, 1=frontend bus */
uint8_t gen_mode = 0; /* 0=master i2c, 1=gpio i2c */
uint8_t en_start = 0;
uint8_t en_stop = 0;
- uint8_t buf[255]; /* TBV: malloc ? */
+ uint8_t *buf = st-buf;
int result, i;
/* Ensure nobody else hits the i2c bus while we're sending our
@@ -221,6 +241,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap,
struct i2c_msg *msg,
}
}
mutex_unlock(d-i2c_mutex);
+
return i;
}
@@ -231,8 +252,9 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
struct i2c_msg *msg, int num)
{
struct dvb_usb_device *d = i2c_get_adapdata(adap);
+ struct dib0700_state *st = d-priv;
int i,len;
- u8 buf[255];
+ u8 *buf = st-buf;
if (mutex_lock_interruptible(d-i2c_mutex) 0)
return -EAGAIN;
@@ -264,8 +286,8 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
break;
Re: [PATCH 1/2 v3] [media] dib0700: get rid of on-stack dma buffers
On Sun, 6 Mar 2011 18:47:56 +0100
Florian Mickler flor...@mickler.org wrote:
+static void dib0700_disconnect(struct usb_interface *intf) {
That { should go on its own line... sorry ;-)
If that patch is acceptable, I can resend with that fixed.
Regards,
Flo
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
