Re: [PATCHv4] driver core/platform: don't leak memory allocated for dma_mask

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Fri, Feb 07, 2014 at 03:20:05PM -0800, Greg Kroah-Hartman wrote:
> On Mon, Jan 27, 2014 at 11:05:52AM +0100, Yann Droneaud wrote:
> > Since commit 01dcc60a7cb8, platform_device_register_full() is
> > available to allocate and register a platform device.
> > 
> > If a dma_mask is provided as part of platform_device_info,
> > platform_device_register_full() allocate memory for a u64 using
> > kmalloc().
> > 
> > A comment in the code state that "[t]his memory isn't freed when the
> > device is put".
> > 
> > It's never a good thing to leak memory, but there's only very few
> > users of platform_device_info's dma_mask, and those are mostly
> > "static" devices that are not going to be plugged/unplugged often.
> > 
> > So memory leak is not really an issue, but allocating 8 bytes through
> > kmalloc() seems overkill.
> > 
> > And it's a pity to have to allocate 8 bytes for the dma_mask while up
> > to 7 bytes are wasted at the end of struct platform_object in the form
> > of padding after name field: unfortunately this padding is not used
> > when allocating the memory to hold the name.
> >
> > To address theses issues, this patch adds dma_mask to platform_object
> > struct so that it is always allocated for all struct platform_device
> > allocated through platform_device_alloc() or platform_device_register_full().
> > And since it's within struct platform_object, dma_mask won't be leaked
> > anymore when struct platform_device got released. Storage for dma_mask
> > is added almost for free by removing the padding at the end of struct
> > platform_object.
How does the padding of name change? The only thing that I see changing
for .name is that it's a char[] now instead of a char[1]. As it was
used as a flexible array already before the padding (which only applies
to a stand alone struct platform_object) doesn't matter.
I guess that is a tool problem that still some padding changes are
reported?

Other than that the patch looks good.

Uwe

> > Padding at the end of the structure is removed by making name a C99
> > flexible array member (eg. a zero sized array). To handle this change,
> > memory allocation is updated to take care of allocating an additional
> > byte for the NUL terminating character.
> > 
> > No more memory leak, no small allocation, no byte wasted and
> > a slight reduction in code size.
> > 
> > Built on Fedora 20, using GCC 4.8, for ARM, i386, x86_64 and PPC64
> > architectures, the size of the object file and the data structure layout
> > are updated as follow, produced with commands:
> > 
> >   $ size drivers/base/platform.o
> >   $ pahole drivers/base/platform.o \
> >            --recursive             \
> >            --class_name device,pdev_archdata,platform_device,platform_object
> > 
> >   --- size+pahole.next-20140124
> >   +++ size+pahole.next-20140124+
> >   @@ -1,5 +1,5 @@
> >       text    data     bss     dec     hex filename
> >   -   5616     472      32    6120    17e8 obj-arm/drivers/base/platform.o
> >   +   5572     472      32    6076    17bc obj-arm/drivers/base/platform.o
> >    struct device {
> >            struct device *            parent;               /*     0     4 */
> >            struct device_private *    p;                    /*     4     4 */
> >   @@ -77,15 +77,15 @@ struct platform_object {
> >            /* XXX last struct has 4 bytes of padding */
> > 
> >            /* --- cacheline 6 boundary (384 bytes) --- */
> >   -        char                       name[1];              /*   384     1 */
> >   +        u64                        dma_mask;             /*   384     8 */
> >   +        char                       name[0];              /*   392     0 */
> > 
> >   -        /* size: 392, cachelines: 7, members: 2 */
> >   -        /* padding: 7 */
> >   +        /* size: 392, cachelines: 7, members: 3 */
> >            /* paddings: 1, sum paddings: 4 */
> >            /* last cacheline: 8 bytes */
> >    };
> > 
> >       text    data     bss     dec     hex filename
> >   -   6007     532      32    6571    19ab obj-i386/drivers/base/platform.o
> >   +   5943     532      32    6507    196b obj-i386/drivers/base/platform.o
> >    struct device {
> >            struct device *            parent;               /*     0     4 */
> >            struct device_private *    p;                    /*     4     4 */
> >   @@ -161,14 +161,14 @@ struct platform_device {
> >    struct platform_object {
> >            struct platform_device     pdev;                 /*     0   392 */
> >            /* --- cacheline 6 boundary (384 bytes) was 8 bytes ago --- */
> >   -        char                       name[1];              /*   392     1 */
> >   +        u64                        dma_mask;             /*   392     8 */
> >   +        char                       name[0];              /*   400     0 */
> > 
> >   -        /* size: 396, cachelines: 7, members: 2 */
> >   -        /* padding: 3 */
> >   -        /* last cacheline: 12 bytes */
> >   +        /* size: 400, cachelines: 7, members: 3 */
> >   +        /* last cacheline: 16 bytes */
> >    };
> > 
> >       text    data     bss     dec     hex filename
> >   -   8851    2112      48   11011    2b03 obj-ppc64/drivers/base/platform.o
> >   +   8787    2104      48   10939    2abb obj-ppc64/drivers/base/platform.o
> >    struct device {
> >            struct device *            parent;               /*     0     8 */
> >            struct device_private *    p;                    /*     8     8 */
> >   @@ -256,14 +256,14 @@ struct platform_device {
> >    struct platform_object {
> >            struct platform_device     pdev;                 /*     0   728 */
> >            /* --- cacheline 11 boundary (704 bytes) was 24 bytes ago --- */
> >   -        char                       name[1];              /*   728     1 */
> >   +        u64                        dma_mask;             /*   728     8 */
> >   +        char                       name[0];              /*   736     0 */
> > 
> >   -        /* size: 736, cachelines: 12, members: 2 */
> >   -        /* padding: 7 */
> >   +        /* size: 736, cachelines: 12, members: 3 */
> >            /* last cacheline: 32 bytes */
> >    };
> > 
> >       text    data     bss     dec     hex filename
> >   -   7100     960      48    8108    1fac obj-x86_64/drivers/base/platform.o
> >   +   7020     960      48    8028    1f5c obj-x86_64/drivers/base/platform.o
> >    struct device {
> >            struct device *            parent;               /*     0     8 */
> >            struct device_private *    p;                    /*     8     8 */
> >   @@ -350,9 +350,9 @@ struct platform_device {
> >    struct platform_object {
> >            struct platform_device     pdev;                 /*     0   712 */
> >            /* --- cacheline 11 boundary (704 bytes) was 8 bytes ago --- */
> >   -        char                       name[1];              /*   712     1 */
> >   +        u64                        dma_mask;             /*   712     8 */
> >   +        char                       name[0];              /*   720     0 */
> > 
> >   -        /* size: 720, cachelines: 12, members: 2 */
> >   -        /* padding: 7 */
> >   +        /* size: 720, cachelines: 12, members: 3 */
> >            /* last cacheline: 16 bytes */
> >    };
> > 
> > Changes from v3 [1]:
> > - fixed commit message so that git am doesn't fail.
> > 
> > Changes from v2 [2]:
> > - move 'dma_mask' to platform_object so that it's always
> >   allocated and won't leak on release; remove all previously
> >   added support functions.
> > - use C99 flexible array member for 'name' to remove padding
> >   at the end of platform_object.
> > 
> > Changes from v1 [3]:
> > - remove unneeded kfree() from error path
> > - add reference to author/commit adding allocation of dmamask
> > 
> > Changes from v0 [4]:
> > - small rewrite to squeeze the patch to a bare minimal
> > 
> > [1] http://lkml.kernel.org/r/1390771138-28348-1-git-send-email-ydroneaud@opteya.com
> >     https://patchwork.kernel.org/patch/3540081/
> > 
> > [2] http://lkml.kernel.org/r/1389683909-17495-1-git-send-email-ydroneaud@opteya.com
> >     https://patchwork.kernel.org/patch/3484411/
> > 
> > [3] http://lkml.kernel.org/r/1389649085-7365-1-git-send-email-ydroneaud@opteya.com
> >     https://patchwork.kernel.org/patch/3480961/
> > 
> > [4] http://lkml.kernel.org/r/1386886207-2735-1-git-send-email-ydroneaud@opteya.com
> > 
> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> > ---
> > Hi,
> > 
> > I've fixed the commit message to move the diff on pahole
> > outside of the scope of git am.
> 
> Uwe, can I get your review of this?
> 
> thanks,
> 
> greg k-h
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |