[PATCH 2/2] drm/edid: Tighten checksum conditions for CEA blocks

[PATCH 2/2] drm/edid: Tighten checksum conditions for CEA blocks

[Stefan Brüns]

4a638b4e38234233f5c7e6705662fbc0b58d80c2 disabled the checksumming
for CEA blocks. If only the checksum is wrong, reading twice should
result in identical data, whereas a bad transfer will most likely
corrupt diffent bytes.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 505960e..9b6b65e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1200,6 +1200,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *block, *new;
+	u8 *saved_block = NULL;
 	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
 	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
@@ -1234,15 +1235,29 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	block = new;
 
 	for (j = 1; j <= block[0x7e]; j++) {
+		u8 *ext_block = block + (valid_extensions + 1) * EDID_LENGTH;
+		u8 csum, last_csum = 0;
 		for (i = 0; i < 4; i++) {
-			if (drm_do_probe_ddc_edid(adapter,
-				  block + (valid_extensions + 1) * EDID_LENGTH,
-				  j, EDID_LENGTH))
+			if (drm_do_probe_ddc_edid(adapter, ext_block, j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			if ((csum = drm_edid_block_checksum(ext_block)) == 0) {
 				valid_extensions++;
 				break;
+			} else if ((ext_block[0] == CEA_EXT) && (csum == last_csum)) {
+			/*
+			 * Some switches mangle CEA contents without fixing the checksum.
+			 * Accept CEA blocks when two reads return identical data.
+			 */
+				if (!saved_block)
+					saved_block = kmalloc(EDID_LENGTH, GFP_KERNEL);
+				if (saved_block && !memcmp(ext_block, saved_block, EDID_LENGTH)) {
+					valid_extensions++;
+					break;
+				}
+				if (saved_block)
+					memcpy(saved_block, ext_block, EDID_LENGTH);
 			}
+			last_csum = csum;
 		}
 
 		if (i == 4 && print_bad_edid) {
@@ -1263,6 +1278,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 		block = new;
 	}
 
+	kfree(saved_block);
 	return block;
 
 carp:
@@ -1273,6 +1289,7 @@ carp:
 	connector->bad_edid_counter++;
 
 out:
+	kfree(saved_block);
 	kfree(block);
 	return NULL;
 }
-- 
1.8.4.5

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

Re: [PATCH 2/2] drm/edid: Tighten checksum conditions for CEA blocks

[Jani Nikula]

On Sun, 16 Nov 2014, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> 4a638b4e38234233f5c7e6705662fbc0b58d80c2 disabled the checksumming

Please include the subject of the referenced commit, e.g.

4a638b4e3823 drm/edid: Allow non-fatal checksum errors in CEA blocks

or

commit 4a638b4e38234233f5c7e6705662fbc0b58d80c2
Author: Adam Jackson <ajax@redhat.com>
Date:   Tue May 25 16:33:09 2010 -0400

    drm/edid: Allow non-fatal checksum errors in CEA blocks

> for CEA blocks. If only the checksum is wrong, reading twice should
> result in identical data, whereas a bad transfer will most likely
> corrupt diffent bytes.
>
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 505960e..9b6b65e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1200,6 +1200,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  {
>  	int i, j = 0, valid_extensions = 0;
>  	u8 *block, *new;
> +	u8 *saved_block = NULL;
>  	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>  
>  	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> @@ -1234,15 +1235,29 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  	block = new;
>  
>  	for (j = 1; j <= block[0x7e]; j++) {
> +		u8 *ext_block = block + (valid_extensions + 1) * EDID_LENGTH;

This bit could be a non-functional prep patch.

> +		u8 csum, last_csum = 0;
>  		for (i = 0; i < 4; i++) {
> -			if (drm_do_probe_ddc_edid(adapter,
> -				  block + (valid_extensions + 1) * EDID_LENGTH,
> -				  j, EDID_LENGTH))
> +			if (drm_do_probe_ddc_edid(adapter, ext_block, j, EDID_LENGTH))
>  				goto out;
> -			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {

Now you skip drm_edid_block_valid for all blocks?

> +			if ((csum = drm_edid_block_checksum(ext_block)) == 0) {

Please don't assign within the if condition.

>  				valid_extensions++;
>  				break;
> +			} else if ((ext_block[0] == CEA_EXT) && (csum == last_csum)) {

Too many braces.

Perhaps it would be sufficient to just check checksums instead of
comparing the data? *shrug*.

> +			/*
> +			 * Some switches mangle CEA contents without fixing the checksum.
> +			 * Accept CEA blocks when two reads return identical data.
> +			 */

So you end up here on the 2nd time you get identical checksums, but you
don't have the saved_block for the 1st attempt. Therefore you end up
saving the 2nd attempt and go for a 3rd attempt to end up here.

> +				if (!saved_block)
> +					saved_block = kmalloc(EDID_LENGTH, GFP_KERNEL);
> +				if (saved_block && !memcmp(ext_block, saved_block, EDID_LENGTH)) {

On the 2nd attempt you compare ext_block to whatever kmalloc returned
you.

> +					valid_extensions++;
> +					break;
> +				}
> +				if (saved_block)
> +					memcpy(saved_block, ext_block, EDID_LENGTH);

You should use kmemdup, and rearrange the code to do what it says on the
commit message.

BR,
Jani.

>  			}
> +			last_csum = csum;
>  		}
>  
>  		if (i == 4 && print_bad_edid) {
> @@ -1263,6 +1278,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  		block = new;
>  	}
>  
> +	kfree(saved_block);
>  	return block;
>  
>  carp:
> @@ -1273,6 +1289,7 @@ carp:
>  	connector->bad_edid_counter++;
>  
>  out:
> +	kfree(saved_block);
>  	kfree(block);
>  	return NULL;
>  }
> -- 
> 1.8.4.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/edid: Tighten checksum conditions for CEA blocks

[Brüns, Stefan]

On Monday, November 17, 2014 11:08:48 Jani Nikula wrote:
> On Sun, 16 Nov 2014, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> > 4a638b4e38234233f5c7e6705662fbc0b58d80c2 disabled the checksumming
> 
> Please include the subject of the referenced commit, e.g.
> 
> 4a638b4e3823 drm/edid: Allow non-fatal checksum errors in CEA blocks
> 
> or
> 
> commit 4a638b4e38234233f5c7e6705662fbc0b58d80c2
> Author: Adam Jackson <ajax@redhat.com>
> Date:   Tue May 25 16:33:09 2010 -0400
> 
>     drm/edid: Allow non-fatal checksum errors in CEA blocks

OK

> 
> > for CEA blocks. If only the checksum is wrong, reading twice should
> > result in identical data, whereas a bad transfer will most likely
> > corrupt diffent bytes.
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> >  drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 505960e..9b6b65e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -1200,6 +1200,7 @@ drm_do_get_edid(struct drm_connector *connector,
> > struct i2c_adapter *adapter)> 
> >  {
> >  
> >  	int i, j = 0, valid_extensions = 0;
> >  	u8 *block, *new;
> > 
> > +	u8 *saved_block = NULL;
> > 
> >  	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug &
> >  	DRM_UT_KMS);
> >  	
> >  	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> > 
> > @@ -1234,15 +1235,29 @@ drm_do_get_edid(struct drm_connector *connector,
> > struct i2c_adapter *adapter)> 
> >  	block = new;
> >  	
> >  	for (j = 1; j <= block[0x7e]; j++) {
> > 
> > +		u8 *ext_block = block + (valid_extensions + 1) * EDID_LENGTH;
> 
> This bit could be a non-functional prep patch.

OK
 
> > +		u8 csum, last_csum = 0;
> > 
> >  		for (i = 0; i < 4; i++) {
> > 
> > -			if (drm_do_probe_ddc_edid(adapter,
> > -				  block + (valid_extensions + 1) * EDID_LENGTH,
> > -				  j, EDID_LENGTH))
> > +			if (drm_do_probe_ddc_edid(adapter, ext_block, j, EDID_LENGTH))
> > 
> >  				goto out;
> > 
> > -			if (drm_edid_block_valid(block + (valid_extensions + 1) * 
EDID_LENGTH,
> > j, print_bad_edid)) {
> Now you skip drm_edid_block_valid for all blocks?

drm_edid_block_valid does a bunch of things, depending on the block number. 
For blocks after the first block, it is essentially just checksumming the 
block, but also ignores any errors for CEA blocks, which causes problems later 
on. Therefor the use of the new checksum helper.
 
> > +			if ((csum = drm_edid_block_checksum(ext_block)) == 0) {
> 
> Please don't assign within the if condition.

OK
 
> >  				valid_extensions++;
> >  				break;
> > 
> > +			} else if ((ext_block[0] == CEA_EXT) && (csum == last_csum)) {
> 
> Too many braces.
> 
> Perhaps it would be sufficient to just check checksums instead of
> comparing the data? *shrug*.

As the EDID checksum is just a linear checksum over all bytes there are a lot 
of error combinations having the same checksum, but different data. I think a 
byte-for-byte comparision is a must here (although the checksum-only would be 
an improvement).
 
> > +			/*
> > +			 * Some switches mangle CEA contents without fixing the checksum.
> > +			 * Accept CEA blocks when two reads return identical data.
> > +			 */
> 
> So you end up here on the 2nd time you get identical checksums, but you
> don't have the saved_block for the 1st attempt. Therefore you end up
> saving the 2nd attempt and go for a 3rd attempt to end up here.

Good catch, this is obviously wrong. How about the following:

...
} else if (ext_block[0] == CEA_EXT) {
	if (saved_block && csum == last_csum &&
		!memcmp(ext_block, saved_block, EDID_LENGTH)) {
		valid_extensions++;
		break;
	kfree(saved_block);
	saved_block = kmemdup(ext_block, EDID_LENGTH, GFP_KERNEL);
	last_csum = csum;
}
...

This should also address the other remarks.
 
Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel