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

2013-07-02 Thread 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 
>> Signed-off-by: Kyungmin Park 
>> ---
>> 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 Center
--

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

2013-07-02 Thread Chris Wilson 
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 
> Signed-off-by: Kyungmin Park 

if (WARN_ON(raw_edid == NULL))
   return false;

Otherwise it is just a WARN_ON() followed by a BUG() :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

2013-07-02 Thread Ville Syrjälä 
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 
> Signed-off-by: Kyungmin Park 
> ---
> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC

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

2013-07-02 Thread Seung-Woo Kim 
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 
Signed-off-by: Kyungmin Park 
---
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);
+
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

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

2013-07-02 Thread Ville Syrjälä 
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@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2013-07-02 Thread 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@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@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
 

-- 
Seung-Woo Kim
Samsung Software RD Center
--

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2013-07-02 Thread Chris Wilson 
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@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com

if (WARN_ON(raw_edid == NULL))
   return false;

Otherwise it is just a WARN_ON() followed by a BUG() :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2013-07-01 Thread Seung-Woo Kim 
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@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@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);
+
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