Clarifying platform_device_unregister

Clarifying platform_device_unregister

[Jaya Kumar]

Hi,

I'm trying to figure out a problem I'm experiencing with
platform_device_unregister. Here's what I'm seeing with a piece of
test code based on corgi_pm.c:

static int mydata;
static struct platform_device *mytest_device;
static int __devinit mytest_init(void)
{
        int ret;

        mytest_device = platform_device_alloc("no_such_driver", -1);

// no_such_driver intentionally doesn't exist. i want to test this
mytest module being insmod-ed/rmmod-ed without ever being bound to a
platform driver.

        if (!mytest_device)
                return -ENOMEM;


        mytest_device->dev.platform_data = &mydata;
        ret = platform_device_add(mytest_device);

        if (ret)
                platform_device_put(mytest_device);

        return ret;
}

static void mytest_exit(void)
{
        platform_device_unregister(mytest_device);
}


# insmod mytest.ko
# rmmod mytest

When I do that, I see a panic, appended below. I was wondering if this
is due to a mistake in my understanding of how the platform code is
intended to be used. The other related question I have is, when a
platform device uses a platform driver through p_d_alloc or
p_add_devices or _register, who is supposed to manage module
refcounting? Should the platform device code call request_module and
try_module_get to manage the counts for platform drivers that they
use? I assumed that the platform support code should do that, since it
knows when the platform drivers get bound to the platform devices.

Thanks,
jaya

Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = c3dcc000
[00000000] *pgd=a3e8c031, *pte=00000000, *ppte=00000000
Internal error: Oops: 817 [#1] PREEMPT
Modules linked in: mytest(-)
CPU: 0    Not tainted  (2.6.25-rc6gum-00000-gd14ba55-dirty #23)
PC is at kfree+0x6c/0xc4
LR is at platform_device_release+0x1c/0x30
pc : [<c00722e0>]    lr : [<c0123ad0>]    psr: 40000093
sp : c3e3fe60  ip : c3e3fe80  fp : c3e3fe7c
r10: 00000000  r9 : c3e3e000  r8 : c001ac84
r7 : 00000880  r6 : bf000540  r5 : a0000013  r4 : c3e4bc00
r3 : 00000000  r2 : 0009f000  r1 : 00000001  r0 : c0247000
Flags: nZcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0000397f  Table: a3dcc000  DAC: 00000015
Process rmmod (pid: 346, stack limit = 0xc3e3e268)
Stack: (0xc3e3fe60 to 0xc3e40000)
fe60: c3c0f338 c3e4bc00 c0235e6c c3db98a0 c3e3fe94 c3e3fe80 c0123ad0 c0072280
fe80: c00b8e50 c3e4bc70 c3e3fea4 c3e3fe98 c011e8ac c0123ac0 c3e3fec4 c3e3fea8
fea0: c00ebae0 c011e858 c3e4bc70 c3e4bc74 c00ebaf0 c3e3ff48 c3e3fed4 c3e3fec8
fec0: c00ebb04 c00eba88 c3e3feec c3e3fed8 c00ec96c c00ebafc c3e4bc00 00000000
fee0: c3e3fefc c3e3fef0 c00eb994 c00ec90c c3e3ff0c c3e3ff00 c011ea50 c00eb980
ff00: c3e3ff1c c3e3ff10 c0123b00 c011ea40 c3e3ff34 c3e3ff20 c0123b84 c0123af0
ff20: c004ff08 bf0003e0 c3e3ff44 c3e3ff38 bf000018 c0123b70 c3e3ffa4 c3e3ff48
ff40: c00535a0 bf00000c 30326d61 64706530 c0077600 c00771c0 c3e3ff84 c3e3ff68
ff60: c0074188 c0077658 0000000a c3e3e000 c3d9c180 c3c27120 c3e3ffa4 c3e3ff88
ff80: 00075728 00000000 be9c0cb0 be9be498 be9be4b0 00000081 00000000 c3e3ffa8
ffa0: c001ab00 c0053464 be9c0cb0 be9be498 be9be498 00000880 d6d6f735 00000000
ffc0: be9c0cb0 be9be498 be9be4b0 00000081 00000880 00000000 00000000 be9c0d24
ffe0: 40018a4c be9be488 00008e6c 40018a58 20000010 be9be498 00000000 00000000
Backtrace:
[<c0072274>] (kfree+0x0/0xc4) from [<c0123ad0>]
(platform_device_release+0x1c/0x30)
 r6:c3db98a0 r5:c0235e6c r4:c3e4bc00
[<c0123ab4>] (platform_device_release+0x0/0x30) from [<c011e8ac>]
(device_release+0x60/0x80)
 r4:c3e4bc70
[<c011e84c>] (device_release+0x0/0x80) from [<c00ebae0>]
(kobject_cleanup+0x64/0x74)
[<c00eba7c>] (kobject_cleanup+0x0/0x74) from [<c00ebb04>]
(kobject_release+0x14/0x18)
 r6:c3e3ff48 r5:c00ebaf0 r4:c3e4bc74
[<c00ebaf0>] (kobject_release+0x0/0x18) from [<c00ec96c>] (kref_put+0x6c/0x80)
[<c00ec900>] (kref_put+0x0/0x80) from [<c00eb994>] (kobject_put+0x20/0x28)
 r5:00000000 r4:c3e4bc00
[<c00eb974>] (kobject_put+0x0/0x28) from [<c011ea50>] (put_device+0x1c/0x20)
[<c011ea34>] (put_device+0x0/0x20) from [<c0123b00>]
(platform_device_put+0x1c/0x20)
[<c0123ae4>] (platform_device_put+0x0/0x20) from [<c0123b84>]
(platform_device_unregister+0x20/0x24)
[<c0123b64>] (platform_device_unregister+0x0/0x24) from [<bf000018>]
(mytest_exit+0x18/0x20 [am200epd])
 r4:bf0003e0
[<bf000000>] (mytest_exit+0x0/0x20 [am200epd]) from [<c00535a0>]
(sys_delete_module+0x148/0x18c)
[<c0053458>] (sys_delete_module+0x0/0x18c) from [<c001ab00>]
(ret_fast_syscall+0x0/0x2c)
 r7:00000081 r6:be9be4b0 r5:be9be498 r4:be9c0cb0
Code: e3530909 0590000c e5903000 e2133080 (05833000)
---[ end trace eb1de0e70e5a62af ]---
Segmentation fault

Re: Clarifying platform_device_unregister

[Dmitry Torokhov]

Hi Jaya,

On Mon, Mar 31, 2008 at 09:14:35PM -0400, Jaya Kumar wrote:
> Hi,
> 
> I'm trying to figure out a problem I'm experiencing with
> platform_device_unregister. Here's what I'm seeing with a piece of
> test code based on corgi_pm.c:
> 
> static int mydata;
> static struct platform_device *mytest_device;
> static int __devinit mytest_init(void)
> {
>         int ret;
> 
>         mytest_device = platform_device_alloc("no_such_driver", -1);
> 
> // no_such_driver intentionally doesn't exist. i want to test this
> mytest module being insmod-ed/rmmod-ed without ever being bound to a
> platform driver.
> 
>         if (!mytest_device)
>                 return -ENOMEM;
> 
> 
>         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));


>         ret = platform_device_add(mytest_device);
> 
>         if (ret)
>                 platform_device_put(mytest_device);
> 
>         return ret;
> }
> 
> static void mytest_exit(void)
> {
>         platform_device_unregister(mytest_device);
> }
> 
> 

Hope this helps.

-- 
Dmitry

Re: Clarifying platform_device_unregister

[Jaya Kumar]

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.

corgi.c:                .platform_data  = &corgi_scoop_setup,
corgi.c:                .platform_data  = &corgi_bl_machinfo,
corgi.c:                .platform_data  = &corgi_ts_machinfo,
corgi_lcd.c:            .platform_data = &corgi_fb_info,
corgi_pm.c:     corgipm_device->dev.platform_data = &corgi_pm_machinfo;
generic.c:              .platform_data  = &pxa_udc_info,
lpd270.c:                       .platform_data  = &lpd270_flash_data[0],
lpd270.c:                       .platform_data  = &lpd270_flash_data[1],
lubbock.c:              .platform_data  = &pxa_ssp_master_info,
lubbock.c:      .platform_data  = &ads_info,
lubbock.c:                      .platform_data = &lubbock_flash_data[0],
lubbock.c:                      .platform_data = &lubbock_flash_data[1],
mainstone.c:    .dev            = { .platform_data = &mst_audio_ops },
mainstone.c:                    .platform_data = &mst_flash_data[0],
mainstone.c:                    .platform_data = &mst_flash_data[1],
poodle.c:               .platform_data  = &poodle_scoop_setup,
poodle.c:               .platform_data  = &poodle_ts_machinfo,
spitz.c:                .platform_data  = &spitz_scoop_setup,
spitz.c:                .platform_data  = &spitz_scoop2_setup,
spitz.c:                .platform_data  = &spitz_bl_machinfo,
spitz.c:                .platform_data  = &spitz_ts_machinfo,
spitz_pm.c:     spitzpm_device->dev.platform_data = &spitz_pm_machinfo;
tosa.c:                 .platform_data  = &tosa_scoop_setup,
tosa.c:                 .platform_data  = &tosa_scoop_jc_setup,
trizeps4.c:             .platform_data = &trizeps4_flash_data,

Re: Clarifying platform_device_unregister

[Dmitry Torokhov]

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

Re: Clarifying platform_device_unregister

[Jaya Kumar]

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

Re: Clarifying platform_device_unregister

[Richard Purdie]

On Tue, 2008-04-01 at 10:54 -0400, Dmitry Torokhov wrote:
> On Tue, Apr 01, 2008 at 12:47:54AM -0700, Jaya Kumar wrote:
> > 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?

Looking at this I agree there are some problems there.

In the real world the spitz/corgi devices are pretty useless without
power management and therefore these code paths aren't well used. I
think this has happened since the platform data was a later addition to
the code and the internal use of kfree on platform_data wasn't realised.

I'll make sure some patches are submitted to address these issues.

Cheers,

Richard

Re: Clarifying platform_device_unregister

[Richard Purdie]

On Tue, 2008-04-01 at 21:57 -0400, Jaya Kumar wrote:
> 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?

I don't think any refcount is needed since corgi/spitz_pm refer to
functions in sharpsl-pm and therefore sharpsl-pm will always be around
as long as corgi/spitz_pm is.

Regards,

Richard