| Message ID | 20230519000407.60744-1-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/exynos: vidi: fix a wrong error return | expand |
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
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
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
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
2023년 5월 19일 (금) 오후 6:21, Andi Shyti <andi.shyti@linux.intel.com>님이 작성: > 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 :) > I didn't check the second email you sent. :) Thanks, Inki Dae > Thanks, > Andi >
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);
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(-)