* [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure
@ 2015-12-08 22:48 Matt Roper
2015-12-08 22:48 ` [PATCH 2/2] drm/i915: Drop unnecessary NULL test Matt Roper
2015-12-10 14:14 ` [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Daniel Vetter
0 siblings, 2 replies; 6+ messages in thread
From: Matt Roper @ 2015-12-08 22:48 UTC (permalink / raw)
To: intel-gfx
In all of our various SDVO setup functions, we allocate an SDVO
connector (along with an associated connector->state) object, then
perform initialization. If that initialization fails, we need to make
sure to free the state object as well as the connector.
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 06679f1..ff28867 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2479,6 +2479,8 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
}
if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
+ drm_atomic_helper_connector_destroy_state(connector,
+ connector->state);
kfree(intel_sdvo_connector);
return false;
}
@@ -2514,6 +2516,8 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
intel_sdvo->is_tv = true;
if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
+ drm_atomic_helper_connector_destroy_state(connector,
+ connector->state);
kfree(intel_sdvo_connector);
return false;
}
@@ -2561,6 +2565,8 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
}
if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
+ drm_atomic_helper_connector_destroy_state(connector,
+ connector->state);
kfree(intel_sdvo_connector);
return false;
}
@@ -2596,6 +2602,8 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
}
if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
+ drm_atomic_helper_connector_destroy_state(connector,
+ connector->state);
kfree(intel_sdvo_connector);
return false;
}
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/i915: Drop unnecessary NULL test
2015-12-08 22:48 [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Matt Roper
@ 2015-12-08 22:48 ` Matt Roper
2015-12-10 14:15 ` Daniel Vetter
2015-12-10 14:14 ` [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Daniel Vetter
1 sibling, 1 reply; 6+ messages in thread
From: Matt Roper @ 2015-12-08 22:48 UTC (permalink / raw)
To: intel-gfx
We can never be passed a NULL crtc_state (and if we could there are
plenty of other places later in this function where we access it
without testing for NULL).
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2e8d1a8..51980a9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11808,7 +11808,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
bool turn_off, turn_on, visible, was_visible;
struct drm_framebuffer *fb = plane_state->fb;
- if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
+ if (INTEL_INFO(dev)->gen >= 9 &&
plane->type != DRM_PLANE_TYPE_CURSOR) {
ret = skl_update_scaler_plane(
to_intel_crtc_state(crtc_state),
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure
2015-12-08 22:48 [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Matt Roper
2015-12-08 22:48 ` [PATCH 2/2] drm/i915: Drop unnecessary NULL test Matt Roper
@ 2015-12-10 14:14 ` Daniel Vetter
2015-12-11 1:57 ` Matt Roper
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2015-12-10 14:14 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Tue, Dec 08, 2015 at 02:48:51PM -0800, Matt Roper wrote:
> In all of our various SDVO setup functions, we allocate an SDVO
> connector (along with an associated connector->state) object, then
> perform initialization. If that initialization fails, we need to make
> sure to free the state object as well as the connector.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Where do we alloc connector->state in sdvo_connector_init()? I thought the
flow is 1) register all the kms objects with our pile of _init() functions
2) do hw readout, which is the thing which creates all these states.
None of the other connectors seem to have this issue (or at least I don't
see a patch for them).
-Daniel
> ---
> drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 06679f1..ff28867 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2479,6 +2479,8 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> }
>
> if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> + drm_atomic_helper_connector_destroy_state(connector,
> + connector->state);
> kfree(intel_sdvo_connector);
> return false;
> }
> @@ -2514,6 +2516,8 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> intel_sdvo->is_tv = true;
>
> if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> + drm_atomic_helper_connector_destroy_state(connector,
> + connector->state);
> kfree(intel_sdvo_connector);
> return false;
> }
> @@ -2561,6 +2565,8 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
> }
>
> if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> + drm_atomic_helper_connector_destroy_state(connector,
> + connector->state);
> kfree(intel_sdvo_connector);
> return false;
> }
> @@ -2596,6 +2602,8 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> }
>
> if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> + drm_atomic_helper_connector_destroy_state(connector,
> + connector->state);
> kfree(intel_sdvo_connector);
> return false;
> }
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Drop unnecessary NULL test
2015-12-08 22:48 ` [PATCH 2/2] drm/i915: Drop unnecessary NULL test Matt Roper
@ 2015-12-10 14:15 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2015-12-10 14:15 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Tue, Dec 08, 2015 at 02:48:52PM -0800, Matt Roper wrote:
> We can never be passed a NULL crtc_state (and if we could there are
> plenty of other places later in this function where we access it
> without testing for NULL).
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Yup, atomic state structures better exist once we can do
atomic_check/commit().
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2e8d1a8..51980a9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11808,7 +11808,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> bool turn_off, turn_on, visible, was_visible;
> struct drm_framebuffer *fb = plane_state->fb;
>
> - if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
> + if (INTEL_INFO(dev)->gen >= 9 &&
> plane->type != DRM_PLANE_TYPE_CURSOR) {
> ret = skl_update_scaler_plane(
> to_intel_crtc_state(crtc_state),
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure
2015-12-10 14:14 ` [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Daniel Vetter
@ 2015-12-11 1:57 ` Matt Roper
2015-12-11 17:28 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Matt Roper @ 2015-12-11 1:57 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Thu, Dec 10, 2015 at 03:14:38PM +0100, Daniel Vetter wrote:
> On Tue, Dec 08, 2015 at 02:48:51PM -0800, Matt Roper wrote:
> > In all of our various SDVO setup functions, we allocate an SDVO
> > connector (along with an associated connector->state) object, then
> > perform initialization. If that initialization fails, we need to make
> > sure to free the state object as well as the connector.
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>
> Where do we alloc connector->state in sdvo_connector_init()? I thought the
> flow is 1) register all the kms objects with our pile of _init() functions
> 2) do hw readout, which is the thing which creates all these states.
It's allocated by intel_sdvo_connector_alloc -> intel_connector_init
which gets run before intel_sdvo_connector_init() even gets called.
There are two separate "init" functions that get called for SDVO
connectors...the first allocates the state object, but the second can
fail too and trigger cleanup (which was failing to destroy the state).
>
> None of the other connectors seem to have this issue (or at least I don't
> see a patch for them).
I don't think any of the other connector types have a second, special
init function that gets run after intel_connector_init() is finished
that I can see.
Matt
> -Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 06679f1..ff28867 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -2479,6 +2479,8 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> > }
> >
> > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > + drm_atomic_helper_connector_destroy_state(connector,
> > + connector->state);
> > kfree(intel_sdvo_connector);
> > return false;
> > }
> > @@ -2514,6 +2516,8 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> > intel_sdvo->is_tv = true;
> >
> > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > + drm_atomic_helper_connector_destroy_state(connector,
> > + connector->state);
> > kfree(intel_sdvo_connector);
> > return false;
> > }
> > @@ -2561,6 +2565,8 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
> > }
> >
> > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > + drm_atomic_helper_connector_destroy_state(connector,
> > + connector->state);
> > kfree(intel_sdvo_connector);
> > return false;
> > }
> > @@ -2596,6 +2602,8 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> > }
> >
> > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > + drm_atomic_helper_connector_destroy_state(connector,
> > + connector->state);
> > kfree(intel_sdvo_connector);
> > return false;
> > }
> > --
> > 2.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure
2015-12-11 1:57 ` Matt Roper
@ 2015-12-11 17:28 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2015-12-11 17:28 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Thu, Dec 10, 2015 at 05:57:57PM -0800, Matt Roper wrote:
> On Thu, Dec 10, 2015 at 03:14:38PM +0100, Daniel Vetter wrote:
> > On Tue, Dec 08, 2015 at 02:48:51PM -0800, Matt Roper wrote:
> > > In all of our various SDVO setup functions, we allocate an SDVO
> > > connector (along with an associated connector->state) object, then
> > > perform initialization. If that initialization fails, we need to make
> > > sure to free the state object as well as the connector.
> > >
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >
> > Where do we alloc connector->state in sdvo_connector_init()? I thought the
> > flow is 1) register all the kms objects with our pile of _init() functions
> > 2) do hw readout, which is the thing which creates all these states.
>
> It's allocated by intel_sdvo_connector_alloc -> intel_connector_init
> which gets run before intel_sdvo_connector_init() even gets called.
> There are two separate "init" functions that get called for SDVO
> connectors...the first allocates the state object, but the second can
> fail too and trigger cleanup (which was failing to destroy the state).
Ah right, didn't find that. Started a full audit but stuck at
intel_dp_init. That has a kfree(intel_connector), but seems to miss this
too.
On the patch itself: Can't we just call drm_connector_cleanup? And an
intel_connectore_free which wraps that + the kfree would be nice. Well,
that would mean we'd need to move drm_connector_init into
intel_connector_init, but I think that would make a lot of sense either
way.
-Daniel
>
> >
> > None of the other connectors seem to have this issue (or at least I don't
> > see a patch for them).
>
> I don't think any of the other connector types have a second, special
> init function that gets run after intel_connector_init() is finished
> that I can see.
>
>
> Matt
>
> > -Daniel
> >
> > > ---
> > > drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > > index 06679f1..ff28867 100644
> > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > @@ -2479,6 +2479,8 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> > > }
> > >
> > > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > + drm_atomic_helper_connector_destroy_state(connector,
> > > + connector->state);
> > > kfree(intel_sdvo_connector);
> > > return false;
> > > }
> > > @@ -2514,6 +2516,8 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> > > intel_sdvo->is_tv = true;
> > >
> > > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > + drm_atomic_helper_connector_destroy_state(connector,
> > > + connector->state);
> > > kfree(intel_sdvo_connector);
> > > return false;
> > > }
> > > @@ -2561,6 +2565,8 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
> > > }
> > >
> > > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > + drm_atomic_helper_connector_destroy_state(connector,
> > > + connector->state);
> > > kfree(intel_sdvo_connector);
> > > return false;
> > > }
> > > @@ -2596,6 +2602,8 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> > > }
> > >
> > > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > + drm_atomic_helper_connector_destroy_state(connector,
> > > + connector->state);
> > > kfree(intel_sdvo_connector);
> > > return false;
> > > }
> > > --
> > > 2.1.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-12-11 17:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-08 22:48 [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Matt Roper
2015-12-08 22:48 ` [PATCH 2/2] drm/i915: Drop unnecessary NULL test Matt Roper
2015-12-10 14:15 ` Daniel Vetter
2015-12-10 14:14 ` [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Daniel Vetter
2015-12-11 1:57 ` Matt Roper
2015-12-11 17:28 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox