| Message ID | 1328786302-25131-1-git-send-email-jkrzyszt@tis.icnet.pl |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Feb 09, 2012 at 12:18:22PM +0100, Janusz Krzysztofik wrote: > The main purpose of this patch is to fix several section mismatch > warnings from the board file and a few board specific drivers, > introduced mostly with recent Amstrad Delta patch series, some of them > rising up only when building with CONFIG_MODULES not set. > > While being at it, section assignments of all init data found in the > board file have been revised and hopefully optimised. There is no optimisation to adding __refdata to stuff. That's screaming that you have lots of bugs here. > > Created and tested on top of current linux-omap/omap1, commit > 967809bd7faf71ddc29c8081e0f21db8b201a0f4. > > Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> > --- > arch/arm/mach-omap1/board-ams-delta.c | 35 +++++++++++++++++---------------- > drivers/input/serio/ams_delta_serio.c | 2 +- > drivers/mtd/nand/ams-delta.c | 2 +- > drivers/video/omap/lcd_ams_delta.c | 2 +- > 4 files changed, 21 insertions(+), 20 deletions(-) > > diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c > index 87b1303..dc2455f 100644 > --- a/arch/arm/mach-omap1/board-ams-delta.c > +++ b/arch/arm/mach-omap1/board-ams-delta.c > @@ -126,7 +126,7 @@ static const unsigned int ams_delta_keymap[] = { > #define LATCH2_PHYS 0x08000000 > #define LATCH2_VIRT 0xEC000000 > > -static struct map_desc ams_delta_io_desc[] __initdata = { > +static struct map_desc ams_delta_io_desc[] __initconst = { You're not making this comst so don't change it to __initconst. > /* AMS_DELTA_LATCH1 */ > { > .virtual = LATCH1_VIRT, > @@ -150,17 +150,17 @@ static struct map_desc ams_delta_io_desc[] __initdata = { > } > }; > > -static struct omap_lcd_config ams_delta_lcd_config = { > +static struct omap_lcd_config ams_delta_lcd_config __initconst = { This change probably adds a bug. Are you sure this will never be used outside init context? > .ctrl_name = "internal", > }; > > -static struct omap_usb_config ams_delta_usb_config __initdata = { > +static struct omap_usb_config ams_delta_usb_config __initdata_or_module = { Even if you don't have modules enabled, have you checked whether the driver can be bound and unbound via /sys/bus/platform/driver/<name>/{bind,unbind} ? I suspect this is a bug. > .register_host = 1, > .hmc_mode = 16, > .pins[0] = 2, > }; > > -static struct omap_board_config_kernel ams_delta_config[] __initdata = { > +static struct omap_board_config_kernel ams_delta_config[] __initconst = { > { OMAP_TAG_LCD, &ams_delta_lcd_config }, > }; > > @@ -181,7 +181,7 @@ static struct bgpio_pdata latch1_pdata __initconst = { > .ngpio = LATCH1_NGPIO, > }; > > -static struct platform_device latch1_gpio_device = { > +static struct platform_device latch1_gpio_device __refdata = { > .name = "basic-mmio-gpio", > .id = 0, > .resource = latch1_resources, > @@ -205,7 +205,7 @@ static struct bgpio_pdata latch2_pdata __initconst = { > .ngpio = AMS_DELTA_LATCH2_NGPIO, > }; > > -static struct platform_device latch2_gpio_device = { > +static struct platform_device latch2_gpio_device __refdata = { > .name = "basic-mmio-gpio", > .id = 1, > .resource = latch2_resources, > @@ -271,7 +271,7 @@ void ams_delta_latch_write(int base, int ngpio, u16 mask, u16 value) > } > EXPORT_SYMBOL(ams_delta_latch_write); > > -static struct resource ams_delta_nand_resources[] = { > +static struct resource ams_delta_nand_resources[] __initconst_or_module = { This change definitely adds a bug. The resources are _used_ _all_ _the_ _time_ _the_ _device_ _is_ _registered_. Try catting /proc/iomem after boot. > [0] = { > .start = OMAP1_MPUIO_BASE, > .end = OMAP1_MPUIO_BASE + > @@ -280,14 +280,14 @@ static struct resource ams_delta_nand_resources[] = { > }, > }; > > -static struct platform_device ams_delta_nand_device = { > +static struct platform_device ams_delta_nand_device __refdata = { Therefore, bug. > .name = "ams-delta-nand", > .id = -1, > .num_resources = ARRAY_SIZE(ams_delta_nand_resources), > .resource = ams_delta_nand_resources, > }; > > -static struct resource ams_delta_kp_resources[] = { > +static struct resource ams_delta_kp_resources[] __initconst_or_module = { Bug. > [0] = { > .start = INT_KEYBOARD, > .end = INT_KEYBOARD, > @@ -300,14 +300,14 @@ static const struct matrix_keymap_data ams_delta_keymap_data = { > .keymap_size = ARRAY_SIZE(ams_delta_keymap), > }; > > -static struct omap_kp_platform_data ams_delta_kp_data __initdata = { > +static struct omap_kp_platform_data ams_delta_kp_data __initconst_or_module = { Probably a bug if you unbind/rebind the associated driver with modules disabled. > .rows = 8, > .cols = 8, > .keymap_data = &ams_delta_keymap_data, > .delay = 9, > }; > > -static struct platform_device ams_delta_kp_device = { > +static struct platform_device ams_delta_kp_device __refdata = { > .name = "omap-keypad", > .id = -1, > .dev = { > @@ -363,7 +363,8 @@ static struct gpio_led_platform_data leds_pdata __initconst = { > .num_leds = ARRAY_SIZE(gpio_leds), > }; > > -static struct i2c_board_info ams_delta_camera_board_info[] = { > +static struct i2c_board_info __initconst_or_module > +ams_delta_camera_board_info[] = { No. It's static struct foo blah[] __whatever_init_attribute not static struct foo __whatever_init_attribute blah[] I've no idea where this fucked idea came from but it's something that's wasting a lot of review time with complaints like this about it. > { > I2C_BOARD_INFO("ov6650", 0x60), > }, > @@ -387,7 +388,7 @@ static int ams_delta_camera_power(struct device *dev, int power) > #define ams_delta_camera_power NULL > #endif > > -static struct soc_camera_link ams_delta_iclink = { > +static struct soc_camera_link ams_delta_iclink __initconst_or_module = { Probably buggy. > .bus_id = 0, /* OMAP1 SoC camera bus */ > .i2c_adapter_id = 1, > .board_info = &ams_delta_camera_board_info[0], > @@ -395,7 +396,7 @@ static struct soc_camera_link ams_delta_iclink = { > .power = ams_delta_camera_power, > }; > > -static struct platform_device ams_delta_camera_device = { > +static struct platform_device ams_delta_camera_device __refdata = { > .name = "soc-camera-pdrv", > .id = 0, > .dev = { > @@ -408,7 +409,7 @@ static struct omap1_cam_platform_data ams_delta_camera_platform_data = { > .lclk_khz_max = 1334, /* results in 5fps CIF, 10fps QCIF */ > }; > > -static struct platform_device *ams_delta_devices[] __initdata = { > +static struct platform_device *ams_delta_devices[] __initconst = { Missing const. If you can't const it, don't put it in __initconst. > &latch1_gpio_device, > &latch2_gpio_device, > &ams_delta_kp_device, > @@ -459,7 +460,7 @@ static void __init ams_delta_init(void) > omap_writew(omap_readw(ARM_RSTCT1) | 0x0004, ARM_RSTCT1); > } > > -static struct plat_serial8250_port ams_delta_modem_ports[] = { > +static struct plat_serial8250_port ams_delta_modem_ports[] __initdata = { Buggy. > { > .membase = IOMEM(MODEM_VIRT), > .mapbase = MODEM_PHYS, > @@ -473,7 +474,7 @@ static struct plat_serial8250_port ams_delta_modem_ports[] = { > { }, > }; > > -static struct platform_device ams_delta_modem_device = { > +static struct platform_device ams_delta_modem_device __refdata = { > .name = "serial8250", > .id = PLAT8250_DEV_PLATFORM1, > .dev = {
On Thursday 09 of February 2012 14:48:53 Russell King - ARM Linux wrote: > There is no optimisation to adding __refdata to stuff. That's screaming > that you have lots of bugs here. Thanks for your teaching. I find those aspects not very exhaustively documented under Documentation/, so any hints are much appreciated. > > -static struct map_desc ams_delta_io_desc[] __initdata = { > > +static struct map_desc ams_delta_io_desc[] __initconst = { > > You're not making this comst so don't change it to __initconst. OK, however, I think there must be a bug in gcc, otherwise it should probably never accept such wrong constructs, while sometimes it does (not only mine, see [1]). > > -static struct omap_lcd_config ams_delta_lcd_config = { > > +static struct omap_lcd_config ams_delta_lcd_config __initconst = { > > This change probably adds a bug. Are you sure this will never be used > outside init context? I may be wrong, but before I've changed this, and now once again, I've verified that a copy of this data is made inside __init omap_init_fb() by means of a structure assignment operation. > > -static struct omap_usb_config ams_delta_usb_config __initdata = { > > +static struct omap_usb_config ams_delta_usb_config __initdata_or_module > > = { > Even if you don't have modules enabled, have you checked whether the > driver can be bound and unbound via > /sys/bus/platform/driver/<name>/{bind,unbind} ? No, I didn't even think of it, I am afraid. > I suspect this is a bug. Indeed, that data is not copied on init, only pointed to, moreover, the ohci-omap driver provides bind/unbind opearations. BTW, what are those bind/unbind operations intended for in case of a driver dedicated to a non-hotplugable SoC hardware exclusively? > > -static struct resource ams_delta_nand_resources[] = { > > +static struct resource ams_delta_nand_resources[] __initconst_or_module > > = { > This change definitely adds a bug. The resources are _used_ _all_ _the_ > _time_ _the_ _device_ _is_ _registered_. Try catting /proc/iomem after > boot. Indeed, I didn't think of that. I expected that standard init data of only those devices which can be actually found during init should be copied for runtime access, then all (found and not found) dropped instead of keeping them all in memory for only some of them being actually used. > > -static struct i2c_board_info ams_delta_camera_board_info[] = { > > +static struct i2c_board_info __initconst_or_module > > +ams_delta_camera_board_info[] = { > > No. It's > > static struct foo blah[] __whatever_init_attribute > > not > > static struct foo __whatever_init_attribute blah[] > > I've no idea where this fucked idea came from but it's something that's > wasting a lot of review time with complaints like this about it. My bad, I blindly copied that pattern from another broken example under arch/arm instead of examining the docs carefully enough. > > -static struct soc_camera_link ams_delta_iclink = { > > +static struct soc_camera_link ams_delta_iclink __initconst_or_module = > > { > > Probably buggy. Indeed. Even if the soc-camera-pdrv driver doesn't support unbindind/rebinding, it doesn't seem to make a copy of that platform data, only stores a pointer to it for runtime access, wich I missed. > > -static struct platform_device *ams_delta_devices[] __initdata = { > > +static struct platform_device *ams_delta_devices[] __initconst = { > > Missing const. If you can't const it, don't put it in __initconst. I gave up trying to use both const and __initconst together after my gcc-4.2.4 (and not only mine, see [1], [2]) refused to accept such constructs a few times. Now I see that this false reporting may probably happen in presence of other incorrect uses of __initconst without const or __initdata with const. Hopefully better patches will follow. Thanks, Janusz [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/062421.html [2] http://permalink.gmane.org/gmane.linux.kernel.commits.head/202285
On Fri, Feb 10, 2012 at 05:31:31PM +0100, Janusz Krzysztofik wrote: > On Thursday 09 of February 2012 14:48:53 Russell King - ARM Linux wrote: > > > -static struct map_desc ams_delta_io_desc[] __initdata = { > > > +static struct map_desc ams_delta_io_desc[] __initconst = { > > > > You're not making this comst so don't change it to __initconst. > > OK, however, I think there must be a bug in gcc, otherwise it should probably > never accept such wrong constructs, while sometimes it does (not only mine, > see [1]). No - because you don't understand what's going on with these tags. The __initconst and other tags are just tell the compiler to place stuff into a named section. The name is interpreted by GCC as a mere string which it passes through to the ELF file. So, gcc has no idea that a section named '.init.rodata' could be placed in read-only memory, and therefore has no idea that it should also be marked 'const'. Therefore, gcc has no way to verify that 'const' and '__initconst' are always paired. > > > -static struct omap_lcd_config ams_delta_lcd_config = { > > > +static struct omap_lcd_config ams_delta_lcd_config __initconst = { > > > > This change probably adds a bug. Are you sure this will never be used > > outside init context? > > I may be wrong, but before I've changed this, and now once again, I've verified > that a copy of this data is made inside __init omap_init_fb() by means of a > structure assignment operation. Ok, in that case add a 'const' to it. > > > -static struct omap_usb_config ams_delta_usb_config __initdata = { > > > +static struct omap_usb_config ams_delta_usb_config __initdata_or_module > > > = { > > Even if you don't have modules enabled, have you checked whether the > > driver can be bound and unbound via > > /sys/bus/platform/driver/<name>/{bind,unbind} ? > > No, I didn't even think of it, I am afraid. > > > I suspect this is a bug. > > Indeed, that data is not copied on init, only pointed to, moreover, the > ohci-omap driver provides bind/unbind opearations. > > BTW, what are those bind/unbind operations intended for in case of a > driver dedicated to a non-hotplugable SoC hardware exclusively? If the driver can be built as a module, they allow exercising the probe/remove paths for drivers which have been built-in. Moreover, it allows you to disable a device driver if desired. For example, I have two platforms which are essentially the same, except one has a daughterboard attached (which isn't hotpluggable). The support for the daughterboard, although it is a platform driver, can't be modularised at the moment because quite a bit of other stuff needs it and it contains IRQ chip code. One has far less idle wakeups than the other. Using unbind, I can effectively detach this board by unbinding its platform device, which results the entire sub-tree of its on-board devices being unregistered and released in one big go, all the way down to things like USB devices on the USB host. I can then rebind it and bring all that hardware back. That allows me to evaluate whether there's any impact from those drivers. However, this bind/unbind is not the only issue here: how do you know whether the driver takes a copy of the platform data, or merely stores a pointer to it - and may access your platform data at runtime. Unless you check, and recheck it regularly (it can change) then placing this stuff into sections which will be discarded is asking for bugs. > > > -static struct platform_device *ams_delta_devices[] __initdata = { > > > +static struct platform_device *ams_delta_devices[] __initconst = { > > > > Missing const. If you can't const it, don't put it in __initconst. > > I gave up trying to use both const and __initconst together after my gcc-4.2.4 > (and not only mine, see [1], [2]) refused to accept such constructs a few > times. Now I see that this false reporting may probably happen in presence of other > incorrect uses of __initconst without const or __initdata with const. I doubt that the compiler was refusing to accept them. What you were probably getting were warnings about where these were used not accepting a const pointer. If that's the case, more analysis needs to be done to find out whether those uses can be converted to accept a const pointer before defining the data with const and optionally __initconst.
diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c index 87b1303..dc2455f 100644 --- a/arch/arm/mach-omap1/board-ams-delta.c +++ b/arch/arm/mach-omap1/board-ams-delta.c @@ -126,7 +126,7 @@ static const unsigned int ams_delta_keymap[] = { #define LATCH2_PHYS 0x08000000 #define LATCH2_VIRT 0xEC000000 -static struct map_desc ams_delta_io_desc[] __initdata = { +static struct map_desc ams_delta_io_desc[] __initconst = { /* AMS_DELTA_LATCH1 */ { .virtual = LATCH1_VIRT, @@ -150,17 +150,17 @@ static struct map_desc ams_delta_io_desc[] __initdata = { } }; -static struct omap_lcd_config ams_delta_lcd_config = { +static struct omap_lcd_config ams_delta_lcd_config __initconst = { .ctrl_name = "internal", }; -static struct omap_usb_config ams_delta_usb_config __initdata = { +static struct omap_usb_config ams_delta_usb_config __initdata_or_module = { .register_host = 1, .hmc_mode = 16, .pins[0] = 2, }; -static struct omap_board_config_kernel ams_delta_config[] __initdata = { +static struct omap_board_config_kernel ams_delta_config[] __initconst = { { OMAP_TAG_LCD, &ams_delta_lcd_config }, }; @@ -181,7 +181,7 @@ static struct bgpio_pdata latch1_pdata __initconst = { .ngpio = LATCH1_NGPIO, }; -static struct platform_device latch1_gpio_device = { +static struct platform_device latch1_gpio_device __refdata = { .name = "basic-mmio-gpio", .id = 0, .resource = latch1_resources, @@ -205,7 +205,7 @@ static struct bgpio_pdata latch2_pdata __initconst = { .ngpio = AMS_DELTA_LATCH2_NGPIO, }; -static struct platform_device latch2_gpio_device = { +static struct platform_device latch2_gpio_device __refdata = { .name = "basic-mmio-gpio", .id = 1, .resource = latch2_resources, @@ -271,7 +271,7 @@ void ams_delta_latch_write(int base, int ngpio, u16 mask, u16 value) } EXPORT_SYMBOL(ams_delta_latch_write); -static struct resource ams_delta_nand_resources[] = { +static struct resource ams_delta_nand_resources[] __initconst_or_module = { [0] = { .start = OMAP1_MPUIO_BASE, .end = OMAP1_MPUIO_BASE + @@ -280,14 +280,14 @@ static struct resource ams_delta_nand_resources[] = { }, }; -static struct platform_device ams_delta_nand_device = { +static struct platform_device ams_delta_nand_device __refdata = { .name = "ams-delta-nand", .id = -1, .num_resources = ARRAY_SIZE(ams_delta_nand_resources), .resource = ams_delta_nand_resources, }; -static struct resource ams_delta_kp_resources[] = { +static struct resource ams_delta_kp_resources[] __initconst_or_module = { [0] = { .start = INT_KEYBOARD, .end = INT_KEYBOARD, @@ -300,14 +300,14 @@ static const struct matrix_keymap_data ams_delta_keymap_data = { .keymap_size = ARRAY_SIZE(ams_delta_keymap), }; -static struct omap_kp_platform_data ams_delta_kp_data __initdata = { +static struct omap_kp_platform_data ams_delta_kp_data __initconst_or_module = { .rows = 8, .cols = 8, .keymap_data = &ams_delta_keymap_data, .delay = 9, }; -static struct platform_device ams_delta_kp_device = { +static struct platform_device ams_delta_kp_device __refdata = { .name = "omap-keypad", .id = -1, .dev = { @@ -363,7 +363,8 @@ static struct gpio_led_platform_data leds_pdata __initconst = { .num_leds = ARRAY_SIZE(gpio_leds), }; -static struct i2c_board_info ams_delta_camera_board_info[] = { +static struct i2c_board_info __initconst_or_module +ams_delta_camera_board_info[] = { { I2C_BOARD_INFO("ov6650", 0x60), }, @@ -387,7 +388,7 @@ static int ams_delta_camera_power(struct device *dev, int power) #define ams_delta_camera_power NULL #endif -static struct soc_camera_link ams_delta_iclink = { +static struct soc_camera_link ams_delta_iclink __initconst_or_module = { .bus_id = 0, /* OMAP1 SoC camera bus */ .i2c_adapter_id = 1, .board_info = &ams_delta_camera_board_info[0], @@ -395,7 +396,7 @@ static struct soc_camera_link ams_delta_iclink = { .power = ams_delta_camera_power, }; -static struct platform_device ams_delta_camera_device = { +static struct platform_device ams_delta_camera_device __refdata = { .name = "soc-camera-pdrv", .id = 0, .dev = { @@ -408,7 +409,7 @@ static struct omap1_cam_platform_data ams_delta_camera_platform_data = { .lclk_khz_max = 1334, /* results in 5fps CIF, 10fps QCIF */ }; -static struct platform_device *ams_delta_devices[] __initdata = { +static struct platform_device *ams_delta_devices[] __initconst = { &latch1_gpio_device, &latch2_gpio_device, &ams_delta_kp_device, @@ -459,7 +460,7 @@ static void __init ams_delta_init(void) omap_writew(omap_readw(ARM_RSTCT1) | 0x0004, ARM_RSTCT1); } -static struct plat_serial8250_port ams_delta_modem_ports[] = { +static struct plat_serial8250_port ams_delta_modem_ports[] __initdata = { { .membase = IOMEM(MODEM_VIRT), .mapbase = MODEM_PHYS, @@ -473,7 +474,7 @@ static struct plat_serial8250_port ams_delta_modem_ports[] = { { }, }; -static struct platform_device ams_delta_modem_device = { +static struct platform_device ams_delta_modem_device __refdata = { .name = "serial8250", .id = PLAT8250_DEV_PLATFORM1, .dev = { diff --git a/drivers/input/serio/ams_delta_serio.c b/drivers/input/serio/ams_delta_serio.c index 0571e2e..0830a76 100644 --- a/drivers/input/serio/ams_delta_serio.c +++ b/drivers/input/serio/ams_delta_serio.c @@ -103,7 +103,7 @@ static void ams_delta_serio_close(struct serio *serio) gpio_set_value(AMS_DELTA_GPIO_PIN_KEYBRD_PWR, 0); } -static struct gpio ams_delta_gpios[] __initconst_or_module = { +static const struct gpio ams_delta_gpios[] = { { .gpio = AMS_DELTA_GPIO_PIN_KEYBRD_DATA, .flags = GPIOF_DIR_IN, diff --git a/drivers/mtd/nand/ams-delta.c b/drivers/mtd/nand/ams-delta.c index 85934dc..7341695 100644 --- a/drivers/mtd/nand/ams-delta.c +++ b/drivers/mtd/nand/ams-delta.c @@ -145,7 +145,7 @@ static int ams_delta_nand_ready(struct mtd_info *mtd) return gpio_get_value(AMS_DELTA_GPIO_PIN_NAND_RB); } -static struct gpio _mandatory_gpio[] __initconst_or_module = { +static const struct gpio _mandatory_gpio[] = { { .gpio = AMS_DELTA_GPIO_PIN_NAND_NCE, .flags = GPIOF_OUT_INIT_HIGH, diff --git a/drivers/video/omap/lcd_ams_delta.c b/drivers/video/omap/lcd_ams_delta.c index 0e71e28..d3a3113 100644 --- a/drivers/video/omap/lcd_ams_delta.c +++ b/drivers/video/omap/lcd_ams_delta.c @@ -99,7 +99,7 @@ static struct lcd_ops ams_delta_lcd_ops = { /* omapfb panel section */ -static struct gpio _gpios[] __initconst_or_module = { +static const struct gpio _gpios[] = { { .gpio = AMS_DELTA_GPIO_PIN_LCD_VBLEN, .flags = GPIOF_OUT_INIT_LOW,
The main purpose of this patch is to fix several section mismatch warnings from the board file and a few board specific drivers, introduced mostly with recent Amstrad Delta patch series, some of them rising up only when building with CONFIG_MODULES not set. While being at it, section assignments of all init data found in the board file have been revised and hopefully optimised. Created and tested on top of current linux-omap/omap1, commit 967809bd7faf71ddc29c8081e0f21db8b201a0f4. Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> --- arch/arm/mach-omap1/board-ams-delta.c | 35 +++++++++++++++++---------------- drivers/input/serio/ams_delta_serio.c | 2 +- drivers/mtd/nand/ams-delta.c | 2 +- drivers/video/omap/lcd_ams_delta.c | 2 +- 4 files changed, 21 insertions(+), 20 deletions(-)