| Message ID | 20190304202758.1802417-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging/intel-ipu3: reduce kernel stack usage | expand |
Hi Bingbu, Arnd, On Tue, Mar 05, 2019 at 12:25:18AM +0000, Cao, Bingbu wrote: ... > > @@ -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()? No, because this is not related to the device, called instead on e.g. VIDIOC_TRY_FMT. > > 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. I agree, the goto is just not needed. > > + > > /* 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,
On Tue, Mar 5, 2019 at 8:53 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > On Tue, Mar 05, 2019 at 12:25:18AM +0000, Cao, Bingbu wrote: > > > 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. > > I agree, the goto is just not needed. Should I remove all the gotos then and do an explicit kfree() in each error path? I'd prefer not to mix the two styles, as that can lead to subtle mistakes when the code is refactored again. Arnd
On Tue, Mar 05, 2019 at 09:40:24AM +0100, Arnd Bergmann wrote: > On Tue, Mar 5, 2019 at 8:53 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > On Tue, Mar 05, 2019 at 12:25:18AM +0000, Cao, Bingbu wrote: > > > > > 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. > > > > I agree, the goto is just not needed. > > Should I remove all the gotos then and do an explicit kfree() in each > error path? > > I'd prefer not to mix the two styles, as that can lead to subtle mistakes > when the code is refactored again. In this case there's no need for kfree as q is NULL. Goto is often useful if you need to do things to undo stuff that was done earlier in the function but it's not the case here. I prefer keeping the rest gotos.
On Tue, Mar 5, 2019 at 9:47 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > On Tue, Mar 05, 2019 at 09:40:24AM +0100, Arnd Bergmann wrote: > > On Tue, Mar 5, 2019 at 8:53 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > On Tue, Mar 05, 2019 at 12:25:18AM +0000, Cao, Bingbu wrote: > > > > > > > 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. > > > > > > I agree, the goto is just not needed. > > > > Should I remove all the gotos then and do an explicit kfree() in each > > error path? > > > > I'd prefer not to mix the two styles, as that can lead to subtle mistakes > > when the code is refactored again. > > In this case there's no need for kfree as q is NULL. Goto is often useful > if you need to do things to undo stuff that was done earlier in the > function but it's not the case here. I prefer keeping the rest gotos. Ok, I'll send an updated patch the way you suggested then. Arnd
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 <linux/device.h> #include <linux/iopoll.h> +#include <linux/slab.h> #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); 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; + } + /* 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,
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 <arnd@arndb.de> --- drivers/staging/media/ipu3/ipu3-css.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)