[Intel-gfx] [PATCH v2] drm/i915: fix dp/sdvo i2c cleanup
Imre Deak
imre.deak at intel.com
Sun Jan 26 01:51:30 CET 2014
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 <imre.deak at intel.com>
>
> 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list