| Message ID | 1245122593.3583.117.camel@localhost.localdomain (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi Yakui, * yakui_zhao <yakui.zhao@intel.com>: > From: Zhao Yakui <yakui.zhao@intel.com> > > Sometimes both acpi video and i915 driver are compiled as modules. > And there exists the strict dependency between the two drivers. > The acpi video bus will be unloaded in course of unloading the i915 driver. > If we unload the acpi video driver, then the kernel oops will be triggered. > > Add the reference count to avoid unloading the ACPI video bus twice. > The reference count should be checked before unregistering the acpi video bus. > If the reference count is already zero, it won't unregister it again. > And after the acpi video bus is already unregistered, the reference count > will be set to zero. Your implementation below isn't really a reference count, so I don't think you should call it that in your changelog. Since you are talking about refcounts, have you tried using try_module_get()? Thanks. /ac > > http://bugzilla.kernel.org/show_bug.cgi?id=13396 > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > --- > drivers/acpi/video.c | 41 +++++++++++++++++++++++++++++------ > drivers/gpu/drm/i915/i915_opregion.c | 2 - > include/acpi/video.h | 4 +-- > 3 files changed, 38 insertions(+), 9 deletions(-) > > Index: linux-2.6/drivers/acpi/video.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/video.c 2009-06-16 10:25:12.000000000 +0800 > +++ linux-2.6/drivers/acpi/video.c 2009-06-16 11:09:57.000000000 +0800 > @@ -76,6 +76,7 @@ > static int brightness_switch_enabled = 1; > module_param(brightness_switch_enabled, bool, 0644); > > +static int register_count = 0; > static int acpi_video_bus_add(struct acpi_device *device); > static int acpi_video_bus_remove(struct acpi_device *device, int type); > static int acpi_video_resume(struct acpi_device *device); > @@ -2318,6 +2319,13 @@ > int acpi_video_register(void) > { > int result = 0; > + if (register_count) { > + /* > + * if the function of acpi_video_register is already called, > + * don't register the acpi_vide_bus again and return no error. > + */ > + return 0; > + } > > acpi_video_dir = proc_mkdir(ACPI_VIDEO_CLASS, acpi_root_dir); > if (!acpi_video_dir) > @@ -2329,10 +2337,35 @@ > return -ENODEV; > } > > + /* > + * When the acpi_video_bus is loaded successfully, increase > + * the counter reference. > + */ > + register_count = 1; > + > return 0; > } > EXPORT_SYMBOL(acpi_video_register); > > +void acpi_video_unregister(void) > +{ > + if (!register_count) { > + /* > + * If the acpi video bus is already unloaded, don't > + * unload it again and return directly. > + */ > + return; > + } > + acpi_bus_unregister_driver(&acpi_video_bus); > + > + remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir); > + > + register_count = 0; > + > + return; > +} > +EXPORT_SYMBOL(acpi_video_unregister); > + > /* > * This is kind of nasty. Hardware using Intel chipsets may require > * the video opregion code to be run first in order to initialise > @@ -2350,16 +2383,12 @@ > return acpi_video_register(); > } > > -void acpi_video_exit(void) > +static void __exit acpi_video_exit(void) > { > - > - acpi_bus_unregister_driver(&acpi_video_bus); > - > - remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir); > + acpi_video_unregister(); > > return; > } > -EXPORT_SYMBOL(acpi_video_exit); > > module_init(acpi_video_init); > module_exit(acpi_video_exit); > Index: linux-2.6/drivers/gpu/drm/i915/i915_opregion.c > =================================================================== > --- linux-2.6.orig/drivers/gpu/drm/i915/i915_opregion.c 2009-06-16 10:25:12.000000000 +0800 > +++ linux-2.6/drivers/gpu/drm/i915/i915_opregion.c 2009-06-16 10:29:50.000000000 +0800 > @@ -419,7 +419,7 @@ > return; > > if (!suspend) > - acpi_video_exit(); > + acpi_video_unregister(); > > opregion->acpi->drdy = 0; > > Index: linux-2.6/include/acpi/video.h > =================================================================== > --- linux-2.6.orig/include/acpi/video.h 2009-06-16 10:25:12.000000000 +0800 > +++ linux-2.6/include/acpi/video.h 2009-06-16 11:07:43.000000000 +0800 > @@ -3,10 +3,10 @@ > > #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) > extern int acpi_video_register(void); > -extern int acpi_video_exit(void); > +extern void acpi_video_unregister(void); > #else > static inline int acpi_video_register(void) { return 0; } > -static inline void acpi_video_exit(void) { return; } > +static inline void acpi_video_unregister(void) { return; } > #endif > > #endif > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-06-16 at 22:50 +0800, Alex Chiang wrote: > Hi Yakui, > > * yakui_zhao <yakui.zhao@intel.com>: > > From: Zhao Yakui <yakui.zhao@intel.com> > > > > Sometimes both acpi video and i915 driver are compiled as modules. > > And there exists the strict dependency between the two drivers. > > The acpi video bus will be unloaded in course of unloading the i915 driver. > > If we unload the acpi video driver, then the kernel oops will be triggered. > > > > Add the reference count to avoid unloading the ACPI video bus twice. > > The reference count should be checked before unregistering the acpi video bus. > > If the reference count is already zero, it won't unregister it again. > > And after the acpi video bus is already unregistered, the reference count > > will be set to zero. > > Your implementation below isn't really a reference count, so I > don't think you should call it that in your changelog. It is only a register count, which indicates whether the acpi video bus is registered or not. And it is not a the reference count for module. Thanks. > > Since you are talking about refcounts, have you tried using > try_module_get()? > > Thanks. > > /ac > > > > > http://bugzilla.kernel.org/show_bug.cgi?id=13396 > > > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > > --- > > drivers/acpi/video.c | 41 +++++++++++++++++++++++++++++------ > > drivers/gpu/drm/i915/i915_opregion.c | 2 - > > include/acpi/video.h | 4 +-- > > 3 files changed, 38 insertions(+), 9 deletions(-) > > > > Index: linux-2.6/drivers/acpi/video.c > > =================================================================== > > --- linux-2.6.orig/drivers/acpi/video.c 2009-06-16 10:25:12.000000000 +0800 > > +++ linux-2.6/drivers/acpi/video.c 2009-06-16 11:09:57.000000000 +0800 > > @@ -76,6 +76,7 @@ > > static int brightness_switch_enabled = 1; > > module_param(brightness_switch_enabled, bool, 0644); > > > > +static int register_count = 0; > > static int acpi_video_bus_add(struct acpi_device *device); > > static int acpi_video_bus_remove(struct acpi_device *device, int type); > > static int acpi_video_resume(struct acpi_device *device); > > @@ -2318,6 +2319,13 @@ > > int acpi_video_register(void) > > { > > int result = 0; > > + if (register_count) { > > + /* > > + * if the function of acpi_video_register is already called, > > + * don't register the acpi_vide_bus again and return no error. > > + */ > > + return 0; > > + } > > > > acpi_video_dir = proc_mkdir(ACPI_VIDEO_CLASS, acpi_root_dir); > > if (!acpi_video_dir) > > @@ -2329,10 +2337,35 @@ > > return -ENODEV; > > } > > > > + /* > > + * When the acpi_video_bus is loaded successfully, increase > > + * the counter reference. > > + */ > > + register_count = 1; > > + > > return 0; > > } > > EXPORT_SYMBOL(acpi_video_register); > > > > +void acpi_video_unregister(void) > > +{ > > + if (!register_count) { > > + /* > > + * If the acpi video bus is already unloaded, don't > > + * unload it again and return directly. > > + */ > > + return; > > + } > > + acpi_bus_unregister_driver(&acpi_video_bus); > > + > > + remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir); > > + > > + register_count = 0; > > + > > + return; > > +} > > +EXPORT_SYMBOL(acpi_video_unregister); > > + > > /* > > * This is kind of nasty. Hardware using Intel chipsets may require > > * the video opregion code to be run first in order to initialise > > @@ -2350,16 +2383,12 @@ > > return acpi_video_register(); > > } > > > > -void acpi_video_exit(void) > > +static void __exit acpi_video_exit(void) > > { > > - > > - acpi_bus_unregister_driver(&acpi_video_bus); > > - > > - remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir); > > + acpi_video_unregister(); > > > > return; > > } > > -EXPORT_SYMBOL(acpi_video_exit); > > > > module_init(acpi_video_init); > > module_exit(acpi_video_exit); > > Index: linux-2.6/drivers/gpu/drm/i915/i915_opregion.c > > =================================================================== > > --- linux-2.6.orig/drivers/gpu/drm/i915/i915_opregion.c 2009-06-16 10:25:12.000000000 +0800 > > +++ linux-2.6/drivers/gpu/drm/i915/i915_opregion.c 2009-06-16 10:29:50.000000000 +0800 > > @@ -419,7 +419,7 @@ > > return; > > > > if (!suspend) > > - acpi_video_exit(); > > + acpi_video_unregister(); > > > > opregion->acpi->drdy = 0; > > > > Index: linux-2.6/include/acpi/video.h > > =================================================================== > > --- linux-2.6.orig/include/acpi/video.h 2009-06-16 10:25:12.000000000 +0800 > > +++ linux-2.6/include/acpi/video.h 2009-06-16 11:07:43.000000000 +0800 > > @@ -3,10 +3,10 @@ > > > > #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) > > extern int acpi_video_register(void); > > -extern int acpi_video_exit(void); > > +extern void acpi_video_unregister(void); > > #else > > static inline int acpi_video_register(void) { return 0; } > > -static inline void acpi_video_exit(void) { return; } > > +static inline void acpi_video_unregister(void) { return; } > > #endif > > > > #endif > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-06-16 at 11:23 +0800, yakui_zhao wrote: > From: Zhao Yakui <yakui.zhao@intel.com> > > Sometimes both acpi video and i915 driver are compiled as modules. > And there exists the strict dependency between the two drivers. > The acpi video bus will be unloaded in course of unloading the i915 driver. > If we unload the acpi video driver, then the kernel oops will be triggered. > > Add the reference count to avoid unloading the ACPI video bus twice. > The reference count should be checked before unregistering the acpi video bus. > If the reference count is already zero, it won't unregister it again. > And after the acpi video bus is already unregistered, the reference count > will be set to zero. > > http://bugzilla.kernel.org/show_bug.cgi?id=13396 > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> Acked-by: Zhang Rui <rui.zhang@intel.com> But we'd better rename the patch to something like "unregister ACPI video driver only once". thanks, rui > --- > drivers/acpi/video.c | 41 +++++++++++++++++++++++++++++------ > drivers/gpu/drm/i915/i915_opregion.c | 2 - > include/acpi/video.h | 4 +-- > 3 files changed, 38 insertions(+), 9 deletions(-) > > Index: linux-2.6/drivers/acpi/video.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/video.c 2009-06-16 10:25:12.000000000 +0800 > +++ linux-2.6/drivers/acpi/video.c 2009-06-16 11:09:57.000000000 +0800 > @@ -76,6 +76,7 @@ > static int brightness_switch_enabled = 1; > module_param(brightness_switch_enabled, bool, 0644); > > +static int register_count = 0; > static int acpi_video_bus_add(struct acpi_device *device); > static int acpi_video_bus_remove(struct acpi_device *device, int type); > static int acpi_video_resume(struct acpi_device *device); > @@ -2318,6 +2319,13 @@ > int acpi_video_register(void) > { > int result = 0; > + if (register_count) { > + /* > + * if the function of acpi_video_register is already called, > + * don't register the acpi_vide_bus again and return no error. > + */ > + return 0; > + } > > acpi_video_dir = proc_mkdir(ACPI_VIDEO_CLASS, acpi_root_dir); > if (!acpi_video_dir) > @@ -2329,10 +2337,35 @@ > return -ENODEV; > } > > + /* > + * When the acpi_video_bus is loaded successfully, increase > + * the counter reference. > + */ > + register_count = 1; > + > return 0; > } > EXPORT_SYMBOL(acpi_video_register); > > +void acpi_video_unregister(void) > +{ > + if (!register_count) { > + /* > + * If the acpi video bus is already unloaded, don't > + * unload it again and return directly. > + */ > + return; > + } > + acpi_bus_unregister_driver(&acpi_video_bus); > + > + remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir); > + > + register_count = 0; > + > + return; > +} > +EXPORT_SYMBOL(acpi_video_unregister); > + > /* > * This is kind of nasty. Hardware using Intel chipsets may require > * the video opregion code to be run first in order to initialise > @@ -2350,16 +2383,12 @@ > return acpi_video_register(); > } > > -void acpi_video_exit(void) > +static void __exit acpi_video_exit(void) > { > - > - acpi_bus_unregister_driver(&acpi_video_bus); > - > - remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir); > + acpi_video_unregister(); > > return; > } > -EXPORT_SYMBOL(acpi_video_exit); > > module_init(acpi_video_init); > module_exit(acpi_video_exit); > Index: linux-2.6/drivers/gpu/drm/i915/i915_opregion.c > =================================================================== > --- linux-2.6.orig/drivers/gpu/drm/i915/i915_opregion.c 2009-06-16 10:25:12.000000000 +0800 > +++ linux-2.6/drivers/gpu/drm/i915/i915_opregion.c 2009-06-16 10:29:50.000000000 +0800 > @@ -419,7 +419,7 @@ > return; > > if (!suspend) > - acpi_video_exit(); > + acpi_video_unregister(); > > opregion->acpi->drdy = 0; > > Index: linux-2.6/include/acpi/video.h > =================================================================== > --- linux-2.6.orig/include/acpi/video.h 2009-06-16 10:25:12.000000000 +0800 > +++ linux-2.6/include/acpi/video.h 2009-06-16 11:07:43.000000000 +0800 > @@ -3,10 +3,10 @@ > > #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) > extern int acpi_video_register(void); > -extern int acpi_video_exit(void); > +extern void acpi_video_unregister(void); > #else > static inline int acpi_video_register(void) { return 0; } > -static inline void acpi_video_exit(void) { return; } > +static inline void acpi_video_unregister(void) { return; } > #endif > > #endif > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6/drivers/acpi/video.c =================================================================== --- linux-2.6.orig/drivers/acpi/video.c 2009-06-16 10:25:12.000000000 +0800 +++ linux-2.6/drivers/acpi/video.c 2009-06-16 11:09:57.000000000 +0800 @@ -76,6 +76,7 @@ static int brightness_switch_enabled = 1; module_param(brightness_switch_enabled, bool, 0644); +static int register_count = 0; static int acpi_video_bus_add(struct acpi_device *device); static int acpi_video_bus_remove(struct acpi_device *device, int type); static int acpi_video_resume(struct acpi_device *device); @@ -2318,6 +2319,13 @@ int acpi_video_register(void) { int result = 0; + if (register_count) { + /* + * if the function of acpi_video_register is already called, + * don't register the acpi_vide_bus again and return no error. + */ + return 0; + } acpi_video_dir = proc_mkdir(ACPI_VIDEO_CLASS, acpi_root_dir); if (!acpi_video_dir) @@ -2329,10 +2337,35 @@ return -ENODEV; } + /* + * When the acpi_video_bus is loaded successfully, increase + * the counter reference. + */ + register_count = 1; + return 0; } EXPORT_SYMBOL(acpi_video_register); +void acpi_video_unregister(void) +{ + if (!register_count) { + /* + * If the acpi video bus is already unloaded, don't + * unload it again and return directly. + */ + return; + } + acpi_bus_unregister_driver(&acpi_video_bus); + + remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir); + + register_count = 0; + + return; +} +EXPORT_SYMBOL(acpi_video_unregister); + /* * This is kind of nasty. Hardware using Intel chipsets may require * the video opregion code to be run first in order to initialise @@ -2350,16 +2383,12 @@ return acpi_video_register(); } -void acpi_video_exit(void) +static void __exit acpi_video_exit(void) { - - acpi_bus_unregister_driver(&acpi_video_bus); - - remove_proc_entry(ACPI_VIDEO_CLASS, acpi_root_dir); + acpi_video_unregister(); return; } -EXPORT_SYMBOL(acpi_video_exit); module_init(acpi_video_init); module_exit(acpi_video_exit); Index: linux-2.6/drivers/gpu/drm/i915/i915_opregion.c =================================================================== --- linux-2.6.orig/drivers/gpu/drm/i915/i915_opregion.c 2009-06-16 10:25:12.000000000 +0800 +++ linux-2.6/drivers/gpu/drm/i915/i915_opregion.c 2009-06-16 10:29:50.000000000 +0800 @@ -419,7 +419,7 @@ return; if (!suspend) - acpi_video_exit(); + acpi_video_unregister(); opregion->acpi->drdy = 0; Index: linux-2.6/include/acpi/video.h =================================================================== --- linux-2.6.orig/include/acpi/video.h 2009-06-16 10:25:12.000000000 +0800 +++ linux-2.6/include/acpi/video.h 2009-06-16 11:07:43.000000000 +0800 @@ -3,10 +3,10 @@ #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) extern int acpi_video_register(void); -extern int acpi_video_exit(void); +extern void acpi_video_unregister(void); #else static inline int acpi_video_register(void) { return 0; } -static inline void acpi_video_exit(void) { return; } +static inline void acpi_video_unregister(void) { return; } #endif #endif