diff mbox series

[1/2] media: v4l: vsp1: Fix bru null pointer access

Message ID	20210301120828.6945-2-biju.das.jz@bp.renesas.com (mailing list archive)
State	Accepted, archived
Delegated to:	Hans Verkuil
Headers	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 1/2] media: v4l: vsp1: Fix bru null pointer access Date: Mon, 1 Mar 2021 12:08:27 +0000 Message-Id: <20210301120828.6945-2-biju.das.jz@bp.renesas.com> In-Reply-To: <20210301120828.6945-1-biju.das.jz@bp.renesas.com> References: <20210301120828.6945-1-biju.das.jz@bp.renesas.com> Precedence: bulk
Series	VSP trivial enhancements \| [0/2] VSP trivial enhancements [1/2] media: v4l: vsp1: Fix bru null pointer access [2/2] media: v4l: vsp1: Fix uif null pointer access

Commit Message

Biju Das March 1, 2021, 12:08 p.m. UTC

RZ/G2L SoC has only BRS. This patch fixes null pointer access,when only
BRS is enabled.

Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kieran Bingham March 8, 2021, 11:06 p.m. UTC | #1

Hi Biju,

On 01/03/2021 12:08, Biju Das wrote:
> RZ/G2L SoC has only BRS. This patch fixes null pointer access,when only
> BRS is enabled.
> 
> Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 86d5e3f4b1ff..f6d2f47a4058 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
>  		brx = &vsp1->bru->entity;
>  	else if (pipe->brx && !drm_pipe->force_brx_release)
>  		brx = pipe->brx;
> -	else if (!vsp1->bru->entity.pipe)
> +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) && !vsp1->bru->entity.pipe)
>  		brx = &vsp1->bru->entity;
>  	else
>  		brx = &vsp1->brs->entity;

The comments here describe that the choice to start at the BRU is
arbitrary, so if we could confirm that there will always be a BRS
otherwise, we could swap those to save an extra feature check.

But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.

I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
brs->entity line to keep the symmetry ... but it wouldn't be needed, as
it should fall through. If there isn't a BRS there must be a BRU or we
wouldn't be setting up a brx in the first place ;-)

So I think what you have is good.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Biju Das March 11, 2021, 7:15 a.m. UTC | #2

Hi Kieran,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access
> 
> Hi Biju,
> 
> On 01/03/2021 12:08, Biju Das wrote:
> > RZ/G2L SoC has only BRS. This patch fixes null pointer access,when
> > only BRS is enabled.
> >
> > Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 86d5e3f4b1ff..f6d2f47a4058 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> vsp1_device *vsp1,
> >  		brx = &vsp1->bru->entity;
> >  	else if (pipe->brx && !drm_pipe->force_brx_release)
> >  		brx = pipe->brx;
> > -	else if (!vsp1->bru->entity.pipe)
> > +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) &&
> > +!vsp1->bru->entity.pipe)
> >  		brx = &vsp1->bru->entity;
> >  	else
> >  		brx = &vsp1->brs->entity;
> 
> 
> The comments here describe that the choice to start at the BRU is
> arbitrary, so if we could confirm that there will always be a BRS
> otherwise, we could swap those to save an extra feature check.

As long as we are supporting composition(Multiple inputs with Blend and Raster operations)
There will be either BRU or BRS or both in R-Car Gen3|RZ/G2 SoC's. Currently this is
the case with all SoC variant of this families.

Cheers,
Biju

> 
> But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
> vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.
> 
> I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
> brs->entity line to keep the symmetry ... but it wouldn't be needed, as
> it should fall through. If there isn't a BRS there must be a BRU or we
> wouldn't be setting up a brx in the first place ;-)
> 
> So I think what you have is good.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Laurent Pinchart March 15, 2021, 3:36 a.m. UTC | #3

Hi Biju,

On Thu, Mar 11, 2021 at 07:15:01AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access
> > 
> > Hi Biju,
> > 
> > On 01/03/2021 12:08, Biju Das wrote:
> > > RZ/G2L SoC has only BRS. This patch fixes null pointer access,when
> > > only BRS is enabled.
> > >
> > > Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")

Given that RZ/G2L isn't supported in mainline, this is hardly a fix, is
it ?

> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > > b/drivers/media/platform/vsp1/vsp1_drm.c
> > > index 86d5e3f4b1ff..f6d2f47a4058 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > > @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> > vsp1_device *vsp1,
> > >  		brx = &vsp1->bru->entity;
> > >  	else if (pipe->brx && !drm_pipe->force_brx_release)
> > >  		brx = pipe->brx;
> > > -	else if (!vsp1->bru->entity.pipe)
> > > +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) &&
> > > +!vsp1->bru->entity.pipe)
> > >  		brx = &vsp1->bru->entity;
> > >  	else
> > >  		brx = &vsp1->brs->entity;
> > 
> > 
> > The comments here describe that the choice to start at the BRU is
> > arbitrary, so if we could confirm that there will always be a BRS
> > otherwise, we could swap those to save an extra feature check.
> 
> As long as we are supporting composition(Multiple inputs with Blend and Raster operations)
> There will be either BRU or BRS or both in R-Car Gen3|RZ/G2 SoC's. Currently this is
> the case with all SoC variant of this families.

Given that the function is called vsp1_du_pipeline_setup_brx(), I think
we can assume there will be either a BRU or a BRS :-)

How many RPF instances does the RG/G2L VSPD have ?

> > But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
> > vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.
> > 
> > I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
> > brs->entity line to keep the symmetry ... but it wouldn't be needed, as
> > it should fall through. If there isn't a BRS there must be a BRU or we
> > wouldn't be setting up a brx in the first place ;-)
> > 
> > So I think what you have is good.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Biju Das March 15, 2021, 9:15 a.m. UTC | #4

Hi Laurent,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access
> 
> Hi Biju,
> 
> On Thu, Mar 11, 2021 at 07:15:01AM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer
> > > access
> > >
> > > Hi Biju,
> > >
> > > On 01/03/2021 12:08, Biju Das wrote:
> > > > RZ/G2L SoC has only BRS. This patch fixes null pointer access,when
> > > > only BRS is enabled.
> > > >
> > > > Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
> 
> Given that RZ/G2L isn't supported in mainline, this is hardly a fix, is it
> ?

I agree RZ/G2L is a new SoC.

Please see my comments for other patch. I have added fixes tag based on pluggable feature on VSP driver.

If we have BRS and BRU, we can select either BRS or BRU. For eg:- VSPDL in R-Car H3/H3-N/M3-N and also R-Car V3M/V3H we have both BRS and BRU. Since both are present on this SoC's, it won't trigger the null pointer issue. 

But as new SoC's like RZ/G2L have only BRS.

> 
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > > > b/drivers/media/platform/vsp1/vsp1_drm.c
> > > > index 86d5e3f4b1ff..f6d2f47a4058 100644
> > > > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > > > @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> > > vsp1_device *vsp1,
> > > >  		brx = &vsp1->bru->entity;
> > > >  	else if (pipe->brx && !drm_pipe->force_brx_release)
> > > >  		brx = pipe->brx;
> > > > -	else if (!vsp1->bru->entity.pipe)
> > > > +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) &&
> > > > +!vsp1->bru->entity.pipe)
> > > >  		brx = &vsp1->bru->entity;
> > > >  	else
> > > >  		brx = &vsp1->brs->entity;
> > >
> > >
> > > The comments here describe that the choice to start at the BRU is
> > > arbitrary, so if we could confirm that there will always be a BRS
> > > otherwise, we could swap those to save an extra feature check.
> >
> > As long as we are supporting composition(Multiple inputs with Blend
> > and Raster operations) There will be either BRU or BRS or both in
> > R-Car Gen3|RZ/G2 SoC's. Currently this is the case with all SoC variant
> of this families.
> 
> Given that the function is called vsp1_du_pipeline_setup_brx(), I think we
> can assume there will be either a BRU or a BRS :-)
> 
> How many RPF instances does the RG/G2L VSPD have ?

2 RPF, 1WPF , 1 BRS, 1 LUT and 1 LIF

Cheers,
Biju

> 
> > > But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
> > > vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.
> > >
> > > I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
> > > brs->entity line to keep the symmetry ... but it wouldn't be needed,
> > > brs->as
> > > it should fall through. If there isn't a BRS there must be a BRU or
> > > we wouldn't be setting up a brx in the first place ;-)
> > >
> > > So I think what you have is good.
> > >
> > > Reviewed-by: Kieran Bingham
> > > <kieran.bingham+renesas@ideasonboard.com>
> 
> --
> Regards,
> 
> Laurent Pinchart

Laurent Pinchart March 15, 2021, 9:27 a.m. UTC | #5

Hi Biju,

On Mon, Mar 15, 2021 at 09:15:00AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access
> > On Thu, Mar 11, 2021 at 07:15:01AM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer
> > > > access
> > > >
> > > > Hi Biju,
> > > >
> > > > On 01/03/2021 12:08, Biju Das wrote:
> > > > > RZ/G2L SoC has only BRS. This patch fixes null pointer access,when
> > > > > only BRS is enabled.
> > > > >
> > > > > Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
> > 
> > Given that RZ/G2L isn't supported in mainline, this is hardly a fix, is it
> > ?
> 
> I agree RZ/G2L is a new SoC.
> 
> Please see my comments for other patch. I have added fixes tag based
> on pluggable feature on VSP driver.
> 
> If we have BRS and BRU, we can select either BRS or BRU. For eg:-
> VSPDL in R-Car H3/H3-N/M3-N and also R-Car V3M/V3H we have both BRS
> and BRU. Since both are present on this SoC's, it won't trigger the
> null pointer issue. 
> 
> But as new SoC's like RZ/G2L have only BRS.

I agree that this issue needs to be addressed, my point was that it
shouldn't have been merged in the -fixes branch (which will likely
result in the patch being backported to stable branches) but in the
-next branch, given that the problem it solves can't be triggered yet by
any platform supported in mainline.

> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > > > > b/drivers/media/platform/vsp1/vsp1_drm.c
> > > > > index 86d5e3f4b1ff..f6d2f47a4058 100644
> > > > > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > > > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > > > > @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> > > > vsp1_device *vsp1,
> > > > >  		brx = &vsp1->bru->entity;
> > > > >  	else if (pipe->brx && !drm_pipe->force_brx_release)
> > > > >  		brx = pipe->brx;
> > > > > -	else if (!vsp1->bru->entity.pipe)
> > > > > +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) &&
> > > > > +!vsp1->bru->entity.pipe)
> > > > >  		brx = &vsp1->bru->entity;
> > > > >  	else
> > > > >  		brx = &vsp1->brs->entity;
> > > >
> > > >
> > > > The comments here describe that the choice to start at the BRU is
> > > > arbitrary, so if we could confirm that there will always be a BRS
> > > > otherwise, we could swap those to save an extra feature check.
> > >
> > > As long as we are supporting composition(Multiple inputs with Blend
> > > and Raster operations) There will be either BRU or BRS or both in
> > > R-Car Gen3|RZ/G2 SoC's. Currently this is the case with all SoC variant
> > > of this families.
> > 
> > Given that the function is called vsp1_du_pipeline_setup_brx(), I think we
> > can assume there will be either a BRU or a BRS :-)
> > 
> > How many RPF instances does the RG/G2L VSPD have ?
> 
> 2 RPF, 1WPF , 1 BRS, 1 LUT and 1 LIF

Thank you.

> > > > But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
> > > > vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.
> > > >
> > > > I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
> > > > brs->entity line to keep the symmetry ... but it wouldn't be needed,
> > > > brs->as
> > > > it should fall through. If there isn't a BRS there must be a BRU or
> > > > we wouldn't be setting up a brx in the first place ;-)
> > > >
> > > > So I think what you have is good.
> > > >
> > > > Reviewed-by: Kieran Bingham
> > > > <kieran.bingham+renesas@ideasonboard.com>

diff mbox series

Patch

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 86d5e3f4b1ff..f6d2f47a4058 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -245,7 +245,7 @@  static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
 		brx = &vsp1->bru->entity;
 	else if (pipe->brx && !drm_pipe->force_brx_release)
 		brx = pipe->brx;
-	else if (!vsp1->bru->entity.pipe)
+	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) && !vsp1->bru->entity.pipe)
 		brx = &vsp1->bru->entity;
 	else
 		brx = &vsp1->brs->entity;