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

From: Eric Engestrom <eric.engestrom@imgtec.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: "David Airlie" <airlied@linux.ie>,
	dri-devel@lists.freedesktop.org,
	"Wei Yongjun" <yongjun_wei@trendmicro.com.cn>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Flora Cui" <Flora.Cui@amd.com>,
	"Gustavo Padovan" <gustavo.padovan@collabora.co.uk>,
	"Tom St Denis" <tom.stdenis@amd.com>,
	"Chunming Zhou" <David1.Zhou@amd.com>,
	"Thomas Hellstrom" <thellstrom@vmware.com>,
	"Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
	"Sinclair Yeh" <syeh@vmware.com>,
	"Xinliang Liu" <z.liuxinliang@hisilicon.com>,
	"Xinwei Kong" <kong.kongxinwei@hisilicon.com>,
	"VMware Graphics" <linux-graphics-maintainer@vmware.com>,
	"Vitaly Prosyak" <vitaly.prosyak@amd.com>,
	"Alexandre Demers" <alexandre.f.demers@gmail.com>,
	intel-gfx@lists.freedesktop.org,
	"Eric Engestrom" <eric@engestrom.ch>,
	"Emily Deng" <Emily.Deng@amd.com>,
	"Junwei Zhang" <Jerry.Zhang@amd.com>,
	"Michel Dänzer" <michel.daenzer@amd.com>
Subject: Re: [PATCH v2] drm: move allocation out of drm_get_format_name()
Date: Mon, 7 Nov 2016 17:12:44 +0000	[thread overview]
Message-ID: <20161107171244.GK25290@imgtec.com> (raw)
In-Reply-To: <8760nzaexm.fsf@intel.com>

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?

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