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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: dri-devel@lists.freedesktop.org
Cc: stefan.bruens@rwth-aachen.de
Subject: Re: [PATCH 3/3] drm/edid: Tighten checksum conditions for CEA blocks
Date: Fri, 21 Nov 2014 15:39:40 +0200	[thread overview]
Message-ID: <87389csmeb.fsf@intel.com> (raw)
In-Reply-To: <75b98802-5906-49c2-8c04-9f613d328ec2@HUB1.rwth-ad.de>

On Thu, 20 Nov 2014, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> Checksumming was disabled for CEA blocks by
>
> 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
>
> If only the checksum is wrong, reading twice should result in identical
> data, whereas a bad transfer will most likely corrupt different bytes.
> Comparing checksums is not sufficient, as there is a considerable chance
> of two bad transfers having the same checksum.
>
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  drivers/gpu/drm/drm_edid.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3a10f3f..55963d5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1203,6 +1203,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)
> @@ -1233,12 +1234,27 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>  	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, ext_block, j, EDID_LENGTH))
>  				goto out;
> -			if (drm_edid_block_valid(ext_block, j, print_bad_edid)) {
> +			csum = drm_edid_block_csum(ext_block);
> +			if (!csum) {
>  				valid_extensions++;
>  				break;
> +			} else if (ext_block[0] == CEA_EXT) {
> +			/*
> +			 * Some switches mangle CEA contents without fixing the checksum.
> +			 * Accept CEA blocks when two reads return identical data.
> +			 */
> +				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;

I still feel like this should be embedded in drm_edid_block_valid
somehow. Who's going to print the bad edid now? Is it simply no longer
printed in this loop?

I'll defer to others; any better suggestions?

Jani.

>  			}
>  		}
>  
> @@ -1249,6 +1265,9 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>  			connector->bad_edid_counter++;
>  		}
> +
> +		kfree(saved_block);
> +		saved_block = NULL;
>  	}
>  
>  	if (valid_extensions != block[0x7e]) {
> @@ -1270,6 +1289,7 @@ carp:
>  	connector->bad_edid_counter++;
>  
>  out:
> +	kfree(saved_block);
>  	kfree(block);
>  	return NULL;
>  }
> -- 
> 2.1.2
>
> _______________________________________________
> 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