Re: [PATCH 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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ä <ville.syrjala@linux.intel.com>
> > 
> > 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 <carlos.santa@intel.com>
> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Tested-by: Carlos Santa <carlos.santa@intel.com>
> > Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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