From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH v2] drm/i915: fix dp/sdvo i2c cleanup Date: Sun, 26 Jan 2014 02:51:30 +0200 Message-ID: <1390697490.4025.29.camel@ideak-mobl> References: <1390554955-26380-1-git-send-email-imre.deak@intel.com> <1390567654-27547-1-git-send-email-imre.deak@intel.com> <20140125203737.GN9772@phenom.ffwll.local> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 57947FA3DE for ; Sat, 25 Jan 2014 16:51:40 -0800 (PST) In-Reply-To: <20140125203737.GN9772@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote: > On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote: > > Atm we try to remove the connector's i2c sysfs entry too late in the > > encoder's destroy callback. By that time the kobject used as the parent > > for all connector sysfs entries is already removed when we do an early > > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix > > this by adding an early_destroy encoder callback, where we remove the > > encoder's i2c adapter. > > > > v2: > > - add missing static to function, use existing sdvo cast helper, > > s/intel_sdvo/sdvo/ (Chris) > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523 > > > > Signed-off-by: Imre Deak > > This looks fishy ... sysfs should all be reference-counted, so I'm > confused what's going on here. Also I think this smells a bit like it's > going overall in the wrong direction - essentially with sysfs we can't > really force-remove stuff but have to wait until the refcount drops to 0. > Or at least that's how I think it works, I'd need to blow through a pile > of time to figure this all out. Hm, I haven't thought about refcounting :) Now, I agree that should normally allow for removing a parent and child device in both order. What happens and why we can't remove first the parent then the child: In intel_dp_init_connector()-> intel_dp_i2c_init() we set the i2c adapter.dev.parent = intel_connector->base.kdev; device_register will then add the i2c sysfs entry to the connector sysfs dir. Refcounting here looks ok, both the parent connector kobject and its sysfs dir entry gets a reference from the child. During module cleanup, we call intel_modeset_cleanup()-> drm_sysfs_connector_remove() device_unregister(connector->kdev) which is the i2c adapter's parent kdev. Then: device_del()-> kobject_del()-> sysfs_remove_dir() will remove all entries recursively in the connector's sysfs dir, along with all the i2c sysfs entries. Afterwards the intel encoder->destroy() callback calls i2c_del_adapter()->device_unregister()->device_del() and that will try to remove its own sysfs attributes, namely the power sysfs group, but won't find it since it was removed above and give a WARN. Note that the parent's recursive removal happens regardless of its kobject's or sysfs entry's refcount. The kobject itself will be put after device_del() in put_device() and only destroyed after the i2c adapter releases the refcount it holds on it. I admit it feels strange that the sysfs entries are removed before the last reference on the kobject is dropped, not sure if it's by design or an overlook.. --Imre > > Someone enlighten me please ;-) > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ > > drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_sdvo.c | 9 ++++++++- > > 4 files changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 5e94901..5d7f1fa 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11451,6 +11451,7 @@ void intel_modeset_cleanup(struct drm_device *dev) > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_crtc *crtc; > > struct drm_connector *connector; > > + struct drm_encoder *encoder; > > > > /* > > * Interrupts and polling as the first thing to avoid creating havoc. > > @@ -11488,6 +11489,13 @@ void intel_modeset_cleanup(struct drm_device *dev) > > /* flush any delayed tasks or pending work */ > > flush_scheduled_work(); > > > > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { > > + struct intel_encoder *intel_encoder = to_intel_encoder(encoder); > > + > > + if (intel_encoder->early_destroy) > > + intel_encoder->early_destroy(intel_encoder); > > + } > > + > > /* destroy the backlight and sysfs files before encoders/connectors */ > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > > intel_panel_destroy_backlight(connector); > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index e37c7a0..33d6943 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3358,7 +3358,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) > > struct intel_dp *intel_dp = &intel_dig_port->dp; > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > > > - i2c_del_adapter(&intel_dp->adapter); > > drm_encoder_cleanup(encoder); > > if (is_edp(intel_dp)) { > > cancel_delayed_work_sync(&intel_dp->panel_vdd_work); > > @@ -3369,6 +3368,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) > > kfree(intel_dig_port); > > } > > > > + > > +static void intel_dp_encoder_early_destroy(struct intel_encoder *intel_encoder) > > +{ > > + struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base); > > + > > + i2c_del_adapter(&intel_dp->adapter); > > +} > > + > > static const struct drm_connector_funcs intel_dp_connector_funcs = { > > .dpms = intel_connector_dpms, > > .detect = intel_dp_detect, > > @@ -3880,6 +3887,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) > > intel_encoder->pre_enable = g4x_pre_enable_dp; > > intel_encoder->enable = g4x_enable_dp; > > } > > + intel_encoder->early_destroy = intel_dp_encoder_early_destroy; > > > > intel_dig_port->port = port; > > intel_dig_port->dp.output_reg = output_reg; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 7b3c209..216f490 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -149,6 +149,7 @@ struct intel_encoder { > > * be set correctly before calling this function. */ > > void (*get_config)(struct intel_encoder *, > > struct intel_crtc_config *pipe_config); > > + void (*early_destroy)(struct intel_encoder *); > > int crtc_mask; > > enum hpd_pin hpd_pin; > > }; > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > index 95bdfb3..8ecc702 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > @@ -2238,7 +2238,6 @@ static void intel_sdvo_enc_destroy(struct drm_encoder *encoder) > > drm_mode_destroy(encoder->dev, > > intel_sdvo->sdvo_lvds_fixed_mode); > > > > - i2c_del_adapter(&intel_sdvo->ddc); > > intel_encoder_destroy(encoder); > > } > > > > @@ -2912,6 +2911,13 @@ intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo, > > return i2c_add_adapter(&sdvo->ddc) == 0; > > } > > > > +static void intel_sdvo_early_destroy(struct intel_encoder *intel_encoder) > > +{ > > + struct intel_sdvo *sdvo = to_sdvo(intel_encoder); > > + > > + i2c_del_adapter(&sdvo->ddc); > > +} > > + > > bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -2951,6 +2957,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob) > > intel_encoder->enable = intel_enable_sdvo; > > intel_encoder->get_hw_state = intel_sdvo_get_hw_state; > > intel_encoder->get_config = intel_sdvo_get_config; > > + intel_encoder->early_destroy = intel_sdvo_early_destroy; > > > > /* In default case sdvo lvds is false */ > > if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps)) > > -- > > 1.8.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >