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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 07, 2016 at 07:38:25PM +0200, Jani Nikula wrote:
> On Mon, 07 Nov 2016, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> > On Monday, 2016-11-07 10:10:13 +0200, Jani Nikula wrote:
> >> On Mon, 07 Nov 2016, Eric Engestrom <eric@engestrom.ch> wrote:
> >> > Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
> >> >
> >> >     drm: make drm_get_format_name thread-safe
> >> >
> >> >     Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> >> >     [danvet: Clarify that the returned pointer must be freed with
> >> >     kfree().]
> >> >     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> 
> >> 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. ;)

Can you pls resend with that and corrected Fixes and all the acks
collected?
-Daniel

> 
> 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@intel.com>
> >
> > Cheers :)
> >   Eric
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch