| Message ID | 20181214012604.13746-7-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MST refcounting/atomic helpers cleanup | expand |
On Thu, Dec 13, 2018 at 08:25:35PM -0500, Lyude Paul wrote: > So that the ports stay around until we've destroyed the connectors, in > order to ensure that we don't pass an invalid pointer to any MST helpers > once we introduce the new MST VCPI helpers. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/i915/intel_connector.c | 4 ++++ > drivers/gpu/drm/i915/intel_dp_mst.c | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_connector.c b/drivers/gpu/drm/i915/intel_connector.c > index 18e370f607bc..37d2c644f4b8 100644 > --- a/drivers/gpu/drm/i915/intel_connector.c > +++ b/drivers/gpu/drm/i915/intel_connector.c > @@ -95,6 +95,10 @@ void intel_connector_destroy(struct drm_connector *connector) > intel_panel_fini(&intel_connector->panel); > > drm_connector_cleanup(connector); > + > + if (intel_connector->port) We set this as the first thing in intel_dp_add_mst_connector, so this check isn't doing anything. > + drm_dp_mst_put_port_malloc(intel_connector->port); > + > kfree(connector); > } > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index f05427b74e34..4d6ced34d465 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -484,6 +484,8 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > if (ret) > goto err; > > + drm_dp_mst_get_port_malloc(port); Needs to be moved up where we assing intel_connector->port, or it'll underflow on cleanup on error paths. > + > return connector; > > err: > -- > 2.19.2 >
On Fri, 2018-12-14 at 10:32 +0100, Daniel Vetter wrote: > On Thu, Dec 13, 2018 at 08:25:35PM -0500, Lyude Paul wrote: > > So that the ports stay around until we've destroyed the connectors, in > > order to ensure that we don't pass an invalid pointer to any MST helpers > > once we introduce the new MST VCPI helpers. > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > --- > > drivers/gpu/drm/i915/intel_connector.c | 4 ++++ > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 ++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_connector.c > > b/drivers/gpu/drm/i915/intel_connector.c > > index 18e370f607bc..37d2c644f4b8 100644 > > --- a/drivers/gpu/drm/i915/intel_connector.c > > +++ b/drivers/gpu/drm/i915/intel_connector.c > > @@ -95,6 +95,10 @@ void intel_connector_destroy(struct drm_connector > > *connector) > > intel_panel_fini(&intel_connector->panel); > > > > drm_connector_cleanup(connector); > > + > > + if (intel_connector->port) > > We set this as the first thing in intel_dp_add_mst_connector, so this > check isn't doing anything. I think this comment might be a mistake? intel_connector_destroy() is shared by all of the intel connectors, so some may not have a value in intel_connector->port. I can move it to it's own destroy callback for MST if you would prefer though. > > > + drm_dp_mst_put_port_malloc(intel_connector->port); > > + > > kfree(connector); > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > index f05427b74e34..4d6ced34d465 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -484,6 +484,8 @@ static struct drm_connector > > *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > > if (ret) > > goto err; > > > > + drm_dp_mst_get_port_malloc(port); > > Needs to be moved up where we assing intel_connector->port, or it'll > underflow on cleanup on error paths. > > > + > > return connector; > > > > err: > > -- > > 2.19.2 > >
On Tue, Dec 18, 2018 at 04:52:24PM -0500, Lyude Paul wrote: > On Fri, 2018-12-14 at 10:32 +0100, Daniel Vetter wrote: > > On Thu, Dec 13, 2018 at 08:25:35PM -0500, Lyude Paul wrote: > > > So that the ports stay around until we've destroyed the connectors, in > > > order to ensure that we don't pass an invalid pointer to any MST helpers > > > once we introduce the new MST VCPI helpers. > > > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > > --- > > > drivers/gpu/drm/i915/intel_connector.c | 4 ++++ > > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 ++ > > > 2 files changed, 6 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_connector.c > > > b/drivers/gpu/drm/i915/intel_connector.c > > > index 18e370f607bc..37d2c644f4b8 100644 > > > --- a/drivers/gpu/drm/i915/intel_connector.c > > > +++ b/drivers/gpu/drm/i915/intel_connector.c > > > @@ -95,6 +95,10 @@ void intel_connector_destroy(struct drm_connector > > > *connector) > > > intel_panel_fini(&intel_connector->panel); > > > > > > drm_connector_cleanup(connector); > > > + > > > + if (intel_connector->port) > > > > We set this as the first thing in intel_dp_add_mst_connector, so this > > check isn't doing anything. > > I think this comment might be a mistake? intel_connector_destroy() is shared > by all of the intel connectors, so some may not have a value in > intel_connector->port. I can move it to it's own destroy callback for MST if > you would prefer though. Oops, didn't look at the patch carefully enough. I think a intel_dp_mst_connector_destroy would be useful in this case. But a bit a bikeshed, so up to you. -Daniel > > > > > + drm_dp_mst_put_port_malloc(intel_connector->port); > > > + > > > kfree(connector); > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > > index f05427b74e34..4d6ced34d465 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > @@ -484,6 +484,8 @@ static struct drm_connector > > > *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > > > if (ret) > > > goto err; > > > > > > + drm_dp_mst_get_port_malloc(port); > > > > Needs to be moved up where we assing intel_connector->port, or it'll > > underflow on cleanup on error paths. > > > > > + > > > return connector; > > > > > > err: > > > -- > > > 2.19.2 > > > > -- > Cheers, > Lyude Paul >
diff --git a/drivers/gpu/drm/i915/intel_connector.c b/drivers/gpu/drm/i915/intel_connector.c index 18e370f607bc..37d2c644f4b8 100644 --- a/drivers/gpu/drm/i915/intel_connector.c +++ b/drivers/gpu/drm/i915/intel_connector.c @@ -95,6 +95,10 @@ void intel_connector_destroy(struct drm_connector *connector) intel_panel_fini(&intel_connector->panel); drm_connector_cleanup(connector); + + if (intel_connector->port) + drm_dp_mst_put_port_malloc(intel_connector->port); + kfree(connector); } diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index f05427b74e34..4d6ced34d465 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -484,6 +484,8 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo if (ret) goto err; + drm_dp_mst_get_port_malloc(port); + return connector; err:
So that the ports stay around until we've destroyed the connectors, in order to ensure that we don't pass an invalid pointer to any MST helpers once we introduce the new MST VCPI helpers. Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/i915/intel_connector.c | 4 ++++ drivers/gpu/drm/i915/intel_dp_mst.c | 2 ++ 2 files changed, 6 insertions(+)