Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1517029

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2] drm: move allocation out of drm_get_format_name() Eric Engestrom <eric@engestrom.ch> - 2016-11-07 01:50 +0100
  Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Jani Nikula <jani.nikula@linux.intel.com> - 2016-11-07 09:20 +0100
    Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Eric Engestrom <eric.engestrom@imgtec.com> - 2016-11-07 18:20 +0100
      Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Jani Nikula <jani.nikula@linux.intel.com> - 2016-11-07 18:40 +0100
        Re: [Intel-gfx] [PATCH v2] drm: move allocation out of  drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-08 11:20 +0100
          [PATCH v3] drm: move allocation out of drm_get_format_name() Eric Engestrom <eric@engestrom.ch> - 2016-11-09 01:20 +0100
            Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-09 02:10 +0100
              Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-09 02:20 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Eric Engestrom <eric.engestrom@imgtec.com> - 2016-11-09 12:50 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-09 14:20 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Eric Engestrom <eric.engestrom@imgtec.com> - 2016-11-09 18:00 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-10 11:10 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Jani Nikula <jani.nikula@intel.com> - 2016-11-10 11:40 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-10 12:00 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Jani Nikula <jani.nikula@intel.com> - 2016-11-10 12:10 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-11 10:30 +0100
  Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Rob Clark <robdclark@gmail.com> - 2016-11-07 15:50 +0100

csiph-web