linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ARM: imx: introduce function imx_free_mx3_camera
@ 2014-05-22 17:51 Emil Goode
  2014-05-22 18:10 ` Uwe Kleine-König
  2014-05-22 20:44 ` Dan Carpenter
  0 siblings, 2 replies; 8+ messages in thread
From: Emil Goode @ 2014-05-22 17:51 UTC (permalink / raw)
  To: Uwe Kleine-König, Shawn Guo, Sascha Hauer, Russell King,
	Dan Carpenter, Alexander Shiyan, Marc Kleine-Budde
  Cc: linux-arm-kernel, linux-kernel, kernel-janitors, Emil Goode

We forgot to free pdev->dev.dma_mask on error after
having called the imx_alloc_mx3_camera function.
This patch introduces the imx_free_mx3_camera function
that adds the missing kfree call and is practical for
future usage with imx_alloc_mx3_camera().

Signed-off-by: Emil Goode <emilgoode@gmail.com>
---
 arch/arm/mach-imx/devices-imx31.h             |    2 ++
 arch/arm/mach-imx/devices-imx35.h             |    2 ++
 arch/arm/mach-imx/devices/devices-common.h    |    1 +
 arch/arm/mach-imx/devices/platform-ipu-core.c |   11 +++++++++--
 arch/arm/mach-imx/mach-mx31_3ds.c             |    2 +-
 arch/arm/mach-imx/mach-mx31moboard.c          |    3 +--
 arch/arm/mach-imx/mach-mx35_3ds.c             |    2 +-
 arch/arm/mach-imx/mach-pcm037.c               |    2 +-
 8 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-imx/devices-imx31.h b/arch/arm/mach-imx/devices-imx31.h
index e8d1611..900d3b0 100644
--- a/arch/arm/mach-imx/devices-imx31.h
+++ b/arch/arm/mach-imx/devices-imx31.h
@@ -45,6 +45,8 @@ extern const struct imx_ipu_core_data imx31_ipu_core_data;
 	imx_add_ipu_core(&imx31_ipu_core_data)
 #define imx31_alloc_mx3_camera(pdata)	\
 	imx_alloc_mx3_camera(&imx31_ipu_core_data, pdata)
+#define imx31_free_mx3_camera(pdev)	\
+	imx_free_mx3_camera(pdev)
 #define imx31_add_mx3_sdc_fb(pdata)	\
 	imx_add_mx3_sdc_fb(&imx31_ipu_core_data, pdata)
 
diff --git a/arch/arm/mach-imx/devices-imx35.h b/arch/arm/mach-imx/devices-imx35.h
index 780d824..02d22e8 100644
--- a/arch/arm/mach-imx/devices-imx35.h
+++ b/arch/arm/mach-imx/devices-imx35.h
@@ -53,6 +53,8 @@ extern const struct imx_ipu_core_data imx35_ipu_core_data;
 	imx_add_ipu_core(&imx35_ipu_core_data)
 #define imx35_alloc_mx3_camera(pdata)	\
 	imx_alloc_mx3_camera(&imx35_ipu_core_data, pdata)
+#define imx35_free_mx3_camera(pdev)	\
+	imx_free_mx3_camera(pdev)
 #define imx35_add_mx3_sdc_fb(pdata)	\
 	imx_add_mx3_sdc_fb(&imx35_ipu_core_data, pdata)
 
diff --git a/arch/arm/mach-imx/devices/devices-common.h b/arch/arm/mach-imx/devices/devices-common.h
index 69bafc8..0b3ca11 100644
--- a/arch/arm/mach-imx/devices/devices-common.h
+++ b/arch/arm/mach-imx/devices/devices-common.h
@@ -185,6 +185,7 @@ struct imx_ipu_core_data {
 };
 struct platform_device *__init imx_add_ipu_core(
 		const struct imx_ipu_core_data *data);
+void imx_free_mx3_camera(struct platform_device *pdev);
 struct platform_device *__init imx_alloc_mx3_camera(
 		const struct imx_ipu_core_data *data,
 		const struct mx3_camera_pdata *pdata);
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c
index fc4dd7c..77424c4 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -58,6 +58,14 @@ struct platform_device *__init imx_add_ipu_core(
 			res, ARRAY_SIZE(res), NULL, 0);
 }
 
+void imx_free_mx3_camera(struct platform_device *pdev)
+{
+	if (pdev)
+		kfree(pdev->dev.dma_mask);
+
+	platform_device_put(pdev);
+}
+
 struct platform_device *__init imx_alloc_mx3_camera(
 		const struct imx_ipu_core_data *data,
 		const struct mx3_camera_pdata *pdata)
@@ -96,8 +104,7 @@ struct platform_device *__init imx_alloc_mx3_camera(
 		ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
 		if (ret) {
 err:
-			kfree(pdev->dev.dma_mask);
-			platform_device_put(pdev);
+			imx_free_mx3_camera(pdev);
 			return ERR_PTR(-ENODEV);
 		}
 		copied_pdata = dev_get_platdata(&pdev->dev);
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c b/arch/arm/mach-imx/mach-mx31_3ds.c
index 453f41a..f8e6158 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -203,7 +203,7 @@ static int __init mx31_3ds_init_camera(void)
 	ret = platform_device_add(pdev);
 	if (ret)
 err:
-		platform_device_put(pdev);
+		imx31_free_mx3_camera(pdev);
 
 	return ret;
 }
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c b/arch/arm/mach-imx/mach-mx31moboard.c
index 6bed570..25228e3 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -496,10 +496,9 @@ static int __init mx31moboard_init_cam(void)
 	ret = platform_device_add(pdev);
 	if (ret)
 err:
-		platform_device_put(pdev);
+		imx31_free_mx3_camera(pdev);
 
 	return ret;
-
 }
 
 static void mx31moboard_poweroff(void)
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c b/arch/arm/mach-imx/mach-mx35_3ds.c
index 72cd77d..77b230e 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -286,7 +286,7 @@ static int __init imx35_3ds_init_camera(void)
 	ret = platform_device_add(pdev);
 	if (ret)
 err:
-		platform_device_put(pdev);
+		imx35_free_mx3_camera(pdev);
 
 	return ret;
 }
diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index 8eb1570..a7ba7b9 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -429,7 +429,7 @@ static int __init pcm037_init_camera(void)
 	ret = platform_device_add(pdev);
 	if (ret)
 err:
-		platform_device_put(pdev);
+		imx31_free_mx3_camera(pdev);
 
 	return ret;
 }
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
  2014-05-22 17:51 [PATCH] ARM: imx: introduce function imx_free_mx3_camera Emil Goode
@ 2014-05-22 18:10 ` Uwe Kleine-König
  2014-05-22 18:58   ` Emil Goode
                     ` (2 more replies)
  2014-05-22 20:44 ` Dan Carpenter
  1 sibling, 3 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2014-05-22 18:10 UTC (permalink / raw)
  To: Emil Goode
  Cc: Shawn Guo, Sascha Hauer, Russell King, Dan Carpenter,
	Alexander Shiyan, Marc Kleine-Budde, linux-arm-kernel,
	linux-kernel, kernel-janitors

Hello Emil,

On Thu, May 22, 2014 at 07:51:19PM +0200, Emil Goode wrote:
> We forgot to free pdev->dev.dma_mask on error after
> having called the imx_alloc_mx3_camera function.
> This patch introduces the imx_free_mx3_camera function
> that adds the missing kfree call and is practical for
> future usage with imx_alloc_mx3_camera().
> 
> Signed-off-by: Emil Goode <emilgoode@gmail.com>
> ---
>  arch/arm/mach-imx/devices-imx31.h             |    2 ++
>  arch/arm/mach-imx/devices-imx35.h             |    2 ++
>  arch/arm/mach-imx/devices/devices-common.h    |    1 +
>  arch/arm/mach-imx/devices/platform-ipu-core.c |   11 +++++++++--
>  arch/arm/mach-imx/mach-mx31_3ds.c             |    2 +-
>  arch/arm/mach-imx/mach-mx31moboard.c          |    3 +--
>  arch/arm/mach-imx/mach-mx35_3ds.c             |    2 +-
>  arch/arm/mach-imx/mach-pcm037.c               |    2 +-
>  8 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/devices-imx31.h b/arch/arm/mach-imx/devices-imx31.h
> index e8d1611..900d3b0 100644
> --- a/arch/arm/mach-imx/devices-imx31.h
> +++ b/arch/arm/mach-imx/devices-imx31.h
> @@ -45,6 +45,8 @@ extern const struct imx_ipu_core_data imx31_ipu_core_data;
>  	imx_add_ipu_core(&imx31_ipu_core_data)
>  #define imx31_alloc_mx3_camera(pdata)	\
>  	imx_alloc_mx3_camera(&imx31_ipu_core_data, pdata)
> +#define imx31_free_mx3_camera(pdev)	\
> +	imx_free_mx3_camera(pdev)
I wouldn't make this a globally visible function. Today all imx machines
should get their devices from an oftree, so the various functions to add
devices started to bitrot. Moreover there is no reason to remove a
device once it was successfully added.

Note that platform_device_register_full has the same problem (i.e.
pdev->dev.dma_mask isn't freed when the last reference to a device is
dropped.) You'd do a better deed if you picked up
http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995
instead of fixing dead code. But feel free to choose yourself where you
want to patch.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
  2014-05-22 18:10 ` Uwe Kleine-König
@ 2014-05-22 18:58   ` Emil Goode
  2014-05-23  6:50   ` Philippe Rétornaz
  2014-05-24 15:22   ` Emil Goode
  2 siblings, 0 replies; 8+ messages in thread
From: Emil Goode @ 2014-05-22 18:58 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Shawn Guo, Sascha Hauer, Russell King, Dan Carpenter,
	Alexander Shiyan, Marc Kleine-Budde, linux-arm-kernel,
	linux-kernel, kernel-janitors

Hello Uwe,

On Thu, May 22, 2014 at 08:10:24PM +0200, Uwe Kleine-König wrote:
> Hello Emil,
> 
> On Thu, May 22, 2014 at 07:51:19PM +0200, Emil Goode wrote:
> > We forgot to free pdev->dev.dma_mask on error after
> > having called the imx_alloc_mx3_camera function.
> > This patch introduces the imx_free_mx3_camera function
> > that adds the missing kfree call and is practical for
> > future usage with imx_alloc_mx3_camera().
> > 
> > Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > ---
> >  arch/arm/mach-imx/devices-imx31.h             |    2 ++
> >  arch/arm/mach-imx/devices-imx35.h             |    2 ++
> >  arch/arm/mach-imx/devices/devices-common.h    |    1 +
> >  arch/arm/mach-imx/devices/platform-ipu-core.c |   11 +++++++++--
> >  arch/arm/mach-imx/mach-mx31_3ds.c             |    2 +-
> >  arch/arm/mach-imx/mach-mx31moboard.c          |    3 +--
> >  arch/arm/mach-imx/mach-mx35_3ds.c             |    2 +-
> >  arch/arm/mach-imx/mach-pcm037.c               |    2 +-
> >  8 files changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/devices-imx31.h b/arch/arm/mach-imx/devices-imx31.h
> > index e8d1611..900d3b0 100644
> > --- a/arch/arm/mach-imx/devices-imx31.h
> > +++ b/arch/arm/mach-imx/devices-imx31.h
> > @@ -45,6 +45,8 @@ extern const struct imx_ipu_core_data imx31_ipu_core_data;
> >  	imx_add_ipu_core(&imx31_ipu_core_data)
> >  #define imx31_alloc_mx3_camera(pdata)	\
> >  	imx_alloc_mx3_camera(&imx31_ipu_core_data, pdata)
> > +#define imx31_free_mx3_camera(pdev)	\
> > +	imx_free_mx3_camera(pdev)
> I wouldn't make this a globally visible function. Today all imx machines
> should get their devices from an oftree, so the various functions to add
> devices started to bitrot. Moreover there is no reason to remove a
> device once it was successfully added.

Ok I see. In mx31_3ds_init_camera() there are two checks that could fail
before the device is added though.

> Note that platform_device_register_full has the same problem (i.e.
> pdev->dev.dma_mask isn't freed when the last reference to a device is
> dropped.) You'd do a better deed if you picked up
> http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995
> instead of fixing dead code. But feel free to choose yourself where you
> want to patch.

Thank you for the hint, I was about to leave this code alone and move on
but couldn't resist one more patch :) Yes I realized that leaking dma_mask
is a general problem, I will look into that thread, thanks.

Best regards,

Emil Goode

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
  2014-05-22 17:51 [PATCH] ARM: imx: introduce function imx_free_mx3_camera Emil Goode
  2014-05-22 18:10 ` Uwe Kleine-König
@ 2014-05-22 20:44 ` Dan Carpenter
  1 sibling, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2014-05-22 20:44 UTC (permalink / raw)
  To: Emil Goode
  Cc: Uwe Kleine-König, Shawn Guo, Sascha Hauer, Russell King,
	Alexander Shiyan, Marc Kleine-Budde, linux-arm-kernel,
	linux-kernel, kernel-janitors

On Thu, May 22, 2014 at 07:51:19PM +0200, Emil Goode wrote:
> We forgot to free pdev->dev.dma_mask on error after
> having called the imx_alloc_mx3_camera function.
> This patch introduces the imx_free_mx3_camera function
> that adds the missing kfree call and is practical for
> future usage with imx_alloc_mx3_camera().
> 
> Signed-off-by: Emil Goode <emilgoode@gmail.com>

Fantastic.  Thanks for this.  :)

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
  2014-05-22 18:10 ` Uwe Kleine-König
  2014-05-22 18:58   ` Emil Goode
@ 2014-05-23  6:50   ` Philippe Rétornaz
  2014-05-24 15:22   ` Emil Goode
  2 siblings, 0 replies; 8+ messages in thread
From: Philippe Rétornaz @ 2014-05-23  6:50 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Emil Goode, linux-arm-kernel, Russell King, Alexander Shiyan,
	kernel-janitors, linux-kernel, Marc Kleine-Budde, Shawn Guo,
	Sascha Hauer, Dan Carpenter

Hi Uwe

Le 22/05/2014 20:10, Uwe Kleine-König a écrit :
[...]
> Today all imx machines should get their devices from an oftree, so
> the various functions to add devices started to bitrot. Moreover
> there is no reason to remove a device once it was successfully
> added.
>
> Note that platform_device_register_full has the same problem (i.e.
> pdev->dev.dma_mask isn't freed when the last reference to a device
> is dropped.) You'd do a better deed if you picked up
> http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995
> instead of fixing dead code.

I would like to object that all imx machine should now boot from devicetree.
mx31moboard is still used and still get its kernel updated.
It has never booted from devicetree, and IIRC the "deal" when devicetree
was enforced on ARM was that all previous boards where still supported
and would not get removed.
So please don't kick us out of the kernel so fast :)

Thanks !

Philippe

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
  2014-05-22 18:10 ` Uwe Kleine-König
  2014-05-22 18:58   ` Emil Goode
  2014-05-23  6:50   ` Philippe Rétornaz
@ 2014-05-24 15:22   ` Emil Goode
  2014-05-24 20:05     ` Greg Kroah-Hartman
  2 siblings, 1 reply; 8+ messages in thread
From: Emil Goode @ 2014-05-24 15:22 UTC (permalink / raw)
  To: Uwe Kleine-König, Greg Kroah-Hartman
  Cc: Shawn Guo, Sascha Hauer, Russell King, Dan Carpenter,
	Alexander Shiyan, Marc Kleine-Budde, linux-arm-kernel,
	linux-kernel, kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

Hello Uwe and Greg,

> You'd do a better deed if you picked up
> http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995

Since there is nothing wrong with the last version of the patch in
the above thread, I feel strange about picking it up and just splitting
it into two patches. However it would be good to have it applied.

I think the reason why the author didn't resend is that the object file
and data structure layout information in the changelog depend on the
changes to both .name and .dma_mask and by splitting the patch this
information would not apply to any one of the resulting two patches.

Perhaps keeping this information in the changelog is a good reason for
applying the patch as it is?

(I have attached the patch in question)

Best regards,

Emil Goode

[-- Attachment #2: 0001-driver-core-platform-don-t-leak-memory-allocated-for.patch --]
[-- Type: text/x-diff, Size: 9958 bytes --]

>From 66b72cb8eb71974903dd40ed67a34412faf818f0 Mon Sep 17 00:00:00 2001
From: Yann Droneaud <ydroneaud@opteya.com>
Date: Mon, 27 Jan 2014 11:05:52 +0100
Subject: [PATCH] driver core/platform: don't leak memory allocated for
 dma_mask
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

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>
---
 drivers/base/platform.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 5b47210..d19769e 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -162,7 +162,8 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
 
 struct platform_object {
 	struct platform_device pdev;
-	char name[1];
+	u64 dma_mask;
+	char name[];
 };
 
 /**
@@ -198,16 +199,20 @@ 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)
 {
 	struct platform_object *pa;
 
-	pa = kzalloc(sizeof(struct platform_object) + strlen(name), GFP_KERNEL);
+	pa = kzalloc(sizeof(*pa) + strlen(name) + 1, GFP_KERNEL);
 	if (pa) {
 		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);
@@ -441,16 +446,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;
 	}
@@ -469,7 +467,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);
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
  2014-05-24 15:22   ` Emil Goode
@ 2014-05-24 20:05     ` Greg Kroah-Hartman
  2014-05-24 21:17       ` Uwe Kleine-König
  0 siblings, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2014-05-24 20:05 UTC (permalink / raw)
  To: Emil Goode
  Cc: Uwe Kleine-König, Shawn Guo, Sascha Hauer, Russell King,
	Dan Carpenter, Alexander Shiyan, Marc Kleine-Budde,
	linux-arm-kernel, linux-kernel, kernel-janitors

On Sat, May 24, 2014 at 05:22:00PM +0200, Emil Goode wrote:
> Hello Uwe and Greg,
> 
> > You'd do a better deed if you picked up
> > http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995
> 
> Since there is nothing wrong with the last version of the patch in
> the above thread, I feel strange about picking it up and just splitting
> it into two patches. However it would be good to have it applied.
> 
> I think the reason why the author didn't resend is that the object file
> and data structure layout information in the changelog depend on the
> changes to both .name and .dma_mask and by splitting the patch this
> information would not apply to any one of the resulting two patches.
> 
> Perhaps keeping this information in the changelog is a good reason for
> applying the patch as it is?

If you read the thread, I explained why I didn't want to take the patch
as-is.  Please feel free to break it up as asked for and I'll be glad to
consider it then.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
  2014-05-24 20:05     ` Greg Kroah-Hartman
@ 2014-05-24 21:17       ` Uwe Kleine-König
  0 siblings, 0 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2014-05-24 21:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Emil Goode, linux-arm-kernel, Russell King, Alexander Shiyan,
	kernel-janitors, linux-kernel, Marc Kleine-Budde, Shawn Guo,
	Sascha Hauer, Dan Carpenter

Hello,

On Sat, May 24, 2014 at 01:05:00PM -0700, Greg Kroah-Hartman wrote:
> Please feel free to break it up as asked for and I'll be glad to
> consider it then.
ack

Uwe

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-05-24 21:18 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-22 17:51 [PATCH] ARM: imx: introduce function imx_free_mx3_camera Emil Goode
2014-05-22 18:10 ` Uwe Kleine-König
2014-05-22 18:58   ` Emil Goode
2014-05-23  6:50   ` Philippe Rétornaz
2014-05-24 15:22   ` Emil Goode
2014-05-24 20:05     ` Greg Kroah-Hartman
2014-05-24 21:17       ` Uwe Kleine-König
2014-05-22 20:44 ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).