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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
