Messages in this thread | | | Date | Tue, 1 Apr 2008 10:54:54 -0400 | From | Dmitry Torokhov <> | Subject | Re: Clarifying platform_device_unregister |
| |
On Tue, Apr 01, 2008 at 12:47:54AM -0700, Jaya Kumar wrote: > On Mon, Mar 31, 2008 at 10:19 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Mon, Mar 31, 2008 at 09:14:35PM -0400, Jaya Kumar wrote: > > > mytest_device->dev.platform_data = &mydata; > > > > Platform device code does kfree(pdev->dev.platform_data) unpon > > unregistration, so it is not a good idea to assign address of > > statically-allocated variable here. You should be using: > > > > platform_device_add_data(mytest_device, &mydata, sizeof(mydata)); > > > > That's interesting. I noticed though that a lot of platform device > code assigns a statically allocated structure to platform_data. For > example: > > arch/arm/mach-pxa/corgi_pm.c > static struct sharpsl_charger_machinfo corgi_pm_machinfo = { > ... > } > corgipm_device->dev.platform_data = &corgi_pm_machinfo; > > same with spitz_pm.c. > > egrep "platform_data.*=.*\&" *.c shows quite a lot of users doing > that. I guess most of these below are probably okay since these > drivers can't be rmmoded. >
Hmm, are you sure they can't be removed? Why do they all have module_exit methods?
Even if they can't be unloaded the whole thing will blow to pieces if registration fails. Consider this:
static int __devinit spitzpm_init(void) { int ret;
spitzpm_device = platform_device_alloc("sharpsl-pm", -1); if (!spitzpm_device) return -ENOMEM;
spitzpm_device->dev.platform_data = &spitz_pm_machinfo; ret = platform_device_add(spitzpm_device);
if (ret) platform_device_put(spitzpm_device); ^^^^^^^^^^^ This will try to kfree(spitzpm_device->dev.platform_data) and it gonna blow. We need to do spitzpm_device->dev.platform_data = NULL before doing put.
Also spitzpm_init() shoudl be marked __init, not __devinit and spitzpm_exit() should be __exit() if it is event needed at all.
Richard, I think you work with spitz and corgi, any comments?
-- Dmitry
| |