[Intel-gfx] [PATCH 3/3] drm/i915: Clean up if drm_sysfs_connector_add() fails
Daniel Vetter
daniel at ffwll.ch
Mon Nov 4 23:36:07 PST 2013
On Tue, Nov 05, 2013 at 09:23:46AM +0200, Jani Nikula wrote:
> On Mon, 04 Nov 2013, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > While we can now call drm_sysfs_connector_remove() even if
> > drm_connector_sysfs_add() failed, it would seem better for
> > the user to know that something went wrong. So instead of
> > ignoring drm_sysfs_connector_add() return value, checl it
> > and fail the whole connector registration.
>
> I think I'd rather see a cleanup of the init time fail paths first,
> using the regular kernel goto style. Which I also don't think should be
> a huge priority. Whaddaya think?
In my dream world we could solve most of this with devres.c. Requires us
to fix the drm midlayer first though, and I'd guess that the interaction
with the shadow attach stuff could be interesting ...
-Daniel
>
> Cheers,
> Jani.
>
>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_crt.c | 10 +++++++++-
> > drivers/gpu/drm/i915/intel_ddi.c | 5 ++++-
> > drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
> > drivers/gpu/drm/i915/intel_drv.h | 2 +-
> > drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++-
> > drivers/gpu/drm/i915/intel_dvo.c | 8 +++++++-
> > drivers/gpu/drm/i915/intel_hdmi.c | 16 +++++++++++++---
> > drivers/gpu/drm/i915/intel_lvds.c | 7 ++++++-
> > drivers/gpu/drm/i915/intel_sdvo.c | 36 +++++++++++++++++++++++++++++-------
> > drivers/gpu/drm/i915/intel_tv.c | 10 +++++++++-
> > 10 files changed, 91 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index 2e01bd3..be8a024 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -773,6 +773,7 @@ void intel_crt_init(struct drm_device *dev)
> > struct intel_crt *crt;
> > struct intel_connector *intel_connector;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + int error;
> >
> > /* Skip machines without VGA that falsely report hotplug events */
> > if (dmi_check_system(intel_no_crt))
> > @@ -836,7 +837,14 @@ void intel_crt_init(struct drm_device *dev)
> >
> > drm_connector_helper_add(connector, &intel_crt_connector_helper_funcs);
> >
> > - drm_sysfs_connector_add(connector);
> > + error = drm_sysfs_connector_add(connector);
> > + if (error) {
> > + drm_encoder_cleanup(&crt->base.base);
> > + drm_connector_cleanup(connector);
> > + kfree(intel_connector);
> > + kfree(crt);
> > + return;
> > + }
> >
> > if (!I915_HAS_HOTPLUG(dev))
> > intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 31f4fe2..687e333 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1375,7 +1375,10 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
> > return NULL;
> >
> > intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
> > - intel_hdmi_init_connector(intel_dig_port, connector);
> > + if (!intel_hdmi_init_connector(intel_dig_port, connector)) {
> > + kfree(connector);
> > + return NULL;
> > + }
> >
> > return connector;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index c8515bb..eb01be7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3603,7 +3603,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> > ironlake_panel_vdd_work);
> >
> > intel_connector_attach_encoder(intel_connector, intel_encoder);
> > - drm_sysfs_connector_add(connector);
> > + error = drm_sysfs_connector_add(connector);
> > + if (error) {
> > + drm_connector_cleanup(connector);
> > + return false;
> > + }
> >
> > if (HAS_DDI(dev))
> > intel_connector->get_hw_state = intel_ddi_connector_get_hw_state;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 9d2624f..a944d6e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -760,7 +760,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
> >
> > /* intel_hdmi.c */
> > void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> > -void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > +bool intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > struct intel_connector *intel_connector);
> > struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
> > bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index d257b09..16684b5 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -535,6 +535,7 @@ bool intel_dsi_init(struct drm_device *dev)
> > struct drm_display_mode *fixed_mode = NULL;
> > const struct intel_dsi_device *dsi;
> > unsigned int i;
> > + int error;
> >
> > DRM_DEBUG_KMS("\n");
> >
> > @@ -598,11 +599,17 @@ bool intel_dsi_init(struct drm_device *dev)
> >
> > intel_connector_attach_encoder(intel_connector, intel_encoder);
> >
> > - drm_sysfs_connector_add(connector);
> > + error = drm_sysfs_connector_add(connector);
> > + if (error) {
> > + drm_connector_cleanup(connector);
> > + goto err;
> > + }
> >
> > fixed_mode = dsi->dev_ops->get_modes(&intel_dsi->dev);
> > if (!fixed_mode) {
> > DRM_DEBUG_KMS("no fixed mode\n");
> > + drm_sysfs_connector_remove(connector);
> > + drm_connector_cleanup(connector);
> > goto err;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > index 3c77365..439e9d9 100644
> > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > @@ -484,6 +484,7 @@ void intel_dvo_init(struct drm_device *dev)
> > struct i2c_adapter *i2c;
> > int gpio;
> > bool dvoinit;
> > + int error;
> >
> > /* Allow the I2C driver info to specify the GPIO to be used in
> > * special cases, but otherwise default to what's defined
> > @@ -555,7 +556,12 @@ void intel_dvo_init(struct drm_device *dev)
> > intel_dvo->panel_wants_dither = true;
> > }
> >
> > - drm_sysfs_connector_add(connector);
> > + error = drm_sysfs_connector_add(connector);
> > + if (error) {
> > + drm_connector_cleanup(connector);
> > + break;
> > + }
> > +
> > return;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 51a8336..f8ee5ca 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1211,7 +1211,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
> > intel_hdmi->color_range_auto = true;
> > }
> >
> > -void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > +bool intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > struct intel_connector *intel_connector)
> > {
> > struct drm_connector *connector = &intel_connector->base;
> > @@ -1220,6 +1220,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > struct drm_device *dev = intel_encoder->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > enum port port = intel_dig_port->port;
> > + int error;
> >
> > drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
> > DRM_MODE_CONNECTOR_HDMIA);
> > @@ -1274,7 +1275,11 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > intel_hdmi_add_properties(intel_hdmi, connector);
> >
> > intel_connector_attach_encoder(intel_connector, intel_encoder);
> > - drm_sysfs_connector_add(connector);
> > + error = drm_sysfs_connector_add(connector);
> > + if (error) {
> > + drm_connector_cleanup(connector);
> > + return false;
> > + }
> >
> > /* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
> > * 0xd. Failure to do so will result in spurious interrupts being
> > @@ -1284,6 +1289,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > u32 temp = I915_READ(PEG_BAND_GAP_DATA);
> > I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
> > }
> > +
> > + return true;
> > }
> >
> > void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
> > @@ -1329,5 +1336,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
> > intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
> > intel_dig_port->dp.output_reg = 0;
> >
> > - intel_hdmi_init_connector(intel_dig_port, intel_connector);
> > + if (!intel_hdmi_init_connector(intel_dig_port, intel_connector)) {
> > + drm_encoder_cleanup(&intel_encoder->base);
> > + return;
> > + }
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index b0ef558..a358a5e 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -941,6 +941,7 @@ void intel_lvds_init(struct drm_device *dev)
> > u32 lvds;
> > int pipe;
> > u8 pin;
> > + int error;
> >
> > if (!intel_lvds_supported(dev))
> > return;
> > @@ -1137,7 +1138,11 @@ out:
> > DRM_DEBUG_KMS("lid notifier registration failed\n");
> > lvds_connector->lid_notifier.notifier_call = NULL;
> > }
> > - drm_sysfs_connector_add(connector);
> > + error = drm_sysfs_connector_add(connector);
> > + if (error) {
> > + acpi_lid_notifier_unregister(&lvds_connector->lid_notifier);
> > + goto failed;
> > + }
> >
> > intel_panel_init(&intel_connector->panel, fixed_mode);
> > intel_panel_setup_backlight(connector);
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index a583e8f..23d758e 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -2361,10 +2361,12 @@ intel_sdvo_get_slave_addr(struct drm_device *dev, struct intel_sdvo *sdvo)
> > return 0x72;
> > }
> >
> > -static void
> > +static bool
> > intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
> > struct intel_sdvo *encoder)
> > {
> > + int error;
> > +
> > drm_connector_init(encoder->base.base.dev,
> > &connector->base.base,
> > &intel_sdvo_connector_funcs,
> > @@ -2379,7 +2381,13 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
> > connector->base.get_hw_state = intel_sdvo_connector_get_hw_state;
> >
> > intel_connector_attach_encoder(&connector->base, &encoder->base);
> > - drm_sysfs_connector_add(&connector->base.base);
> > + error = drm_sysfs_connector_add(&connector->base.base);
> > + if (error) {
> > + drm_connector_cleanup(&connector->base.base);
> > + return false;
> > + }
> > +
> > + return true;
> > }
> >
> > static void
> > @@ -2439,7 +2447,11 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> > intel_sdvo->is_hdmi = true;
> > }
> >
> > - intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> > + if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> > + kfree(intel_sdvo_connector);
> > + return false;
> > + }
> > +
> > if (intel_sdvo->is_hdmi)
> > intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
> >
> > @@ -2470,7 +2482,10 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> >
> > intel_sdvo->is_tv = true;
> >
> > - intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> > + if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> > + kfree(intel_sdvo_connector);
> > + return false;
> > + }
> >
> > if (!intel_sdvo_tv_create_property(intel_sdvo, intel_sdvo_connector, type))
> > goto err;
> > @@ -2514,8 +2529,11 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
> > intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB1;
> > }
> >
> > - intel_sdvo_connector_init(intel_sdvo_connector,
> > - intel_sdvo);
> > + if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> > + kfree(intel_sdvo_connector);
> > + return false;
> > + }
> > +
> > return true;
> > }
> >
> > @@ -2546,7 +2564,11 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> > intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS1;
> > }
> >
> > - intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> > + if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> > + kfree(intel_sdvo_connector);
> > + return false;
> > + }
> > +
> > if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
> > goto err;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> > index 18c4062..c1e66f6 100644
> > --- a/drivers/gpu/drm/i915/intel_tv.c
> > +++ b/drivers/gpu/drm/i915/intel_tv.c
> > @@ -1554,6 +1554,7 @@ intel_tv_init(struct drm_device *dev)
> > u32 tv_dac_on, tv_dac_off, save_tv_dac;
> > char *tv_format_names[ARRAY_SIZE(tv_modes)];
> > int i, initial_mode = 0;
> > + int error;
> >
> > if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
> > return;
> > @@ -1668,5 +1669,12 @@ intel_tv_init(struct drm_device *dev)
> > drm_object_attach_property(&connector->base,
> > dev->mode_config.tv_bottom_margin_property,
> > intel_tv->margin[TV_MARGIN_BOTTOM]);
> > - drm_sysfs_connector_add(connector);
> > + error = drm_sysfs_connector_add(connector);
> > + if (error) {
> > + drm_connector_cleanup(connector);
> > + drm_encoder_cleanup(&intel_encoder->base);
> > + kfree(intel_connector);
> > + kfree(intel_tv);
> > + return;
> > + }
> > }
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list