From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Date: Thu, 13 Oct 2016 12:13:11 +0000 Subject: Re: [patch] drm/i915: fix a read size argument Message-Id: <87zim8wip4.fsf@intel.com> List-Id: References: <20161013085508.GJ16198@mwanda> <57FF5380.1080704@bfs.de> In-Reply-To: <57FF5380.1080704@bfs.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: wharms@bfs.de, Dan Carpenter Cc: intel-gfx@lists.freedesktop.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, drm-intel-fixes@lists.freedesktop.org, Daniel Vetter On Thu, 13 Oct 2016, walter harms wrote: > Am 13.10.2016 10:55, schrieb Dan Carpenter: >> We want to read 3 bytes here, but because the parenthesis are in the >> wrong place we instead read: >> >> sizeof(intel_dp->edp_dpcd) = sizeof(intel_dp->edp_dpcd) >> >> which is one byte. >> >> Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only once on eDP") >> Signed-off-by: Dan Carpenter Dan, good catch, thank you. Luckily we only really use the first byte currently... Cc: >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 14a3cf0..ee8aa95 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3551,8 +3551,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) >> /* Read the eDP Display control capabilities registers */ >> if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) && >> drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, >> - intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd) = >> - sizeof(intel_dp->edp_dpcd))) >> + intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) = >> + sizeof(intel_dp->edp_dpcd)) > > > > perhaps we can morph that into something more readable ? I would suggest: > > > if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) > { > size_t size=sizeof(intel_dp->edp_dpcd); /* = EDP_DISPLAY_CTL_CAP_SIZE */ > int ret; > > ret=drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV,intel_dp->edp_dpcd,size); > > if (ret != size ) > ...... > > } > > this way it improves readability and reduces line length. Not convinced, let's just take the simple brace fix now. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [patch] drm/i915: fix a read size argument Date: Thu, 13 Oct 2016 15:13:11 +0300 Message-ID: <87zim8wip4.fsf@intel.com> References: <20161013085508.GJ16198@mwanda> <57FF5380.1080704@bfs.de> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <57FF5380.1080704@bfs.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: wharms@bfs.de, Dan Carpenter Cc: intel-gfx@lists.freedesktop.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, drm-intel-fixes@lists.freedesktop.org, Daniel Vetter List-Id: dri-devel@lists.freedesktop.org T24gVGh1LCAxMyBPY3QgMjAxNiwgd2FsdGVyIGhhcm1zIDx3aGFybXNAYmZzLmRlPiB3cm90ZToK PiBBbSAxMy4xMC4yMDE2IDEwOjU1LCBzY2hyaWViIERhbiBDYXJwZW50ZXI6Cj4+IFdlIHdhbnQg dG8gcmVhZCAzIGJ5dGVzIGhlcmUsIGJ1dCBiZWNhdXNlIHRoZSBwYXJlbnRoZXNpcyBhcmUgaW4g dGhlCj4+IHdyb25nIHBsYWNlIHdlIGluc3RlYWQgcmVhZDoKPj4gCj4+IAlzaXplb2YoaW50ZWxf ZHAtPmVkcF9kcGNkKSA9PSBzaXplb2YoaW50ZWxfZHAtPmVkcF9kcGNkKQo+PiAKPj4gd2hpY2gg aXMgb25lIGJ5dGUuCj4+IAo+PiBGaXhlczogZmU1YTY2ZjkxYzg4ICgiZHJtL2k5MTU6IFJlYWQg UFNSIGNhcHMvaW50ZXJtZWRpYXRlIGZyZXFzL2V0Yy4gb25seSBvbmNlIG9uIGVEUCIpCj4+IFNp Z25lZC1vZmYtYnk6IERhbiBDYXJwZW50ZXIgPGRhbi5jYXJwZW50ZXJAb3JhY2xlLmNvbT4KCkRh biwgZ29vZCBjYXRjaCwgdGhhbmsgeW91LiBMdWNraWx5IHdlIG9ubHkgcmVhbGx5IHVzZSB0aGUg Zmlyc3QgYnl0ZQpjdXJyZW50bHkuLi4KCkNjOiA8ZHJtLWludGVsLWZpeGVzQGxpc3RzLmZyZWVk ZXNrdG9wLm9yZz4KCj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9k cC5jIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHAuYwo+PiBpbmRleCAxNGEzY2YwLi5l ZThhYTk1IDEwMDY0NAo+PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kcC5jCj4+ ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMKPj4gQEAgLTM1NTEsOCArMzU1 MSw4IEBAIGludGVsX2VkcF9pbml0X2RwY2Qoc3RydWN0IGludGVsX2RwICppbnRlbF9kcCkKPj4g IAkvKiBSZWFkIHRoZSBlRFAgRGlzcGxheSBjb250cm9sIGNhcGFiaWxpdGllcyByZWdpc3RlcnMg Ki8KPj4gIAlpZiAoKGludGVsX2RwLT5kcGNkW0RQX0VEUF9DT05GSUdVUkFUSU9OX0NBUF0gJiBE UF9EUENEX0RJU1BMQVlfQ09OVFJPTF9DQVBBQkxFKSAmJgo+PiAgCSAgICBkcm1fZHBfZHBjZF9y ZWFkKCZpbnRlbF9kcC0+YXV4LCBEUF9FRFBfRFBDRF9SRVYsCj4+IC0JCQkgICAgIGludGVsX2Rw LT5lZHBfZHBjZCwgc2l6ZW9mKGludGVsX2RwLT5lZHBfZHBjZCkgPT0KPj4gLQkJCSAgICAgc2l6 ZW9mKGludGVsX2RwLT5lZHBfZHBjZCkpKQo+PiArCQkJICAgICBpbnRlbF9kcC0+ZWRwX2RwY2Qs IHNpemVvZihpbnRlbF9kcC0+ZWRwX2RwY2QpKSA9PQo+PiArCQkJICAgICBzaXplb2YoaW50ZWxf ZHAtPmVkcF9kcGNkKSkKPgo+Cj4KPiBwZXJoYXBzIHdlIGNhbiBtb3JwaCB0aGF0IGludG8gc29t ZXRoaW5nIG1vcmUgcmVhZGFibGUgPyBJIHdvdWxkIHN1Z2dlc3Q6Cj4KPgo+IGlmIChpbnRlbF9k cC0+ZHBjZFtEUF9FRFBfQ09ORklHVVJBVElPTl9DQVBdICYgRFBfRFBDRF9ESVNQTEFZX0NPTlRS T0xfQ0FQQUJMRSkKPiAgIHsKPiAgICAgc2l6ZV90IHNpemU9c2l6ZW9mKGludGVsX2RwLT5lZHBf ZHBjZCk7IC8qID09IEVEUF9ESVNQTEFZX0NUTF9DQVBfU0laRSAqLwo+ICAgICBpbnQgcmV0OwkK Pgo+ICAgICByZXQ9ZHJtX2RwX2RwY2RfcmVhZCgmaW50ZWxfZHAtPmF1eCwgRFBfRURQX0RQQ0Rf UkVWLGludGVsX2RwLT5lZHBfZHBjZCxzaXplKTsJCj4KPiAgICAgaWYgKHJldCAhPSBzaXplICkK PiAgICAgICAgIC4uLi4uLgo+Cj4gIH0KPgo+IHRoaXMgd2F5IGl0IGltcHJvdmVzIHJlYWRhYmls aXR5IGFuZCByZWR1Y2VzIGxpbmUgbGVuZ3RoLgoKTm90IGNvbnZpbmNlZCwgbGV0J3MganVzdCB0 YWtlIHRoZSBzaW1wbGUgYnJhY2UgZml4IG5vdy4KCkJSLApKYW5pLgoKCi0tIApKYW5pIE5pa3Vs YSwgSW50ZWwgT3BlbiBTb3VyY2UgVGVjaG5vbG9neSBDZW50ZXIKX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmkt ZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3Jn L21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932133AbcJMMN1 (ORCPT ); Thu, 13 Oct 2016 08:13:27 -0400 Received: from mga11.intel.com ([192.55.52.93]:47979 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755391AbcJMMNP (ORCPT ); Thu, 13 Oct 2016 08:13:15 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,339,1473145200"; d="scan'208";a="772156220" From: Jani Nikula To: wharms@bfs.de, Dan Carpenter Cc: Daniel Vetter , Ville =?utf-8?B?U3lyasOkbMOk?= , David Airlie , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, drm-intel-fixes@lists.freedesktop.org Subject: Re: [patch] drm/i915: fix a read size argument In-Reply-To: <57FF5380.1080704@bfs.de> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20161013085508.GJ16198@mwanda> <57FF5380.1080704@bfs.de> Date: Thu, 13 Oct 2016 15:13:11 +0300 Message-ID: <87zim8wip4.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 Thu, 13 Oct 2016, walter harms wrote: > Am 13.10.2016 10:55, schrieb Dan Carpenter: >> We want to read 3 bytes here, but because the parenthesis are in the >> wrong place we instead read: >> >> sizeof(intel_dp->edp_dpcd) == sizeof(intel_dp->edp_dpcd) >> >> which is one byte. >> >> Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only once on eDP") >> Signed-off-by: Dan Carpenter Dan, good catch, thank you. Luckily we only really use the first byte currently... Cc: >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 14a3cf0..ee8aa95 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3551,8 +3551,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) >> /* Read the eDP Display control capabilities registers */ >> if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) && >> drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, >> - intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd) == >> - sizeof(intel_dp->edp_dpcd))) >> + intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) == >> + sizeof(intel_dp->edp_dpcd)) > > > > perhaps we can morph that into something more readable ? I would suggest: > > > if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) > { > size_t size=sizeof(intel_dp->edp_dpcd); /* == EDP_DISPLAY_CTL_CAP_SIZE */ > int ret; > > ret=drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV,intel_dp->edp_dpcd,size); > > if (ret != size ) > ...... > > } > > this way it improves readability and reduces line length. Not convinced, let's just take the simple brace fix now. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center