| Message ID | 20210301120828.6945-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Hans Verkuil |
| Headers |
Received: from vger.kernel.org ([23.128.96.18])
by www.linuxtv.org with esmtp (Exim 4.92)
(envelope-from <linux-media-owner@vger.kernel.org>)
id 1lGhNL-00ET1M-8p; Mon, 01 Mar 2021 12:10:11 +0000
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S233064AbhCAMJk (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other);
Mon, 1 Mar 2021 07:09:40 -0500
Received: from relmlor1.renesas.com ([210.160.252.171]:39165 "EHLO
relmlie5.idc.renesas.com" rhost-flags-OK-OK-OK-FAIL)
by vger.kernel.org with ESMTP id S232943AbhCAMJ3 (ORCPT
<rfc822;linux-media@vger.kernel.org>);
Mon, 1 Mar 2021 07:09:29 -0500
X-IronPort-AV: E=Sophos;i="5.81,215,1610377200";
d="scan'208";a="73742493"
Received: from unknown (HELO relmlir5.idc.renesas.com) ([10.200.68.151])
by relmlie5.idc.renesas.com with ESMTP; 01 Mar 2021 21:08:38 +0900
Received: from localhost.localdomain (unknown [172.29.52.32])
by relmlir5.idc.renesas.com (Postfix) with ESMTP id B403340062A0;
Mon, 1 Mar 2021 21:08:36 +0900 (JST)
From: Biju Das <biju.das.jz@bp.renesas.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Biju Das <biju.das.jz@bp.renesas.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
Geert Uytterhoeven <geert+renesas@glider.be>,
Chris Paterson <Chris.Paterson2@renesas.com>,
Biju Das <biju.das@bp.renesas.com>,
Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access
Date: Mon, 1 Mar 2021 12:08:28 +0000
Message-Id: <20210301120828.6945-3-biju.das.jz@bp.renesas.com>
X-Mailer: git-send-email 2.17.1
In-Reply-To: <20210301120828.6945-1-biju.das.jz@bp.renesas.com>
References: <20210301120828.6945-1-biju.das.jz@bp.renesas.com>
Precedence: bulk
List-ID: <linux-media.vger.kernel.org>
X-Mailing-List: linux-media@vger.kernel.org
X-LSpam-Score: -2.4 (--)
X-LSpam-Report: No,
score=-2.4 required=5.0 tests=BAYES_00=-1.9,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1
autolearn=ham autolearn_force=no
|
| Series |
VSP trivial enhancements
|
|
Commit Message
Biju Das
March 1, 2021, 12:08 p.m. UTC
RZ/G2L SoC has no UIF. This patch fixes null pointer access, when UIF
module is not used.
Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display pipeline")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
Hi Biju, On 01/03/2021 12:08, Biju Das wrote: > RZ/G2L SoC has no UIF. This patch fixes null pointer access, when UIF > module is not used. > > Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display pipeline") > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c > index f6d2f47a4058..06f74d410973 100644 > --- a/drivers/media/platform/vsp1/vsp1_drm.c > +++ b/drivers/media/platform/vsp1/vsp1_drm.c > @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, This looks like it complicates these conditionals more than we perhaps need to. What do you think about adding something above the block comment here?: if (!drm_pipe->uif) return 0; > * make sure it is present in the pipeline's list of entities if it > * wasn't already. > */ > - if (!use_uif) { > + if (drm_pipe->uif && !use_uif) { > drm_pipe->uif->pipe = NULL; > - } else if (!drm_pipe->uif->pipe) { > + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) {> drm_pipe->uif->pipe = pipe; > list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); > } >
Hi Kieran, Thanks for the feedback. > Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access > > Hi Biju, > > On 01/03/2021 12:08, Biju Das wrote: > > RZ/G2L SoC has no UIF. This patch fixes null pointer access, when UIF > > module is not used. > > > > Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display > > pipeline") > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c > > b/drivers/media/platform/vsp1/vsp1_drm.c > > index f6d2f47a4058..06f74d410973 100644 > > --- a/drivers/media/platform/vsp1/vsp1_drm.c > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c > > @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct > > vsp1_device *vsp1, > > > This looks like it complicates these conditionals more than we perhaps > need to. > > What do you think about adding something above the block comment here?: It is much better. This patch is accepted in media tree[1]. So not sure, should I send a follow up patch as optimization or drop this patch and send new one. Please suggest. [1] https://git.linuxtv.org/media_tree.git/commit/?h=fixes&id=c4f27003ec3d84ef0c333c74ae2aff326537e583 Cheers, Biju > if (!drm_pipe->uif) > return 0; > > > > * make sure it is present in the pipeline's list of entities if it > > * wasn't already. > > */ > > - if (!use_uif) { > > + if (drm_pipe->uif && !use_uif) { > > drm_pipe->uif->pipe = NULL; > > - } else if (!drm_pipe->uif->pipe) { > > + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) {> > drm_pipe->uif->pipe = pipe; > > list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); > > } > >
Hi Biju, On 10/03/2021 13:56, Biju Das wrote: > Hi Kieran, > > Thanks for the feedback. >> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access >> >> Hi Biju, >> >> On 01/03/2021 12:08, Biju Das wrote: >>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when UIF >>> module is not used. >>> >>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display >>> pipeline") >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> --- >>> drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c >>> b/drivers/media/platform/vsp1/vsp1_drm.c >>> index f6d2f47a4058..06f74d410973 100644 >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c >>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct >>> vsp1_device *vsp1, >> >> >> This looks like it complicates these conditionals more than we perhaps >> need to. >> >> What do you think about adding something above the block comment here?: > > It is much better. > > This patch is accepted in media tree[1]. So not sure, > should I send a follow up patch as optimization or drop this patch and send new one. Oh, I didn't realise these were in already. Sorry, I didn't see any review on the list, and it was the earliest I had got to them. > Please suggest. Up to you, I don't think this would get dropped now it's integrated. It's in, so if you want to update on top I believe that's fine. -- Kieran > [1] https://git.linuxtv.org/media_tree.git/commit/?h=fixes&id=c4f27003ec3d84ef0c333c74ae2aff326537e583 > > Cheers, > Biju > >> if (!drm_pipe->uif) >> return 0; >> >> >>> * make sure it is present in the pipeline's list of entities if it >>> * wasn't already. >>> */ >>> - if (!use_uif) { >>> + if (drm_pipe->uif && !use_uif) { >>> drm_pipe->uif->pipe = NULL; >>> - } else if (!drm_pipe->uif->pipe) { >>> + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) {> >> drm_pipe->uif->pipe = pipe; >>> list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); >>> } >>> >
Hi Kieran, > Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access > > Hi Biju, > > On 10/03/2021 13:56, Biju Das wrote: > > Hi Kieran, > > > > Thanks for the feedback. > >> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer > >> access > >> > >> Hi Biju, > >> > >> On 01/03/2021 12:08, Biju Das wrote: > >>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when > >>> UIF module is not used. > >>> > >>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display > >>> pipeline") > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> --- > >>> drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c > >>> b/drivers/media/platform/vsp1/vsp1_drm.c > >>> index f6d2f47a4058..06f74d410973 100644 > >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c > >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c > >>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct > >>> vsp1_device *vsp1, > >> > >> > >> This looks like it complicates these conditionals more than we > >> perhaps need to. > >> > >> What do you think about adding something above the block comment here?: > > > > It is much better. > > > > This patch is accepted in media tree[1]. So not sure, should I send a > > follow up patch as optimization or drop this patch and send new one. > > Oh, I didn't realise these were in already. Sorry, I didn't see any review > on the list, and it was the earliest I had got to them. > > > Please suggest. > > Up to you, I don't think this would get dropped now it's integrated. > It's in, so if you want to update on top I believe that's fine. OK, Will send follow up patch as optimization. Cheers, Biju > -- > Kieran > > > > [1] > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit. > > linuxtv.org%2Fmedia_tree.git%2Fcommit%2F%3Fh%3Dfixes%26id%3Dc4f27003ec > > 3d84ef0c333c74ae2aff326537e583&data=04%7C01%7Cbiju.das.jz%40bp.ren > > esas.com%7Cad7bafadc61345db66e908d8e3d24f3c%7C53d82571da1947e49cb4625a > > 166a4a2a%7C0%7C0%7C637509839726919053%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi > > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000& > > sdata=jb%2BzwAJD6%2FxBKsQEryhDeKnJbgI%2F3RNZCIXfYU4EBCw%3D&reserve > > d=0 > > > > Cheers, > > Biju > > > >> if (!drm_pipe->uif) > >> return 0; > >> > >> > >>> * make sure it is present in the pipeline's list of entities if it > >>> * wasn't already. > >>> */ > >>> - if (!use_uif) { > >>> + if (drm_pipe->uif && !use_uif) { > >>> drm_pipe->uif->pipe = NULL; > >>> - } else if (!drm_pipe->uif->pipe) { > >>> + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) {> > >> drm_pipe->uif->pipe = pipe; > >>> list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); > >>> } > >>> > >
Hi Biju, On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote: > > On 10/03/2021 13:56, Biju Das wrote: > > > Thanks for the feedback. > > >> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer > > >> access > > >> > > >> Hi Biju, > > >> > > >> On 01/03/2021 12:08, Biju Das wrote: > > >>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when > > >>> UIF module is not used. > > >>> > > >>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display > > >>> pipeline") > > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > >>> --- > > >>> drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- > > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c > > >>> b/drivers/media/platform/vsp1/vsp1_drm.c > > >>> index f6d2f47a4058..06f74d410973 100644 > > >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c > > >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c > > >>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct > > >>> vsp1_device *vsp1, > > >> > > >> > > >> This looks like it complicates these conditionals more than we > > >> perhaps need to. > > >> > > >> What do you think about adding something above the block comment here?: > > > > > > It is much better. > > > > > > This patch is accepted in media tree[1]. So not sure, should I send a > > > follow up patch as optimization or drop this patch and send new one. > > > > Oh, I didn't realise these were in already. Sorry, I didn't see any review > > on the list, and it was the earliest I had got to them. > > > > > Please suggest. > > > > Up to you, I don't think this would get dropped now it's integrated. > > It's in, so if you want to update on top I believe that's fine. > > OK, Will send follow up patch as optimization. That would be nice. I don't think this patch should have been fast-tracked as a fix, as RZ/G2L isn't supported in mainline yet as far as I can tell. Hans, next time, could we get a notification instead of a silent merge ? > > >> if (!drm_pipe->uif) > > >> return 0; > > >> > > >> > > >>> * make sure it is present in the pipeline's list of entities if it > > >>> * wasn't already. > > >>> */ > > >>> - if (!use_uif) { > > >>> + if (drm_pipe->uif && !use_uif) { > > >>> drm_pipe->uif->pipe = NULL; > > >>> - } else if (!drm_pipe->uif->pipe) { > > >>> + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) {> > > >> drm_pipe->uif->pipe = pipe; > > >>> list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); > > >>> } > > >>>
Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 15 March 2021 03:43 > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: kieran.bingham+renesas@ideasonboard.com; Mauro Carvalho Chehab > <mchehab@kernel.org>; linux-media@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>; Chris > Paterson <Chris.Paterson2@renesas.com>; Biju Das > <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl> > Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access > > Hi Biju, > > On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote: > > > On 10/03/2021 13:56, Biju Das wrote: > > > > Thanks for the feedback. > > > >> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer > > > >> access > > > >> > > > >> Hi Biju, > > > >> > > > >> On 01/03/2021 12:08, Biju Das wrote: > > > >>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, > > > >>> when UIF module is not used. > > > >>> > > > >>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in > > > >>> display > > > >>> pipeline") > > > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > >>> --- > > > >>> drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- > > > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > > > >>> > > > >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c > > > >>> b/drivers/media/platform/vsp1/vsp1_drm.c > > > >>> index f6d2f47a4058..06f74d410973 100644 > > > >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c > > > >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c > > > >>> @@ -462,9 +462,9 @@ static int > > > >>> vsp1_du_pipeline_setup_inputs(struct > > > >>> vsp1_device *vsp1, > > > >> > > > >> > > > >> This looks like it complicates these conditionals more than we > > > >> perhaps need to. > > > >> > > > >> What do you think about adding something above the block comment > here?: > > > > > > > > It is much better. > > > > > > > > This patch is accepted in media tree[1]. So not sure, should I > > > > send a follow up patch as optimization or drop this patch and send > new one. > > > > > > Oh, I didn't realise these were in already. Sorry, I didn't see any > > > review on the list, and it was the earliest I had got to them. > > > > > > > Please suggest. > > > > > > Up to you, I don't think this would get dropped now it's integrated. > > > It's in, so if you want to update on top I believe that's fine. > > > > OK, Will send follow up patch as optimization. > > That would be nice. > > I don't think this patch should have been fast-tracked as a fix, as RZ/G2L > isn't supported in mainline yet as far as I can tell. Please correct me, if I am wrong. We have pluggable modules in VSP and with routing Register we can achieve any combination with the VSP driver we have. If it is the case, it is a bug which is fast-tracked as a fix. Otherwise(ie, driver doesn't have flexibility to support pluggable feature) I am agreeing with your statement. Cheers, Biju > Hans, next time, could we get a notification instead of a silent merge ? > > > > >> if (!drm_pipe->uif) > > > >> return 0; > > > >> > > > >> > > > >>> * make sure it is present in the pipeline's list of entities > if it > > > >>> * wasn't already. > > > >>> */ > > > >>> - if (!use_uif) { > > > >>> + if (drm_pipe->uif && !use_uif) { > > > >>> drm_pipe->uif->pipe = NULL; > > > >>> - } else if (!drm_pipe->uif->pipe) { > > > >>> + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) {> > > > >> drm_pipe->uif->pipe = pipe; > > > >>> list_add_tail(&drm_pipe->uif->list_pipe, &pipe- > >entities); > > > >>> } > > > >>> > > -- > Regards, > > Laurent Pinchart
Hi Biju, On Mon, Mar 15, 2021 at 08:21:38AM +0000, Biju Das wrote: > On 15 March 2021 03:43, Laurent Pinchart wrote: > > On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote: > >>> On 10/03/2021 13:56, Biju Das wrote: > >>>> Thanks for the feedback. > >>>>> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer > >>>>> access > >>>>> > >>>>> Hi Biju, > >>>>> > >>>>> On 01/03/2021 12:08, Biju Das wrote: > >>>>>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, > >>>>>> when UIF module is not used. > >>>>>> > >>>>>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in > >>>>>> display pipeline") > >>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>>>>> --- > >>>>>> drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- > >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c > >>>>>> b/drivers/media/platform/vsp1/vsp1_drm.c > >>>>>> index f6d2f47a4058..06f74d410973 100644 > >>>>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c > >>>>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c > >>>>>> @@ -462,9 +462,9 @@ static int > >>>>>> vsp1_du_pipeline_setup_inputs(struct > >>>>>> vsp1_device *vsp1, > >>>>> > >>>>> > >>>>> This looks like it complicates these conditionals more than we > >>>>> perhaps need to. > >>>>> > >>>>> What do you think about adding something above the block comment here?: > >>>> > >>>> It is much better. > >>>> > >>>> This patch is accepted in media tree[1]. So not sure, should I > >>>> send a follow up patch as optimization or drop this patch and send new one. > >>> > >>> Oh, I didn't realise these were in already. Sorry, I didn't see any > >>> review on the list, and it was the earliest I had got to them. > >>> > >>>> Please suggest. > >>> > >>> Up to you, I don't think this would get dropped now it's integrated. > >>> It's in, so if you want to update on top I believe that's fine. > >> > >> OK, Will send follow up patch as optimization. > > > > That would be nice. > > > > I don't think this patch should have been fast-tracked as a fix, as RZ/G2L > > isn't supported in mainline yet as far as I can tell. > > Please correct me, if I am wrong. We have pluggable modules in VSP and with routing > Register we can achieve any combination with the VSP driver we have. > > If it is the case, it is a bug which is fast-tracked as a fix. > Otherwise(ie, driver doesn't have flexibility to support pluggable > feature) I am agreeing with your statement. My point was that this code is currently used only on platforms that have a UIF, so there's should be no risk of this problem being triggered. It should certainly be fixed to support RZ/G2L, but that's not upstream yet. > > Hans, next time, could we get a notification instead of a silent > > merge ? > > > >>>>> if (!drm_pipe->uif) > >>>>> return 0; > >>>>> > >>>>>> * make sure it is present in the pipeline's list of entities if it > >>>>>> * wasn't already. > >>>>>> */ > >>>>>> - if (!use_uif) { > >>>>>> + if (drm_pipe->uif && !use_uif) { > >>>>>> drm_pipe->uif->pipe = NULL; > >>>>>> - } else if (!drm_pipe->uif->pipe) { > >>>>>> + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) { > >>>>>> drm_pipe->uif->pipe = pipe; > >>>>>> list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); > >>>>>> }
On 15/03/2021 04:42, Laurent Pinchart wrote: > Hi Biju, > > On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote: >>> On 10/03/2021 13:56, Biju Das wrote: >>>> Thanks for the feedback. >>>>> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer >>>>> access >>>>> >>>>> Hi Biju, >>>>> >>>>> On 01/03/2021 12:08, Biju Das wrote: >>>>>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when >>>>>> UIF module is not used. >>>>>> >>>>>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display >>>>>> pipeline") >>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>>> --- >>>>>> drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c >>>>>> b/drivers/media/platform/vsp1/vsp1_drm.c >>>>>> index f6d2f47a4058..06f74d410973 100644 >>>>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c >>>>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c >>>>>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct >>>>>> vsp1_device *vsp1, >>>>> >>>>> >>>>> This looks like it complicates these conditionals more than we >>>>> perhaps need to. >>>>> >>>>> What do you think about adding something above the block comment here?: >>>> >>>> It is much better. >>>> >>>> This patch is accepted in media tree[1]. So not sure, should I send a >>>> follow up patch as optimization or drop this patch and send new one. >>> >>> Oh, I didn't realise these were in already. Sorry, I didn't see any review >>> on the list, and it was the earliest I had got to them. >>> >>>> Please suggest. >>> >>> Up to you, I don't think this would get dropped now it's integrated. >>> It's in, so if you want to update on top I believe that's fine. >> >> OK, Will send follow up patch as optimization. > > That would be nice. > > I don't think this patch should have been fast-tracked as a fix, as > RZ/G2L isn't supported in mainline yet as far as I can tell. > > Hans, next time, could we get a notification instead of a silent merge ? My apologies, it seemed a trivial fix, but I should have checked with you. I jumped the gun here :-( Regards, Hans > >>>>> if (!drm_pipe->uif) >>>>> return 0; >>>>> >>>>> >>>>>> * make sure it is present in the pipeline's list of entities if it >>>>>> * wasn't already. >>>>>> */ >>>>>> - if (!use_uif) { >>>>>> + if (drm_pipe->uif && !use_uif) { >>>>>> drm_pipe->uif->pipe = NULL; >>>>>> - } else if (!drm_pipe->uif->pipe) { >>>>>> + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) {> >>>>> drm_pipe->uif->pipe = pipe; >>>>>> list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); >>>>>> } >>>>>> >
Hi Hans, On Tue, Mar 16, 2021 at 09:21:15AM +0100, Hans Verkuil wrote: > On 15/03/2021 04:42, Laurent Pinchart wrote: > > On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote: > >>> On 10/03/2021 13:56, Biju Das wrote: > >>>> Thanks for the feedback. > >>>>> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer > >>>>> access > >>>>> > >>>>> Hi Biju, > >>>>> > >>>>> On 01/03/2021 12:08, Biju Das wrote: > >>>>>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when > >>>>>> UIF module is not used. > >>>>>> > >>>>>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display > >>>>>> pipeline") > >>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>>>>> --- > >>>>>> drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- > >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c > >>>>>> b/drivers/media/platform/vsp1/vsp1_drm.c > >>>>>> index f6d2f47a4058..06f74d410973 100644 > >>>>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c > >>>>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c > >>>>>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct > >>>>>> vsp1_device *vsp1, > >>>>> > >>>>> > >>>>> This looks like it complicates these conditionals more than we > >>>>> perhaps need to. > >>>>> > >>>>> What do you think about adding something above the block comment here?: > >>>> > >>>> It is much better. > >>>> > >>>> This patch is accepted in media tree[1]. So not sure, should I send a > >>>> follow up patch as optimization or drop this patch and send new one. > >>> > >>> Oh, I didn't realise these were in already. Sorry, I didn't see any review > >>> on the list, and it was the earliest I had got to them. > >>> > >>>> Please suggest. > >>> > >>> Up to you, I don't think this would get dropped now it's integrated. > >>> It's in, so if you want to update on top I believe that's fine. > >> > >> OK, Will send follow up patch as optimization. > > > > That would be nice. > > > > I don't think this patch should have been fast-tracked as a fix, as > > RZ/G2L isn't supported in mainline yet as far as I can tell. > > > > Hans, next time, could we get a notification instead of a silent merge ? > > My apologies, it seemed a trivial fix, but I should have checked with you. > > I jumped the gun here :-( No worries, it can happen :-) > >>>>> if (!drm_pipe->uif) > >>>>> return 0; > >>>>> > >>>>> > >>>>>> * make sure it is present in the pipeline's list of entities if it > >>>>>> * wasn't already. > >>>>>> */ > >>>>>> - if (!use_uif) { > >>>>>> + if (drm_pipe->uif && !use_uif) { > >>>>>> drm_pipe->uif->pipe = NULL; > >>>>>> - } else if (!drm_pipe->uif->pipe) { > >>>>>> + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) {> > >>>>> drm_pipe->uif->pipe = pipe; > >>>>>> list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); > >>>>>> } > >>>>>>
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index f6d2f47a4058..06f74d410973 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, * make sure it is present in the pipeline's list of entities if it * wasn't already. */ - if (!use_uif) { + if (drm_pipe->uif && !use_uif) { drm_pipe->uif->pipe = NULL; - } else if (!drm_pipe->uif->pipe) { + } else if (drm_pipe->uif && !drm_pipe->uif->pipe) { drm_pipe->uif->pipe = pipe; list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities); }