Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933239AbcKHKQI (ORCPT ); Tue, 8 Nov 2016 05:16:08 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35799 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932562AbcKHKQD (ORCPT ); Tue, 8 Nov 2016 05:16:03 -0500 Date: Tue, 8 Nov 2016 11:15:58 +0100 From: Daniel Vetter To: Jani Nikula Cc: Eric Engestrom , David Airlie , dri-devel@lists.freedesktop.org, Wei Yongjun , Daniel Vetter , Flora Cui , Gustavo Padovan , Tom St Denis , Chunming Zhou , Thomas Hellstrom , Laurent Pinchart , Sinclair Yeh , Xinliang Liu , Xinwei Kong , VMware Graphics , Vitaly Prosyak , Alexandre Demers , intel-gfx@lists.freedesktop.org, Eric Engestrom , Emily Deng , Junwei Zhang , Michel =?iso-8859-1?Q?D=E4nzer?= , linux-kernel@vger.kernel.org, Alex Deucher , Colin Ian King , Christian =?iso-8859-1?Q?K=F6nig?= Subject: Re: [Intel-gfx] [PATCH v2] drm: move allocation out of drm_get_format_name() Message-ID: <20161108101558.ihvrprbbdqjwu5wg@phenom.ffwll.local> Mail-Followup-To: Jani Nikula , Eric Engestrom , David Airlie , dri-devel@lists.freedesktop.org, Wei Yongjun , Daniel Vetter , Flora Cui , Gustavo Padovan , Tom St Denis , Chunming Zhou , Thomas Hellstrom , Laurent Pinchart , Sinclair Yeh , Xinliang Liu , Xinwei Kong , VMware Graphics , Vitaly Prosyak , Alexandre Demers , intel-gfx@lists.freedesktop.org, Eric Engestrom , Emily Deng , Junwei Zhang , Michel =?iso-8859-1?Q?D=E4nzer?= , linux-kernel@vger.kernel.org, Alex Deucher , Colin Ian King , Christian =?iso-8859-1?Q?K=F6nig?= References: <20161107004713.GA26032@engestrom.ch> <20161107004821.25369-1-eric@engestrom.ch> <8760nzaexm.fsf@intel.com> <20161107171244.GK25290@imgtec.com> <87eg2n8a26.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87eg2n8a26.fsf@intel.com> X-Operating-System: Linux phenom 4.6.0-1-amd64 User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2922 Lines: 78 On Mon, Nov 07, 2016 at 07:38:25PM +0200, Jani Nikula wrote: > On Mon, 07 Nov 2016, Eric Engestrom wrote: > > On Monday, 2016-11-07 10:10:13 +0200, Jani Nikula wrote: > >> On Mon, 07 Nov 2016, Eric Engestrom wrote: > >> > Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07 > >> > > >> > drm: make drm_get_format_name thread-safe > >> > > >> > Signed-off-by: Eric Engestrom > >> > [danvet: Clarify that the returned pointer must be freed with > >> > kfree().] > >> > Signed-off-by: Daniel Vetter > >> > >> 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 > > > > 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