From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter Date: Wed, 26 Oct 2016 16:02:31 +0300 Message-ID: <20161026130231.GC4617@intel.com> References: <1477472755-15288-1-git-send-email-ville.syrjala@linux.intel.com> <1477472755-15288-4-git-send-email-ville.syrjala@linux.intel.com> <20161026125301.la37v6njolevq2ds@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <20161026125301.la37v6njolevq2ds@phenom.ffwll.local> Sender: stable-owner@vger.kernel.org To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org, "Kirill A . Shutemov" , Carlos Santa , stable@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org On Wed, Oct 26, 2016 at 02:53:01PM +0200, Daniel Vetter wrote: > On Wed, Oct 26, 2016 at 12:05:54PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä > > > > The i2c adapter is only relevant for some peer device types, so > > let's clear the pdt if it's still the same as the old_pdt when we > > tear down the i2c adapter. > > > > I don't really like this design pattern of updating port->whatever > > before doing the accompanying changes and passing around old_whatever > > to figure stuff out. Would make much more sense to me to the pass the > > new value around and only update the port->whatever when things are > > consistent. But let's try to work with what we have right now. > > > > Cc: stable@vger.kernel.org > > Cc: Carlos Santa > > Cc: Kirill A. Shutemov > > Tested-by: Carlos Santa > > Tested-by: Kirill A. Shutemov > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666 > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > > index 04e457117980..956babc161e5 100644 > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > @@ -882,6 +882,9 @@ static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt) > > drm_dp_put_mst_branch_device(mstb); > > break; > > } > > + > > + if (port->pdt == old_pdt) > > + port->pdt = DP_PEER_DEVICE_NONE; > > So from my understanding this is needed for the callsite in > drm_dp_destroy_connector_work(). All others are either the final destroy > path, or set up the ->pdt to something before calling this function. Only > this call site passes port->pdt. I think we should instead change this > callsite to set the port->pdt to NONE after the call, i.e. > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 04e457117980..36f47092c703 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -2919,6 +2919,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work) > mgr->cbs->destroy_connector(mgr, port->connector); > > drm_dp_port_teardown_pdt(port, port->pdt); > + port->pdt = DP_PEER_DEVICE_NONE; > > if (!port->input && port->vcpi.vcpi > 0) { > drm_dp_mst_reset_vcpi_slots(mgr, port); > > > I think that would be more consistent than spreading the control flow even > more like in your patch. Yeah, makes sense. -- Ville Syrjälä Intel OTC