RE: [PATCH] media: staging: ipu3: Fix stale list entries on parameter queue failure
Hi, Tomasz, Thanks for your patch. BRs, Bingbu Cao > -Original Message- > From: Tomasz Figa > Sent: Sunday, April 12, 2020 1:37 AM > To: linux-me...@vger.kernel.org > Cc: Sakari Ailus ; Mauro Carvalho Chehab > ; Greg Kroah-Hartman ; Qiu, > Tian Shu ; Cao, Bingbu ; > de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; Laurent Pinchart > ; jacopo mondi ; > Tomasz Figa > Subject: [PATCH] media: staging: ipu3: Fix stale list entries on parameter > queue failure > > When queuing parameters fails, current code bails out without deleting the > corresponding vb2 buffer from the driver buffer list, but the buffer is > returned to vb2. This leads to stale list entries and a crash when the > driver stops streaming: > > [ 224.935561] ipu3-imgu :00:05.0: set parameters failed. > [ 224.998932] ipu3-imgu :00:05.0: set parameters failed. > [ 225.064430] ipu3-imgu :00:05.0: set parameters failed. > [ 225.128534] ipu3-imgu :00:05.0: set parameters failed. > [ 225.194945] ipu3-imgu :00:05.0: set parameters failed. > [ 225.360363] [ cut here ] [ 225.360372] WARNING: > CPU: 0 PID: 6704 at > drivers/media/common/videobuf2/videobuf2-core.c:927 > vb2_buffer_done+0x20f/0x21a [videobuf2_common] [ 225.360374] Modules linked > in: snd_seq_dummy snd_seq snd_seq_device veth bridge stp llc tun nf_nat_tftp > nf_conntrack_tftp nf_nat_ftp nf_conntrack_ftp esp6 ah6 ip6t_REJECT > ip6t_ipv6header cmac rfcomm uinput > ipu3_imgu(C) ipu3_cio2 iova videobuf2_v4l2 videobuf2_common videobuf2_dma_sg > videobuf2_memops ov13858 ov5670 v4l2_fwnode dw9714 acpi_als xt_MASQUERADE > fuse iio_trig_sysfs cros_ec_sensors_ring cros_ec_light_prox cros_ec_sensors > cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf industrialio > cros_ec_sensorsupport cdc_ether btusb btrtl btintel btbcm usbnet bluetooth > ecdh_generic ecc hid_google_hammer iwlmvm iwl7000_mac80211 > r8152 mii lzo_rle lzo_compress iwlwifi zram cfg80211 joydev [ 225.360400] > CPU: 0 PID: 6704 Comm: CameraDeviceOps Tainted: G > C5.4.30 #5 > [ 225.360402] Hardware name: HP Soraka/Soraka, BIOS > Google_Soraka.10431.106.0 12/03/2019 > [ 225.360405] RIP: 0010:vb2_buffer_done+0x20f/0x21a [videobuf2_common] > [ 225.360408] Code: 5e 41 5f 5d e9 e0 16 5a d4 41 8b 55 08 48 c7 c7 8f 8b > 5c c0 48 c7 c6 36 9a 5c c0 44 89 f9 31 c0 e8 a5 1c 5b d4 e9 53 fe ff ff <0f> > 0b eb a3 e8 12 d7 43 d4 eb 97 0f 1f 44 00 00 55 48 89 e5 41 56 [ 225.360410] > RSP: 0018:9468ab32fba8 EFLAGS: 00010297 [ 225.360412] RAX: > 8aa7a51577a8 RBX: dead0122 RCX: > 8aa7a51577a8 > [ 225.360414] RDX: RSI: 0006 RDI: > 8aa7a5157400 > [ 225.360416] RBP: 9468ab32fbd8 R08: 8aa64e47e600 R09: > > [ 225.360418] R10: R11: c06036e6 R12: > dead0100 > [ 225.360420] R13: 8aa7820f1940 R14: 8aa7a51577a8 R15: > 0006 > [ 225.360422] FS: 7c1146ffd700() GS:8aa7baa0() > knlGS: > [ 225.360424] CS: 0010 DS: ES: CR0: 80050033 > [ 225.360426] CR2: 7aea3473a000 CR3: 537d6004 CR4: > 003606f0 > [ 225.360427] Call Trace: > [ 225.360434] imgu_return_all_buffers+0x6f/0x8e [ipu3_imgu] [ 225.360438] > imgu_vb2_stop_streaming+0xd6/0xf0 [ipu3_imgu] [ 225.360441] > __vb2_queue_cancel+0x33/0x22d [videobuf2_common] [ 225.360443] > vb2_core_streamoff+0x16/0x78 [videobuf2_common] [ 225.360448] > __video_do_ioctl+0x33d/0x42a [ 225.360452] video_usercopy+0x34a/0x615 > [ 225.360455] ? video_ioctl2+0x16/0x16 [ 225.360458] > v4l2_ioctl+0x46/0x53 [ 225.360462] do_vfs_ioctl+0x50a/0x787 [ 225.360465] > ksys_ioctl+0x58/0x83 [ 225.360468] __x64_sys_ioctl+0x1a/0x1e [ 225.360470] > do_syscall_64+0x54/0x68 [ 225.360474] > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [ 225.360476] RIP: 0033:0x7c118030f497 > [ 225.360479] Code: 8a 66 90 48 8b 05 d1 d9 2b 00 64 c7 00 26 00 00 00 > 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f > 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a1 d9 2b 00 f7 d8 64 89 01 48 > [ 225.360480] RSP: 002b:7c1146ffa5a8 EFLAGS: 0246 ORIG_RAX: > 0010 > [ 225.360483] RAX: ffda RBX: 7c1140010018 RCX: > 7c118030f497 > [ 225.360484] RDX: 7c114001019c RSI: 40045613 RDI: > 004c > [ 225.360486] RBP: 7c1146ffa700 R08: 7c1140010048 R09: > > [ 225.360488] R10: R11: 0246 R12: > 7c11400101b0 > [ 225.360489] R13: 7c1140010200 R14: 7c1140010048 R15: > 0001 > [ 225.360492
RE: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
__
BRs,
Cao, Bingbu
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, March 5, 2019 4:28 AM
> To: Sakari Ailus ; Mauro Carvalho Chehab
> ; Greg Kroah-Hartman
> Cc: Arnd Bergmann ; Zhi, Yong ; Cao,
> Bingbu ; linux-me...@vger.kernel.org;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
> Subject: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
>
> The imgu_css_queue structure is too large to be put on the kernel stack,
> as we can see in 32-bit builds:
>
> drivers/staging/media/ipu3/ipu3-css.c: In function 'imgu_css_fmt_try':
> drivers/staging/media/ipu3/ipu3-css.c:1863:1: error: the frame size of
> 1172 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> By dynamically allocating this array, the stack usage goes down to an
> acceptable 140 bytes for the same x86-32 configuration.
>
> Fixes: f5f2e4273518 ("media: staging/intel-ipu3: Add css pipeline
> programming")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/media/ipu3/ipu3-css.c | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-css.c
> b/drivers/staging/media/ipu3/ipu3-css.c
> index 15ab77e4b766..664c14b7a518 100644
> --- a/drivers/staging/media/ipu3/ipu3-css.c
> +++ b/drivers/staging/media/ipu3/ipu3-css.c
> @@ -3,6 +3,7 @@
>
> #include
> #include
> +#include
>
> #include "ipu3-css.h"
> #include "ipu3-css-fw.h"
> @@ -1744,7 +1745,7 @@ int imgu_css_fmt_try(struct imgu_css *css,
> struct v4l2_rect *const bds = &r[IPU3_CSS_RECT_BDS];
> struct v4l2_rect *const env = &r[IPU3_CSS_RECT_ENVELOPE];
> struct v4l2_rect *const gdc = &r[IPU3_CSS_RECT_GDC];
> - struct imgu_css_queue q[IPU3_CSS_QUEUES];
> + struct imgu_css_queue *q = kcalloc(IPU3_CSS_QUEUES, sizeof(struct
> +imgu_css_queue), GFP_KERNEL);
Could you use the devm_kcalloc()?
> struct v4l2_pix_format_mplane *const in =
> &q[IPU3_CSS_QUEUE_IN].fmt.mpix;
> struct v4l2_pix_format_mplane *const out = @@ -1753,6 +1754,11 @@
> int imgu_css_fmt_try(struct imgu_css *css,
> &q[IPU3_CSS_QUEUE_VF].fmt.mpix;
> int i, s, ret;
>
> + if (!q) {
> + ret = -ENOMEM;
> + goto out;
> + }
[Cao, Bingbu]
The goto here is wrong, you can just report an error, and I prefer it is next
to the alloc.
> +
> /* Adjust all formats, get statistics buffer sizes and formats */
> for (i = 0; i < IPU3_CSS_QUEUES; i++) {
> if (fmts[i])
> @@ -1766,7 +1772,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> dev_notice(css->dev, "can not initialize queue %s\n",
> qnames[i]);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> }
> for (i = 0; i < IPU3_CSS_RECTS; i++) { @@ -1788,7 +1795,8 @@ int
> imgu_css_fmt_try(struct imgu_css *css,
> if (!imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_IN]) ||
> !imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_OUT])) {
> dev_warn(css->dev, "required queues are disabled\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> if (!imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_OUT])) { @@ -1829,7
> +1837,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> ret = imgu_css_find_binary(css, pipe, q, r);
> if (ret < 0) {
> dev_err(css->dev, "failed to find suitable binary\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> css->pipes[pipe].bindex = ret;
>
> @@ -1843,7 +1852,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> dev_err(css->dev,
> "final resolution adjustment failed\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> *fmts[i] = q[i].fmt.mpix;
> }
> @@ -1859,7 +1869,10 @@ int imgu_css_fmt_try(struct imgu_css *css,
>bds->width, bds->height, gdc->width, gdc->height,
>out->width, out->height, vf->width, vf->height);
>
> - return 0;
> + ret = 0;
> +out:
> + kfree(q);
> + return ret;
> }
>
> int imgu_css_fmt_set(struct imgu_css *css,
> --
> 2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] media: staging/intel-ipu3: mark PM function as __maybe_unused
Hi, Bergmann,
Thanks for your patch.
Reviewed-by: Cao, Bingbu
__
BRs,
Cao, Bingbu
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, March 5, 2019 4:29 AM
> To: Sakari Ailus ; Mauro Carvalho Chehab
> ; Greg Kroah-Hartman
> Cc: Arnd Bergmann ; Zhi, Yong ;
> Tomasz Figa ; Qiu, Tian Shu
> ; Cao, Bingbu ; linux-
> me...@vger.kernel.org; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] media: staging/intel-ipu3: mark PM function as
> __maybe_unused
>
> The imgu_rpm_dummy_cb() looks like an API misuse that is explained in
> the comment above it. Aside from that, it also causes a warning when
> power management support is disabled:
>
> drivers/staging/media/ipu3/ipu3.c:794:12: error: 'imgu_rpm_dummy_cb'
> defined but not used [-Werror=unused-function]
>
> The warning is at least easy to fix by marking the function as
> __maybe_unused.
>
> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci
> device driver")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/media/ipu3/ipu3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3.c
> b/drivers/staging/media/ipu3/ipu3.c
> index d575ac78c8f0..d00d26264c37 100644
> --- a/drivers/staging/media/ipu3/ipu3.c
> +++ b/drivers/staging/media/ipu3/ipu3.c
> @@ -791,7 +791,7 @@ static int __maybe_unused imgu_resume(struct device
> *dev)
> * PCI rpm framework checks the existence of driver rpm callbacks.
> * Place a dummy callback here to avoid rpm going into error state.
> */
> -static int imgu_rpm_dummy_cb(struct device *dev)
> +static __maybe_unused int imgu_rpm_dummy_cb(struct device *dev)
> {
> return 0;
> }
> --
> 2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
