Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932717AbcKGRMy (ORCPT ); Mon, 7 Nov 2016 12:12:54 -0500 Received: from mailapp01.imgtec.com ([195.59.15.196]:36354 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932411AbcKGRMw (ORCPT ); Mon, 7 Nov 2016 12:12:52 -0500 Date: Mon, 7 Nov 2016 17:12:44 +0000 From: Eric Engestrom To: Jani Nikula CC: Eric Engestrom , , Ville =?utf-8?B?U3lyasOkbMOk?= , Rob Clark , Christian =?utf-8?B?S8O2bmln?= , Alex Deucher , David Airlie , Xinliang Liu , Daniel Vetter , VMware Graphics , Sinclair Yeh , Thomas Hellstrom , Tom St Denis , Michel =?utf-8?Q?D=C3=A4nzer?= , Gustavo Padovan , Emily Deng , Chunming Zhou , Flora Cui , Vitaly Prosyak , Colin Ian King , Ken Wang , Alexandre Demers , Junwei Zhang , Xinwei Kong , Wei Yongjun , Chris Wilson , Laurent Pinchart , , Subject: Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Message-ID: <20161107171244.GK25290@imgtec.com> References: <20161107004713.GA26032@engestrom.ch> <20161107004821.25369-1-eric@engestrom.ch> <8760nzaexm.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <8760nzaexm.fsf@intel.com> User-Agent: Mutt/1.7.1 (2016-10-04) X-Originating-IP: [10.60.4.28] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2156 Lines: 53 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? > > @@ -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