| Message ID | 1401122483-31603-1-git-send-email-emilgoode@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote: > @@ -211,6 +215,7 @@ struct platform_device *platform_device_alloc(const char *name, int id) > strcpy(pa->name, name); > pa->pdev.name = pa->name; > pa->pdev.id = id; > + pa->pdev.dev.dma_mask = &pa->dma_mask; There is code in the kernel which, rightly or wrongly, checks whether dev->dma_mask is NULL to determine whether the device can do any kind of DMA. The above results in devices allocated via this interface always having this member set, which is a change of core kernel behaviour. How sure are you that this will not break anything?
On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote: > From: Yann Droneaud <ydroneaud@opteya.com> > > 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. > > To address this issue, this patch adds dma_mask to struct platform_object > and when using platform_device_alloc() or platform_device_register_full() > the .dma_mask pointer of struct device is assigned the address of this new > dma_mask member of struct platform_object. And since it's within struct > platform_object, dma_mask won't be leaked anymore when struct platform_device > get released. > > No more memory leak, no small allocation and a slight reduction in code > size. > > Built on Debian jessie, using GCC 4.8, for ARM and x86_64 architectures, > the size of the object file and the data structure layout are updated > as follows, 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_3.15-rc6_ARM > ++ size+pahole_3.15-rc6_ARM+ > @@ -2,7 +2,7 @@ > text data bss dec hex filename > - 6530 1008 8 7546 1d7a drivers/base/platform.o > + 6482 1008 8 7498 1d4a drivers/base/platform.o > > @@ -93,8 +93,13 @@ struct platform_object { > /* --- cacheline 12 boundary (768 bytes) was 40 bytes ago --- */ > char name[1]; /* 808 1 */ > > - /* size: 816, cachelines: 13, members: 2 */ > - /* padding: 7 */ > + /* XXX 7 bytes hole, try to pack */ > > + u64 dma_mask; /* 816 8 */ > > + /* size: 824, cachelines: 13, members: 3 */ > + /* sum members: 817, holes: 1, sum holes: 7 */ > /* paddings: 1, sum paddings: 4 */ > - /* last cacheline: 48 bytes */ > + /* last cacheline: 56 bytes */ > }; > > -- size+pahole_3.15-rc6_x86_64 > ++ size+pahole_3.15-rc6_x86_64+ > @@ -2,7 +2,7 @@ > text data bss dec hex filename > - 8570 5032 3424 17026 4282 drivers/base/platform.o > + 8509 5032 3408 16949 4235 drivers/base/platform.o > > @@ -95,7 +95,11 @@ struct platform_object { > /* --- cacheline 22 boundary (1408 bytes) was 32 bytes ago --- */ > char name[1]; /* 1440 1 */ > > - /* size: 1448, cachelines: 23, members: 2 */ > - /* padding: 7 */ > - /* last cacheline: 40 bytes */ > + /* XXX 7 bytes hole, try to pack */ > > + u64 dma_mask; /* 1448 8 */ > > + /* size: 1456, cachelines: 23, members: 3 */ > + /* sum members: 1449, holes: 1, sum holes: 7 */ > + /* last cacheline: 48 bytes */ > }; > > Changes from v4 [1]: > - Split v4 of the patch into two separate patches. > - Generated new object file size and data structure layout info. > - Updated the changelog message. > > Changes from v3 [2]: > - fixed commit message so that git am doesn't fail. > > Changes from v2 [3]: > - 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 [4]: > - remove unneeded kfree() from error path > - add reference to author/commit adding allocation of dmamask > > Changes from v0 [5]: > - small rewrite to squeeze the patch to a bare minimal > > [1] http://lkml.kernel.org/r/1390817152-30898-1-git-send-email-ydroneaud@opteya.com > https://patchwork.kernel.org/patch/3541871/ > > [2] http://lkml.kernel.org/r/1390771138-28348-1-git-send-email-ydroneaud@opteya.com > https://patchwork.kernel.org/patch/3540081/ > > [3] http://lkml.kernel.org/r/1389683909-17495-1-git-send-email-ydroneaud@opteya.com > https://patchwork.kernel.org/patch/3484411/ > > [4] http://lkml.kernel.org/r/1389649085-7365-1-git-send-email-ydroneaud@opteya.com > https://patchwork.kernel.org/patch/3480961/ > > [5] http://lkml.kernel.org/r/1386886207-2735-1-git-send-email-ydroneaud@opteya.com > > Cc: Yann Droneaud <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> > [Emil: split v4 of the patch in two and updated changelog] > Signed-off-by: Emil Goode <emilgoode@gmail.com> > --- > Hello Greg, > > The first two patches in the series are created from v4 of the > original patch, since I have not changed how the code works I think > it is correct to keep the original author and Signed-off-by line. > > Best regards, > > Emil Goode > > drivers/base/platform.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 9e9227e..dd1fa07 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -166,6 +166,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices); > struct platform_object { > struct platform_device pdev; > char name[1]; > + u64 dma_mask; This is broken. .name is used as flexible array, so .name and dma_mask overlap. Uwe
Hello Russell, On Mon, May 26, 2014 at 05:51:05PM +0100, Russell King - ARM Linux wrote: > On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote: > > @@ -211,6 +215,7 @@ struct platform_device *platform_device_alloc(const char *name, int id) > > strcpy(pa->name, name); > > pa->pdev.name = pa->name; > > pa->pdev.id = id; > > + pa->pdev.dev.dma_mask = &pa->dma_mask; > > There is code in the kernel which, rightly or wrongly, checks whether > dev->dma_mask is NULL to determine whether the device can do any kind > of DMA. The above results in devices allocated via this interface > always having this member set, which is a change of core kernel behaviour. > > How sure are you that this will not break anything? Thank you for pointing this out, considering the number of calls made to platform_device_alloc it would be easy to miss an occurrance of this problem. I would say that the risk heavily outweighs the gain and it's better to not apply this series. Best regards, Emil Goode
Hi Emil, Le lundi 26 mai 2014 à 18:41 +0200, Emil Goode a écrit : > The first two patches in the series are created from v4 of the > original patch, since I have not changed how the code works I think > it is correct to keep the original author and Signed-off-by line. > > Best regards, Thanks for the update. I wasn't interested in splitting the patch in two separate chunks, thinking that shrinking the size of the structure then increasing it of roughly the same amount was not the best way to sell the changes :) Unfortunately, as noted by Uwe, you not only split the patch but also broke it ;) If we're going to split the patch, it should be split in: 1) replace name[1] by name[] (or name[0]) to remove the implicit padding from platform_device structure 2) add dma_mask to platform_object structure and use it to initialize dev.dma_mask. Anyway, as Russel explained in another mail, unconditionally set dev.dma_mask pointer is probably going to break, so this part (2) need some rework, I'm gonna try to do. Thanks for reminding me about this patch. Regards.
On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote: > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 9e9227e..dd1fa07 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -166,6 +166,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices); > struct platform_object { > struct platform_device pdev; > char name[1]; > + u64 dma_mask; > }; Heh. No this doesn't work as patch #1. You have to have name at the end of the struct. regards, dan carpenter
Hello, On Mon, May 26, 2014 at 10:30:46PM +0300, Dan Carpenter wrote: > On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote: > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > index 9e9227e..dd1fa07 100644 > > --- a/drivers/base/platform.c > > +++ b/drivers/base/platform.c > > @@ -166,6 +166,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices); > > struct platform_object { > > struct platform_device pdev; > > char name[1]; > > + u64 dma_mask; > > }; > > Heh. No this doesn't work as patch #1. You have to have name at the > end of the struct. Yes I missed that one, obviously the order is important here. Best regards, Emil Goode
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 9e9227e..dd1fa07 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -166,6 +166,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices); struct platform_object { struct platform_device pdev; char name[1]; + u64 dma_mask; }; /** @@ -201,6 +202,9 @@ static void platform_device_release(struct device *dev) * * Create a platform device object which can have other objects attached * to it, and which will have attached objects freed when it is released. + * + * The associated struct device will be set up so that its dma_mask field + * is a valid pointer to an u64. This pointer must not be free'd manually. */ struct platform_device *platform_device_alloc(const char *name, int id) { @@ -211,6 +215,7 @@ struct platform_device *platform_device_alloc(const char *name, int id) strcpy(pa->name, name); pa->pdev.name = pa->name; pa->pdev.id = id; + pa->pdev.dev.dma_mask = &pa->dma_mask; device_initialize(&pa->pdev.dev); pa->pdev.dev.release = platform_device_release; arch_setup_pdev_archdata(&pa->pdev); @@ -444,16 +449,9 @@ struct platform_device *platform_device_register_full( if (pdevinfo->dma_mask) { /* - * This memory isn't freed when the device is put, - * I don't have a nice idea for that though. Conceptually * dma_mask in struct device should not be a pointer. * See http://thread.gmane.org/gmane.linux.kernel.pci/9081 */ - pdev->dev.dma_mask = - kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); - if (!pdev->dev.dma_mask) - goto err; - *pdev->dev.dma_mask = pdevinfo->dma_mask; pdev->dev.coherent_dma_mask = pdevinfo->dma_mask; } @@ -472,7 +470,6 @@ struct platform_device *platform_device_register_full( if (ret) { err: ACPI_COMPANION_SET(&pdev->dev, NULL); - kfree(pdev->dev.dma_mask); err_alloc: platform_device_put(pdev);