| Message ID | 1372726322-29261-1-git-send-email-sw0312.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <sw0312.kim@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > 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, bool print_bad_edid) > u8 csum = 0; > struct edid *edid = (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 = 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
Hello Ville, Thanks for comment. On 2013? 07? 02? 17:29, Ville Syrjälä wrote: > 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 <sw0312.kim@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> 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, bool print_bad_edid) >> u8 csum = 0; >> struct edid *edid = (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. > Ok, it looks better than just WARN_ON(). I'll updated patch as you commented. Best Regards, - Seung-Woo Kim > >> if (edid_fixup > 8 || edid_fixup < 0) >> edid_fixup = 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 >
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 <sw0312.kim@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> if (WARN_ON(raw_edid == NULL)) return false; Otherwise it is just a WARN_ON() followed by a BUG() :) -Chris
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, bool print_bad_edid) u8 csum = 0; struct edid *edid = (struct edid *)raw_edid; + WARN_ON(!raw_edid); + if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 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);