Re: Using statically allocated memory for platform_data.

[Antonio Ospite <ospite@studenti.unina.it>]

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

On Mon, 2 Nov 2009 11:23:16 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:

> Hi,
> 
> I noted that in some mfd drivers (drivers/mfd/ezx-pcap.c and
> drivers/mfd/da903x.c) there is code like this:
> 
> static int __devinit pcap_add_subdev(struct pcap_chip *pcap,
>                                      struct pcap_subdev *subdev)
> {
>         struct platform_device *pdev;
> 
>         pdev = platform_device_alloc(subdev->name, subdev->id);
>         pdev->dev.parent = &pcap->spi->dev;
>         pdev->dev.platform_data = subdev->platform_data;
> 
>         return platform_device_add(pdev);
> }
> 
> Note the _direct_assignment_ of platform data; then in board init code
> there are often global struct pointers passed as subdev platform data,
> see arch/arm/mach-pxa/em-x270.c::em_x270_da9030_subdevs for instance.
> 
> In these cases, whenever the subdev platform device is unregistered,
> the call to platform_device_release() tries to kfree the platform data,
> and being it statically allocated memory this triggers a bug from SLAB:
> 	kernel BUG at mm/slab.c:521!
> In my case this prevented proper device poweroff.
> 
> The question: should these mfd drivers use platform_device_add_data()
> which allocates dynamic memory for *a copy* of platform data? Is this
> simple solution acceptable even if there will be more memory used?
> Or should we setup platform_data in dynamic memory from the beginning
> in board init code? (which would be way less pretty IMHO).
> 

Just for reference, in our local repository I am _temporarily_ working
around the bug with a change like the following, even if I am leaking
some memory. Any comments?

diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
index df405af..e4e999b 100644
--- a/drivers/mfd/ezx-pcap.c
+++ b/drivers/mfd/ezx-pcap.c
@@ -379,12 +379,17 @@ static int pcap_remove_subdev(struct device *dev, void *unused)
        return 0;
 }

+static void pcap_subdev_release(struct device *dev)
+{
+}
+
 static int __devinit pcap_add_subdev(struct pcap_chip *pcap,
                                                struct pcap_subdev *subdev)
 {
        struct platform_device *pdev;

        pdev = platform_device_alloc(subdev->name, subdev->id);
+       pdev->dev.release = pcap_subdev_release;
        pdev->dev.parent = &pcap->spi->dev;
        pdev->dev.platform_data = subdev->platform_data;

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]