From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:57526 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755335AbcCPToj (ORCPT ); Wed, 16 Mar 2016 15:44:39 -0400 Message-ID: <1458157477.9055.4.camel@redhat.com> Subject: Re: [PATCH] drm/i915: Fix race condition in intel_dp_destroy_mst_connector() From: Lyude Paul To: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org, Rob Clark , "open list:INTEL DRM DRIVERS excluding Poulsbo, Moorestow..., " "linux-kernel@vger.kernel.org open list" , stable@vger.kernel.org, Daniel Vetter Date: Wed, 16 Mar 2016 15:44:37 -0400 In-Reply-To: <20160316193949.GU4329@intel.com> References: <1458155884-13877-1-git-send-email-cpaul@redhat.com> <20160316193949.GU4329@intel.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: On Wed, 2016-03-16 at 21:39 +0200, Ville Syrjälä wrote: > On Wed, Mar 16, 2016 at 03:18:04PM -0400, Lyude wrote: > > > > After unplugging a DP MST display from the system, we have to go through > > and destroy all of the DRM connectors associated with it since none of > > them are valid anymore. Unfortunately, intel_dp_destroy_mst_connector() > > doesn't do a good enough job of ensuring that throughout the destruction > > process that no modesettings can be done with the connectors. As it is > > right now, intel_dp_destroy_mst_connector() works like this: > > > > * Take all modeset locks > > * Clear the configuration of the crtc on the connector, if there is one > > * Drop all modeset locks, this is required because of circular > >   dependency issues that arise with trying to remove the connector from > >   sysfs with modeset locks held > > * Unregister the connector > > * Take all modeset locks, again > > * Do the rest of the required cleaning for destroying the connector > > * Finally drop all modeset locks for good > So pretty much what I suspected > https://lists.freedesktop.org/archives/intel-gfx/2016-February/087734.html > > > > > > > This only works sometimes. During the destruction process, it's very > > possible that a userspace application will attempt to do a modesetting > > using the connector. When we drop the modeset locks, an ioctl handler > > such as drm_mode_setcrtc has the oppurtunity to take all of the modeset > > locks from us. When this happens, one thing leads to another and > > eventually we end up committing a mode with the non-existent connector: > > > > [drm:intel_dp_link_training_clock_recovery [i915]] *ERROR* failed to > > enable link training > > [drm:intel_dp_aux_ch] dp_aux_ch timeout status 0x7cf0001f > > [drm:intel_dp_start_link_train [i915]] *ERROR* failed to start channel > > equalization > > [drm:intel_dp_aux_ch] dp_aux_ch timeout status 0x7cf0001f > > [drm:intel_mst_pre_enable_dp [i915]] *ERROR* failed to allocate vcpi > > > > And in some cases, such as with the T460s using an MST dock, this > > results in breaking modesetting and/or panicking the system. > Are these just kernel oopses etc.? If the hardware gets upset from > modesetting when the sink is gone, well, then we still have a problem > because the user can of course yank the cable while the modeset is already > underway. It is more then that. Unfortunately though, fixing that part is not as easy. We never expect an atomic modesetting commit to fail, but unfortunately any code having to do with turning on DP MST has the chance of failing and we turn on DP MST during commits. So fixing that would take moving quite a bit of code around. > > > > > > > To work around this, we now unregister the connector at the very > > beginning of intel_dp_destroy_mst_connector(), grab all the modesetting > > locks, and then hold them until we finish the rest of the function. > > > > CC: stable@vger.kernel.org > > Signed-off-by: Lyude > > Signed-off-by: Rob Clark > These sobs don't make much sense to me. I should have mentioned that Rob Clark was the one who came up with the idea of just moving the connector->unregister() call to the top of the function. > > Patch itself does make sense to me, so  > Reviewed-by: Ville Syrjälä > > > > > --- > >  drivers/gpu/drm/i915/intel_dp_mst.c | 6 ++---- > >  1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > index fa0dabf..b21ac88 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -499,6 +499,8 @@ static void intel_dp_destroy_mst_connector(struct > > drm_dp_mst_topology_mgr *mgr, > >   struct intel_connector *intel_connector = > > to_intel_connector(connector); > >   struct drm_device *dev = connector->dev; > >   > > + intel_connector->unregister(intel_connector); > > + > >   /* need to nuke the connector */ > >   drm_modeset_lock_all(dev); > >   if (connector->state->crtc) { > > @@ -512,11 +514,7 @@ static void intel_dp_destroy_mst_connector(struct > > drm_dp_mst_topology_mgr *mgr, > >   > >   WARN(ret, "Disabling mst crtc failed with %i\n", ret); > >   } > > - drm_modeset_unlock_all(dev); > >   > > - intel_connector->unregister(intel_connector); > > - > > - drm_modeset_lock_all(dev); > >   intel_connector_remove_from_fbdev(intel_connector); > >   drm_connector_cleanup(connector); > >   drm_modeset_unlock_all(dev); > > --  > > 2.5.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lyude Paul Subject: Re: [PATCH] drm/i915: Fix race condition in intel_dp_destroy_mst_connector() Date: Wed, 16 Mar 2016 15:44:37 -0400 Message-ID: <1458157477.9055.4.camel@redhat.com> References: <1458155884-13877-1-git-send-email-cpaul@redhat.com> <20160316193949.GU4329@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160316193949.GU4329@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, Rob Clark , stable@vger.kernel.org, "open list:INTEL DRM DRIVERS excluding Poulsbo, Moorestow..., linux-kernel@vger.kernel.org open list" List-Id: dri-devel@lists.freedesktop.org T24gV2VkLCAyMDE2LTAzLTE2IGF0IDIxOjM5ICswMjAwLCBWaWxsZSBTeXJqw6Rsw6Qgd3JvdGU6 Cj4gT24gV2VkLCBNYXIgMTYsIDIwMTYgYXQgMDM6MTg6MDRQTSAtMDQwMCwgTHl1ZGUgd3JvdGU6 Cj4gPiAKPiA+IEFmdGVyIHVucGx1Z2dpbmcgYSBEUCBNU1QgZGlzcGxheSBmcm9tIHRoZSBzeXN0 ZW0sIHdlIGhhdmUgdG8gZ28gdGhyb3VnaAo+ID4gYW5kIGRlc3Ryb3kgYWxsIG9mIHRoZSBEUk0g Y29ubmVjdG9ycyBhc3NvY2lhdGVkIHdpdGggaXQgc2luY2Ugbm9uZSBvZgo+ID4gdGhlbSBhcmUg dmFsaWQgYW55bW9yZS4gVW5mb3J0dW5hdGVseSwgaW50ZWxfZHBfZGVzdHJveV9tc3RfY29ubmVj dG9yKCkKPiA+IGRvZXNuJ3QgZG8gYSBnb29kIGVub3VnaCBqb2Igb2YgZW5zdXJpbmcgdGhhdCB0 aHJvdWdob3V0IHRoZSBkZXN0cnVjdGlvbgo+ID4gcHJvY2VzcyB0aGF0IG5vIG1vZGVzZXR0aW5n cyBjYW4gYmUgZG9uZSB3aXRoIHRoZSBjb25uZWN0b3JzLiBBcyBpdCBpcwo+ID4gcmlnaHQgbm93 LCBpbnRlbF9kcF9kZXN0cm95X21zdF9jb25uZWN0b3IoKSB3b3JrcyBsaWtlIHRoaXM6Cj4gPiAK PiA+ICogVGFrZSBhbGwgbW9kZXNldCBsb2Nrcwo+ID4gKiBDbGVhciB0aGUgY29uZmlndXJhdGlv biBvZiB0aGUgY3J0YyBvbiB0aGUgY29ubmVjdG9yLCBpZiB0aGVyZSBpcyBvbmUKPiA+ICogRHJv cCBhbGwgbW9kZXNldCBsb2NrcywgdGhpcyBpcyByZXF1aXJlZCBiZWNhdXNlIG9mIGNpcmN1bGFy Cj4gPiDCoCBkZXBlbmRlbmN5IGlzc3VlcyB0aGF0IGFyaXNlIHdpdGggdHJ5aW5nIHRvIHJlbW92 ZSB0aGUgY29ubmVjdG9yIGZyb20KPiA+IMKgIHN5c2ZzIHdpdGggbW9kZXNldCBsb2NrcyBoZWxk Cj4gPiAqIFVucmVnaXN0ZXIgdGhlIGNvbm5lY3Rvcgo+ID4gKiBUYWtlIGFsbCBtb2Rlc2V0IGxv Y2tzLCBhZ2Fpbgo+ID4gKiBEbyB0aGUgcmVzdCBvZiB0aGUgcmVxdWlyZWQgY2xlYW5pbmcgZm9y IGRlc3Ryb3lpbmcgdGhlIGNvbm5lY3Rvcgo+ID4gKiBGaW5hbGx5IGRyb3AgYWxsIG1vZGVzZXQg bG9ja3MgZm9yIGdvb2QKPiBTbyBwcmV0dHkgbXVjaCB3aGF0IEkgc3VzcGVjdGVkCj4gaHR0cHM6 Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvYXJjaGl2ZXMvaW50ZWwtZ2Z4LzIwMTYtRmVicnVhcnkv MDg3NzM0Lmh0bWwKPiAKPiA+IAo+ID4gCj4gPiBUaGlzIG9ubHkgd29ya3Mgc29tZXRpbWVzLiBE dXJpbmcgdGhlIGRlc3RydWN0aW9uIHByb2Nlc3MsIGl0J3MgdmVyeQo+ID4gcG9zc2libGUgdGhh dCBhIHVzZXJzcGFjZSBhcHBsaWNhdGlvbiB3aWxsIGF0dGVtcHQgdG8gZG8gYSBtb2Rlc2V0dGlu Zwo+ID4gdXNpbmcgdGhlIGNvbm5lY3Rvci4gV2hlbiB3ZSBkcm9wIHRoZSBtb2Rlc2V0IGxvY2tz LCBhbiBpb2N0bCBoYW5kbGVyCj4gPiBzdWNoIGFzIGRybV9tb2RlX3NldGNydGMgaGFzIHRoZSBv cHB1cnR1bml0eSB0byB0YWtlIGFsbCBvZiB0aGUgbW9kZXNldAo+ID4gbG9ja3MgZnJvbSB1cy4g V2hlbiB0aGlzIGhhcHBlbnMsIG9uZSB0aGluZyBsZWFkcyB0byBhbm90aGVyIGFuZAo+ID4gZXZl bnR1YWxseSB3ZSBlbmQgdXAgY29tbWl0dGluZyBhIG1vZGUgd2l0aCB0aGUgbm9uLWV4aXN0ZW50 IGNvbm5lY3RvcjoKPiA+IAo+ID4gCVtkcm06aW50ZWxfZHBfbGlua190cmFpbmluZ19jbG9ja19y ZWNvdmVyeSBbaTkxNV1dICpFUlJPUiogZmFpbGVkIHRvCj4gPiBlbmFibGUgbGluayB0cmFpbmlu Zwo+ID4gCVtkcm06aW50ZWxfZHBfYXV4X2NoXSBkcF9hdXhfY2ggdGltZW91dCBzdGF0dXMgMHg3 Y2YwMDAxZgo+ID4gCVtkcm06aW50ZWxfZHBfc3RhcnRfbGlua190cmFpbiBbaTkxNV1dICpFUlJP UiogZmFpbGVkIHRvIHN0YXJ0IGNoYW5uZWwKPiA+IGVxdWFsaXphdGlvbgo+ID4gCVtkcm06aW50 ZWxfZHBfYXV4X2NoXSBkcF9hdXhfY2ggdGltZW91dCBzdGF0dXMgMHg3Y2YwMDAxZgo+ID4gCVtk cm06aW50ZWxfbXN0X3ByZV9lbmFibGVfZHAgW2k5MTVdXSAqRVJST1IqIGZhaWxlZCB0byBhbGxv Y2F0ZSB2Y3BpCj4gPiAKPiA+IEFuZCBpbiBzb21lIGNhc2VzLCBzdWNoIGFzIHdpdGggdGhlIFQ0 NjBzIHVzaW5nIGFuIE1TVCBkb2NrLCB0aGlzCj4gPiByZXN1bHRzIGluIGJyZWFraW5nIG1vZGVz ZXR0aW5nIGFuZC9vciBwYW5pY2tpbmcgdGhlIHN5c3RlbS4KPiBBcmUgdGhlc2UganVzdCBrZXJu ZWwgb29wc2VzIGV0Yy4/IElmIHRoZSBoYXJkd2FyZSBnZXRzIHVwc2V0IGZyb20KPiBtb2Rlc2V0 dGluZyB3aGVuIHRoZSBzaW5rIGlzIGdvbmUsIHdlbGwsIHRoZW4gd2Ugc3RpbGwgaGF2ZSBhIHBy b2JsZW0KPiBiZWNhdXNlIHRoZSB1c2VyIGNhbiBvZiBjb3Vyc2UgeWFuayB0aGUgY2FibGUgd2hp bGUgdGhlIG1vZGVzZXQgaXMgYWxyZWFkeQo+IHVuZGVyd2F5LgpJdCBpcyBtb3JlIHRoZW4gdGhh dC4gVW5mb3J0dW5hdGVseSB0aG91Z2gsIGZpeGluZyB0aGF0IHBhcnQgaXMgbm90IGFzIGVhc3ku IFdlCm5ldmVyIGV4cGVjdCBhbiBhdG9taWMgbW9kZXNldHRpbmcgY29tbWl0IHRvIGZhaWwsIGJ1 dCB1bmZvcnR1bmF0ZWx5IGFueSBjb2RlCmhhdmluZyB0byBkbyB3aXRoIHR1cm5pbmcgb24gRFAg TVNUIGhhcyB0aGUgY2hhbmNlIG9mIGZhaWxpbmcgYW5kIHdlIHR1cm4gb24gRFAKTVNUIGR1cmlu ZyBjb21taXRzLiBTbyBmaXhpbmcgdGhhdCB3b3VsZCB0YWtlIG1vdmluZyBxdWl0ZSBhIGJpdCBv ZiBjb2RlIGFyb3VuZC4KCj4gCj4gPiAKPiA+IAo+ID4gVG8gd29yayBhcm91bmQgdGhpcywgd2Ug bm93IHVucmVnaXN0ZXIgdGhlIGNvbm5lY3RvciBhdCB0aGUgdmVyeQo+ID4gYmVnaW5uaW5nIG9m IGludGVsX2RwX2Rlc3Ryb3lfbXN0X2Nvbm5lY3RvcigpLCBncmFiIGFsbCB0aGUgbW9kZXNldHRp bmcKPiA+IGxvY2tzLCBhbmQgdGhlbiBob2xkIHRoZW0gdW50aWwgd2UgZmluaXNoIHRoZSByZXN0 IG9mIHRoZSBmdW5jdGlvbi4KPiA+IAo+ID4gQ0M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcKPiA+ IFNpZ25lZC1vZmYtYnk6IEx5dWRlIDxjcGF1bEByZWRoYXQuY29tPgo+ID4gU2lnbmVkLW9mZi1i eTogUm9iIENsYXJrIDxyY2xhcmtAcmVkaGF0LmNvbT4KPiBUaGVzZSBzb2JzIGRvbid0IG1ha2Ug bXVjaCBzZW5zZSB0byBtZS4KSSBzaG91bGQgaGF2ZSBtZW50aW9uZWQgdGhhdCBSb2IgQ2xhcmsg d2FzIHRoZSBvbmUgd2hvIGNhbWUgdXAgd2l0aCB0aGUgaWRlYSBvZgpqdXN0IG1vdmluZyB0aGUg Y29ubmVjdG9yLT51bnJlZ2lzdGVyKCkgY2FsbCB0byB0aGUgdG9wIG9mIHRoZSBmdW5jdGlvbi4K Cj4gCj4gUGF0Y2ggaXRzZWxmIGRvZXMgbWFrZSBzZW5zZSB0byBtZSwgc2/CoAo+IFJldmlld2Vk LWJ5OiBWaWxsZSBTeXJqw6Rsw6QgPHZpbGxlLnN5cmphbGFAbGludXguaW50ZWwuY29tPgo+IAo+ ID4gCj4gPiAtLS0KPiA+IMKgZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHBfbXN0LmMgfCA2 ICsrLS0tLQo+ID4gwqAxIGZpbGUgY2hhbmdlZCwgMiBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9u cygtKQo+ID4gCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHBf bXN0LmMKPiA+IGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHBfbXN0LmMKPiA+IGluZGV4 IGZhMGRhYmYuLmIyMWFjODggMTAwNjQ0Cj4gPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9p bnRlbF9kcF9tc3QuYwo+ID4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHBfbXN0 LmMKPiA+IEBAIC00OTksNiArNDk5LDggQEAgc3RhdGljIHZvaWQgaW50ZWxfZHBfZGVzdHJveV9t c3RfY29ubmVjdG9yKHN0cnVjdAo+ID4gZHJtX2RwX21zdF90b3BvbG9neV9tZ3IgKm1nciwKPiA+ IMKgCXN0cnVjdCBpbnRlbF9jb25uZWN0b3IgKmludGVsX2Nvbm5lY3RvciA9Cj4gPiB0b19pbnRl bF9jb25uZWN0b3IoY29ubmVjdG9yKTsKPiA+IMKgCXN0cnVjdCBkcm1fZGV2aWNlICpkZXYgPSBj b25uZWN0b3ItPmRldjsKPiA+IMKgCj4gPiArCWludGVsX2Nvbm5lY3Rvci0+dW5yZWdpc3Rlcihp bnRlbF9jb25uZWN0b3IpOwo+ID4gKwo+ID4gwqAJLyogbmVlZCB0byBudWtlIHRoZSBjb25uZWN0 b3IgKi8KPiA+IMKgCWRybV9tb2Rlc2V0X2xvY2tfYWxsKGRldik7Cj4gPiDCoAlpZiAoY29ubmVj dG9yLT5zdGF0ZS0+Y3J0Yykgewo+ID4gQEAgLTUxMiwxMSArNTE0LDcgQEAgc3RhdGljIHZvaWQg aW50ZWxfZHBfZGVzdHJveV9tc3RfY29ubmVjdG9yKHN0cnVjdAo+ID4gZHJtX2RwX21zdF90b3Bv bG9neV9tZ3IgKm1nciwKPiA+IMKgCj4gPiDCoAkJV0FSTihyZXQsICJEaXNhYmxpbmcgbXN0IGNy dGMgZmFpbGVkIHdpdGggJWlcbiIsIHJldCk7Cj4gPiDCoAl9Cj4gPiAtCWRybV9tb2Rlc2V0X3Vu bG9ja19hbGwoZGV2KTsKPiA+IMKgCj4gPiAtCWludGVsX2Nvbm5lY3Rvci0+dW5yZWdpc3Rlcihp bnRlbF9jb25uZWN0b3IpOwo+ID4gLQo+ID4gLQlkcm1fbW9kZXNldF9sb2NrX2FsbChkZXYpOwo+ ID4gwqAJaW50ZWxfY29ubmVjdG9yX3JlbW92ZV9mcm9tX2ZiZGV2KGludGVsX2Nvbm5lY3Rvcik7 Cj4gPiDCoAlkcm1fY29ubmVjdG9yX2NsZWFudXAoY29ubmVjdG9yKTsKPiA+IMKgCWRybV9tb2Rl c2V0X3VubG9ja19hbGwoZGV2KTsKPiA+IC0twqAKPiA+IDIuNS4wCj4gPiAKPiA+IF9fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCj4gPiBkcmktZGV2ZWwgbWFp bGluZyBsaXN0Cj4gPiBkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCj4gPiBodHRwczov L2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbApfX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGlu ZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVl ZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK