From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Engestrom Subject: Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Date: Mon, 7 Nov 2016 17:12:44 +0000 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-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <8760nzaexm.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jani Nikula Cc: 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 =?utf-8?Q?D=C3=A4nzer?= , linux-ke List-Id: dri-devel@lists.freedesktop.org T24gTW9uZGF5LCAyMDE2LTExLTA3IDEwOjEwOjEzICswMjAwLCBKYW5pIE5pa3VsYSB3cm90ZToK PiBPbiBNb24sIDA3IE5vdiAyMDE2LCBFcmljIEVuZ2VzdHJvbSA8ZXJpY0Blbmdlc3Ryb20uY2g+ IHdyb3RlOgo+ID4gRml4ZXM6IDkwODQ0ZjAwMDQ5ZTlmNDI1NzNmZDMxZDdjMzJlOGZkMzFkM2Zk MDcKPiA+Cj4gPiAgICAgZHJtOiBtYWtlIGRybV9nZXRfZm9ybWF0X25hbWUgdGhyZWFkLXNhZmUK PiA+Cj4gPiAgICAgU2lnbmVkLW9mZi1ieTogRXJpYyBFbmdlc3Ryb20gPGVyaWNAZW5nZXN0cm9t LmNoPgo+ID4gICAgIFtkYW52ZXQ6IENsYXJpZnkgdGhhdCB0aGUgcmV0dXJuZWQgcG9pbnRlciBt dXN0IGJlIGZyZWVkIHdpdGgKPiA+ICAgICBrZnJlZSgpLl0KPiA+ICAgICBTaWduZWQtb2ZmLWJ5 OiBEYW5pZWwgVmV0dGVyIDxkYW5pZWwudmV0dGVyQGZmd2xsLmNoPgo+IAo+IFRoZSBGaXhlczog Zm9ybWF0IGlzOgo+IAo+IEZpeGVzOiA5MDg0NGYwMDA0OWUgKCJkcm06IG1ha2UgZHJtX2dldF9m b3JtYXRfbmFtZSB0aHJlYWQtc2FmZSIpCj4gCj4gQnV0IGlzIHRoaXMgYSBmaXgsIHJlYWxseSwg b3IganVzdCBhbiBpbXByb3ZlbWVudD8gV2hhdCBleGFjdGx5IGlzIHRoZQo+IGJ1ZyBiZWluZyBm aXhlZD8gVGhlIGNvbW1pdCBtZXNzYWdlIGlzIG5vdCBzdWZmaWNpZW50LgoKIlRoZSBmdW5jdGlv bidzIGJlaGF2aW91ciB3YXMgY2hhbmdlZCBpbiA5MDg0NGYwMDA0OWUsIHdpdGhvdXQgY2hhbmdp bmcKaXRzIHNpZ25hdHVyZSwgY2F1c2luZyBwZW9wbGUgdG8ga2VlcCB1c2luZyBpdCB0aGUgb2xk IHdheSB3aXRob3V0CnJlYWxpc2luZyB0aGV5IHdlcmUgbm93IGxlYWtpbmcgbWVtb3J5LgpSb2Ig Q2xhcmsgYWxzbyBub3RpY2VkIGl0IHdhcyBhbHNvIGFsbG9jYXRpbmcgR0ZQX0tFUk5FTCBtZW1v cnkgaW4KYXRvbWljIGNvbnRleHRzLCBicmVha2luZyB0aGVtLgoKSW5zdGVhZCBvZiBoYXZpbmcg dG8gYWxsb2NhdGUgR0ZQX0FUT01JQyBtZW1vcnkgYW5kIGZpeGluZyB0aGUgY2FsbGVycwp0byBt YWtlIHRoZW0gY2xlYW51cCB0aGUgbWVtb3J5IGFmdGVyd2FyZHMsIGxldCdzIGNoYW5nZSB0aGUg ZnVuY3Rpb24ncwpzaWduYXR1cmUgYnkgaGF2aW5nIHRoZSBjYWxsZXIgdGFrZSBjYXJlIG9mIHRo ZSBtZW1vcnkgYW5kIHBhc3NpbmcgaXQgdG8KdGhlIGZ1bmN0aW9uLgpUaGUgbmV3IHBhcmFtZXRl ciBpcyBhIHNpbmdsZS1maWVsZCBzdHJ1Y3QgaW4gb3JkZXIgdG8gZW5mb3JjZSB0aGUgc2l6ZQpv ZiBpdHMgYnVmZmVyIGFuZCBoZWxwIGNhbGxlcnMgdG8gY29ycmVjdGx5IG1hbmFnZSB0aGVpciBt ZW1vcnkuIgoKRG9lcyB0aGlzIHNvdW5kIGdvb2Q/Cgo+ID4gQEAgLTU0LDYgKzYyLDYgQEAgaW50 IGRybV9mb3JtYXRfaG9yel9jaHJvbWFfc3Vic2FtcGxpbmcodWludDMyX3QgZm9ybWF0KTsKPiA+ ICBpbnQgZHJtX2Zvcm1hdF92ZXJ0X2Nocm9tYV9zdWJzYW1wbGluZyh1aW50MzJfdCBmb3JtYXQp Owo+ID4gIGludCBkcm1fZm9ybWF0X3BsYW5lX3dpZHRoKGludCB3aWR0aCwgdWludDMyX3QgZm9y bWF0LCBpbnQgcGxhbmUpOwo+ID4gIGludCBkcm1fZm9ybWF0X3BsYW5lX2hlaWdodChpbnQgaGVp Z2h0LCB1aW50MzJfdCBmb3JtYXQsIGludCBwbGFuZSk7Cj4gPiAtY2hhciAqZHJtX2dldF9mb3Jt YXRfbmFtZSh1aW50MzJfdCBmb3JtYXQpIF9fbWFsbG9jOwo+ID4gK2NoYXIgKmRybV9nZXRfZm9y bWF0X25hbWUodWludDMyX3QgZm9ybWF0LCBzdHJ1Y3QgZHJtX2Zvcm1hdF9uYW1lX2J1ZiAqYnVm KTsKPiAKPiBJIHdvbmRlciBpZiBpdCB3b3VsZCBiZSBiZXR0ZXIgdG8gbWFrZSB0aGF0IHJldHVy biAiY29uc3QgY2hhciAqIi4gSWYKPiB0aGUgdXNlciByZWFsbHkgd2FudHMgdG8gbG9vayB1bmRl ciB0aGUgaG9vZCwgdGhlcmUncyBidWYtPnN0ci4gKnNocnVnKgoKR29vZCBpZGVhLCBJJ2xsIGRv IHRoYXQgaW4gdjMgd2l0aCB0aGUgcHJvcGVyIGNvbW1pdCBtc2cgYW5kIHRhZ3MuIEl0J2xsCmhh dmUgdG8gd2FpdCBhbm90aGVyIGRheSB0aG91Z2gsIC1FTk9USU1FIGFuZCBhbGwuCgo+IAo+IFdp dGggdGhlIGNvbW1pdCBtZXNzYWdlIGltcHJvdmVkLAo+IAo+IFJldmlld2VkLWJ5OiBKYW5pIE5p a3VsYSA8amFuaS5uaWt1bGFAaW50ZWwuY29tPgoKQ2hlZXJzIDopCiAgRXJpYwpfX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBs aXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVz a3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 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 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