| Message ID | 1372673193-18824-3-git-send-email-sw0312.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 01, 2013 at 07:06:32PM +0900, Seung-Woo Kim wrote: > If raw_edid is null, it will crash, so checking in bad label is > meaningless. It would be an error on part of the caller, but the defense looks sane. As the function is a bool, I would have preferred it returned true/false, but your patch is correct wrt to the rest of the function. > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Mon, Jul 1, 2013 at 12:21 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, Jul 01, 2013 at 07:06:32PM +0900, Seung-Woo Kim wrote: >> If raw_edid is null, it will crash, so checking in bad label is >> meaningless. > > It would be an error on part of the caller, but the defense looks sane. > As the function is a bool, I would have preferred it returned > true/false, but your patch is correct wrt to the rest of the function. If we consider passing a NULL raw_edid here a caller-error, shouldn't this be a WARN on top? And I concur on the s/0/false/ bikeshed, return 0 could be misleading since for errno returning functions that reads as success. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel, On 2013? 07? 01? 23:56, Daniel Vetter wrote: > On Mon, Jul 1, 2013 at 12:21 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> On Mon, Jul 01, 2013 at 07:06:32PM +0900, Seung-Woo Kim wrote: >>> If raw_edid is null, it will crash, so checking in bad label is >>> meaningless. >> >> It would be an error on part of the caller, but the defense looks sane. >> As the function is a bool, I would have preferred it returned >> true/false, but your patch is correct wrt to the rest of the function. > > If we consider passing a NULL raw_edid here a caller-error, shouldn't > this be a WARN on top? And I concur on the s/0/false/ bikeshed, return > 0 could be misleading since for errno returning functions that reads > as success. Yes, you are right. WARN_ON() is better because there was no crash until now. and I will also update all return values as false/true instead of 0/1. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 2dc1a60..dbaed34 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -968,6 +968,9 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) u8 csum = 0; struct edid *edid = (struct edid *)raw_edid; + if (!raw_edid) + return 0; + if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6; @@ -1013,7 +1016,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) return 1; 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);