Re: [PATCH v2] drm: move allocation out of drm_get_format_name()

[Jani Nikula]

On Mon, 07 Nov 2016, Eric Engestrom <eric.engestrom@xxxxxxxxxx> wrote:
> On Monday, 2016-11-07 10:10:13 +0200, Jani Nikula wrote:
>> On Mon, 07 Nov 2016, Eric Engestrom <eric@xxxxxxxxxxxx> wrote:
>> > Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
>> >
>> >     drm: make drm_get_format_name thread-safe
>> >
>> >     Signed-off-by: Eric Engestrom <eric@xxxxxxxxxxxx>
>> >     [danvet: Clarify that the returned pointer must be freed with
>> >     kfree().]
>> >     Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> 
>> The Fixes: format is:
>> 
>> Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
>> 
>> But is this a fix, really, or just an improvement? What exactly is the
>> bug being fixed? The commit message is not sufficient.
>
> "The function's behaviour was changed in 90844f00049e, without changing
> its signature, causing people to keep using it the old way without
> realising they were now leaking memory.
> Rob Clark also noticed it was also allocating GFP_KERNEL memory in
> atomic contexts, breaking them.
>
> Instead of having to allocate GFP_ATOMIC memory and fixing the callers
> to make them cleanup the memory afterwards, let's change the function's
> signature by having the caller take care of the memory and passing it to
> the function.
> The new parameter is a single-field struct in order to enforce the size
> of its buffer and help callers to correctly manage their memory."
>
> Does this sound good?

It's fine; no need to go overboard. ;)

BR,
Jani.

>
>> > @@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>> >  int drm_format_vert_chroma_subsampling(uint32_t format);
>> >  int drm_format_plane_width(int width, uint32_t format, int plane);
>> >  int drm_format_plane_height(int height, uint32_t format, int plane);
>> > -char *drm_get_format_name(uint32_t format) __malloc;
>> > +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>> 
>> I wonder if it would be better to make that return "const char *". If
>> the user really wants to look under the hood, there's buf->str. *shrug*
>
> Good idea, I'll do that in v3 with the proper commit msg and tags. It'll
> have to wait another day though, -ENOTIME and all.
>
>> 
>> With the commit message improved,
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> Cheers :)
>   Eric

-- 
Jani Nikula, Intel Open Source Technology Center