* [RESEND,V2] drm: fsl-dcu: Fix no fb check bug
@ 2016-01-06 4:12 Meng Yi
2016-01-08 9:20 ` Emil Velikov
0 siblings, 1 reply; 6+ messages in thread
From: Meng Yi @ 2016-01-06 4:12 UTC (permalink / raw)
To: airlied; +Cc: Meng Yi, dri-devel
For state->fb or state->crtc may be NULL in fsl_dcu_drm_plane_atomic_check
function, if so, return 0.
Signed-off-by: Meng Yi <meng.yi@nxp.com>
Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
---
change in v2:
-Add state->crtc check
-return 0 when state->fb or state->crtc is NULL, instead of -EINVAL
Adviced by Daniel Stone
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
index 4b13cf9..8965580 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane,
{
struct drm_framebuffer *fb = state->fb;
+ if (!state->fb || !state->crtc)
+ return 0;
+
switch (fb->pixel_format) {
case DRM_FORMAT_RGB565:
case DRM_FORMAT_RGB888:
@@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
unsigned int alpha, bpp;
int index, ret;
- if (!fb)
- return;
-
index = fsl_dcu_drm_plane_index(plane);
if (index < 0)
return;
--
2.1.0.27.g96db324
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND,V2] drm: fsl-dcu: Fix no fb check bug
2016-01-06 4:12 [RESEND,V2] drm: fsl-dcu: Fix no fb check bug Meng Yi
@ 2016-01-08 9:20 ` Emil Velikov
2016-01-14 5:54 ` Stefan Agner
0 siblings, 1 reply; 6+ messages in thread
From: Emil Velikov @ 2016-01-08 9:20 UTC (permalink / raw)
To: Meng Yi; +Cc: ML dri-devel
Hi guys,
Am I loosing the plot here or something feels amiss here ?
On 6 January 2016 at 06:12, Meng Yi <meng.yi@nxp.com> wrote:
> For state->fb or state->crtc may be NULL in fsl_dcu_drm_plane_atomic_check
> function, if so, return 0.
>
> Signed-off-by: Meng Yi <meng.yi@nxp.com>
> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
>
> ---
>
> change in v2:
> -Add state->crtc check
> -return 0 when state->fb or state->crtc is NULL, instead of -EINVAL
> Adviced by Daniel Stone
>
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> index 4b13cf9..8965580 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> @@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane,
> {
> struct drm_framebuffer *fb = state->fb;
>
> + if (!state->fb || !state->crtc)
> + return 0;
> +
Namely: if we return success here core drm will end up calling the
atomic_update...
> switch (fb->pixel_format) {
> case DRM_FORMAT_RGB565:
> case DRM_FORMAT_RGB888:
> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
> unsigned int alpha, bpp;
> int index, ret;
>
> - if (!fb)
> - return;
> -
... which no longer has the !fb check, and we'll crash with null deref
a few lines below ?
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND,V2] drm: fsl-dcu: Fix no fb check bug
2016-01-08 9:20 ` Emil Velikov
@ 2016-01-14 5:54 ` Stefan Agner
2016-01-14 8:23 ` Meng Yi
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Agner @ 2016-01-14 5:54 UTC (permalink / raw)
To: Emil Velikov; +Cc: Meng Yi, ML dri-devel
On 2016-01-08 01:20, Emil Velikov wrote:
> Hi guys,
>
> Am I loosing the plot here or something feels amiss here ?
>
> On 6 January 2016 at 06:12, Meng Yi <meng.yi@nxp.com> wrote:
>> For state->fb or state->crtc may be NULL in fsl_dcu_drm_plane_atomic_check
>> function, if so, return 0.
>>
>> Signed-off-by: Meng Yi <meng.yi@nxp.com>
>> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
>>
>> ---
>>
>> change in v2:
>> -Add state->crtc check
>> -return 0 when state->fb or state->crtc is NULL, instead of -EINVAL
>> Adviced by Daniel Stone
>>
>> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
>> index 4b13cf9..8965580 100644
>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
>> @@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane,
>> {
>> struct drm_framebuffer *fb = state->fb;
>>
>> + if (!state->fb || !state->crtc)
>> + return 0;
>> +
> Namely: if we return success here core drm will end up calling the
> atomic_update...
>
After atomic_check atomic_disable could be called too. However, this
seem not directly depend on state'>fb, but more on plane->state->crtc.
>> switch (fb->pixel_format) {
>> case DRM_FORMAT_RGB565:
>> case DRM_FORMAT_RGB888:
>> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
>> unsigned int alpha, bpp;
>> int index, ret;
>>
>> - if (!fb)
>> - return;
>> -
> ... which no longer has the !fb check, and we'll crash with null deref
> a few lines below ?
If there is a legitimate situation where fb is null which also
ultimately leads to a atomic_commit, I guess we should keep the return
here...
--
Stefan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [RESEND,V2] drm: fsl-dcu: Fix no fb check bug
2016-01-14 5:54 ` Stefan Agner
@ 2016-01-14 8:23 ` Meng Yi
2016-01-26 21:18 ` Emil Velikov
0 siblings, 1 reply; 6+ messages in thread
From: Meng Yi @ 2016-01-14 8:23 UTC (permalink / raw)
To: Stefan Agner, Emil Velikov; +Cc: ML dri-devel
> >> switch (fb->pixel_format) {
> >> case DRM_FORMAT_RGB565:
> >> case DRM_FORMAT_RGB888:
> >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct
> drm_plane *plane,
> >> unsigned int alpha, bpp;
> >> int index, ret;
> >>
> >> - if (!fb)
> >> - return;
> >> -
> > ... which no longer has the !fb check, and we'll crash with null deref
> > a few lines below ?
>
>
> If there is a legitimate situation where fb is null which also ultimately leads to a
> atomic_commit, I guess we should keep the return here...
I think I made a mistake here, fb check should not be removed . As Stefan mentioned, if fb check in fsl_dcu_drm_plane_atomic_check return 0, fsl_dcu_drm_plane_atomic_update will ultimately called, and we'll crash since plane->state->fb is NULL.
> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Thursday, January 14, 2016 1:54 PM
> To: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Meng Yi <meng.yi@nxp.com>; ML dri-devel <dri-
> devel@lists.freedesktop.org>
> Subject: Re: [RESEND,V2] drm: fsl-dcu: Fix no fb check bug
>
> On 2016-01-08 01:20, Emil Velikov wrote:
> > Hi guys,
> >
> > Am I loosing the plot here or something feels amiss here ?
> >
> > On 6 January 2016 at 06:12, Meng Yi <meng.yi@nxp.com> wrote:
> >> For state->fb or state->crtc may be NULL in
> >> fsl_dcu_drm_plane_atomic_check function, if so, return 0.
> >>
> >> Signed-off-by: Meng Yi <meng.yi@nxp.com>
> >> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
> >>
> >> ---
> >>
> >> change in v2:
> >> -Add state->crtc check
> >> -return 0 when state->fb or state->crtc is NULL, instead of -EINVAL
> >> Adviced by Daniel Stone
> >>
> >> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> >> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> >> index 4b13cf9..8965580 100644
> >> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> >> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> >> @@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct
> >> drm_plane *plane, {
> >> struct drm_framebuffer *fb = state->fb;
> >>
> >> + if (!state->fb || !state->crtc)
> >> + return 0;
> >> +
> > Namely: if we return success here core drm will end up calling the
> > atomic_update...
> >
>
> After atomic_check atomic_disable could be called too. However, this seem
> not directly depend on state'>fb, but more on plane->state->crtc.
>
>
>
> >> switch (fb->pixel_format) {
> >> case DRM_FORMAT_RGB565:
> >> case DRM_FORMAT_RGB888:
> >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct
> drm_plane *plane,
> >> unsigned int alpha, bpp;
> >> int index, ret;
> >>
> >> - if (!fb)
> >> - return;
> >> -
> > ... which no longer has the !fb check, and we'll crash with null deref
> > a few lines below ?
>
>
> If there is a legitimate situation where fb is null which also ultimately leads to a
> atomic_commit, I guess we should keep the return here...
>
> --
> Stefan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND,V2] drm: fsl-dcu: Fix no fb check bug
2016-01-14 8:23 ` Meng Yi
@ 2016-01-26 21:18 ` Emil Velikov
2016-01-28 2:23 ` Stefan Agner
0 siblings, 1 reply; 6+ messages in thread
From: Emil Velikov @ 2016-01-26 21:18 UTC (permalink / raw)
To: Meng Yi; +Cc: ML dri-devel
On 14 January 2016 at 08:23, Meng Yi <meng.yi@nxp.com> wrote:
>> >> switch (fb->pixel_format) {
>> >> case DRM_FORMAT_RGB565:
>> >> case DRM_FORMAT_RGB888:
>> >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct
>> drm_plane *plane,
>> >> unsigned int alpha, bpp;
>> >> int index, ret;
>> >>
>> >> - if (!fb)
>> >> - return;
>> >> -
>> > ... which no longer has the !fb check, and we'll crash with null deref
>> > a few lines below ?
>>
>>
>> If there is a legitimate situation where fb is null which also ultimately leads to a
>> atomic_commit, I guess we should keep the return here...
>
> I think I made a mistake here, fb check should not be removed . As Stefan mentioned, if fb check in fsl_dcu_drm_plane_atomic_check return 0, fsl_dcu_drm_plane_atomic_update will ultimately called, and we'll crash since plane->state->fb is NULL.
>
I believe you meant "Emil" in the above ;-) But seriously... afaict a
fair few drivers do a similar !fb (even !state->crtc) check(s)...
which makes me wonder if core DRM isn't the better place for it ? Or
perhaps that's intentional as core provides the flexibility for each
driver to mangle with the fb between .check and .disable ?
Cheers
Emil
P.S. Please don't top post, use interleaved style [1]
[1] https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND,V2] drm: fsl-dcu: Fix no fb check bug
2016-01-26 21:18 ` Emil Velikov
@ 2016-01-28 2:23 ` Stefan Agner
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Agner @ 2016-01-28 2:23 UTC (permalink / raw)
To: Emil Velikov, Meng Yi; +Cc: ML dri-devel
On 2016-01-26 13:18, Emil Velikov wrote:
> On 14 January 2016 at 08:23, Meng Yi <meng.yi@nxp.com> wrote:
>>> >> switch (fb->pixel_format) {
>>> >> case DRM_FORMAT_RGB565:
>>> >> case DRM_FORMAT_RGB888:
>>> >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct
>>> drm_plane *plane,
>>> >> unsigned int alpha, bpp;
>>> >> int index, ret;
>>> >>
>>> >> - if (!fb)
>>> >> - return;
>>> >> -
>>> > ... which no longer has the !fb check, and we'll crash with null deref
>>> > a few lines below ?
>>>
>>>
>>> If there is a legitimate situation where fb is null which also ultimately leads to a
>>> atomic_commit, I guess we should keep the return here...
>>
>> I think I made a mistake here, fb check should not be removed . As Stefan mentioned, if fb check in fsl_dcu_drm_plane_atomic_check return 0, fsl_dcu_drm_plane_atomic_update will ultimately called, and we'll crash since plane->state->fb is NULL.
>>
> I believe you meant "Emil" in the above ;-) But seriously... afaict a
> fair few drivers do a similar !fb (even !state->crtc) check(s)...
> which makes me wonder if core DRM isn't the better place for it ? Or
> perhaps that's intentional as core provides the flexibility for each
> driver to mangle with the fb between .check and .disable ?
>
There seem to be a consensus to check crtc and fb on atomic_check.
However, in atomic_update, some drives do a NULL check on crtc only, and
some on both, crtc and fb.
The comment in drm_atomic_plane_disabling says CRTC and FB should always
be NULL together... So I guess it does not really matter all that much,
unless there is anyway a bug.
Furthermore, it seems that in the null case, atomic_disable is called
anyway (which this driver implements). Not sure if there is another case
in which either of this two could be NULL and atomic_update could be
called.
Since this patch is mainly addressing the null check in atomic_check, I
will apply it without the change in atomic_update for now.
--
Stefan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-28 2:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-06 4:12 [RESEND,V2] drm: fsl-dcu: Fix no fb check bug Meng Yi
2016-01-08 9:20 ` Emil Velikov
2016-01-14 5:54 ` Stefan Agner
2016-01-14 8:23 ` Meng Yi
2016-01-26 21:18 ` Emil Velikov
2016-01-28 2:23 ` Stefan Agner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).