Re: Clarifying platform_device_unregister

["Jaya Kumar" <jayakumar.lkml@gmail.com>]

On Tue, Apr 1, 2008 at 10:54 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> 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?

Sorry, I was unclear. I agree that corgi_pm and spitz_pm are suspect
because they are unloadable. The others that I listed such as lpd270,
lubbock, and mainstone are machine definitions (is that the right term
for me to use?) and can't be unloaded.

>
>  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?

I also have a followup. Does corgi/spitz_pm need to manage the module
refcount of sharpsl-pm? I couldn't find any platform device code that
manages the refcount of the platform driver that it binds them to. So
I suspect this means that platform devices must do the try_module_get
stuff themselves. Out of curiosity, what's the reason for not doing
this inside the base/platform.c code?

Thanks,
jaya