Messages in this thread | | | Date | Sat, 15 Feb 2014 11:39:56 -0800 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCHv4] driver core/platform: don't leak memory allocated for dma_mask |
| |
On Sun, Feb 09, 2014 at 10:30:32AM +0100, Uwe Kleine-König wrote: > Hello Yann, > > On Sun, Feb 09, 2014 at 08:47:22AM +0100, Yann Droneaud wrote: > > Hi, > > > > Le samedi 08 février 2014 à 16:09 +0100, Uwe Kleine-König a écrit : > > > 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? > > > > > > > When name is defined as char name[1] at the end of the structure, the > > compiler is required to add padding after it (since the structure is not > > "packed" through some compiler extension). > > This padding is added in order to have a size multiple of the highest > > required alignement for types insided the structure. This is required so > > that each item of an array of "struct platform_object" are aligned. > > > > Changing name[1] to name[0] or name[] free the compiler from adding > > storage space for name and thus remove the need for padding after it. > Ah, I see, even though .name was used as a flexible array there was too > much allocated because sizeof(struct platform_object) includes the 7 > bytes of padding. > > Maybe it makes sense to split the patch into a) s/name[1]/name[]/ and b) > add dma_mask? Up to you, you can have my Ack also for the single patch. > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Breaking it up would be good, if for no other reason than people got confused with it as-is.
thanks,
greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |