From: Daniel Vetter <daniel@ffwll.ch> To: ville.syrjala@linux.intel.com Cc: dri-devel@lists.freedesktop.org, "Kirill A . Shutemov" <kirill@shutemov.name>, Carlos Santa <carlos.santa@intel.com>, stable@vger.kernel.org Subject: Re: [PATCH 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter Date: Wed, 26 Oct 2016 14:53:01 +0200 [thread overview] Message-ID: <20161026125301.la37v6njolevq2ds@phenom.ffwll.local> (raw) In-Reply-To: <1477472755-15288-4-git-send-email-ville.syrjala@linux.intel.com> 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: ville.syrjala@linux.intel.com Cc: dri-devel@lists.freedesktop.org, "Kirill A . Shutemov" <kirill@shutemov.name>, Carlos Santa <carlos.santa@intel.com>, stable@vger.kernel.org Subject: Re: [PATCH 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter Date: Wed, 26 Oct 2016 14:53:01 +0200 [thread overview] Message-ID: <20161026125301.la37v6njolevq2ds@phenom.ffwll.local> (raw) In-Reply-To: <1477472755-15288-4-git-send-email-ville.syrjala@linux.intel.com> 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
next prev parent reply other threads:[~2016-10-26 12:53 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-10-26 9:05 [PATCH 0/4] drm: Fix MST oopses in fbdev restore ville.syrjala 2016-10-26 9:05 ` [PATCH 1/4] drm/fb-helper: Fix connector ref leak on error ville.syrjala 2016-10-26 9:48 ` Chris Wilson 2016-10-26 9:48 ` Chris Wilson 2016-10-26 9:05 ` [PATCH 2/4] drm/fb-helper: Keep references for the current set of used connectors ville.syrjala 2016-10-26 9:05 ` ville.syrjala 2016-10-26 9:52 ` Chris Wilson 2016-10-26 9:52 ` Chris Wilson 2016-10-26 12:34 ` Ville Syrjälä 2016-10-26 12:34 ` Ville Syrjälä 2016-10-26 13:31 ` [PATCH v2 " ville.syrjala 2016-10-26 13:54 ` Chris Wilson 2016-10-26 13:54 ` Chris Wilson 2016-10-26 14:11 ` Ville Syrjälä 2016-10-26 14:11 ` Ville Syrjälä 2016-10-26 14:41 ` [PATCH v3 " ville.syrjala 2016-10-26 14:41 ` ville.syrjala 2016-10-26 9:05 ` [PATCH 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter ville.syrjala 2016-10-26 9:05 ` ville.syrjala 2016-10-26 12:53 ` Daniel Vetter [this message] 2016-10-26 12:53 ` Daniel Vetter 2016-10-26 13:02 ` Ville Syrjälä 2016-10-26 13:02 ` Ville Syrjälä 2016-10-26 13:30 ` [PATCH v2 " ville.syrjala 2016-10-26 13:30 ` ville.syrjala 2016-10-26 13:35 ` Ville Syrjälä 2016-10-26 13:35 ` Ville Syrjälä 2016-10-26 14:02 ` Daniel Vetter 2016-10-26 14:02 ` Daniel Vetter 2016-10-26 16:54 ` Daniel Vetter 2016-10-26 16:54 ` Daniel Vetter 2016-10-26 14:46 ` [PATCH v3 " ville.syrjala 2016-10-26 14:46 ` ville.syrjala 2016-10-26 9:05 ` [PATCH 4/4] drm/dp/mst: Check peer device type before attempting EDID read ville.syrjala 2016-10-26 12:55 ` Daniel Vetter 2016-10-26 12:55 ` Daniel Vetter 2016-10-26 13:05 ` Ville Syrjälä 2016-10-26 13:05 ` Ville Syrjälä 2016-10-26 18:31 ` Carlos Santa 2016-10-26 18:31 ` Carlos Santa 2016-10-26 18:45 ` Ville Syrjälä 2016-10-26 18:45 ` Ville Syrjälä 2016-10-26 13:38 ` [PATCH 0/4] drm: Fix MST oopses in fbdev restore Ville Syrjälä
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20161026125301.la37v6njolevq2ds@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=carlos.santa@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=kirill@shutemov.name \ --cc=stable@vger.kernel.org \ --cc=ville.syrjala@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.