Re: [PATCH 3.0-rc2] OMAP: ams-delta: fix broken uevent sysfs entries

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5743 bytes --]

Hi,

On Wed, Jun 15, 2011 at 02:53:01PM +0200, Janusz Krzysztofik wrote:
> > to me it sounds like missing __init/__exit annotations on the driver.
> > See ams-delta-leds for instance:
> > 
> > static int ams_delta_led_probe(struct platform_device *pdev)
> > static int ams_delta_led_remove(struct platform_device *pdev)
> > 
> > which means those drivers will have probe sitting outside or
> > .init.text and trying to reference name which sits in .init.text ???
> > Could that be the case here ?
> 
> Missing or not, addig them didn't help.

ok...

> > But it could also be that the platform_device shouldn't be marked
> > __initdata.
> 
> After either reverting one of commits mentioned, or applying my patch, 
> a read from /sys/devices/platform/ams-delta-led/uevent, for example, 
> returns:
> 
> DRIVER=ams-delta-led
> MODALIAS=platform:ams-delta-led
> 
> The code responsible for returning these strings can be found in 
> drivers/base/core.c:
> 
> static int dev_uevent(struct kset *kset, struct kobject *kobj,
> 		struct kobj_uevent_env *env)
> {
> 	struct device *dev = to_dev(kobj);
> ...
> 	if (dev->driver)
> 		add_uevent_var(env, "DRIVER=%s", dev->driver->name);
> 
> 	/* have the bus specific function add its stuff */
> 	if (dev->bus && dev->bus->uevent) {
>                 retval = dev->bus->uevent(dev, env);
> ...
> 
> The dev->bus->uevent for a platform device happens to sit in 
> drivers/base/platform.c:
> 
> static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
> {
> 	struct platform_device  *pdev = to_platform_device(dev);
> ...
> 	add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
> 		(pdev->id_entry) ? pdev->id_entry->name : pdev->name);
> ...
> 
> For me, it looks like struct kobject, pointed to by kobj, being a member 
> of struct device, which in turn happens to be a member of struct 
> platform_device.
> 
> AFAICU, there exist two sorts of platform devices from memory allocation 
> point of view: those created with platform_device_alloc(), which 
> allocates a memory where struct platform_device is kept, and those 
> created with platfrom_device_add(), which is provided with a pointer to 
> an already allocated platform device structure.
> 
> I can't find any piece of code which makes a copy of a platfrom deivce 
> structure content pointed to while platform_device_add() is called from 
> a board or machine file, either directly or indirectly via 
> platform_device_register() or platform_add_devices().
> Why should it be actually copied after all?.

true, it makes sense to remove __initdata from the platform_device
structures.

> Searching for an example usage of _initdata similiar to that introduced 
> by commit bdc58fb950a3a1b0bc3cbd8e23d21ecdde2ac9a2, "arm: omap1: fix a 
> bunch of section mismatches", I can find the following:
> 
> $ grep -r "struct .* platform_device .* = {" .|grep "__initdata"|grep -v '*'
> ./arch/arm/mach-omap1/board-ams-delta.c:static struct platform_device ams_delta_kp_device __initdata = {
> ./arch/arm/mach-omap1/board-ams-delta.c:static struct platform_device ams_delta_lcd_device __initdata = {
> ./arch/arm/mach-omap1/board-ams-delta.c:static struct platform_device ams_delta_led_device __initdata = {
> ./arch/arm/mach-omap1/board-ams-delta.c:static struct platform_device ams_delta_camera_device __initdata = {
> ./arch/arm/mach-omap1/gpio7xx.c:static struct __initdata platform_device omap7xx_mpu_gpio = {
> ./arch/arm/mach-omap1/gpio7xx.c:static struct __initdata platform_device omap7xx_gpio1 = {
> ./arch/arm/mach-omap1/gpio7xx.c:static struct __initdata platform_device omap7xx_gpio2 = {
> ./arch/arm/mach-omap1/gpio7xx.c:static struct __initdata platform_device omap7xx_gpio3 = {
> ./arch/arm/mach-omap1/gpio7xx.c:static struct __initdata platform_device omap7xx_gpio4 = {
> ./arch/arm/mach-omap1/gpio7xx.c:static struct __initdata platform_device omap7xx_gpio5 = {
> ./arch/arm/mach-omap1/gpio7xx.c:static struct __initdata platform_device omap7xx_gpio6 = {
> ./arch/arm/mach-omap1/gpio15xx.c:static struct __initdata platform_device omap15xx_mpu_gpio = {
> ./arch/arm/mach-omap1/gpio15xx.c:static struct __initdata platform_device omap15xx_gpio = {
> ./arch/arm/mach-omap1/gpio16xx.c:static struct __initdata platform_device omap16xx_mpu_gpio = {
> ./arch/arm/mach-omap1/gpio16xx.c:static struct __initdata platform_device omap16xx_gpio1 = {
> ./arch/arm/mach-omap1/gpio16xx.c:static struct __initdata platform_device omap16xx_gpio2 = {
> ./arch/arm/mach-omap1/gpio16xx.c:static struct __initdata platform_device omap16xx_gpio3 = {
> ./arch/arm/mach-omap1/gpio16xx.c:static struct __initdata platform_device omap16xx_gpio4 = {
> ./arch/arm/mach-omap2/board-rx51-peripherals.c:static struct platform_device rx51_si4713_dev __initdata_or_module = {
> 
> So, there is no single exact pattern found in the whole tree, and a few 
> instances of similiar patterns of two kinds found only inside omap. If I 
> follow any of the two, either moving '__initdata' in front of 
> 'platform_device' or using '__initdata_or_module' instead, the problem 
> no longer hits me (using my custom defconfig). However, the former seems 
> not conformant to what one can learn from include/linux/init.h, so I 
> suspect that placing __initdata like this can be a noop, while the 
> latter means "can be init if no module support", which would probably 
> still exhibit the problem if so configured.
> 
> How would you like to have this corrected then?

I guess removing __initdata from all platoform_device structures is the
way to go. We need to find another way to silent the section mismatch
warnings.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]