Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752117AbcKFNDu (ORCPT ); Sun, 6 Nov 2016 08:03:50 -0500 Received: from mail-yw0-f195.google.com ([209.85.161.195]:34271 "EHLO mail-yw0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751577AbcKFNDs (ORCPT ); Sun, 6 Nov 2016 08:03:48 -0500 MIME-Version: 1.0 In-Reply-To: <8af873a2-31a9-1146-289d-35d2e48edffa@amd.com> References: <20161105012344.GA28349@engestrom.ch> <20161105013325.3889-1-eric@engestrom.ch> <20161105163844.GA29546@engestrom.ch> <8af873a2-31a9-1146-289d-35d2e48edffa@amd.com> From: Rob Clark Date: Sun, 6 Nov 2016 08:03:47 -0500 Message-ID: Subject: Re: [PATCH] drm: move allocation out of drm_get_format_name() To: =?UTF-8?Q?Christian_K=C3=B6nig?= Cc: Eric Engestrom , "dri-devel@lists.freedesktop.org" , Wei Yongjun , Daniel Vetter , Flora Cui , Gustavo Padovan , Tom St Denis , Thomas Hellstrom , Laurent Pinchart , Xinliang Liu , VMware Graphics , Vitaly Prosyak , Alexandre Demers , Intel Graphics Development , Emily Deng , Ken Wang , Junwei Zhang , =?UTF-8?Q?Michel_D=C3=A4nzer?= , Linux Kernel Mailing List , Alex Deucher , Colin Ian King Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uA6D3srS027493 Content-Length: 3078 Lines: 91 On Sun, Nov 6, 2016 at 4:47 AM, Christian König wrote: > Am 05.11.2016 um 17:49 schrieb Rob Clark: >> >> On Sat, Nov 5, 2016 at 12:38 PM, Eric Engestrom wrote: >>> >>> On Saturday, 2016-11-05 13:11:36 +0100, Christian König wrote: >>>> >>>> Am 05.11.2016 um 02:33 schrieb Eric Engestrom: >>>>> >>>>> +typedef char drm_format_name_buf[32]; >>>> >>>> Please don't use a typedef for this, just define the maximum size of >>>> characters the function might write somewhere. >>>> >>>> See the kernel coding style as well: >>>>> >>>>> In general, a pointer, or a struct that has elements that can >>>>> reasonably >>>>> be directly accessed should **never** be a typedef. >>> >>> I would normally agree as I tend to hate typedefs ($DAYJOB {ab,mis}uses >>> them way too much), and your way was what I wrote at first, but Rob >>> Clark's >>> typedef idea makes it much harder for someone to allocate a buffer of >>> the wrong size, which IMO is good thing here. >> >> IMHO I would make a small test program to verify this actually helps >> the compiler catch problems. And if it does, I would stick with it. >> The coding-style should be guidelines, not something that supersedes >> common sense / practicality. > > > Well completely agree that we should be able to question the coding style > rules, but when we do it we discuss this on a the mailing list first and > then start to use it in code. Not the other way around. if I'm not mistaken, that is what we are doing ;-) >> >> That is my $0.02 anyways.. if others vehemently disagree and want to >> dogmatically stick to the coding-style guidelines, ok then. OTOH, if >> this approach doesn't help the compiler catch issues, then it isn't >> worth it. > > > Yeah, exactly that's the point. If I'm not completely mistaken the compiler > won't issue a warning here if you pass an array with the wrong size. > > I think you need something like "struct drm_format_name_buf { char str[32]; > };" to trigger this. hmm, actually the struct is a nice idea then if the compiler wouldn't catch the wrong-size-array > Apart from that is this function really called so often that using > kasprintf() is a problem here? Or is there another motivation behind the > change? Two things trouble me about the kasprintf approach.. (ignoring the fact that atm it is not GFP_ATOMIC) 1) you can't do DRM_DEBUG("format: %s\n", drm_get_format_name(..)) so it pulls the memory allocation and sprintf outside of the drm_debug check 2) seems awfully easy to forget the kfree... I wouldn't have even known that now you need to free the result (with some patches I'm working on) if it weren't for the fact that lockdep alerted me to the GFP_KERNEL allocation in atomic ctx ;-) BR, -R > Regards, > Christian. > > >> >> BR, >> -R >> >>> I can rewrite the typedef out if you think it's better. >>> >>> Cheers, >>> Eric >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > >