From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Engestrom Subject: Re: [PATCH] drm: make drm_get_format_name thread-safe Date: Mon, 15 Aug 2016 13:59:28 +0100 Message-ID: <20160815125928.GC10429@imgtec.com> References: <20160815000247.20063-1-eric@engestrom.ch> <87eg5quzly.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: <87eg5quzly.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: Tom St Denis , intel-gfx@lists.freedesktop.org, Eric Engestrom , Michel =?utf-8?Q?D=C3=A4nzer?= , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Xinliang Liu , David Zhang , Wei Yongjun , Vitaly Prosyak , Daniel Vetter , Junwei Zhang , Alex Deucher , Flora Cui , Gustavo Padovan , Christian =?utf-8?B?S8O2bmln?= List-Id: dri-devel@lists.freedesktop.org T24gTW9uLCBBdWcgMTUsIDIwMTYgYXQgMTI6NTQ6MDFQTSArMDMwMCwgSmFuaSBOaWt1bGEgd3Jv dGU6Cj4gT24gTW9uLCAxNSBBdWcgMjAxNiwgRXJpYyBFbmdlc3Ryb20gPGVyaWNAZW5nZXN0cm9t LmNoPiB3cm90ZToKPiA+IFNpZ25lZC1vZmYtYnk6IEVyaWMgRW5nZXN0cm9tIDxlcmljQGVuZ2Vz dHJvbS5jaD4KPiA+IC0tLQo+ID4KPiA+IEkgbW92ZWQgdGhlIG1haW4gYml0cyB0byBiZSB0aGUg Zmlyc3QgZGlmZnMsIHNob3VsZG4ndCBhZmZlY3QgYW55dGhpbmcKPiA+IHdoZW4gYXBwbHlpbmcg dGhlIHBhdGNoLCBidXQgSSB3YW50ZWQgdG8gYXNrOgo+ID4gSSBkb24ndCBsaWtlIHRoZSBoYXJk LWNvZGVkIGAzMmAgdGhlIGFwcGVhcnMgaW4gYm90aCBrbWFsbG9jKCkgYW5kCj4gPiBzbnByaW50 ZigpLCB3aGF0IGRvIHlvdSB0aGluaz8gSWYgeW91IGRvbid0IGxpa2UgaXQgZWl0aGVyLCB3aGF0 IHdvdWxkCj4gPiB5b3Ugc3VnZ2VzdD8gU2hvdWxkIEkgI2RlZmluZSBpdD8KPiA+Cj4gPiBTZWNv bmQgcXVlc3Rpb24gaXMgYWJvdXQgdGhlIHBhdGNoIG1haWwgaXRzZWxmOiBzaG91bGQgSSBzZW5k IHRoaXMga2luZAo+ID4gb2YgcGF0Y2ggc2VwYXJhdGVkIGJ5IG1vZHVsZSwgd2l0aCBhIG5vdGUg cmVxdWVzdGluZyB0aGVtIHRvIGJlIHNxdWFzaGVkCj4gPiB3aGVuIGFwcGx5aW5nPyBJdCBoYXMg dG8gbGFuZCBhcyBhIHNpbmdsZSBwYXRjaCwgYnV0IGZvciByZXZpZXcgaXQgbWlnaHQKPiA+IGJl IGVhc2llciBpZiBwZW9wbGUgb25seSBzZWUgdGhlIGJpdHMgdGhleSBlYWNoIGNhcmUgYWJvdXQs IGFzIHdlbGwgYXMKPiA+IHRvIGNvbGxlY3QgYWNrJ3Mvci1iJ3MuCj4gPgo+ID4gQ2hlZXJzLAo+ ID4gICBFcmljCj4gPgo+ID4gLS0tCj4gPiAgZHJpdmVycy9ncHUvZHJtL2FtZC9hbWRncHUvZGNl X3YxMF8wLmMgICAgICAgICAgfCAgNiArKy0tCj4gPiAgZHJpdmVycy9ncHUvZHJtL2FtZC9hbWRn cHUvZGNlX3YxMV8wLmMgICAgICAgICAgfCAgNiArKy0tCj4gPiAgZHJpdmVycy9ncHUvZHJtL2Ft ZC9hbWRncHUvZGNlX3Y4XzAuYyAgICAgICAgICAgfCAgNiArKy0tCj4gPiAgZHJpdmVycy9ncHUv ZHJtL2RybV9hdG9taWMuYyAgICAgICAgICAgICAgICAgICAgfCAgNSArKy0tCj4gPiAgZHJpdmVy cy9ncHUvZHJtL2RybV9jcnRjLmMgICAgICAgICAgICAgICAgICAgICAgfCAyMSArKysrKysrKy0t LS0tCj4gPiAgZHJpdmVycy9ncHUvZHJtL2RybV9mb3VyY2MuYyAgICAgICAgICAgICAgICAgICAg fCAxNyArKysrKystLS0tLQo+ID4gIGRyaXZlcnMvZ3B1L2RybS9oaXNpbGljb24va2lyaW4va2ly aW5fZHJtX2FkZS5jIHwgIDYgKystLQo+ID4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZGVi dWdmcy5jICAgICAgICAgICAgIHwgMTEgKysrKysrLQo+ID4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1 L2ludGVsX2F0b21pY19wbGFuZS5jICAgICAgIHwgIDYgKystLQo+ID4gIGRyaXZlcnMvZ3B1L2Ry bS9pOTE1L2ludGVsX2Rpc3BsYXkuYyAgICAgICAgICAgIHwgMzkgKysrKysrKysrKysrKysrKy0t LS0tLS0tLQo+ID4gIGRyaXZlcnMvZ3B1L2RybS9yYWRlb24vYXRvbWJpb3NfY3J0Yy5jICAgICAg ICAgIHwgMTIgKysrKystLS0KPiA+ICBpbmNsdWRlL2RybS9kcm1fZm91cmNjLmggICAgICAgICAg ICAgICAgICAgICAgICB8ICAyICstCj4gPiAgMTIgZmlsZXMgY2hhbmdlZCwgODkgaW5zZXJ0aW9u cygrKSwgNDggZGVsZXRpb25zKC0pCj4gPgo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2Ry bS9kcm1fZm91cmNjLmMgYi9kcml2ZXJzL2dwdS9kcm0vZHJtX2ZvdXJjYy5jCj4gPiBpbmRleCAw NjQ1Yzg1Li4zODIxNmExIDEwMDY0NAo+ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2RybV9mb3Vy Y2MuYwo+ID4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2RybV9mb3VyY2MuYwo+ID4gQEAgLTM5LDE2 ICszOSwxNCBAQCBzdGF0aWMgY2hhciBwcmludGFibGVfY2hhcihpbnQgYykKPiA+ICAgKiBkcm1f Z2V0X2Zvcm1hdF9uYW1lIC0gcmV0dXJuIGEgc3RyaW5nIGZvciBkcm0gZm91cmNjIGZvcm1hdAo+ ID4gICAqIEBmb3JtYXQ6IGZvcm1hdCB0byBjb21wdXRlIG5hbWUgb2YKPiA+ICAgKgo+ID4gLSAq IE5vdGUgdGhhdCB0aGUgYnVmZmVyIHVzZWQgYnkgdGhpcyBmdW5jdGlvbiBpcyBnbG9iYWxseSBz aGFyZWQgYW5kIG93bmVkIGJ5Cj4gPiAtICogdGhlIGZ1bmN0aW9uIGl0c2VsZi4KPiA+IC0gKgo+ ID4gLSAqIEZJWE1FOiBUaGlzIGlzbid0IHJlYWxseSBtdWx0aXRocmVhZGluZyBzYWZlLgo+ID4g KyAqIE5vdGUgdGhhdCB0aGUgYnVmZmVyIHJldHVybmVkIGJ5IHRoaXMgZnVuY3Rpb24gaXMgb3du ZWQgYnkgdGhlIGNhbGxlcgo+ID4gKyAqIGFuZCB3aWxsIG5lZWQgdG8gYmUgZnJlZWQuCj4gPiAg ICovCj4gPiAgY29uc3QgY2hhciAqZHJtX2dldF9mb3JtYXRfbmFtZSh1aW50MzJfdCBmb3JtYXQp Cj4gCj4gSSBmaW5kIGl0IHN1cnByaXNpbmcgdGhhdCBhIGZ1bmN0aW9uIHRoYXQgYWxsb2NhdGVz IGEgYnVmZmVyIHJldHVybnMgYQo+IGNvbnN0IHBvaW50ZXIuIFNvbWUgdXNlcnNwYWNlIGxpYnJh cmllcyBoYXZlIGNvbnZlbnRpb25zIGFib3V0IHRoZQo+IG93bmVyc2hpcCBiYXNlZCBvbiBjb25z dG5lc3MuCj4gCj4gKEkgYWxzbyBmaW5kIGl0IHN1cHJpc2luZyB0aGF0IGtmcmVlKCkgdGFrZXMg YSBjb25zdCBwb2ludGVyOyBhcmd1YWJseQo+IHRoYXQgY2FsbCBjaGFuZ2VzIHRoZSBtZW1vcnku KQo+IAo+IElzIHRoZXJlIHByZWNlZGVudCBmb3IgdGhpcz8KPiAKPiBCUiwKPiBKYW5pLgoKSXQn cyBub3QgYSBjb25zdCBwb2ludGVyLCBpdCdzIGEgbm9ybWFsIHBvaW50ZXIgdG8gYSBjb25zdCBj aGFyLCBpLmUuCnlvdSBjYW4gZG8gYXMgeW91IHdhbnQgd2l0aCB0aGUgcG9pbnRlciBidXQgeW91 IHNob3VsZG4ndCBjaGFuZ2UgdGhlCmNoYXJzIGl0IHBvaW50cyB0by4KCkNoZWVycywKICBFcmlj Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCkludGVsLWdm eCBtYWlsaW5nIGxpc3QKSW50ZWwtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xp c3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2ludGVsLWdmeAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753147AbcHONAF (ORCPT ); Mon, 15 Aug 2016 09:00:05 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:15002 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752796AbcHOM7l (ORCPT ); Mon, 15 Aug 2016 08:59:41 -0400 Date: Mon, 15 Aug 2016 13:59:28 +0100 From: Eric Engestrom To: Jani Nikula CC: Eric Engestrom , , Tom St Denis , , Michel =?utf-8?Q?D=C3=A4nzer?= , Wei Yongjun , , Junwei Zhang , Xinliang Liu , David Zhang , Vitaly Prosyak , Alex Deucher , Daniel Vetter , Flora Cui , Gustavo Padovan , Christian =?utf-8?B?S8O2bmln?= Subject: Re: [PATCH] drm: make drm_get_format_name thread-safe Message-ID: <20160815125928.GC10429@imgtec.com> References: <20160815000247.20063-1-eric@engestrom.ch> <87eg5quzly.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <87eg5quzly.fsf@intel.com> User-Agent: Mutt/1.6.2 (2016-07-01) 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 Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote: > On Mon, 15 Aug 2016, Eric Engestrom wrote: > > Signed-off-by: Eric Engestrom > > --- > > > > I moved the main bits to be the first diffs, shouldn't affect anything > > when applying the patch, but I wanted to ask: > > I don't like the hard-coded `32` the appears in both kmalloc() and > > snprintf(), what do you think? If you don't like it either, what would > > you suggest? Should I #define it? > > > > Second question is about the patch mail itself: should I send this kind > > of patch separated by module, with a note requesting them to be squashed > > when applying? It has to land as a single patch, but for review it might > > be easier if people only see the bits they each care about, as well as > > to collect ack's/r-b's. > > > > Cheers, > > Eric > > > > --- > > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++-- > > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++-- > > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++-- > > drivers/gpu/drm/drm_atomic.c | 5 ++-- > > drivers/gpu/drm/drm_crtc.c | 21 ++++++++----- > > drivers/gpu/drm/drm_fourcc.c | 17 ++++++----- > > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++-- > > drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++- > > drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++-- > > drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++--------- > > drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++--- > > include/drm/drm_fourcc.h | 2 +- > > 12 files changed, 89 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > > index 0645c85..38216a1 100644 > > --- a/drivers/gpu/drm/drm_fourcc.c > > +++ b/drivers/gpu/drm/drm_fourcc.c > > @@ -39,16 +39,14 @@ static char printable_char(int c) > > * drm_get_format_name - return a string for drm fourcc format > > * @format: format to compute name of > > * > > - * Note that the buffer used by this function is globally shared and owned by > > - * the function itself. > > - * > > - * FIXME: This isn't really multithreading safe. > > + * Note that the buffer returned by this function is owned by the caller > > + * and will need to be freed. > > */ > > const char *drm_get_format_name(uint32_t format) > > I find it surprising that a function that allocates a buffer returns a > const pointer. Some userspace libraries have conventions about the > ownership based on constness. > > (I also find it suprising that kfree() takes a const pointer; arguably > that call changes the memory.) > > Is there precedent for this? > > BR, > Jani. It's not a const pointer, it's a normal pointer to a const char, i.e. you can do as you want with the pointer but you shouldn't change the chars it points to. Cheers, Eric