Re: [PATCH]: ACPI: Add the reference count to avoid unloading ACPI video bus twice

[yakui_zhao <yakui.zhao@intel.com>]

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