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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC