From: Emese Revfy <[email protected]>
Hello everyone!
The following patch series attempts to constify several structures
that hold function pointers. This is only the initial batch, there
are about over 150 candidate structures, some of which can be
constified as well, I plan to submit them in the future.
The list of constified structures in this series (* marks this thread):
acpi_dock_ops
address_space_operations
* backlight_ops
block_device_operations
dma_map_ops
extent_io_ops
file_lock_operations
file_operations
hv_ops
intel_dvo_dev_ops
item_operations
iwl_ops
kgdb_arch
kgdb_io
kset_uevent_ops
lock_manager_operations
microcode_ops
mtrr_ops
neigh_ops
nlmsvc_binding
pci_raw_ops
platform_hibernation_ops
platform_suspend_ops
snd_ac97_build_ops
sysfs_ops
usb_mon_operations
wd_ops
There are certain exceptions where a given instance of the structure
cannot be const, they are marked with a comment in the patch.
The patches compile fine with an allyesconfig kernel on i386 and x86_64.
Please let me know if any of these structures should not be constified
and any other issues you see with them.
Changelog:
----------
v1 -> v2
- updated to linus-git-053fe57
- extended comments with a reference to code that prevents constification
- split up patches by subsystem as suggested by Greg KH, Jiri Slaby
- added all Acked-by's received so far
- removed patch for super_operations for now
- removed patch for ptmx_fops
Thanks,
Emese
drivers/acpi/video.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_backlight.c | 4 ++--
drivers/macintosh/via-pmu-backlight.c | 4 ++--
drivers/platform/x86/acer-wmi.c | 2 +-
drivers/platform/x86/asus-laptop.c | 2 +-
drivers/platform/x86/asus_acpi.c | 2 +-
drivers/platform/x86/compal-laptop.c | 2 +-
drivers/platform/x86/dell-laptop.c | 2 +-
drivers/platform/x86/eeepc-laptop.c | 2 +-
drivers/platform/x86/fujitsu-laptop.c | 2 +-
drivers/platform/x86/msi-laptop.c | 2 +-
drivers/platform/x86/panasonic-laptop.c | 2 +-
drivers/platform/x86/sony-laptop.c | 2 +-
drivers/platform/x86/thinkpad_acpi.c | 2 +-
drivers/platform/x86/toshiba_acpi.c | 2 +-
drivers/staging/samsung-laptop/samsung-laptop.c | 2 +-
drivers/usb/misc/appledisplay.c | 2 +-
drivers/video/atmel_lcdfb.c | 2 +-
drivers/video/aty/aty128fb.c | 2 +-
drivers/video/aty/atyfb_base.c | 2 +-
drivers/video/aty/radeon_backlight.c | 2 +-
drivers/video/backlight/adp5520_bl.c | 2 +-
drivers/video/backlight/adx_bl.c | 2 +-
drivers/video/backlight/atmel-pwm-bl.c | 2 +-
drivers/video/backlight/backlight.c | 2 +-
drivers/video/backlight/corgi_lcd.c | 2 +-
drivers/video/backlight/cr_bllcd.c | 2 +-
drivers/video/backlight/da903x_bl.c | 2 +-
drivers/video/backlight/generic_bl.c | 2 +-
drivers/video/backlight/hp680_bl.c | 2 +-
drivers/video/backlight/jornada720_bl.c | 2 +-
drivers/video/backlight/kb3886_bl.c | 2 +-
drivers/video/backlight/locomolcd.c | 2 +-
drivers/video/backlight/mbp_nvidia_bl.c | 2 +-
drivers/video/backlight/omap1_bl.c | 2 +-
drivers/video/backlight/progear_bl.c | 2 +-
drivers/video/backlight/pwm_bl.c | 2 +-
drivers/video/backlight/tosa_bl.c | 2 +-
drivers/video/backlight/wm831x_bl.c | 2 +-
drivers/video/bf54x-lq043fb.c | 2 +-
drivers/video/bfin-t350mcqb-fb.c | 2 +-
drivers/video/nvidia/nv_backlight.c | 2 +-
drivers/video/omap2/displays/panel-taal.c | 2 +-
drivers/video/riva/fbdev.c | 2 +-
include/linux/backlight.h | 12 ++++++------
45 files changed, 52 insertions(+), 52 deletions(-)
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/acer-wmi.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 454970d..98f0e9a 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -918,7 +918,7 @@ static int update_bl_status(struct backlight_device *bd)
return 0;
}
-static struct backlight_ops acer_bl_ops = {
+static const struct backlight_ops acer_bl_ops = {
.get_brightness = read_brightness,
.update_status = update_bl_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/acpi/video.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 05dff63..b662ab7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -359,7 +359,7 @@ static int acpi_video_set_brightness(struct backlight_device *bd)
vd->brightness->levels[request_level]);
}
-static struct backlight_ops acpi_backlight_ops = {
+static const struct backlight_ops acpi_backlight_ops = {
.get_brightness = acpi_video_get_brightness,
.update_status = acpi_video_set_brightness,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/asus-laptop.c | 2 +-
drivers/platform/x86/asus_acpi.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
index b39d2bb..c9a3dc4 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -249,7 +249,7 @@ static struct backlight_device *asus_backlight_device;
*/
static int read_brightness(struct backlight_device *bd);
static int update_bl_status(struct backlight_device *bd);
-static struct backlight_ops asusbl_ops = {
+static const struct backlight_ops asusbl_ops = {
.get_brightness = read_brightness,
.update_status = update_bl_status,
};
diff --git a/drivers/platform/x86/asus_acpi.c b/drivers/platform/x86/asus_acpi.c
index ddf5240..61e6673 100644
--- a/drivers/platform/x86/asus_acpi.c
+++ b/drivers/platform/x86/asus_acpi.c
@@ -1402,7 +1402,7 @@ static int asus_hotk_remove(struct acpi_device *device, int type)
return 0;
}
-static struct backlight_ops asus_backlight_data = {
+static const struct backlight_ops asus_backlight_data = {
.get_brightness = read_brightness,
.update_status = set_brightness_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/video/atmel_lcdfb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 3d886c6..22ee7f8 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -110,7 +110,7 @@ static int atmel_bl_get_brightness(struct backlight_device *bl)
return lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
}
-static struct backlight_ops atmel_lcdc_bl_ops = {
+static const struct backlight_ops atmel_lcdc_bl_ops = {
.update_status = atmel_bl_update_status,
.get_brightness = atmel_bl_get_brightness,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/video/backlight/adp5520_bl.c | 2 +-
drivers/video/backlight/adx_bl.c | 2 +-
drivers/video/backlight/atmel-pwm-bl.c | 2 +-
drivers/video/backlight/backlight.c | 2 +-
drivers/video/backlight/corgi_lcd.c | 2 +-
drivers/video/backlight/cr_bllcd.c | 2 +-
drivers/video/backlight/da903x_bl.c | 2 +-
drivers/video/backlight/generic_bl.c | 2 +-
drivers/video/backlight/hp680_bl.c | 2 +-
drivers/video/backlight/jornada720_bl.c | 2 +-
drivers/video/backlight/kb3886_bl.c | 2 +-
drivers/video/backlight/locomolcd.c | 2 +-
drivers/video/backlight/mbp_nvidia_bl.c | 2 +-
drivers/video/backlight/omap1_bl.c | 2 +-
drivers/video/backlight/progear_bl.c | 2 +-
drivers/video/backlight/pwm_bl.c | 2 +-
drivers/video/backlight/tosa_bl.c | 2 +-
drivers/video/backlight/wm831x_bl.c | 2 +-
include/linux/backlight.h | 12 ++++++------
19 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/video/backlight/adp5520_bl.c b/drivers/video/backlight/adp5520_bl.c
index ad05da5..3cb2cb9 100644
--- a/drivers/video/backlight/adp5520_bl.c
+++ b/drivers/video/backlight/adp5520_bl.c
@@ -84,7 +84,7 @@ static int adp5520_bl_get_brightness(struct backlight_device *bl)
return error ? data->current_brightness : reg_val;
}
-static struct backlight_ops adp5520_bl_ops = {
+static const struct backlight_ops adp5520_bl_ops = {
.update_status = adp5520_bl_update_status,
.get_brightness = adp5520_bl_get_brightness,
};
diff --git a/drivers/video/backlight/adx_bl.c b/drivers/video/backlight/adx_bl.c
index 2c3bdfc..d769b0b 100644
--- a/drivers/video/backlight/adx_bl.c
+++ b/drivers/video/backlight/adx_bl.c
@@ -61,7 +61,7 @@ static int adx_backlight_check_fb(struct fb_info *fb)
return 1;
}
-static struct backlight_ops adx_backlight_ops = {
+static const struct backlight_ops adx_backlight_ops = {
.options = 0,
.update_status = adx_backlight_update_status,
.get_brightness = adx_backlight_get_brightness,
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index 2cf7ba5..f625ffc 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -113,7 +113,7 @@ static int atmel_pwm_bl_init_pwm(struct atmel_pwm_bl *pwmbl)
return pwm_channel_enable(&pwmbl->pwmc);
}
-static struct backlight_ops atmel_pwm_bl_ops = {
+static const struct backlight_ops atmel_pwm_bl_ops = {
.get_brightness = atmel_pwm_bl_get_intensity,
.update_status = atmel_pwm_bl_set_intensity,
};
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 6615ac7..18829cf 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -269,7 +269,7 @@ EXPORT_SYMBOL(backlight_force_update);
* ERR_PTR() or a pointer to the newly allocated device.
*/
struct backlight_device *backlight_device_register(const char *name,
- struct device *parent, void *devdata, struct backlight_ops *ops)
+ struct device *parent, void *devdata, const struct backlight_ops *ops)
{
struct backlight_device *new_bd;
int rc;
diff --git a/drivers/video/backlight/corgi_lcd.c b/drivers/video/backlight/corgi_lcd.c
index 9677494..b4bcf80 100644
--- a/drivers/video/backlight/corgi_lcd.c
+++ b/drivers/video/backlight/corgi_lcd.c
@@ -451,7 +451,7 @@ void corgi_lcd_limit_intensity(int limit)
}
EXPORT_SYMBOL(corgi_lcd_limit_intensity);
-static struct backlight_ops corgi_bl_ops = {
+static const struct backlight_ops corgi_bl_ops = {
.get_brightness = corgi_bl_get_intensity,
.update_status = corgi_bl_update_status,
};
diff --git a/drivers/video/backlight/cr_bllcd.c b/drivers/video/backlight/cr_bllcd.c
index b9fe62b..2914bf1 100644
--- a/drivers/video/backlight/cr_bllcd.c
+++ b/drivers/video/backlight/cr_bllcd.c
@@ -108,7 +108,7 @@ static int cr_backlight_get_intensity(struct backlight_device *bd)
return intensity;
}
-static struct backlight_ops cr_backlight_ops = {
+static const struct backlight_ops cr_backlight_ops = {
.get_brightness = cr_backlight_get_intensity,
.update_status = cr_backlight_set_intensity,
};
diff --git a/drivers/video/backlight/da903x_bl.c b/drivers/video/backlight/da903x_bl.c
index 7fcb0eb..206cf99 100644
--- a/drivers/video/backlight/da903x_bl.c
+++ b/drivers/video/backlight/da903x_bl.c
@@ -95,7 +95,7 @@ static int da903x_backlight_get_brightness(struct backlight_device *bl)
return data->current_brightness;
}
-static struct backlight_ops da903x_backlight_ops = {
+static const struct backlight_ops da903x_backlight_ops = {
.update_status = da903x_backlight_update_status,
.get_brightness = da903x_backlight_get_brightness,
};
diff --git a/drivers/video/backlight/generic_bl.c b/drivers/video/backlight/generic_bl.c
index 6d27f62..e6d348e 100644
--- a/drivers/video/backlight/generic_bl.c
+++ b/drivers/video/backlight/generic_bl.c
@@ -70,7 +70,7 @@ void corgibl_limit_intensity(int limit)
}
EXPORT_SYMBOL(corgibl_limit_intensity);
-static struct backlight_ops genericbl_ops = {
+static const struct backlight_ops genericbl_ops = {
.options = BL_CORE_SUSPENDRESUME,
.get_brightness = genericbl_get_intensity,
.update_status = genericbl_send_intensity,
diff --git a/drivers/video/backlight/hp680_bl.c b/drivers/video/backlight/hp680_bl.c
index 7fb4eef..f7cc528 100644
--- a/drivers/video/backlight/hp680_bl.c
+++ b/drivers/video/backlight/hp680_bl.c
@@ -98,7 +98,7 @@ static int hp680bl_get_intensity(struct backlight_device *bd)
return current_intensity;
}
-static struct backlight_ops hp680bl_ops = {
+static const struct backlight_ops hp680bl_ops = {
.get_brightness = hp680bl_get_intensity,
.update_status = hp680bl_set_intensity,
};
diff --git a/drivers/video/backlight/jornada720_bl.c b/drivers/video/backlight/jornada720_bl.c
index 7aed256..db9071f 100644
--- a/drivers/video/backlight/jornada720_bl.c
+++ b/drivers/video/backlight/jornada720_bl.c
@@ -93,7 +93,7 @@ out:
return ret;
}
-static struct backlight_ops jornada_bl_ops = {
+static const struct backlight_ops jornada_bl_ops = {
.get_brightness = jornada_bl_get_brightness,
.update_status = jornada_bl_update_status,
.options = BL_CORE_SUSPENDRESUME,
diff --git a/drivers/video/backlight/kb3886_bl.c b/drivers/video/backlight/kb3886_bl.c
index a38fda1..939e7b8 100644
--- a/drivers/video/backlight/kb3886_bl.c
+++ b/drivers/video/backlight/kb3886_bl.c
@@ -134,7 +134,7 @@ static int kb3886bl_get_intensity(struct backlight_device *bd)
return kb3886bl_intensity;
}
-static struct backlight_ops kb3886bl_ops = {
+static const struct backlight_ops kb3886bl_ops = {
.get_brightness = kb3886bl_get_intensity,
.update_status = kb3886bl_send_intensity,
};
diff --git a/drivers/video/backlight/locomolcd.c b/drivers/video/backlight/locomolcd.c
index 6b488b8..00a9591 100644
--- a/drivers/video/backlight/locomolcd.c
+++ b/drivers/video/backlight/locomolcd.c
@@ -141,7 +141,7 @@ static int locomolcd_get_intensity(struct backlight_device *bd)
return current_intensity;
}
-static struct backlight_ops locomobl_data = {
+static const struct backlight_ops locomobl_data = {
.get_brightness = locomolcd_get_intensity,
.update_status = locomolcd_set_intensity,
};
diff --git a/drivers/video/backlight/mbp_nvidia_bl.c b/drivers/video/backlight/mbp_nvidia_bl.c
index 9edb8d7..5812468 100644
--- a/drivers/video/backlight/mbp_nvidia_bl.c
+++ b/drivers/video/backlight/mbp_nvidia_bl.c
@@ -33,7 +33,7 @@ struct dmi_match_data {
unsigned long iostart;
unsigned long iolen;
/* Backlight operations structure. */
- struct backlight_ops backlight_ops;
+ const struct backlight_ops backlight_ops;
};
/* Module parameters. */
diff --git a/drivers/video/backlight/omap1_bl.c b/drivers/video/backlight/omap1_bl.c
index 8693e5f..409ca96 100644
--- a/drivers/video/backlight/omap1_bl.c
+++ b/drivers/video/backlight/omap1_bl.c
@@ -125,7 +125,7 @@ static int omapbl_get_intensity(struct backlight_device *dev)
return bl->current_intensity;
}
-static struct backlight_ops omapbl_ops = {
+static const struct backlight_ops omapbl_ops = {
.get_brightness = omapbl_get_intensity,
.update_status = omapbl_update_status,
};
diff --git a/drivers/video/backlight/progear_bl.c b/drivers/video/backlight/progear_bl.c
index 9edaf24..075786e 100644
--- a/drivers/video/backlight/progear_bl.c
+++ b/drivers/video/backlight/progear_bl.c
@@ -54,7 +54,7 @@ static int progearbl_get_intensity(struct backlight_device *bd)
return intensity - HW_LEVEL_MIN;
}
-static struct backlight_ops progearbl_ops = {
+static const struct backlight_ops progearbl_ops = {
.get_brightness = progearbl_get_intensity,
.update_status = progearbl_set_intensity,
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 8871662..df9e0b3 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -56,7 +56,7 @@ static int pwm_backlight_get_brightness(struct backlight_device *bl)
return bl->props.brightness;
}
-static struct backlight_ops pwm_backlight_ops = {
+static const struct backlight_ops pwm_backlight_ops = {
.update_status = pwm_backlight_update_status,
.get_brightness = pwm_backlight_get_brightness,
};
diff --git a/drivers/video/backlight/tosa_bl.c b/drivers/video/backlight/tosa_bl.c
index 43edbad..e14ce4d 100644
--- a/drivers/video/backlight/tosa_bl.c
+++ b/drivers/video/backlight/tosa_bl.c
@@ -72,7 +72,7 @@ static int tosa_bl_get_brightness(struct backlight_device *dev)
return props->brightness;
}
-static struct backlight_ops bl_ops = {
+static const struct backlight_ops bl_ops = {
.get_brightness = tosa_bl_get_brightness,
.update_status = tosa_bl_update_status,
};
diff --git a/drivers/video/backlight/wm831x_bl.c b/drivers/video/backlight/wm831x_bl.c
index 467bdb7..e32add3 100644
--- a/drivers/video/backlight/wm831x_bl.c
+++ b/drivers/video/backlight/wm831x_bl.c
@@ -112,7 +112,7 @@ static int wm831x_backlight_get_brightness(struct backlight_device *bl)
return data->current_brightness;
}
-static struct backlight_ops wm831x_backlight_ops = {
+static const struct backlight_ops wm831x_backlight_ops = {
.options = BL_CORE_SUSPENDRESUME,
.update_status = wm831x_backlight_update_status,
.get_brightness = wm831x_backlight_get_brightness,
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 0f5f578..8c4f884 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -36,18 +36,18 @@ struct backlight_device;
struct fb_info;
struct backlight_ops {
- unsigned int options;
+ const unsigned int options;
#define BL_CORE_SUSPENDRESUME (1 << 0)
/* Notify the backlight driver some property has changed */
- int (*update_status)(struct backlight_device *);
+ int (* const update_status)(struct backlight_device *);
/* Return the current backlight brightness (accounting for power,
fb_blank etc.) */
- int (*get_brightness)(struct backlight_device *);
+ int (* const get_brightness)(struct backlight_device *);
/* Check if given framebuffer device is the one bound to this backlight;
return 0 if not, !=0 if it is. If NULL, backlight always matches the fb. */
- int (*check_fb)(struct fb_info *);
+ int (* const check_fb)(struct fb_info *);
};
/* This structure defines all the properties of a backlight */
@@ -86,7 +86,7 @@ struct backlight_device {
registered this device has been unloaded, and if class_get_devdata()
points to something in the body of that driver, it is also invalid. */
struct mutex ops_lock;
- struct backlight_ops *ops;
+ const struct backlight_ops *ops;
/* The framebuffer notifier block */
struct notifier_block fb_notif;
@@ -103,7 +103,7 @@ static inline void backlight_update_status(struct backlight_device *bd)
}
extern struct backlight_device *backlight_device_register(const char *name,
- struct device *dev, void *devdata, struct backlight_ops *ops);
+ struct device *dev, void *devdata, const struct backlight_ops *ops);
extern void backlight_device_unregister(struct backlight_device *bd);
extern void backlight_force_update(struct backlight_device *bd,
enum backlight_update_reason reason);
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/compal-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
index 11003bb..550ff1b 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -163,7 +163,7 @@ static int bl_update_status(struct backlight_device *b)
return set_lcd_level(b->props.brightness);
}
-static struct backlight_ops compalbl_ops = {
+static const struct backlight_ops compalbl_ops = {
.get_brightness = bl_get_brightness,
.update_status = bl_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/acpi/dock.c | 4 ++--
include/acpi/acpi_drivers.h | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 30be3c1..b3a5492 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -77,7 +77,7 @@ struct dock_dependent_device {
struct list_head list;
struct list_head hotplug_list;
acpi_handle handle;
- struct acpi_dock_ops *ops;
+ const struct acpi_dock_ops *ops;
void *context;
};
@@ -605,7 +605,7 @@ EXPORT_SYMBOL_GPL(unregister_dock_notifier);
* the dock driver after _DCK is executed.
*/
int
-register_hotplug_dock_device(acpi_handle handle, struct acpi_dock_ops *ops,
+register_hotplug_dock_device(acpi_handle handle, const struct acpi_dock_ops *ops,
void *context)
{
struct dock_dependent_device *dd;
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index f4906f6..71feb73 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -119,8 +119,8 @@ int acpi_processor_set_thermal_limit(acpi_handle handle, int type);
Dock Station
-------------------------------------------------------------------------- */
struct acpi_dock_ops {
- acpi_notify_handler handler;
- acpi_notify_handler uevent;
+ const acpi_notify_handler handler;
+ const acpi_notify_handler uevent;
};
#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
@@ -128,7 +128,7 @@ extern int is_dock_device(acpi_handle handle);
extern int register_dock_notifier(struct notifier_block *nb);
extern void unregister_dock_notifier(struct notifier_block *nb);
extern int register_hotplug_dock_device(acpi_handle handle,
- struct acpi_dock_ops *ops,
+ const struct acpi_dock_ops *ops,
void *context);
extern void unregister_hotplug_dock_device(acpi_handle handle);
#else
@@ -144,7 +144,7 @@ static inline void unregister_dock_notifier(struct notifier_block *nb)
{
}
static inline int register_hotplug_dock_device(acpi_handle handle,
- struct acpi_dock_ops *ops,
+ const struct acpi_dock_ops *ops,
void *context)
{
return -ENODEV;
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/dell-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 74909c4..e1ac2ba 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -305,7 +305,7 @@ static int dell_get_intensity(struct backlight_device *bd)
return buffer.output[1];
}
-static struct backlight_ops dell_ops = {
+static const struct backlight_ops dell_ops = {
.get_brightness = dell_get_intensity,
.update_status = dell_send_intensity,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/pci/hotplug/acpiphp_glue.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 8e952fd..bdef77f 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -109,7 +109,7 @@ static int post_dock_fixups(struct notifier_block *nb, unsigned long val,
}
-static struct acpi_dock_ops acpiphp_dock_ops = {
+static const struct acpi_dock_ops acpiphp_dock_ops = {
.handler = handle_hotplug_event_func,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/gpu/drm/nouveau/nouveau_backlight.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 20564f8..9a8c990 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -58,7 +58,7 @@ static int nv40_set_intensity(struct backlight_device *bd)
return 0;
}
-static struct backlight_ops nv40_bl_ops = {
+static const struct backlight_ops nv40_bl_ops = {
.options = BL_CORE_SUSPENDRESUME,
.get_brightness = nv40_get_intensity,
.update_status = nv40_set_intensity,
@@ -81,7 +81,7 @@ static int nv50_set_intensity(struct backlight_device *bd)
return 0;
}
-static struct backlight_ops nv50_bl_ops = {
+static const struct backlight_ops nv50_bl_ops = {
.options = BL_CORE_SUSPENDRESUME,
.get_brightness = nv50_get_intensity,
.update_status = nv50_set_intensity,
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/eeepc-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 4226e53..387aad9 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -242,7 +242,7 @@ static struct device *eeepc_hwmon_device;
*/
static int read_brightness(struct backlight_device *bd);
static int update_bl_status(struct backlight_device *bd);
-static struct backlight_ops eeepcbl_ops = {
+static const struct backlight_ops eeepcbl_ops = {
.get_brightness = read_brightness,
.update_status = update_bl_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/ata/libata-acpi.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 1245838..667111d 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -223,12 +223,12 @@ static void ata_acpi_dev_uevent(acpi_handle handle, u32 event, void *data)
ata_acpi_uevent(dev->link->ap, dev, event);
}
-static struct acpi_dock_ops ata_acpi_dev_dock_ops = {
+static const struct acpi_dock_ops ata_acpi_dev_dock_ops = {
.handler = ata_acpi_dev_notify_dock,
.uevent = ata_acpi_dev_uevent,
};
-static struct acpi_dock_ops ata_acpi_ap_dock_ops = {
+static const struct acpi_dock_ops ata_acpi_ap_dock_ops = {
.handler = ata_acpi_ap_notify_dock,
.uevent = ata_acpi_ap_uevent,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index bcd4ba8..a249b35 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -436,7 +436,7 @@ static int bl_update_status(struct backlight_device *b)
return ret;
}
-static struct backlight_ops fujitsubl_ops = {
+static const struct backlight_ops fujitsubl_ops = {
.get_brightness = bl_get_brightness,
.update_status = bl_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/msi-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 759763d..1093ba2 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -161,7 +161,7 @@ static int bl_update_status(struct backlight_device *b)
return set_lcd_level(b->props.brightness);
}
-static struct backlight_ops msibl_ops = {
+static const struct backlight_ops msibl_ops = {
.get_brightness = bl_get_brightness,
.update_status = bl_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/video/nvidia/nv_backlight.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/nvidia/nv_backlight.c b/drivers/video/nvidia/nv_backlight.c
index 443e3c8..c443d6a 100644
--- a/drivers/video/nvidia/nv_backlight.c
+++ b/drivers/video/nvidia/nv_backlight.c
@@ -87,7 +87,7 @@ static int nvidia_bl_get_brightness(struct backlight_device *bd)
return bd->props.brightness;
}
-static struct backlight_ops nvidia_bl_ops = {
+static const struct backlight_ops nvidia_bl_ops = {
.get_brightness = nvidia_bl_get_brightness,
.update_status = nvidia_bl_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/panasonic-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index fe7cf01..9012d8d 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -352,7 +352,7 @@ static int bl_set_status(struct backlight_device *bd)
return acpi_pcc_write_sset(pcc, SINF_DC_CUR_BRIGHT, bright);
}
-static struct backlight_ops pcc_backlight_ops = {
+static const struct backlight_ops pcc_backlight_ops = {
.get_brightness = bl_get,
.update_status = bl_set_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/video/aty/radeon_backlight.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/aty/radeon_backlight.c b/drivers/video/aty/radeon_backlight.c
index 1a056ad..221bd6a 100644
--- a/drivers/video/aty/radeon_backlight.c
+++ b/drivers/video/aty/radeon_backlight.c
@@ -127,7 +127,7 @@ static int radeon_bl_get_brightness(struct backlight_device *bd)
return bd->props.brightness;
}
-static struct backlight_ops radeon_bl_data = {
+static const struct backlight_ops radeon_bl_data = {
.get_brightness = radeon_bl_get_brightness,
.update_status = radeon_bl_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/video/aty/aty128fb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/aty/aty128fb.c b/drivers/video/aty/aty128fb.c
index e4e4d43..137cd34 100644
--- a/drivers/video/aty/aty128fb.c
+++ b/drivers/video/aty/aty128fb.c
@@ -1787,7 +1787,7 @@ static int aty128_bl_get_brightness(struct backlight_device *bd)
return bd->props.brightness;
}
-static struct backlight_ops aty128_bl_data = {
+static const struct backlight_ops aty128_bl_data = {
.get_brightness = aty128_bl_get_brightness,
.update_status = aty128_bl_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/sony-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 7a2cc8a..466e8d5 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -850,7 +850,7 @@ static int sony_backlight_get_brightness(struct backlight_device *bd)
}
static struct backlight_device *sony_backlight_device;
-static struct backlight_ops sony_backlight_ops = {
+static const struct backlight_ops sony_backlight_ops = {
.update_status = sony_backlight_update_status,
.get_brightness = sony_backlight_get_brightness,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/staging/samsung-laptop/samsung-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/samsung-laptop/samsung-laptop.c b/drivers/staging/samsung-laptop/samsung-laptop.c
index 4877138..0cf2d58 100644
--- a/drivers/staging/samsung-laptop/samsung-laptop.c
+++ b/drivers/staging/samsung-laptop/samsung-laptop.c
@@ -268,7 +268,7 @@ static int update_status(struct backlight_device *bd)
return 0;
}
-static struct backlight_ops backlight_ops = {
+static const struct backlight_ops backlight_ops = {
.get_brightness = get_brightness,
.update_status = update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
include/linux/fs.h | 34 +++++++++++++++++-----------------
1 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a057f48..3d3d6af 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -566,41 +566,41 @@ typedef int (*read_actor_t)(read_descriptor_t *, struct page *,
unsigned long, unsigned long);
struct address_space_operations {
- int (*writepage)(struct page *page, struct writeback_control *wbc);
- int (*readpage)(struct file *, struct page *);
- void (*sync_page)(struct page *);
+ int (* const writepage)(struct page *page, struct writeback_control *wbc);
+ int (* const readpage)(struct file *, struct page *);
+ void (* const sync_page)(struct page *);
/* Write back some dirty pages from this mapping. */
- int (*writepages)(struct address_space *, struct writeback_control *);
+ int (* const writepages)(struct address_space *, struct writeback_control *);
/* Set a page dirty. Return true if this dirtied it */
- int (*set_page_dirty)(struct page *page);
+ int (* const set_page_dirty)(struct page *page);
- int (*readpages)(struct file *filp, struct address_space *mapping,
+ int (* const readpages)(struct file *filp, struct address_space *mapping,
struct list_head *pages, unsigned nr_pages);
- int (*write_begin)(struct file *, struct address_space *mapping,
+ int (* const write_begin)(struct file *, struct address_space *mapping,
loff_t pos, unsigned len, unsigned flags,
struct page **pagep, void **fsdata);
- int (*write_end)(struct file *, struct address_space *mapping,
+ int (* const write_end)(struct file *, struct address_space *mapping,
loff_t pos, unsigned len, unsigned copied,
struct page *page, void *fsdata);
/* Unfortunately this kludge is needed for FIBMAP. Don't use it */
- sector_t (*bmap)(struct address_space *, sector_t);
- void (*invalidatepage) (struct page *, unsigned long);
- int (*releasepage) (struct page *, gfp_t);
- ssize_t (*direct_IO)(int, struct kiocb *, const struct iovec *iov,
+ sector_t (* const bmap)(struct address_space *, sector_t);
+ void (* const invalidatepage) (struct page *, unsigned long);
+ int (* const releasepage) (struct page *, gfp_t);
+ ssize_t (* const direct_IO)(int, struct kiocb *, const struct iovec *iov,
loff_t offset, unsigned long nr_segs);
- int (*get_xip_mem)(struct address_space *, pgoff_t, int,
+ int (* const get_xip_mem)(struct address_space *, pgoff_t, int,
void **, unsigned long *);
/* migrate the contents of a page to the specified target */
- int (*migratepage) (struct address_space *,
+ int (* const migratepage) (struct address_space *,
struct page *, struct page *);
- int (*launder_page) (struct page *);
- int (*is_partially_uptodate) (struct page *, read_descriptor_t *,
+ int (* const launder_page) (struct page *);
+ int (* const is_partially_uptodate) (struct page *, read_descriptor_t *,
unsigned long);
- int (*error_remove_page)(struct address_space *, struct page *);
+ int (* const error_remove_page)(struct address_space *, struct page *);
};
/*
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/video/riva/fbdev.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/riva/fbdev.c b/drivers/video/riva/fbdev.c
index d94c57f..912984c 100644
--- a/drivers/video/riva/fbdev.c
+++ b/drivers/video/riva/fbdev.c
@@ -331,7 +331,7 @@ static int riva_bl_get_brightness(struct backlight_device *bd)
return bd->props.brightness;
}
-static struct backlight_ops riva_bl_ops = {
+static const struct backlight_ops riva_bl_ops = {
.get_brightness = riva_bl_get_brightness,
.update_status = riva_bl_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0ed8480..f7cc276 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6095,7 +6095,7 @@ static int brightness_get(struct backlight_device *bd)
return status & TP_EC_BACKLIGHT_LVLMSK;
}
-static struct backlight_ops ibm_backlight_data = {
+static const struct backlight_ops ibm_backlight_data = {
.get_brightness = brightness_get,
.update_status = brightness_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 51c0a8b..0786629 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -671,7 +671,7 @@ static acpi_status remove_device(void)
return AE_OK;
}
-static struct backlight_ops toshiba_backlight_data = {
+static const struct backlight_ops toshiba_backlight_data = {
.get_brightness = get_lcd,
.update_status = set_lcd_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/usb/misc/appledisplay.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index 1d8e39a..7aef786 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -178,7 +178,7 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
return pdata->msgdata[1];
}
-static struct backlight_ops appledisplay_bl_data = {
+static const struct backlight_ops appledisplay_bl_data = {
.get_brightness = appledisplay_bl_get_brightness,
.update_status = appledisplay_bl_update_status,
};
--
1.6.5.3
From: Emese Revfy <[email protected]>
Signed-off-by: Emese Revfy <[email protected]>
---
drivers/macintosh/via-pmu-backlight.c | 4 ++--
drivers/video/aty/atyfb_base.c | 2 +-
drivers/video/bf54x-lq043fb.c | 2 +-
drivers/video/bfin-t350mcqb-fb.c | 2 +-
drivers/video/omap2/displays/panel-taal.c | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/macintosh/via-pmu-backlight.c b/drivers/macintosh/via-pmu-backlight.c
index a348bb0..ecd9b3f 100644
--- a/drivers/macintosh/via-pmu-backlight.c
+++ b/drivers/macintosh/via-pmu-backlight.c
@@ -15,7 +15,7 @@
#define MAX_PMU_LEVEL 0xFF
-static struct backlight_ops pmu_backlight_data;
+static const struct backlight_ops pmu_backlight_data;
static DEFINE_SPINLOCK(pmu_backlight_lock);
static int sleeping, uses_pmu_bl;
static u8 bl_curve[FB_BACKLIGHT_LEVELS];
@@ -115,7 +115,7 @@ static int pmu_backlight_get_brightness(struct backlight_device *bd)
return bd->props.brightness;
}
-static struct backlight_ops pmu_backlight_data = {
+static const struct backlight_ops pmu_backlight_data = {
.get_brightness = pmu_backlight_get_brightness,
.update_status = pmu_backlight_update_status,
diff --git a/drivers/video/aty/atyfb_base.c b/drivers/video/aty/atyfb_base.c
index 1ddeb4c..fc4e221 100644
--- a/drivers/video/aty/atyfb_base.c
+++ b/drivers/video/aty/atyfb_base.c
@@ -2225,7 +2225,7 @@ static int aty_bl_get_brightness(struct backlight_device *bd)
return bd->props.brightness;
}
-static struct backlight_ops aty_bl_data = {
+static const struct backlight_ops aty_bl_data = {
.get_brightness = aty_bl_get_brightness,
.update_status = aty_bl_update_status,
};
diff --git a/drivers/video/bf54x-lq043fb.c b/drivers/video/bf54x-lq043fb.c
index e49ae5e..db4e6f7 100644
--- a/drivers/video/bf54x-lq043fb.c
+++ b/drivers/video/bf54x-lq043fb.c
@@ -463,7 +463,7 @@ static int bl_get_brightness(struct backlight_device *bd)
return 0;
}
-static struct backlight_ops bfin_lq043fb_bl_ops = {
+static const struct backlight_ops bfin_lq043fb_bl_ops = {
.get_brightness = bl_get_brightness,
};
diff --git a/drivers/video/bfin-t350mcqb-fb.c b/drivers/video/bfin-t350mcqb-fb.c
index 5cc36cf..4e06416 100644
--- a/drivers/video/bfin-t350mcqb-fb.c
+++ b/drivers/video/bfin-t350mcqb-fb.c
@@ -381,7 +381,7 @@ static int bl_get_brightness(struct backlight_device *bd)
return 0;
}
-static struct backlight_ops bfin_lq043fb_bl_ops = {
+static const struct backlight_ops bfin_lq043fb_bl_ops = {
.get_brightness = bl_get_brightness,
};
diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
index 1f01dfc..a5f7f82 100644
--- a/drivers/video/omap2/displays/panel-taal.c
+++ b/drivers/video/omap2/displays/panel-taal.c
@@ -313,7 +313,7 @@ static int taal_bl_get_intensity(struct backlight_device *dev)
return 0;
}
-static struct backlight_ops taal_bl_ops = {
+static const struct backlight_ops taal_bl_ops = {
.get_brightness = taal_bl_get_intensity,
.update_status = taal_bl_update_status,
};
--
1.6.5.3
Hi
> From: Emese Revfy <[email protected]>
>
> Signed-off-by: Emese Revfy <[email protected]>
> ---
> drivers/platform/x86/fujitsu-laptop.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index bcd4ba8..a249b35 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -436,7 +436,7 @@ static int bl_update_status(struct backlight_device *b)
> return ret;
> }
>
> -static struct backlight_ops fujitsubl_ops = {
> +static const struct backlight_ops fujitsubl_ops = {
> .get_brightness = bl_get_brightness,
> .update_status = bl_update_status,
> };
I have no problem with this, If this is the "accepted" convention here then
let's go for it.
Acked-by: Jonathan Woithe <[email protected]>
Regards
jonathan
On Mon, Dec 14, 2009 at 12:59:08AM +0100, [email protected] wrote:
> From: Emese Revfy <[email protected]>
>
> Hello everyone!
>
> The following patch series attempts to constify several structures
> that hold function pointers. This is only the initial batch, there
> are about over 150 candidate structures, some of which can be
> constified as well, I plan to submit them in the future.
What a complete waste of time. Until you respond to Al's:
This is ugly and has all marks of cargo-cult patches. NAKed at least
until you give exceptionally good reasons for doing that.
you should stop sending patches.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Matthew Wilcox wrote:
> On Mon, Dec 14, 2009 at 12:59:08AM +0100, [email protected] wrote:
>> From: Emese Revfy <[email protected]>
>>
>> Hello everyone!
>>
>> The following patch series attempts to constify several structures
>> that hold function pointers. This is only the initial batch, there
>> are about over 150 candidate structures, some of which can be
>> constified as well, I plan to submit them in the future.
>
> What a complete waste of time. Until you respond to Al's:
I did: http://lkml.org/lkml/2009/12/5/140
For even more discussion see: http://lkml.org/lkml/2009/12/6/111
--
Emese
On Mon, Dec 14, 2009 at 02:33:27AM +0100, Emese Revfy wrote:
> Matthew Wilcox wrote:
> > On Mon, Dec 14, 2009 at 12:59:08AM +0100, [email protected] wrote:
> >> The following patch series attempts to constify several structures
> >> that hold function pointers. This is only the initial batch, there
> >> are about over 150 candidate structures, some of which can be
> >> constified as well, I plan to submit them in the future.
> >
> > What a complete waste of time. Until you respond to Al's:
>
> I did: http://lkml.org/lkml/2009/12/5/140
>
> For even more discussion see: http://lkml.org/lkml/2009/12/6/111
>
Since you seem to have both the interest and abundance of spare time
for working on this, have you considered just doing this in sparse? Al
mentioned it here:
http://lkml.org/lkml/2009/12/8/511
which you don't seem to have replied to.
Until such a consensus is reached one way or the other, please refrain
from sending hundreds of patches -- one or two are sufficient for showing
what you want to do until folks are on board with it, as is the typical
nature of mechanical changes.
Paul Mundt wrote:
> On Mon, Dec 14, 2009 at 02:33:27AM +0100, Emese Revfy wrote:
>> Matthew Wilcox wrote:
>>> On Mon, Dec 14, 2009 at 12:59:08AM +0100, [email protected] wrote:
>>>> The following patch series attempts to constify several structures
>>>> that hold function pointers. This is only the initial batch, there
>>>> are about over 150 candidate structures, some of which can be
>>>> constified as well, I plan to submit them in the future.
>>> What a complete waste of time. Until you respond to Al's:
>> I did: http://lkml.org/lkml/2009/12/5/140
>>
>> For even more discussion see: http://lkml.org/lkml/2009/12/6/111
>>
> Since you seem to have both the interest and abundance of spare time
> for working on this, have you considered just doing this in sparse? Al
> mentioned it here:
>
> http://lkml.org/lkml/2009/12/8/511
>
> which you don't seem to have replied to.
Please see my thoughts on sparse and related topics:
http://lkml.org/lkml/2009/12/10/283
> Until such a consensus is reached one way or the other, please refrain
> from sending hundreds of patches -- one or two are sufficient for showing
> what you want to do until folks are on board with it, as is the typical
> nature of mechanical changes.
I think there is consensus to constify ops variables as much as
possible (e.g., Alexey's similar patches).
The discussions in these threads were about constifying the ops structure
fields themselves and I already explained why they are useful, see the
above link and this one: http://lkml.org/lkml/2009/12/8/492
--
Emese
On Mon 2009-12-14 02:33:27, Emese Revfy wrote:
> Matthew Wilcox wrote:
> > On Mon, Dec 14, 2009 at 12:59:08AM +0100, [email protected] wrote:
> >> From: Emese Revfy <[email protected]>
> >>
> >> Hello everyone!
> >>
> >> The following patch series attempts to constify several structures
> >> that hold function pointers. This is only the initial batch, there
> >> are about over 150 candidate structures, some of which can be
> >> constified as well, I plan to submit them in the future.
> >
> > What a complete waste of time. Until you respond to Al's:
>
> I did: http://lkml.org/lkml/2009/12/5/140
Yes, you said we should ignore the patches.
> For even more discussion see: http://lkml.org/lkml/2009/12/6/111
I don't think you explained why we want the patches.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> > Until such a consensus is reached one way or the other, please refrain
> > from sending hundreds of patches -- one or two are sufficient for showing
> > what you want to do until folks are on board with it, as is the typical
> > nature of mechanical changes.
>
> I think there is consensus to constify ops variables as much as
> possible (e.g., Alexey's similar patches).
No such consensus exists. It is very clear from the patch reactions.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Dec 14, 2009 at 08:08:44AM +0100, Emese Revfy wrote:
> Paul Mundt wrote:
> > On Mon, Dec 14, 2009 at 02:33:27AM +0100, Emese Revfy wrote:
> >> Matthew Wilcox wrote:
> >>> On Mon, Dec 14, 2009 at 12:59:08AM +0100, [email protected] wrote:
> >>>> The following patch series attempts to constify several structures
> >>>> that hold function pointers. This is only the initial batch, there
> >>>> are about over 150 candidate structures, some of which can be
> >>>> constified as well, I plan to submit them in the future.
> >>> What a complete waste of time. Until you respond to Al's:
> >> I did: http://lkml.org/lkml/2009/12/5/140
> >>
> >> For even more discussion see: http://lkml.org/lkml/2009/12/6/111
> >>
> > Since you seem to have both the interest and abundance of spare time
> > for working on this, have you considered just doing this in sparse? Al
> > mentioned it here:
> >
> > http://lkml.org/lkml/2009/12/8/511
> >
> > which you don't seem to have replied to.
>
> Please see my thoughts on sparse and related topics:
> http://lkml.org/lkml/2009/12/10/283
>
I don't see anything relating to sparse in that mail. You've effectively
lumped sparse and constification together in the same camp, but it's
unclear why this makes constification a better option other than that
it's simply the option you opted for. All of your arguments "against"
sparse in that context are equally applicable to constification, so I'll
reiterate that you haven't sufficiently addressed the sparse angle.
At present you seem to be the only one convinced that constification is
the way to go, despite it being highly intrusive and ignoring the
potential for more favourable and less intrusive options. You've also
failed to adequately address the issues and suggestsions pointed out by
others, and until this happens there is little point in posting any
follow-up patches.
> > Until such a consensus is reached one way or the other, please refrain
> > from sending hundreds of patches -- one or two are sufficient for showing
> > what you want to do until folks are on board with it, as is the typical
> > nature of mechanical changes.
>
> I think there is consensus to constify ops variables as much as
> possible (e.g., Alexey's similar patches).
>
> The discussions in these threads were about constifying the ops structure
> fields themselves and I already explained why they are useful, see the
> above link and this one: http://lkml.org/lkml/2009/12/8/492
And in here as well in the reply to that mail the same criticism exists
as does the suggestion to look at doing it cleanly in sparse, which
brings us back to what was already mentioned earlier.
Thinking you have consensus because you don't see a difference and don't
bother replying to the feedback you've gotten doesn't bode well for the
future of your patch series or killfile avoidance strategy.
On Mon, 14 Dec 2009 12:26:56 +0100
Pavel Machek <[email protected]> wrote:
> > > Until such a consensus is reached one way or the other, please
> > > refrain from sending hundreds of patches -- one or two are
> > > sufficient for showing what you want to do until folks are on
> > > board with it, as is the typical nature of mechanical changes.
> >
> > I think there is consensus to constify ops variables as much as
> > possible (e.g., Alexey's similar patches).
>
> No such consensus exists. It is very clear from the patch reactions.
I for one am not opposed to using const where we could be using const.
It's a fundamental language feature, that helps the compiler and
developer. Yes there is sparse, and no, almost nobody uses that.
If it's const, it won't be and can't be changed, allowing
more aggressive optimization. It also means all these structures get
put in the rodata section, which means by definition they can no longer
have false sharing with data structures that are written to,.. and that
section is often even really read only (cpu protection bits), which is
also a nice, but secundary; _ops are one of those targets for rootkits
and accidental overwrites.
Rodata is also something that in theory we can replicate between numa
nodes (we don't do that yet I think, but it's one of those
chicken-and-egg things.. once rodata contains enough hot/critical
structures it becomes worth it)
Now, a 300 patch series to lkml is not the way to do this.
First step is to make checkpatch.pl warn about new cases.
Second step should be to convert all definitions, but using the "one
patch per maintainer" rule, not "one patch per file" rule. Yes it's
somewhat janitorial, but no it's not a big deal as long as it's not 300
patches to lkml. And it is much better than whitespace changes; it's
a real quality improvement to the kernel (in terms of code generation
and API)
Once everything is converted after a kernel release or two, you can
consider the third step of changing the actual prototypes/definitions,
which at that point is a small one-liner that isn't a big deal.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Mon, Dec 14, 2009 at 08:00:49AM -0800, Arjan van de Ven wrote:
> I for one am not opposed to using const where we could be using const.
> It's a fundamental language feature, that helps the compiler and
> developer. Yes there is sparse, and no, almost nobody uses that.
>
> If it's const, it won't be and can't be changed, allowing
> more aggressive optimization. It also means all these structures get
> put in the rodata section, which means by definition they can no longer
> have false sharing with data structures that are written to,.. and that
> section is often even really read only (cpu protection bits), which is
> also a nice, but secundary; _ops are one of those targets for rootkits
> and accidental overwrites.
Yes, but that's accomplished by tagging each of the arrays of function
pointers in question as const. Not by tagging the individual members
within the structure definition as const, which is what this patch
series does.
I don't think anyone objects to what you're trying to accomplish, but
this series of patches, as Al says, feel like pure cargo-culting.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Mon 2009-12-14 08:00:49, Arjan van de Ven wrote:
> On Mon, 14 Dec 2009 12:26:56 +0100
> Pavel Machek <[email protected]> wrote:
>
> > > > Until such a consensus is reached one way or the other, please
> > > > refrain from sending hundreds of patches -- one or two are
> > > > sufficient for showing what you want to do until folks are on
> > > > board with it, as is the typical nature of mechanical changes.
> > >
> > > I think there is consensus to constify ops variables as much as
> > > possible (e.g., Alexey's similar patches).
> >
> > No such consensus exists. It is very clear from the patch reactions.
>
> I for one am not opposed to using const where we could be using
> const.
I certainly object "constify ops... as much as possible". If it
uglifies the code, it should not be done. If it is as simple as adding
const to few lines, its probably ok.
But .... the patch contained huge load of
- int (* resume)()
+ int (* const resume)()
What is that?
> Now, a 300 patch series to lkml is not the way to do this.
> First step is to make checkpatch.pl warn about new cases.
> Second step should be to convert all definitions, but using the "one
> patch per maintainer" rule, not "one patch per file" rule. Yes it's
The zeroth step is get one small series accepted. I'd suggest VFS for
a start.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, 14 Dec 2009 22:25:26 +0100
Pavel Machek <[email protected]> wrote:
> On Mon 2009-12-14 08:00:49, Arjan van de Ven wrote:
> > On Mon, 14 Dec 2009 12:26:56 +0100
> > Pavel Machek <[email protected]> wrote:
> >
> > > > > Until such a consensus is reached one way or the other, please
> > > > > refrain from sending hundreds of patches -- one or two are
> > > > > sufficient for showing what you want to do until folks are on
> > > > > board with it, as is the typical nature of mechanical changes.
> > > >
> > > > I think there is consensus to constify ops variables as much as
> > > > possible (e.g., Alexey's similar patches).
> > >
> > > No such consensus exists. It is very clear from the patch
> > > reactions.
> >
> > I for one am not opposed to using const where we could be using
> > const.
>
> I certainly object "constify ops... as much as possible". If it
> uglifies the code, it should not be done. If it is as simple as adding
> const to few lines, its probably ok.
>
> But .... the patch contained huge load of
>
> - int (* resume)()
> + int (* const resume)()
>
> What is that?
the ops stuct instantiation itself should be const.
the members not so much; that makes no sense.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
Paul Mundt wrote:
> I don't see anything relating to sparse in that mail. You've effectively
> lumped sparse and constification together in the same camp, but it's
> unclear why this makes constification a better option other than that
> it's simply the option you opted for. All of your arguments "against"
> sparse in that context are equally applicable to constification, so I'll
> reiterate that you haven't sufficiently addressed the sparse angle.
>
> At present you seem to be the only one convinced that constification is
> the way to go, despite it being highly intrusive and ignoring the
> potential for more favourable and less intrusive options. You've also
> failed to adequately address the issues and suggestsions pointed out by
> others, and until this happens there is little point in posting any
> follow-up patches.
>
>>> Until such a consensus is reached one way or the other, please refrain
>>> from sending hundreds of patches -- one or two are sufficient for showing
>>> what you want to do until folks are on board with it, as is the typical
>>> nature of mechanical changes.
>> I think there is consensus to constify ops variables as much as
>> possible (e.g., Alexey's similar patches).
>>
>> The discussions in these threads were about constifying the ops structure
>> fields themselves and I already explained why they are useful, see the
>> above link and this one: http://lkml.org/lkml/2009/12/8/492
>
> And in here as well in the reply to that mail the same criticism exists
> as does the suggestion to look at doing it cleanly in sparse, which
> brings us back to what was already mentioned earlier.
Let me summarise the discussion so far:
As per Al Viro, Arjan and other developers the goal is to force
static allocations and prevent runtime modification of ops structures
(where it is possible, there are always exceptions like ata_port_operations).
The current strategy of constifying variables achieves the second goal only,
it still requires human review to catch violations of the first goal.
This is where consitfying the structure field becomes important: it prevents
direct modifications of runtime allocated ops structures therefore it
gives a strong signal to the programmer that he's trying to do something
undesired (this approach is in fact already used in the kernel, see: iwl_ops).
There is another benefit in that static but non-const ops structures cannot be
directly modified either, therefore it will be easier to make them const later.
Of course both constification efforts can be bypassed, a "clever" programmer can
write code in many ways that will write to otherwise "const" structures.
Nor is it possible to detect all such attempts by tools in fact, it would be
equivalent to solving the halting problem.
Therefore I think that it's a lot easier to have the compiler detect unwanted
direct modifications by constifying the structure fields than use sparse (which,
unlike a compiler, isn't used by everyone and would require more complex changes
than field constification for no real gain). In any case, constifying structure
fields is not exclusive of teaching sparse or other tools like checkpatch about
some bad code constructs, I will try my best on checkpatch.
To wrap it all up: human review will always be required to catch bad code and
we can help the process if we force would-be violators to go to lengths to
bypass the policy and make it easy for the reviewer to notice that something
is up.
> Thinking you have consensus because you don't see a difference and don't
> bother replying to the feedback you've gotten doesn't bode well for the
> future of your patch series or killfile avoidance strategy.
Please let me know whose feedback I didn't address.
--
Emese
On Mon 2009-12-14 14:17:57, Arjan van de Ven wrote:
> On Mon, 14 Dec 2009 22:25:26 +0100
> Pavel Machek <[email protected]> wrote:
>
> > On Mon 2009-12-14 08:00:49, Arjan van de Ven wrote:
> > > On Mon, 14 Dec 2009 12:26:56 +0100
> > > Pavel Machek <[email protected]> wrote:
> > >
> > > > > > Until such a consensus is reached one way or the other, please
> > > > > > refrain from sending hundreds of patches -- one or two are
> > > > > > sufficient for showing what you want to do until folks are on
> > > > > > board with it, as is the typical nature of mechanical changes.
> > > > >
> > > > > I think there is consensus to constify ops variables as much as
> > > > > possible (e.g., Alexey's similar patches).
> > > >
> > > > No such consensus exists. It is very clear from the patch
> > > > reactions.
> > >
> > > I for one am not opposed to using const where we could be using
> > > const.
> >
> > I certainly object "constify ops... as much as possible". If it
> > uglifies the code, it should not be done. If it is as simple as adding
> > const to few lines, its probably ok.
> >
> > But .... the patch contained huge load of
> >
> > - int (* resume)()
> > + int (* const resume)()
> >
> > What is that?
>
> the ops stuct instantiation itself should be const.
> the members not so much; that makes no sense.
I thought so; but that was half of the patches I saw, therefore
complains...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Arjan van de Ven wrote:
> On Mon, 14 Dec 2009 22:25:26 +0100
> Pavel Machek <[email protected]> wrote:
>
>> On Mon 2009-12-14 08:00:49, Arjan van de Ven wrote:
>>> On Mon, 14 Dec 2009 12:26:56 +0100
>>> Pavel Machek <[email protected]> wrote:
>> I certainly object "constify ops... as much as possible". If it
>> uglifies the code, it should not be done. If it is as simple as adding
>> const to few lines, its probably ok.
>>
>> But .... the patch contained huge load of
>>
>> - int (* resume)()
>> + int (* const resume)()
>>
>> What is that?
>
> the ops stuct instantiation itself should be const.
> the members not so much; that makes no sense.
Consitfying the structure fields prevents direct modifications of runtime
allocated ops structures therefore it gives a strong signal to the programmer
that he's trying to do something undesired (this approach is in fact already
used in the kernel, see: iwl_ops).
There is another benefit in that static but non-const ops structures cannot be
directly modified either, therefore it will be easier to make them const later.
Example:
1 struct a {
2 void (* f)(void);
3 void (* const g)(void);
4 } s;
5
6 void h(void)
7 {
8 struct a *p = &s;
9 s.f = 0;
10 s.g = 0;
11 p->f = 0;
12 p->g = 0;
13 }
gcc -c a.c
a.c: In function 'h':
a.c:10: error: assignment of read-only member 'g'
a.c:12: error: assignment of read-only member 'g'
--
Emese
Arjan van de Ven wrote:
> Now, a 300 patch series to lkml is not the way to do this.
> First step is to make checkpatch.pl warn about new cases.
I will add this structures the checkpatch in the next patch series.
> Second step should be to convert all definitions, but using the "one
> patch per maintainer" rule, not "one patch per file" rule. Yes it's
> somewhat janitorial, but no it's not a big deal as long as it's not 300
> patches to lkml. And it is much better than whitespace changes; it's
> a real quality improvement to the kernel (in terms of code generation
> and API)
The first series was based on one patch per structure type, the current
split-up is based on one patch per structure type *and* subsystem as
suggested by Greg KH. I used get_maintainer --subsystem to find them which
resulted in 150 patches. If you know a better way or want per-type patches,
please let me know (and discuss it with Greg ;).
--
Emese
On Mon, 14 Dec 2009 23:20:58 +0100
Emese Revfy <[email protected]> wrote:
> Paul Mundt wrote:
> > I don't see anything relating to sparse in that mail. You've
> > effectively lumped sparse and constification together in the same
> > camp, but it's unclear why this makes constification a better
> > option other than that it's simply the option you opted for. All of
> > your arguments "against" sparse in that context are equally
> > applicable to constification, so I'll reiterate that you haven't
> > sufficiently addressed the sparse angle.
> >
> > At present you seem to be the only one convinced that
> > constification is the way to go, despite it being highly intrusive
> > and ignoring the potential for more favourable and less intrusive
> > options. You've also failed to adequately address the issues and
> > suggestsions pointed out by others, and until this happens there is
> > little point in posting any follow-up patches.
> >
> >>> Until such a consensus is reached one way or the other, please
> >>> refrain from sending hundreds of patches -- one or two are
> >>> sufficient for showing what you want to do until folks are on
> >>> board with it, as is the typical nature of mechanical changes.
> >> I think there is consensus to constify ops variables as much as
> >> possible (e.g., Alexey's similar patches).
> >>
> >> The discussions in these threads were about constifying the ops
> >> structure fields themselves and I already explained why they are
> >> useful, see the above link and this one:
> >> http://lkml.org/lkml/2009/12/8/492
> >
> > And in here as well in the reply to that mail the same criticism
> > exists as does the suggestion to look at doing it cleanly in
> > sparse, which brings us back to what was already mentioned earlier.
>
> Let me summarise the discussion so far:
>
> As per Al Viro, Arjan and other developers the goal is to force
> static allocations and prevent runtime modification of ops structures
> (where it is possible, there are always exceptions like
> ata_port_operations).
>
> The current strategy of constifying variables achieves the second
> goal only, it still requires human review to catch violations of the
> first goal.
this is not correct.
When the ops variable is const... the compiler will also warn if you
change it. Make some core APIs use const in their parameter that gets
a pointer to the ops structure, so that the compiler can optimize.
That is all goodness.
But if someone somewhere makes one that is not const.. that's what
checkpatch.pl is for .. make it warn!
But don't crap all over structures... I agree with Pavel/Al/etc..
that's bad code without gains.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Tue 2009-12-15 00:13:17, Emese Revfy wrote:
> Arjan van de Ven wrote:
> > Now, a 300 patch series to lkml is not the way to do this.
> > First step is to make checkpatch.pl warn about new cases.
>
> I will add this structures the checkpatch in the next patch series.
>
> > Second step should be to convert all definitions, but using the "one
> > patch per maintainer" rule, not "one patch per file" rule. Yes it's
> > somewhat janitorial, but no it's not a big deal as long as it's not 300
> > patches to lkml. And it is much better than whitespace changes; it's
> > a real quality improvement to the kernel (in terms of code generation
> > and API)
>
> The first series was based on one patch per structure type, the current
> split-up is based on one patch per structure type *and* subsystem as
> suggested by Greg KH. I used get_maintainer --subsystem to find them which
> resulted in 150 patches. If you know a better way or want per-type patches,
> please let me know (and discuss it with Greg ;).
Please push VFS patches first -- in small series. Unless you can
address Al Viro's objections, I do not think rest should go in (and no
need to spam l-k in the process).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> Arjan van de Ven wrote:
> > On Mon, 14 Dec 2009 22:25:26 +0100
> > Pavel Machek <[email protected]> wrote:
> >
> >> On Mon 2009-12-14 08:00:49, Arjan van de Ven wrote:
> >>> On Mon, 14 Dec 2009 12:26:56 +0100
> >>> Pavel Machek <[email protected]> wrote:
> >> I certainly object "constify ops... as much as possible". If it
> >> uglifies the code, it should not be done. If it is as simple as adding
> >> const to few lines, its probably ok.
> >>
> >> But .... the patch contained huge load of
> >>
> >> - int (* resume)()
> >> + int (* const resume)()
> >>
> >> What is that?
> >
> > the ops stuct instantiation itself should be const.
> > the members not so much; that makes no sense.
>
> Consitfying the structure fields prevents direct modifications of runtime
> allocated ops structures therefore it gives a strong signal to the programmer
> that he's trying to do something undesired (this approach is in fact already
> used in the kernel, see: iwl_ops).
One const in structure declaration seems to be just enough, see:
const struct a {
void (* f)(void);
void (* const g)(void);
} s;
void h(void)
{
struct a *p = &s;
s.f = 0;
s.g = 0;
p->f = 0;
p->g = 0;
}
delme.c: In function 'h':
delme.c:8: warning: initialization discards qualifiers from pointer target type
delme.c:9: error: assignment of read-only variable 's'
delme.c:10: error: assignment of read-only variable 's'
delme.c:12: error: assignment of read-only member 'g'
You get clean-enough warnings.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Dec 14, 2009 at 08:00:49AM -0800, Arjan van de Ven wrote:
> On Mon, 14 Dec 2009 12:26:56 +0100
> I for one am not opposed to using const where we could be using const.
> It's a fundamental language feature, that helps the compiler and
> developer. Yes there is sparse, and no, almost nobody uses that.
And as just about any language feature it can be used to make the thing
less readable.
Note that marking a pointer to method table as pointer to const is just fine;
nobody AFAICS has any problems with that. It's "let's make pointers to
methods themselves const" that gets flamed.
On Mon, 2009-12-14 at 00:58 +0100, [email protected] wrote:
> From: Emese Revfy <[email protected]>
>
>
> Signed-off-by: Emese Revfy <[email protected]>
Applied to the backlight tree, thanks.
Richard
--
Richard Purdie
Intel Open Source Technology Centre
Pavel Machek wrote:
> Hi!
>
>> Arjan van de Ven wrote:
>>> On Mon, 14 Dec 2009 22:25:26 +0100
>>> Pavel Machek <[email protected]> wrote:
>>>
>>>> On Mon 2009-12-14 08:00:49, Arjan van de Ven wrote:
>>>>> On Mon, 14 Dec 2009 12:26:56 +0100
>>>>> Pavel Machek <[email protected]> wrote:
>>>> I certainly object "constify ops... as much as possible". If it
>>>> uglifies the code, it should not be done. If it is as simple as adding
>>>> const to few lines, its probably ok.
>>>>
>>>> But .... the patch contained huge load of
>>>>
>>>> - int (* resume)()
>>>> + int (* const resume)()
>>>>
>>>> What is that?
>>> the ops stuct instantiation itself should be const.
>>> the members not so much; that makes no sense.
>> Consitfying the structure fields prevents direct modifications of runtime
>> allocated ops structures therefore it gives a strong signal to the programmer
>> that he's trying to do something undesired (this approach is in fact already
>> used in the kernel, see: iwl_ops).
>
> One const in structure declaration seems to be just enough, see:
>
> const struct a {
> void (* f)(void);
> void (* const g)(void);
> } s;
>
> void h(void)
> {
> struct a *p = &s;
> s.f = 0;
> s.g = 0;
> p->f = 0;
> p->g = 0;
> }
>
>
> delme.c: In function 'h':
> delme.c:8: warning: initialization discards qualifiers from pointer target type
> delme.c:9: error: assignment of read-only variable 's'
> delme.c:10: error: assignment of read-only variable 's'
> delme.c:12: error: assignment of read-only member 'g'
>
> You get clean-enough warnings.
Pave
Notice how you got an error for line 12 (p->g assignment) but no warning or error
at all for line 11 (p->f assignment). This example illustrates what I was explaining
so far:
if you dynamically allocate an ops structure (the result of which is a pointer type like
p in the above example) then with a non-const structure field you get no indication
from the compiler that you are doing something undesired, whereas with a const
structure field you get an error immediately. This is what helps a future developer
as it gives him a hint that he is doing something wrong and if he still insists on his
way of dynamic allocation, he will have to come up with ugly code
(e.g., void *(**)(void))(&p->g) = 0) that will not pass human review. You can teach gcc,
sparse, checkpatch, etc to recognize some of this ugliness but you cannot
programmatically detect all possible ways of evasion.
And if the compiler can help the developers, why not make use of it?
Note also that a const structure field helps the statically allocated non-const
variable case as well as the compiler will error out on such field modifications
(s.g assignment in my example) so the developer will again get a hint that he is
doing something undesired and will have to use direct initialisation (or write
the same ugly code as above that will not pass human review)
--
Emese
Arjan van de Ven wrote:
> On Mon, 14 Dec 2009 23:20:58 +0100
> Emese Revfy <[email protected]> wrote:
>
>> Paul Mundt wrote:
>>> I don't see anything relating to sparse in that mail. You've
>>> effectively lumped sparse and constification together in the same
>>> camp, but it's unclear why this makes constification a better
>>> option other than that it's simply the option you opted for. All of
>>> your arguments "against" sparse in that context are equally
>>> applicable to constification, so I'll reiterate that you haven't
>>> sufficiently addressed the sparse angle.
>>>
>>> At present you seem to be the only one convinced that
>>> constification is the way to go, despite it being highly intrusive
>>> and ignoring the potential for more favourable and less intrusive
>>> options. You've also failed to adequately address the issues and
>>> suggestsions pointed out by others, and until this happens there is
>>> little point in posting any follow-up patches.
>>>
>>>>> Until such a consensus is reached one way or the other, please
>>>>> refrain from sending hundreds of patches -- one or two are
>>>>> sufficient for showing what you want to do until folks are on
>>>>> board with it, as is the typical nature of mechanical changes.
>>>> I think there is consensus to constify ops variables as much as
>>>> possible (e.g., Alexey's similar patches).
>>>>
>>>> The discussions in these threads were about constifying the ops
>>>> structure fields themselves and I already explained why they are
>>>> useful, see the above link and this one:
>>>> http://lkml.org/lkml/2009/12/8/492
>>> And in here as well in the reply to that mail the same criticism
>>> exists as does the suggestion to look at doing it cleanly in
>>> sparse, which brings us back to what was already mentioned earlier.
>> Let me summarise the discussion so far:
>>
>> As per Al Viro, Arjan and other developers the goal is to force
>> static allocations and prevent runtime modification of ops structures
>> (where it is possible, there are always exceptions like
>> ata_port_operations).
>>
>> The current strategy of constifying variables achieves the second
>> goal only, it still requires human review to catch violations of the
>> first goal.
>
> this is not correct.
>
> When the ops variable is const... the compiler will also warn if you
> change it. Make some core APIs use const in their parameter that gets
> a pointer to the ops structure, so that the compiler can optimize.
> That is all goodness.
>
> But if someone somewhere makes one that is not const.. that's what
> checkpatch.pl is for .. make it warn!
> But don't crap all over structures... I agree with Pavel/Al/etc..
> that's bad code without gains.
I still think it is a good idea for several reasons (see my last
response to Pavel, http://lkml.org/lkml/2009/12/15/559), but I will
remove the field constifications from the next patch series.
As for splitting up the patches, do you all agree that it should
be one series per structure type at a time (as suggested by Pavel),
with each patch mailed to the respective maintainers? If so,
how can I reliably determine the maintainers of a given file
without spamming too many people?
--
Emese
On Wed, Dec 16, 2009 at 12:28:36AM +0100, Emese Revfy wrote:
> Notice how you got an error for line 12 (p->g assignment) but no warning or error
> at all for line 11 (p->f assignment). This example illustrates what I was explaining
> so far:
>
> if you dynamically allocate an ops structure (the result of which is a pointer type like
> p in the above example) then with a non-const structure field you get no indication
> from the compiler that you are doing something undesired, whereas with a const
> structure field you get an error immediately. This is what helps a future developer
> as it gives him a hint that he is doing something wrong and if he still insists on his
> way of dynamic allocation, he will have to come up with ugly code
> (e.g., void *(**)(void))(&p->g) = 0) that will not pass human review. You can teach gcc,
> sparse, checkpatch, etc to recognize some of this ugliness but you cannot
> programmatically detect all possible ways of evasion.
> And if the compiler can help the developers, why not make use of it?
Sigh... Could you please stop assuming that nobody knows C? Your point
about const members of struct making the entire lvalue non-modifiable
is *NOT* *ARGUED*. It is correct.
HOWEVER, such use of const is highly non-idiomatic. Combined with the
general clumsiness of syntax for qualifiers, it creates a stumbling
block for human readers.
Compiler is not the only thing that has to parse C; programmers need to do
the same and hitting slow paths in our parsers is a Bad Thing(tm).
_The_ requirement for any kind of annotations is non-intrusiveness for
casual human reader. And that's what is violated here.
You are trying to express a property of struct; however, it ends up spread
all over the members' declarators. Bad for readers...
Hi!
> > One const in structure declaration seems to be just enough, see:
> >
> > const struct a {
> > void (* f)(void);
> > void (* const g)(void);
> > } s;
> >
> > void h(void)
> > {
> > struct a *p = &s;
> > s.f = 0;
> > s.g = 0;
> > p->f = 0;
> > p->g = 0;
> > }
> >
> >
> > delme.c: In function 'h':
> > delme.c:8: warning: initialization discards qualifiers from pointer target type
> > delme.c:9: error: assignment of read-only variable 's'
> > delme.c:10: error: assignment of read-only variable 's'
> > delme.c:12: error: assignment of read-only member 'g'
> >
> > You get clean-enough warnings.
>
> Notice how you got an error for line 12 (p->g assignment) but no warning or error
> at all for line 11 (p->f assignment). This example illustrates what I was explaining
> so far:
And notice how you get warning for line 8? That's what I'm talking
about, and it should be enough to make the developer think about what
he's doing.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek wrote:
> Hi!
>
>>> One const in structure declaration seems to be just enough, see:
>>>
>>> const struct a {
>>> void (* f)(void);
>>> void (* const g)(void);
>>> } s;
>>>
>>> void h(void)
>>> {
>>> struct a *p = &s;
>>> s.f = 0;
>>> s.g = 0;
>>> p->f = 0;
>>> p->g = 0;
>>> }
>>>
>>>
>>> delme.c: In function 'h':
>>> delme.c:8: warning: initialization discards qualifiers from pointer target type
>>> delme.c:9: error: assignment of read-only variable 's'
>>> delme.c:10: error: assignment of read-only variable 's'
>>> delme.c:12: error: assignment of read-only member 'g'
>>>
>>> You get clean-enough warnings.
>> Notice how you got an error for line 12 (p->g assignment) but no warning or error
>> at all for line 11 (p->f assignment). This example illustrates what I was explaining
>> so far:
>
> And notice how you get warning for line 8? That's what I'm talking
> about, and it should be enough to make the developer think about what
> he's doing.
> Pavel
You are talking about in-kernel ops structures whose constness will
prevent bad code from being written. On the other hand, I was talking
about new code yet-to-enter the kernel where the developer has no indication
that he should be using a const ops structure (other than perhaps checkpatch
except apparently things easily fall through the cracks, see my series for
file_operations) and this is where const structure fields would help.
In any case, as I indicated already, I will remove these parts from the patches.
--
Emese
Richard Purdie wrote:
> On Mon, 2009-12-14 at 00:58 +0100, [email protected] wrote:
>> From: Emese Revfy <[email protected]>
>>
>>
>> Signed-off-by: Emese Revfy <[email protected]>
>
> Applied to the backlight tree, thanks.
>
> Richard
Thank you! Please note that this patch series contains a hunk that
constifies structure fields as well which raised some controversy
among other developer (see the address_space_operations sub-thread
for details). If you also don't feel comfortable with that change,
please let me know and I will resend the patches without that hunk.
--
Emese