Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933148AbcKGRir (ORCPT ); Mon, 7 Nov 2016 12:38:47 -0500 Received: from mga14.intel.com ([192.55.52.115]:46995 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932544AbcKGRin (ORCPT ); Mon, 7 Nov 2016 12:38:43 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,606,1473145200"; d="scan'208";a="783505860" From: Jani Nikula To: Eric Engestrom Cc: Eric Engestrom , linux-kernel@vger.kernel.org, Ville =?utf-8?B?U3lyasOkbMOk?= , Rob Clark , Christian =?utf-8?Q?K=C3=B6nig?= , 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 , dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org Subject: Re: [PATCH v2] drm: move allocation out of drm_get_format_name() In-Reply-To: <20161107171244.GK25290@imgtec.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20161107004713.GA26032@engestrom.ch> <20161107004821.25369-1-eric@engestrom.ch> <8760nzaexm.fsf@intel.com> <20161107171244.GK25290@imgtec.com> Date: Mon, 07 Nov 2016 19:38:25 +0200 Message-ID: <87eg2n8a26.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2403 Lines: 63 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. ;) 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