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

[Jani Nikula <jani.nikula@linux.intel.com>]

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