[PATCH] drm/exynos: vidi: fix a wrong error return

[PATCH] drm/exynos: vidi: fix a wrong error return

[Inki Dae]

Fix a wrong error return by dropping an error return.

When vidi driver is remvoed, if ctx->raw_edid isn't same as fake_edid_info
then only what we have to is to free ctx->raw_edid so that driver removing
can work correctly - it's not an error case.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 4d56c8c799c5..f5e1adfcaa51 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -469,8 +469,6 @@ static int vidi_remove(struct platform_device *pdev)
 	if (ctx->raw_edid != (struct edid *)fake_edid_info) {
 		kfree(ctx->raw_edid);
 		ctx->raw_edid = NULL;
-
-		return -EINVAL;
 	}
 
 	component_del(&pdev->dev, &vidi_component_ops);
-- 
2.25.1

Re: [PATCH] drm/exynos: vidi: fix a wrong error return

[Andi Shyti]

Hi Inki,

On Fri, May 19, 2023 at 09:04:07AM +0900, Inki Dae wrote:
> Fix a wrong error return by dropping an error return.
> 
> When vidi driver is remvoed, if ctx->raw_edid isn't same as fake_edid_info
> then only what we have to is to free ctx->raw_edid so that driver removing
> can work correctly - it's not an error case.
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index 4d56c8c799c5..f5e1adfcaa51 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -469,8 +469,6 @@ static int vidi_remove(struct platform_device *pdev)
>  	if (ctx->raw_edid != (struct edid *)fake_edid_info) {
>  		kfree(ctx->raw_edid);
>  		ctx->raw_edid = NULL;
> -
> -		return -EINVAL;

It doesn't look right to me, I think the correct patch should be:

-       if (ctx->raw_edid != (struct edid *)fake_edid_info) {
-               kfree(ctx->raw_edid);
-               ctx->raw_edid = NULL;
-
-               return -EINVAL;
-       }
-
+       ctx->raw_edid = NULL;

because "ctx->raw_edid" points to a non allocated memory in the
.data segment and you cannot free it.

A follow-up cleanup should be to remove the "const" from
fake_edid_info because you are assigning its address to pointers
(raw_edid), so that what's the point for having it const? You are
just fooling the compiler :)

Andi

>  	}
>  
>  	component_del(&pdev->dev, &vidi_component_ops);
> -- 
> 2.25.1

Re: [PATCH] drm/exynos: vidi: fix a wrong error return

[Andi Shyti]

Hi Inki,

On Fri, May 19, 2023 at 02:26:40AM +0200, Andi Shyti wrote:
> Hi Inki,
> 
> On Fri, May 19, 2023 at 09:04:07AM +0900, Inki Dae wrote:
> > Fix a wrong error return by dropping an error return.
> > 
> > When vidi driver is remvoed, if ctx->raw_edid isn't same as fake_edid_info
> > then only what we have to is to free ctx->raw_edid so that driver removing
> > can work correctly - it's not an error case.
> > 
> > Signed-off-by: Inki Dae <inki.dae@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_vidi.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> > index 4d56c8c799c5..f5e1adfcaa51 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> > @@ -469,8 +469,6 @@ static int vidi_remove(struct platform_device *pdev)
> >  	if (ctx->raw_edid != (struct edid *)fake_edid_info) {
> >  		kfree(ctx->raw_edid);
> >  		ctx->raw_edid = NULL;
> > -
> > -		return -EINVAL;
> 
> It doesn't look right to me, I think the correct patch should be:
> 
> -       if (ctx->raw_edid != (struct edid *)fake_edid_info) {
> -               kfree(ctx->raw_edid);
> -               ctx->raw_edid = NULL;
> -
> -               return -EINVAL;
> -       }
> -
> +       ctx->raw_edid = NULL;
> 
> because "ctx->raw_edid" points to a non allocated memory in the
> .data segment and you cannot free it.
> 
> A follow-up cleanup should be to remove the "const" from
> fake_edid_info because you are assigning its address to pointers
> (raw_edid), so that what's the point for having it const? You are
> just fooling the compiler :)

please ignore, this is what happens when reading patches at
2.26am, that a "!=" becomes "==". The patch is correct, still
some cleanups is needed here, though.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Andi

PS I was actually sleeping and this woke me up :)

> Andi
> 
> >  	}
> >  
> >  	component_del(&pdev->dev, &vidi_component_ops);
> > -- 
> > 2.25.1

RE: [PATCH] drm/exynos: vidi: fix a wrong error return

[대인기]

Hi Andi, :)

> -----Original Message-----
> From: Andi Shyti <andi.shyti@linux.intel.com>
> Sent: Friday, May 19, 2023 9:27 AM
> To: Inki Dae <inki.dae@samsung.com>
> Cc: dri-devel@lists.freedesktop.org; linux-samsung-soc@vger.kernel.org; Andi
> Shyti <andi.shyti@kernel.org>
> Subject: Re: [PATCH] drm/exynos: vidi: fix a wrong error return
> 
> Hi Inki,
> 
> On Fri, May 19, 2023 at 09:04:07AM +0900, Inki Dae wrote:
> > Fix a wrong error return by dropping an error return.
> >
> > When vidi driver is remvoed, if ctx->raw_edid isn't same as fake_edid_info
> > then only what we have to is to free ctx->raw_edid so that driver removing
> > can work correctly - it's not an error case.
> >
> > Signed-off-by: Inki Dae <inki.dae@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_vidi.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> > index 4d56c8c799c5..f5e1adfcaa51 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> > @@ -469,8 +469,6 @@ static int vidi_remove(struct platform_device *pdev)
> >  	if (ctx->raw_edid != (struct edid *)fake_edid_info) {
> >  		kfree(ctx->raw_edid);
> >  		ctx->raw_edid = NULL;
> > -
> > -		return -EINVAL;
> 
> It doesn't look right to me, I think the correct patch should be:
> 
> -       if (ctx->raw_edid != (struct edid *)fake_edid_info) {
> -               kfree(ctx->raw_edid);
> -               ctx->raw_edid = NULL;
> -
> -               return -EINVAL;
> -       }
> -
> +       ctx->raw_edid = NULL;
> 
> because "ctx->raw_edid" points to a non allocated memory in the
> .data segment and you cannot free it.
> 
> A follow-up cleanup should be to remove the "const" from
> fake_edid_info because you are assigning its address to pointers
> (raw_edid), so that what's the point for having it const? You are
> just fooling the compiler :)

Thanks for review comment. 

"ctx->raw_edid != fake_edid_info" means that the edid sent by the user through
the ictl system call - vidi_connection_ioctl - is used instead of fake one -
face_edid_info.
In this case, ctx->raw_edid object needs to be released because ctx->raw_edid
object is allocated and the edid object sent by user is copied to the ctx-
>raw_edid by kmemdup(). :)

Thanks,
Inki Dae

> 
> Andi
> 
> >  	}
> >
> >  	component_del(&pdev->dev, &vidi_component_ops);
> > --
> > 2.25.1

Re: [PATCH] drm/exynos: vidi: fix a wrong error return

[Andi Shyti]

Hi Inki,

> > > @@ -469,8 +469,6 @@ static int vidi_remove(struct platform_device *pdev)
> > >  	if (ctx->raw_edid != (struct edid *)fake_edid_info) {
> > >  		kfree(ctx->raw_edid);
> > >  		ctx->raw_edid = NULL;
> > > -
> > > -		return -EINVAL;
> > 
> > It doesn't look right to me, I think the correct patch should be:
> > 
> > -       if (ctx->raw_edid != (struct edid *)fake_edid_info) {
> > -               kfree(ctx->raw_edid);
> > -               ctx->raw_edid = NULL;
> > -
> > -               return -EINVAL;
> > -       }
> > -
> > +       ctx->raw_edid = NULL;
> > 
> > because "ctx->raw_edid" points to a non allocated memory in the
> > .data segment and you cannot free it.
> > 
> > A follow-up cleanup should be to remove the "const" from
> > fake_edid_info because you are assigning its address to pointers
> > (raw_edid), so that what's the point for having it const? You are
> > just fooling the compiler :)
> 
> Thanks for review comment. 
> 
> "ctx->raw_edid != fake_edid_info" means that the edid sent by the user through
> the ictl system call - vidi_connection_ioctl - is used instead of fake one -
> face_edid_info.
> In this case, ctx->raw_edid object needs to be released because ctx->raw_edid
> object is allocated and the edid object sent by user is copied to the ctx-
> >raw_edid by kmemdup(). :)

yes... yes... I sent you another e-mail after this :)

Thanks,
Andi