From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Date: Tue, 24 Oct 2017 16:13:27 +0000 Subject: Re: [PATCH 2/2] drm/i915/dp: Use common error handling code in intel_dp_sink_crc_stop() Message-Id: <873768h6c8.fsf@intel.com> List-Id: References: <668a4b56-091d-d4e7-2204-daaec6675e11@users.sourceforge.net> <1d3d0395-957a-2a87-3cb5-0d7355d04b65@users.sourceforge.net> In-Reply-To: <1d3d0395-957a-2a87-3cb5-0d7355d04b65@users.sourceforge.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: SF Markus Elfring , dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, David Airlie , Joonas Lahtinen , Rodrigo Vivi Cc: kernel-janitors@vger.kernel.org, LKML On Tue, 24 Oct 2017, SF Markus Elfring wrote: > From: Markus Elfring > Date: Tue, 24 Oct 2017 15:40:47 +0200 > > Adjust jump targets so that a specific error code assignment > will be in the implementation only at the end of this function. This is not an issue that needs to be "fixed". > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring > --- > drivers/gpu/drm/i915/intel_dp.c | 30 ++++++++++++++---------------- > 1 file changed, 14 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 342f1a1fa085..3dd514a77008 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3894,27 +3894,19 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp) > int count = 0; > int attempts = 10; > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) { > - DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > - ret = -EIO; > - goto out; > - } > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) > + goto report_failure; > > if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > - buf & ~DP_TEST_SINK_START) < 0) { > - DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > - ret = -EIO; > - goto out; I'd rather change the debug messages to distinguish the cases. I also want to keep ret = -EIO in each individual branch, to distinguish from the cases where it's set to something other than -EIO. > - } > + buf & ~DP_TEST_SINK_START) < 0) > + goto report_failure; > > do { > intel_wait_for_vblank(dev_priv, intel_crtc->pipe); > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) > + < 0) > + goto e_io; > > - if (drm_dp_dpcd_readb(&intel_dp->aux, > - DP_TEST_SINK_MISC, &buf) < 0) { > - ret = -EIO; > - goto out; > - } This is good as it is. The change makes it worse. > count = buf & DP_TEST_COUNT_MASK; > } while (--attempts && count); > > @@ -3923,9 +3915,15 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp) > ret = -ETIMEDOUT; > } > > - out: > +enable_ips: > hsw_enable_ips(intel_crtc); > return ret; > + > +report_failure: > + DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > +e_io: > + ret = -EIO; > + goto enable_ips; *shudder* Please no. BR, Jani. > } > > static int intel_dp_sink_crc_start(struct intel_dp *intel_dp) -- Jani Nikula, Intel Open Source Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 2/2] drm/i915/dp: Use common error handling code in intel_dp_sink_crc_stop() Date: Tue, 24 Oct 2017 19:13:27 +0300 Message-ID: <873768h6c8.fsf@intel.com> References: <668a4b56-091d-d4e7-2204-daaec6675e11@users.sourceforge.net> <1d3d0395-957a-2a87-3cb5-0d7355d04b65@users.sourceforge.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1d3d0395-957a-2a87-3cb5-0d7355d04b65@users.sourceforge.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: SF Markus Elfring , dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, David Airlie , Joonas Lahtinen , Rodrigo Vivi Cc: kernel-janitors@vger.kernel.org, LKML List-Id: dri-devel@lists.freedesktop.org T24gVHVlLCAyNCBPY3QgMjAxNywgU0YgTWFya3VzIEVsZnJpbmcgPGVsZnJpbmdAdXNlcnMuc291 cmNlZm9yZ2UubmV0PiB3cm90ZToKPiBGcm9tOiBNYXJrdXMgRWxmcmluZyA8ZWxmcmluZ0B1c2Vy cy5zb3VyY2Vmb3JnZS5uZXQ+Cj4gRGF0ZTogVHVlLCAyNCBPY3QgMjAxNyAxNTo0MDo0NyArMDIw MAo+Cj4gQWRqdXN0IGp1bXAgdGFyZ2V0cyBzbyB0aGF0IGEgc3BlY2lmaWMgZXJyb3IgY29kZSBh c3NpZ25tZW50Cj4gd2lsbCBiZSBpbiB0aGUgaW1wbGVtZW50YXRpb24gb25seSBhdCB0aGUgZW5k IG9mIHRoaXMgZnVuY3Rpb24uCgpUaGlzIGlzIG5vdCBhbiBpc3N1ZSB0aGF0IG5lZWRzIHRvIGJl ICJmaXhlZCIuCgo+Cj4gVGhpcyBpc3N1ZSB3YXMgZGV0ZWN0ZWQgYnkgdXNpbmcgdGhlIENvY2Np bmVsbGUgc29mdHdhcmUuCj4KPiBTaWduZWQtb2ZmLWJ5OiBNYXJrdXMgRWxmcmluZyA8ZWxmcmlu Z0B1c2Vycy5zb3VyY2Vmb3JnZS5uZXQ+Cj4gLS0tCj4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2lu dGVsX2RwLmMgfCAzMCArKysrKysrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0KPiAgMSBmaWxlIGNo YW5nZWQsIDE0IGluc2VydGlvbnMoKyksIDE2IGRlbGV0aW9ucygtKQo+Cj4gZGlmZiAtLWdpdCBh L2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9p bnRlbF9kcC5jCj4gaW5kZXggMzQyZjFhMWZhMDg1Li4zZGQ1MTRhNzcwMDggMTAwNjQ0Cj4gLS0t IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHAuYwo+ICsrKyBiL2RyaXZlcnMvZ3B1L2Ry bS9pOTE1L2ludGVsX2RwLmMKPiBAQCAtMzg5NCwyNyArMzg5NCwxOSBAQCBzdGF0aWMgaW50IGlu dGVsX2RwX3NpbmtfY3JjX3N0b3Aoc3RydWN0IGludGVsX2RwICppbnRlbF9kcCkKPiAgCWludCBj b3VudCA9IDA7Cj4gIAlpbnQgYXR0ZW1wdHMgPSAxMDsKPiAgCj4gLQlpZiAoZHJtX2RwX2RwY2Rf cmVhZGIoJmludGVsX2RwLT5hdXgsIERQX1RFU1RfU0lOSywgJmJ1ZikgPCAwKSB7Cj4gLQkJRFJN X0RFQlVHX0tNUygiU2luayBDUkMgY291bGRuJ3QgYmUgc3RvcHBlZCBwcm9wZXJseVxuIik7Cj4g LQkJcmV0ID0gLUVJTzsKPiAtCQlnb3RvIG91dDsKPiAtCX0KPiArCWlmIChkcm1fZHBfZHBjZF9y ZWFkYigmaW50ZWxfZHAtPmF1eCwgRFBfVEVTVF9TSU5LLCAmYnVmKSA8IDApCj4gKwkJZ290byBy ZXBvcnRfZmFpbHVyZTsKPiAgCj4gIAlpZiAoZHJtX2RwX2RwY2Rfd3JpdGViKCZpbnRlbF9kcC0+ YXV4LCBEUF9URVNUX1NJTkssCj4gLQkJCSAgICAgICBidWYgJiB+RFBfVEVTVF9TSU5LX1NUQVJU KSA8IDApIHsKPiAtCQlEUk1fREVCVUdfS01TKCJTaW5rIENSQyBjb3VsZG4ndCBiZSBzdG9wcGVk IHByb3Blcmx5XG4iKTsKPiAtCQlyZXQgPSAtRUlPOwo+IC0JCWdvdG8gb3V0OwoKSSdkIHJhdGhl ciBjaGFuZ2UgdGhlIGRlYnVnIG1lc3NhZ2VzIHRvIGRpc3Rpbmd1aXNoIHRoZSBjYXNlcy4gSSBh bHNvCndhbnQgdG8ga2VlcCByZXQgPSAtRUlPIGluIGVhY2ggaW5kaXZpZHVhbCBicmFuY2gsIHRv IGRpc3Rpbmd1aXNoIGZyb20KdGhlIGNhc2VzIHdoZXJlIGl0J3Mgc2V0IHRvIHNvbWV0aGluZyBv dGhlciB0aGFuIC1FSU8uCgo+IC0JfQo+ICsJCQkgICAgICAgYnVmICYgfkRQX1RFU1RfU0lOS19T VEFSVCkgPCAwKQo+ICsJCWdvdG8gcmVwb3J0X2ZhaWx1cmU7Cj4gIAo+ICAJZG8gewo+ICAJCWlu dGVsX3dhaXRfZm9yX3ZibGFuayhkZXZfcHJpdiwgaW50ZWxfY3J0Yy0+cGlwZSk7Cj4gKwkJaWYg KGRybV9kcF9kcGNkX3JlYWRiKCZpbnRlbF9kcC0+YXV4LCBEUF9URVNUX1NJTktfTUlTQywgJmJ1 ZikKPiArCQkgICAgPCAwKQo+ICsJCQlnb3RvIGVfaW87Cj4gIAo+IC0JCWlmIChkcm1fZHBfZHBj ZF9yZWFkYigmaW50ZWxfZHAtPmF1eCwKPiAtCQkJCSAgICAgIERQX1RFU1RfU0lOS19NSVNDLCAm YnVmKSA8IDApIHsKPiAtCQkJcmV0ID0gLUVJTzsKPiAtCQkJZ290byBvdXQ7Cj4gLQkJfQoKVGhp cyBpcyBnb29kIGFzIGl0IGlzLiBUaGUgY2hhbmdlIG1ha2VzIGl0IHdvcnNlLgoKPiAgCQljb3Vu dCA9IGJ1ZiAmIERQX1RFU1RfQ09VTlRfTUFTSzsKPiAgCX0gd2hpbGUgKC0tYXR0ZW1wdHMgJiYg Y291bnQpOwo+ICAKPiBAQCAtMzkyMyw5ICszOTE1LDE1IEBAIHN0YXRpYyBpbnQgaW50ZWxfZHBf c2lua19jcmNfc3RvcChzdHJ1Y3QgaW50ZWxfZHAgKmludGVsX2RwKQo+ICAJCXJldCA9IC1FVElN RURPVVQ7Cj4gIAl9Cj4gIAo+IC0gb3V0Ogo+ICtlbmFibGVfaXBzOgo+ICAJaHN3X2VuYWJsZV9p cHMoaW50ZWxfY3J0Yyk7Cj4gIAlyZXR1cm4gcmV0Owo+ICsKPiArcmVwb3J0X2ZhaWx1cmU6Cj4g KwlEUk1fREVCVUdfS01TKCJTaW5rIENSQyBjb3VsZG4ndCBiZSBzdG9wcGVkIHByb3Blcmx5XG4i KTsKPiArZV9pbzoKPiArCXJldCA9IC1FSU87Cj4gKwlnb3RvIGVuYWJsZV9pcHM7Cgoqc2h1ZGRl ciogUGxlYXNlIG5vLgoKQlIsCkphbmkuCgo+ICB9Cj4gIAo+ICBzdGF0aWMgaW50IGludGVsX2Rw X3NpbmtfY3JjX3N0YXJ0KHN0cnVjdCBpbnRlbF9kcCAqaW50ZWxfZHApCgotLSAKSmFuaSBOaWt1 bGEsIEludGVsIE9wZW4gU291cmNlIFRlY2hub2xvZ3kgQ2VudGVyCl9fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCkludGVsLWdmeCBtYWlsaW5nIGxpc3QKSW50 ZWwtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2ludGVsLWdmeAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751778AbdJXQNz (ORCPT ); Tue, 24 Oct 2017 12:13:55 -0400 Received: from mga04.intel.com ([192.55.52.120]:37039 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751682AbdJXQNw (ORCPT ); Tue, 24 Oct 2017 12:13:52 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,428,1503385200"; d="scan'208";a="913235965" From: Jani Nikula To: SF Markus Elfring , dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, David Airlie , Joonas Lahtinen , Rodrigo Vivi Cc: LKML , kernel-janitors@vger.kernel.org Subject: Re: [PATCH 2/2] drm/i915/dp: Use common error handling code in intel_dp_sink_crc_stop() In-Reply-To: <1d3d0395-957a-2a87-3cb5-0d7355d04b65@users.sourceforge.net> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <668a4b56-091d-d4e7-2204-daaec6675e11@users.sourceforge.net> <1d3d0395-957a-2a87-3cb5-0d7355d04b65@users.sourceforge.net> Date: Tue, 24 Oct 2017 19:13:27 +0300 Message-ID: <873768h6c8.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 24 Oct 2017, SF Markus Elfring wrote: > From: Markus Elfring > Date: Tue, 24 Oct 2017 15:40:47 +0200 > > Adjust jump targets so that a specific error code assignment > will be in the implementation only at the end of this function. This is not an issue that needs to be "fixed". > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring > --- > drivers/gpu/drm/i915/intel_dp.c | 30 ++++++++++++++---------------- > 1 file changed, 14 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 342f1a1fa085..3dd514a77008 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3894,27 +3894,19 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp) > int count = 0; > int attempts = 10; > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) { > - DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > - ret = -EIO; > - goto out; > - } > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) > + goto report_failure; > > if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > - buf & ~DP_TEST_SINK_START) < 0) { > - DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > - ret = -EIO; > - goto out; I'd rather change the debug messages to distinguish the cases. I also want to keep ret = -EIO in each individual branch, to distinguish from the cases where it's set to something other than -EIO. > - } > + buf & ~DP_TEST_SINK_START) < 0) > + goto report_failure; > > do { > intel_wait_for_vblank(dev_priv, intel_crtc->pipe); > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) > + < 0) > + goto e_io; > > - if (drm_dp_dpcd_readb(&intel_dp->aux, > - DP_TEST_SINK_MISC, &buf) < 0) { > - ret = -EIO; > - goto out; > - } This is good as it is. The change makes it worse. > count = buf & DP_TEST_COUNT_MASK; > } while (--attempts && count); > > @@ -3923,9 +3915,15 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp) > ret = -ETIMEDOUT; > } > > - out: > +enable_ips: > hsw_enable_ips(intel_crtc); > return ret; > + > +report_failure: > + DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > +e_io: > + ret = -EIO; > + goto enable_ips; *shudder* Please no. BR, Jani. > } > > static int intel_dp_sink_crc_start(struct intel_dp *intel_dp) -- Jani Nikula, Intel Open Source Technology Center