| Message ID | 20210301120828.6945-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 6732f313938027a910e1f7351951ff52c0329e70 |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | VSP trivial enhancements | expand |
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); }
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(-)