diff mbox

drm/i915: prevent crash with .disable_display parameter

Message ID 1484699848-1605-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Taylor, Clinton A Jan. 18, 2017, 12:37 a.m. UTC 
From: Clint Taylor <clinton.a.taylor@intel.com>

The .disable_display parameter was causing a fatal crash when fbdev was
dereferenced during driver init.

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Chris Wilson Jan. 18, 2017, 9:52 a.m. UTC | #1 
On Tue, Jan 17, 2017 at 04:37:28PM -0800, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> The .disable_display parameter was causing a fatal crash when fbdev was
> dereferenced during driver init.

The other guards are within intel_fbdev.c, it would be consistent to put
this as the start of intel_fbdev_initial_config_async().

Testcase: igt/drv_module_reload/basic-no-display
-Chris
Chris Wilson Jan. 18, 2017, 10:16 a.m. UTC | #2 
On Wed, Jan 18, 2017 at 09:52:35AM +0000, Chris Wilson wrote:
> On Tue, Jan 17, 2017 at 04:37:28PM -0800, clinton.a.taylor@intel.com wrote:
> > From: Clint Taylor <clinton.a.taylor@intel.com>
> > 
> > The .disable_display parameter was causing a fatal crash when fbdev was
> > dereferenced during driver init.
> 
> The other guards are within intel_fbdev.c, it would be consistent to put
> this as the start of intel_fbdev_initial_config_async().
> 
> Testcase: igt/drv_module_reload/basic-no-display

Also

Fixes: 43cee314345a ("drm/i915/fbdev: Limit the global async-domain synchronization")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.8+
kernel test robot Jan. 18, 2017, 3:54 p.m. UTC | #3 
Hi Clint,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.10-rc4 next-20170118]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/clinton-a-taylor-intel-com/drm-i915-prevent-crash-with-disable_display-parameter/20170118-230141
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x005-201703 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/ioport.h:12:0,
                    from include/linux/acpi.h:25,
                    from drivers/gpu/drm/i915/i915_drv.c:30:
   drivers/gpu/drm/i915/i915_drv.c: In function 'i915_driver_register':
>> drivers/gpu/drm/i915/i915_drv.c:1143:14: error: 'struct drm_i915_private' has no member named 'fbdev'
     if (dev_priv->fbdev) {
                 ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/gpu/drm/i915/i915_drv.c:1143:2: note: in expansion of macro 'if'
     if (dev_priv->fbdev) {
     ^~
>> drivers/gpu/drm/i915/i915_drv.c:1143:14: error: 'struct drm_i915_private' has no member named 'fbdev'
     if (dev_priv->fbdev) {
                 ^
   include/linux/compiler.h:149:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
>> drivers/gpu/drm/i915/i915_drv.c:1143:2: note: in expansion of macro 'if'
     if (dev_priv->fbdev) {
     ^~
>> drivers/gpu/drm/i915/i915_drv.c:1143:14: error: 'struct drm_i915_private' has no member named 'fbdev'
     if (dev_priv->fbdev) {
                 ^
   include/linux/compiler.h:160:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
>> drivers/gpu/drm/i915/i915_drv.c:1143:2: note: in expansion of macro 'if'
     if (dev_priv->fbdev) {
     ^~

vim +1143 drivers/gpu/drm/i915/i915_drv.c

  1137		 * Some ports require correctly set-up hpd registers for detection to
  1138		 * work properly (leading to ghost connected connector status), e.g. VGA
  1139		 * on gm45.  Hence we can only set up the initial fbdev config after hpd
  1140		 * irqs are fully enabled. We do it last so that the async config
  1141		 * cannot run before the connectors are registered.
  1142		 */
> 1143		if (dev_priv->fbdev) {
  1144			intel_fbdev_initial_config_async(dev);
  1145		}
  1146	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Taylor, Clinton A Jan. 18, 2017, 4:22 p.m. UTC | #4 
On 01/18/2017 01:52 AM, Chris Wilson wrote:
> On Tue, Jan 17, 2017 at 04:37:28PM -0800, clinton.a.taylor@intel.com wrote:
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> The .disable_display parameter was causing a fatal crash when fbdev was
>> dereferenced during driver init.
>
> The other guards are within intel_fbdev.c, it would be consistent to put
> this as the start of intel_fbdev_initial_config_async().

I came to that conclusion last night as well.

>
> Testcase: igt/drv_module_reload/basic-no-display
> -Chris
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4e5ea58..b2a9195 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1140,7 +1140,9 @@  static void i915_driver_register(struct drm_i915_private *dev_priv)
 	 * irqs are fully enabled. We do it last so that the async config
 	 * cannot run before the connectors are registered.
 	 */
-	intel_fbdev_initial_config_async(dev);
+	if (dev_priv->fbdev) {
+		intel_fbdev_initial_config_async(dev);
+	}
 }
 
 /**