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 <[email protected]>
---
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 <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,
--
2.20.0
__________________________
BRs,
Cao, Bingbu
> -----Original Message-----
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: Tuesday, March 5, 2019 4:28 AM
> To: Sakari Ailus <[email protected]>; Mauro Carvalho Chehab
> <[email protected]>; Greg Kroah-Hartman <[email protected]>
> Cc: Arnd Bergmann <[email protected]>; Zhi, Yong <[email protected]>; Cao,
> Bingbu <[email protected]>; [email protected];
> [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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 <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);
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
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,
--
Regards,
Sakari Ailus
[email protected]
On Tue, Mar 5, 2019 at 8:53 AM Sakari Ailus
<[email protected]> 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
> <[email protected]> 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.
--
Sakari Ailus
[email protected]
On Tue, Mar 5, 2019 at 9:47 AM Sakari Ailus
<[email protected]> 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 <[email protected]> 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