Date: Mon, 7 Nov 2016 00:47:13 +0000
From: Eric Engestrom <eric@engestrom.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: Christian =?utf-8?B?S8O2bmln?= <christian.koenig@amd.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        Wei Yongjun <yongjun_wei@trendmicro.com.cn>,
        Daniel Vetter <daniel.vetter@intel.com>, Flora Cui <Flora.Cui@amd.com>,
        Gustavo Padovan <gustavo.padovan@collabora.co.uk>,
        Tom St Denis <tom.stdenis@amd.com>,
        Thomas Hellstrom <thellstrom@vmware.com>,
        Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
        Xinliang Liu <z.liuxinliang@hisilicon.com>,
        VMware Graphics <linux-graphics-maintainer@vmware.com>,
        Vitaly Prosyak <vitaly.prosyak@amd.com>,
        Alexandre Demers <alexandre.f.demers@gmail.com>,
        Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
        Emily Deng <Emily.Deng@amd.com>, Ken Wang <Qingqing.Wang@amd.com>,
        Junwei Zhang <Jerry.Zhang@amd.com>,
        Michel =?utf-8?Q?D=C3=A4nzer?= <michel.daenzer@amd.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alex Deucher <alexander.deucher@amd.com>,
        Colin Ian King <colin.king@canonical.com>
Subject: Re: [PATCH] drm: move allocation out of drm_get_format_name()
Message-ID: <20161107004713.GA26032@engestrom.ch>
References: <20161105012344.GA28349@engestrom.ch>
 <20161105013325.3889-1-eric@engestrom.ch>
 <cc7acbea-a721-22b4-67d3-22198decced6@amd.com>
 <20161105163844.GA29546@engestrom.ch>
 <CAF6AEGuEB82fjZ_YF61MAAqU=8ahXuMo06MN0_ZuWY5fkyHnoQ@mail.gmail.com>
 <8af873a2-31a9-1146-289d-35d2e48edffa@amd.com>
 <CAF6AEGt5Mz1S5h+pth2bz820ZirsXcEt0X7k5Z4P0q180ASg3A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAF6AEGt5Mz1S5h+pth2bz820ZirsXcEt0X7k5Z4P0q180ASg3A@mail.gmail.com>
User-Agent: Mutt/1.7.1 (2016-10-04)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3147
Lines: 81

On Sunday, 2016-11-06 08:03:47 -0500, Rob Clark wrote:
> On Sun, Nov 6, 2016 at 4:47 AM, Christian König
> <christian.koenig@amd.com> wrote:
> > Am 05.11.2016 um 17:49 schrieb Rob Clark:
> >>
> >> On Sat, Nov 5, 2016 at 12:38 PM, Eric Engestrom <eric@engestrom.ch> 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

Sending the patch in a minute.

> 
> > 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 actually found a couple of these memory leaks while doing this patch,
look for files where i don't remove kfree :)
(eg. vmwgfx at the end of the patch)

> 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