| Message ID | 20210315123406.1523607-1-ribalda@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] media: staging/intel-ipu3: Fix memory leak in imu_fmt | expand |
Hi, Ricardo Thanks for your patch. It looks fine for me, do you mind squash 2 patchsets into 1 commit? On 3/15/21 8:34 PM, Ricardo Ribalda wrote: > We are losing the reference to an allocated memory if try. Change the > order of the check to avoid that. > > Cc: stable@vger.kernel.org > Fixes: 6d5f26f2e045 ("media: staging/intel-ipu3-v4l: reduce kernel stack usage") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/staging/media/ipu3/ipu3-v4l2.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c > index 60aa02eb7d2a..35a74d99322f 100644 > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > @@ -693,6 +693,13 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, > if (inode == IMGU_NODE_STAT_3A || inode == IMGU_NODE_PARAMS) > continue; > > + /* CSS expects some format on OUT queue */ > + if (i != IPU3_CSS_QUEUE_OUT && > + !imgu_pipe->nodes[inode].enabled) { > + fmts[i] = NULL; > + continue; > + } > + > if (try) { > fmts[i] = kmemdup(&imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp, > sizeof(struct v4l2_pix_format_mplane), > @@ -705,10 +712,6 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, > fmts[i] = &imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp; > } > > - /* CSS expects some format on OUT queue */ > - if (i != IPU3_CSS_QUEUE_OUT && > - !imgu_pipe->nodes[inode].enabled) > - fmts[i] = NULL; > } > > if (!try) { > -- Best regards, Bingbu Cao
Hi Bingbu Thanks for your review On Tue, Mar 16, 2021 at 12:29 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > > Hi, Ricardo > > Thanks for your patch. > It looks fine for me, do you mind squash 2 patchsets into 1 commit? Are you sure? There are two different issues that we are solving. Best regards! > > On 3/15/21 8:34 PM, Ricardo Ribalda wrote: > > We are losing the reference to an allocated memory if try. Change the > > order of the check to avoid that. > > > > Cc: stable@vger.kernel.org > > Fixes: 6d5f26f2e045 ("media: staging/intel-ipu3-v4l: reduce kernel stack usage") > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/staging/media/ipu3/ipu3-v4l2.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c > > index 60aa02eb7d2a..35a74d99322f 100644 > > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > > @@ -693,6 +693,13 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, > > if (inode == IMGU_NODE_STAT_3A || inode == IMGU_NODE_PARAMS) > > continue; > > > > + /* CSS expects some format on OUT queue */ > > + if (i != IPU3_CSS_QUEUE_OUT && > > + !imgu_pipe->nodes[inode].enabled) { > > + fmts[i] = NULL; > > + continue; > > + } > > + > > if (try) { > > fmts[i] = kmemdup(&imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp, > > sizeof(struct v4l2_pix_format_mplane), > > @@ -705,10 +712,6 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, > > fmts[i] = &imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp; > > } > > > > - /* CSS expects some format on OUT queue */ > > - if (i != IPU3_CSS_QUEUE_OUT && > > - !imgu_pipe->nodes[inode].enabled) > > - fmts[i] = NULL; > > } > > > > if (!try) { > > > > -- > Best regards, > Bingbu Cao -- Ricardo Ribalda
On 3/17/21 1:50 AM, Ricardo Ribalda wrote: > Hi Bingbu > > Thanks for your review > > On Tue, Mar 16, 2021 at 12:29 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >> >> Hi, Ricardo >> >> Thanks for your patch. >> It looks fine for me, do you mind squash 2 patchsets into 1 commit? > > Are you sure? There are two different issues that we are solving. Oh, I see. I thought you were fixing 1 issue here. Thanks! > > Best regards! > >> >> On 3/15/21 8:34 PM, Ricardo Ribalda wrote: >>> We are losing the reference to an allocated memory if try. Change the >>> order of the check to avoid that. >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 6d5f26f2e045 ("media: staging/intel-ipu3-v4l: reduce kernel stack usage") >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>> --- >>> drivers/staging/media/ipu3/ipu3-v4l2.c | 11 +++++++---- >>> 1 file changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c >>> index 60aa02eb7d2a..35a74d99322f 100644 >>> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c >>> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c >>> @@ -693,6 +693,13 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, >>> if (inode == IMGU_NODE_STAT_3A || inode == IMGU_NODE_PARAMS) >>> continue; >>> >>> + /* CSS expects some format on OUT queue */ >>> + if (i != IPU3_CSS_QUEUE_OUT && >>> + !imgu_pipe->nodes[inode].enabled) { >>> + fmts[i] = NULL; >>> + continue; >>> + } >>> + >>> if (try) { >>> fmts[i] = kmemdup(&imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp, >>> sizeof(struct v4l2_pix_format_mplane), >>> @@ -705,10 +712,6 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, >>> fmts[i] = &imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp; >>> } >>> >>> - /* CSS expects some format on OUT queue */ >>> - if (i != IPU3_CSS_QUEUE_OUT && >>> - !imgu_pipe->nodes[inode].enabled) >>> - fmts[i] = NULL; >>> } >>> >>> if (!try) { >>> >> >> -- >> Best regards, >> Bingbu Cao > > >
Hi Bingbu Maybe you want to add your Reviewed-by ? ;) Thanks! On Wed, Mar 17, 2021 at 7:48 AM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > > > On 3/17/21 1:50 AM, Ricardo Ribalda wrote: > > Hi Bingbu > > > > Thanks for your review > > > > On Tue, Mar 16, 2021 at 12:29 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >> > >> Hi, Ricardo > >> > >> Thanks for your patch. > >> It looks fine for me, do you mind squash 2 patchsets into 1 commit? > > > > Are you sure? There are two different issues that we are solving. > > Oh, I see. I thought you were fixing 1 issue here. > Thanks! > > > > > Best regards! > > > >> > >> On 3/15/21 8:34 PM, Ricardo Ribalda wrote: > >>> We are losing the reference to an allocated memory if try. Change the > >>> order of the check to avoid that. > >>> > >>> Cc: stable@vger.kernel.org > >>> Fixes: 6d5f26f2e045 ("media: staging/intel-ipu3-v4l: reduce kernel stack usage") > >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >>> --- > >>> drivers/staging/media/ipu3/ipu3-v4l2.c | 11 +++++++---- > >>> 1 file changed, 7 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c > >>> index 60aa02eb7d2a..35a74d99322f 100644 > >>> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > >>> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > >>> @@ -693,6 +693,13 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, > >>> if (inode == IMGU_NODE_STAT_3A || inode == IMGU_NODE_PARAMS) > >>> continue; > >>> > >>> + /* CSS expects some format on OUT queue */ > >>> + if (i != IPU3_CSS_QUEUE_OUT && > >>> + !imgu_pipe->nodes[inode].enabled) { > >>> + fmts[i] = NULL; > >>> + continue; > >>> + } > >>> + > >>> if (try) { > >>> fmts[i] = kmemdup(&imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp, > >>> sizeof(struct v4l2_pix_format_mplane), > >>> @@ -705,10 +712,6 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, > >>> fmts[i] = &imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp; > >>> } > >>> > >>> - /* CSS expects some format on OUT queue */ > >>> - if (i != IPU3_CSS_QUEUE_OUT && > >>> - !imgu_pipe->nodes[inode].enabled) > >>> - fmts[i] = NULL; > >>> } > >>> > >>> if (!try) { > >>> > >> > >> -- > >> Best regards, > >> Bingbu Cao > > > > > > > > -- > Best regards, > Bingbu Cao
On Mon, Mar 15, 2021 at 01:34:06PM +0100, Ricardo Ribalda wrote: > If there in an error during a set_fmt, do not overwrite the previous > sizes with the invalid config. > > [ 38.662975] ipu3-imgu 0000:00:05.0: swiotlb buffer is full (sz: 4096 bytes) > [ 38.662980] DMA: Out of SW-IOMMU space for 4096 bytes at device 0000:00:05.0 > [ 38.663010] general protection fault: 0000 [#1] PREEMPT SMP > > Cc: stable@vger.kernel.org > Fixes: 6d5f26f2e045 ("media: staging/intel-ipu3-v4l: reduce kernel stack usage") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/staging/media/ipu3/ipu3-v4l2.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c index 60aa02eb7d2a..35a74d99322f 100644 --- a/drivers/staging/media/ipu3/ipu3-v4l2.c +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c @@ -693,6 +693,13 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, if (inode == IMGU_NODE_STAT_3A || inode == IMGU_NODE_PARAMS) continue; + /* CSS expects some format on OUT queue */ + if (i != IPU3_CSS_QUEUE_OUT && + !imgu_pipe->nodes[inode].enabled) { + fmts[i] = NULL; + continue; + } + if (try) { fmts[i] = kmemdup(&imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp, sizeof(struct v4l2_pix_format_mplane), @@ -705,10 +712,6 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int pipe, int node, fmts[i] = &imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp; } - /* CSS expects some format on OUT queue */ - if (i != IPU3_CSS_QUEUE_OUT && - !imgu_pipe->nodes[inode].enabled) - fmts[i] = NULL; } if (!try) {
We are losing the reference to an allocated memory if try. Change the order of the check to avoid that. Cc: stable@vger.kernel.org Fixes: 6d5f26f2e045 ("media: staging/intel-ipu3-v4l: reduce kernel stack usage") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/staging/media/ipu3/ipu3-v4l2.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)