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  ...
[ message continues ]

Hi Jaya,


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:


Hope this helps.

-- 
Dmitry
--

[ message continues ]

On Mon, Mar 31, 2008 at 10:19 PM, Dmitry Torokhov

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 ...
[ message continues ]

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

[ message continues ]

On Tue, Apr 1, 2008 at 10:54 AM, Dmitry Torokhov

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

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

[ message continues ]

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

--

[ message continues ]

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


--

[ message continues ]