From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 2/3] drm: add assertion for checking null edid to drm_edid_block_valid Date: Tue, 2 Jul 2013 11:29:23 +0300 Message-ID: <20130702082923.GJ5004@intel.com> References: <1372726322-29261-1-git-send-email-sw0312.kim@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 3B950E5C8B for ; Tue, 2 Jul 2013 01:29:27 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1372726322-29261-1-git-send-email-sw0312.kim@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Seung-Woo Kim Cc: kyungmin.park@samsung.com, yj44.cho@samsung.com, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Tue, Jul 02, 2013 at 09:52:02AM +0900, Seung-Woo Kim wrote: > If raw_edid of drm_edid_block_vaild() is null, it will crash, so > checking in bad label is removed and instead assertion is added at > the top of the function. > The type of return for the function is bool, so it fixes to return > true and false instead of 1 and 0. > = > Signed-off-by: Seung-Woo Kim > Signed-off-by: Kyungmin Park > --- > chages from v1 > - NULL checking is replaced with WARN_ON() as Daniel commented > - all return value is replaced as true/false as Chris and Daniel commented > = > drivers/gpu/drm/drm_edid.c | 8 +++++--- > 1 files changed, 5 insertions(+), 3 deletions(-) > = > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 2dc1a60..1117f02 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -968,6 +968,8 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bo= ol print_bad_edid) > u8 csum =3D 0; > struct edid *edid =3D (struct edid *)raw_edid; > = > + WARN_ON(!raw_edid); > + I don't see this buying us anything. All you get is two backtraces instead of one. if (WARN_ON(!raw_edid)) return false; would make more sense if we want tolerate programmer errors a bit better. I'm not a huge fan of such defensive programming though since it tends to make it less clear what the function actually expects from you. But here the WARN_ON() would at least make it clear that it's not meant to happen. > if (edid_fixup > 8 || edid_fixup < 0) > edid_fixup =3D 6; > = > @@ -1010,15 +1012,15 @@ bool drm_edid_block_valid(u8 *raw_edid, int block= , bool print_bad_edid) > break; > } > = > - return 1; > + return true; > = > bad: > - if (raw_edid && print_bad_edid) { > + if (print_bad_edid) { > printk(KERN_ERR "Raw EDID:\n"); > print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, > raw_edid, EDID_LENGTH, false); > } > - return 0; > + return false; > } > EXPORT_SYMBOL(drm_edid_block_valid); > = > -- = > 1.7.4.1 > = > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- = Ville Syrj=E4l=E4 Intel OTC