diff mbox series

[v2,1/2] drm/vc4: crtc: Rework a bit the CRTC state code

Message ID 20200923084032.218619-1-maxime@cerno.tech
State New, archived
Headers show
Series [v2,1/2] drm/vc4: crtc: Rework a bit the CRTC state code | expand

Commit Message

Maxime Ripard Sept. 23, 2020, 8:40 a.m. UTC
The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
embeds drm_crtc_state as its first member, and therefore can be safely
casted.

However, this is pretty fragile especially since there's no check for this
in place, and we're going to need to access vc4_crtc_state member at reset
so this looks like a good occasion to make it more robust.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

---

Changes from v1:
  - new patch
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Dave Stevenson Sept. 23, 2020, 2:59 p.m. UTC | #1
Hi Maxime

On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> embeds drm_crtc_state as its first member, and therefore can be safely
> casted.

s/casted/cast

> However, this is pretty fragile especially since there's no check for this
> in place, and we're going to need to access vc4_crtc_state member at reset
> so this looks like a good occasion to make it more robust.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Based on the issue I perceived with the previous patch, I'm happy. I
haven't thought about how the framework handles losing the state, but
that's not the driver's problem.

There is still an implicit assumption that drm_crtc_state is the first
member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
make it even more robust that could be a container_of instead. I
haven't checked for any other places that make the assumption though.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>
> Changes from v1:
>   - new patch
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index a393f93390a2..7ef20adedee5 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
>
>  void vc4_crtc_reset(struct drm_crtc *crtc)
>  {
> +       struct vc4_crtc_state *vc4_crtc_state;
> +
>         if (crtc->state)
>                 vc4_crtc_destroy_state(crtc, crtc->state);
> -       crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
> -       if (crtc->state)
> -               __drm_atomic_helper_crtc_reset(crtc, crtc->state);
> +
> +       vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
> +       if (!vc4_crtc_state) {
> +               crtc->state = NULL;
> +               return;
> +       }
> +
> +       __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
>  }
>
>  static const struct drm_crtc_funcs vc4_crtc_funcs = {
> --
> 2.26.2
>
Daniel Vetter Sept. 23, 2020, 3:41 p.m. UTC | #2
On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.

Also your next atomic_check will probably look at this, most definitely in
the memcpy to take over the old state. So yeah KASAN should complain if
you get this wrong :-)

Probably want a Fixes: line for this too.
-Daniel

> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> > ---
> >
> > Changes from v1:
> >   - new patch
> > ---
> >  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> > index a393f93390a2..7ef20adedee5 100644
> > --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> > @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
> >
> >  void vc4_crtc_reset(struct drm_crtc *crtc)
> >  {
> > +       struct vc4_crtc_state *vc4_crtc_state;
> > +
> >         if (crtc->state)
> >                 vc4_crtc_destroy_state(crtc, crtc->state);
> > -       crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
> > -       if (crtc->state)
> > -               __drm_atomic_helper_crtc_reset(crtc, crtc->state);
> > +
> > +       vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
> > +       if (!vc4_crtc_state) {
> > +               crtc->state = NULL;
> > +               return;
> > +       }
> > +
> > +       __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
> >  }
> >
> >  static const struct drm_crtc_funcs vc4_crtc_funcs = {
> > --
> > 2.26.2
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Maxime Ripard Sept. 25, 2020, 11:38 a.m. UTC | #3
Hi Dave,

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.

Good catch, I'll send another patch to fix it

> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Does it apply to the second patch as well?

Thanks!
Maxime
Dave Stevenson Sept. 25, 2020, 11:47 a.m. UTC | #4
On Fri, 25 Sep 2020 at 12:38, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Dave,
>
> On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> > Hi Maxime
> >
> > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > > embeds drm_crtc_state as its first member, and therefore can be safely
> > > casted.
> >
> > s/casted/cast
> >
> > > However, this is pretty fragile especially since there's no check for this
> > > in place, and we're going to need to access vc4_crtc_state member at reset
> > > so this looks like a good occasion to make it more robust.
> > >
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > Based on the issue I perceived with the previous patch, I'm happy. I
> > haven't thought about how the framework handles losing the state, but
> > that's not the driver's problem.
> >
> > There is still an implicit assumption that drm_crtc_state is the first
> > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> > make it even more robust that could be a container_of instead. I
> > haven't checked for any other places that make the assumption though.
>
> Good catch, I'll send another patch to fix it
>
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> Does it apply to the second patch as well?

No. I got another interrupt before I'd refreshed my memory over what
the second patch was doing and responding to it. I'll do that now (I
don't think it changed much from v1)

  Dave.
Maxime Ripard Sept. 25, 2020, 2:57 p.m. UTC | #5
Hi,

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> > embeds drm_crtc_state as its first member, and therefore can be safely
> > casted.
> 
> s/casted/cast
> 
> > However, this is pretty fragile especially since there's no check for this
> > in place, and we're going to need to access vc4_crtc_state member at reset
> > so this looks like a good occasion to make it more robust.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Based on the issue I perceived with the previous patch, I'm happy. I
> haven't thought about how the framework handles losing the state, but
> that's not the driver's problem.
> 
> There is still an implicit assumption that drm_crtc_state is the first
> member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
> make it even more robust that could be a container_of instead. I
> haven't checked for any other places that make the assumption though.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Applied 1 and 2, with the typo fixed (and the fixes tag suggested by Daniel)

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index a393f93390a2..7ef20adedee5 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -852,11 +852,18 @@  void vc4_crtc_destroy_state(struct drm_crtc *crtc,
 
 void vc4_crtc_reset(struct drm_crtc *crtc)
 {
+	struct vc4_crtc_state *vc4_crtc_state;
+
 	if (crtc->state)
 		vc4_crtc_destroy_state(crtc, crtc->state);
-	crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
-	if (crtc->state)
-		__drm_atomic_helper_crtc_reset(crtc, crtc->state);
+
+	vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
+	if (!vc4_crtc_state) {
+		crtc->state = NULL;
+		return;
+	}
+
+	__drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
 }
 
 static const struct drm_crtc_funcs vc4_crtc_funcs = {