Hi Marteen,
On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote:
> Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
> > On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
> >> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
> >> writing its own version. Instead of open coding destroy_state(), call
> >> it directly for freeing the old state.
> > I don't think the second sentence applies to this patch.
> >
> >> Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> >> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> >> Cc: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> >> Cc: linux-renesas-...@vger.kernel.org
> >> ---
> >> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
> >> 1 file changed, 3 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> index 4cdea14d552f..7766551e67fc 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc
> >> *crtc)
> >>
> >> static void rcar_du_crtc_reset(struct drm_crtc *crtc)
> >> {
> >> - struct rcar_du_crtc_state *state;
> >> + struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
> >>
> >> - if (crtc->state) {
> >> + if (crtc->state)
> >> rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> >> - crtc->state = NULL;
> >> - }
> >>
> >> - state = kzalloc(sizeof(*state), GFP_KERNEL);
> >> + __drm_atomic_helper_crtc_reset(crtc, &state->state);
> >
> > state may be NULL here if the above kzalloc() failed. Let's keep the
> > original order of the function, and simply call
> > __drm_atomic_helper_crtc_reset() after the NULL check below.
>
> There were 10 different ways crtc was implemented, I felt it was good to
> settle on one.
>
> We don't handle during reset at all, would need to start propagating this
> first before we should handle errors, imho.
That's not the point. As state can be NULL, you could end up
dereferencing a NULL pointer. The fact that the base state is the first
field in the rcar_du_crtc_state structure is just luck, and shouldn't be
relied on.
> Looking more closely, it's the same way that errors in
> rcar_du_plane_reset() are handled. :)
It's not, the return value of kzalloc() is checked explicitly in
rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset().
Please copy the code flow of rcar_du_plane_reset() to implement
rcar_du_crtc_reset().
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
