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 at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Seung-Woo Kim
Samsung Software R&D Center
--
