Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.lankhorst@linux.intel.com>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> Cc: linux-renesas-soc@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