Op 01-03-2019 om 15:36 schreef Laurent Pinchart:
> 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@xxxxxxxxxxxxxxx>
>>>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>>> Cc: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>>>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
>>>> ---
>>>> 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.
Would it be ok if I changed it to state ? &state->state : NULL and let the compiler deal with it?
Will probably fix up all other patches as well before committing.
>> 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().
>
