Clarifying platform_device_unregister - Kernel

This is a discussion on Clarifying platform_device_unregister - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: Clarifying platform_device_unregister

  1. Clarifying platform_device_unregister

    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 : [] lr : [] 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:
    [] (kfree+0x0/0xc4) from []
    (platform_device_release+0x1c/0x30)
    r6:c3db98a0 r5:c0235e6c r4:c3e4bc00
    [] (platform_device_release+0x0/0x30) from []
    (device_release+0x60/0x80)
    r4:c3e4bc70
    [] (device_release+0x0/0x80) from []
    (kobject_cleanup+0x64/0x74)
    [] (kobject_cleanup+0x0/0x74) from []
    (kobject_release+0x14/0x18)
    r6:c3e3ff48 r5:c00ebaf0 r4:c3e4bc74
    [] (kobject_release+0x0/0x18) from [] (kref_put+0x6c/0x80)
    [] (kref_put+0x0/0x80) from [] (kobject_put+0x20/0x28)
    r5:00000000 r4:c3e4bc00
    [] (kobject_put+0x0/0x28) from [] (put_device+0x1c/0x20)
    [] (put_device+0x0/0x20) from []
    (platform_device_put+0x1c/0x20)
    [] (platform_device_put+0x0/0x20) from []
    (platform_device_unregister+0x20/0x24)
    [] (platform_device_unregister+0x0/0x24) from []
    (mytest_exit+0x18/0x20 [am200epd])
    r4:bf0003e0
    [] (mytest_exit+0x0/0x20 [am200epd]) from []
    (sys_delete_module+0x148/0x18c)
    [] (sys_delete_module+0x0/0x18c) from []
    (ret_fast_syscall+0x0/0x2c)
    r7:00000081 r6:be9be4b0 r5:be9be498 r4:be9c0cb0
    Code: e3530909 0590000c e5903000 e2133080 (05833000)
    ---[ end trace eb1de0e70e5a62af ]---
    Segmentation fault
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: Clarifying platform_device_unregister

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: Clarifying platform_device_unregister

    On Mon, Mar 31, 2008 at 10:19 PM, Dmitry Torokhov
    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,
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. 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
    > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: Clarifying platform_device_unregister

    On Tue, Apr 1, 2008 at 10:54 AM, Dmitry Torokhov
    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
    > > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: Clarifying platform_device_unregister


    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


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: Clarifying platform_device_unregister

    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread