* [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
@ 2019-03-04 20:27 Arnd Bergmann
2019-03-05 0:25 ` Cao, Bingbu
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2019-03-04 20:27 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Greg Kroah-Hartman
Cc: Arnd Bergmann, Yong Zhi, Bingbu Cao, linux-media, devel, linux-kernel
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
2019-03-04 20:27 [PATCH] media: staging/intel-ipu3: reduce kernel stack usage Arnd Bergmann
@ 2019-03-05 0:25 ` Cao, Bingbu
2019-03-05 7:53 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Cao, Bingbu @ 2019-03-05 0:25 UTC (permalink / raw)
To: Arnd Bergmann, Sakari Ailus, Mauro Carvalho Chehab, Greg Kroah-Hartman
Cc: Zhi, Yong, linux-media, devel, linux-kernel
__________________________
BRs,
Cao, Bingbu
> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Tuesday, March 5, 2019 4:28 AM
> To: Sakari Ailus <sakari.ailus@linux.intel.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>; Zhi, Yong <yong.zhi@intel.com>; Cao,
> Bingbu <bingbu.cao@intel.com>; linux-media@vger.kernel.org;
> devel@driverdev.osuosl.org; linux-kernel@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 <arnd@arndb.de>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
2019-03-05 0:25 ` Cao, Bingbu
@ 2019-03-05 7:53 ` Sakari Ailus
2019-03-05 8:40 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2019-03-05 7:53 UTC (permalink / raw)
To: Cao, Bingbu
Cc: Arnd Bergmann, Mauro Carvalho Chehab, Greg Kroah-Hartman, Zhi,
Yong, linux-media, devel, linux-kernel
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
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
2019-03-05 7:53 ` Sakari Ailus
@ 2019-03-05 8:40 ` Arnd Bergmann
2019-03-05 8:47 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2019-03-05 8:40 UTC (permalink / raw)
To: Sakari Ailus
Cc: Cao, Bingbu, Mauro Carvalho Chehab, Greg Kroah-Hartman, Zhi,
Yong, linux-media, devel, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
2019-03-05 8:40 ` Arnd Bergmann
@ 2019-03-05 8:47 ` Sakari Ailus
2019-03-05 9:30 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2019-03-05 8:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Cao, Bingbu, Mauro Carvalho Chehab, Greg Kroah-Hartman, Zhi,
Yong, linux-media, devel, linux-kernel
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.
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
2019-03-05 8:47 ` Sakari Ailus
@ 2019-03-05 9:30 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2019-03-05 9:30 UTC (permalink / raw)
To: Sakari Ailus
Cc: Cao, Bingbu, Mauro Carvalho Chehab, Greg Kroah-Hartman, Zhi,
Yong, linux-media, devel, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-05 9:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-04 20:27 [PATCH] media: staging/intel-ipu3: reduce kernel stack usage Arnd Bergmann
2019-03-05 0:25 ` Cao, Bingbu
2019-03-05 7:53 ` Sakari Ailus
2019-03-05 8:40 ` Arnd Bergmann
2019-03-05 8:47 ` Sakari Ailus
2019-03-05 9:30 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).