[PATCH v2 2/3] drm: add assertion for checking null edid to drm_edid_block_valid

[Seung-Woo Kim]

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
--