Re: [Intel-gfx] [PATCH 01/12] drm/edid: use struct edid * in drm_do_get_edid()

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

On Tue, Mar 29, 2022 at 09:42:08PM +0300, Jani Nikula wrote:
> Mixing u8 * and struct edid * is confusing, switch to the latter.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index d79b06f7f34c..0650b9217aa2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1991,29 +1991,28 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	void *data)
>  {
>  	int i, j = 0, valid_extensions = 0;
> -	u8 *edid, *new;
> -	struct edid *override;
> +	struct edid *edid, *new, *override;
>  
>  	override = drm_get_override_edid(connector);
>  	if (override)
>  		return override;
>  
> -	edid = (u8 *)drm_do_get_edid_base_block(connector, get_edid_block, data);
> +	edid = drm_do_get_edid_base_block(connector, get_edid_block, data);
>  	if (!edid)
>  		return NULL;
>  
>  	/* if there's no extensions or no connector, we're done */
> -	valid_extensions = edid[0x7e];
> +	valid_extensions = edid->extensions;
>  	if (valid_extensions == 0)
> -		return (struct edid *)edid;
> +		return edid;
>  
>  	new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
>  	if (!new)
>  		goto out;
>  	edid = new;
>  
> -	for (j = 1; j <= edid[0x7e]; j++) {
> -		u8 *block = edid + j * EDID_LENGTH;
> +	for (j = 1; j <= edid->extensions; j++) {
> +		void *block = edid + j;
>  
>  		for (i = 0; i < 4; i++) {
>  			if (get_edid_block(data, block, j, EDID_LENGTH))
> @@ -2026,13 +2025,13 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  			valid_extensions--;
>  	}
>  
> -	if (valid_extensions != edid[0x7e]) {
> -		u8 *base;
> +	if (valid_extensions != edid->extensions) {
> +		struct edid *base;

This one points to extension blocks too so using 
struct edid doesn't seem entirely appropriate.

>  
> -		connector_bad_edid(connector, edid, edid[0x7e] + 1);
> +		connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
>  
> -		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> -		edid[0x7e] = valid_extensions;
> +		edid->checksum += edid->extensions - valid_extensions;
> +		edid->extensions = valid_extensions;
>  
>  		new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
>  				    GFP_KERNEL);
> @@ -2040,21 +2039,21 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  			goto out;
>  
>  		base = new;
> -		for (i = 0; i <= edid[0x7e]; i++) {
> -			u8 *block = edid + i * EDID_LENGTH;
> +		for (i = 0; i <= edid->extensions; i++) {
> +			void *block = edid + i;

Hmm. This code seems very broken to me. We read each block
into its expected offset based on the original base block extension
count, but here we only iterate up to the new ext block count. So
if we had eg. a 4 block EDID where block 2 is busted, we set 
the new ext count to 2, copy over blocks 0 and 1, skip block 2,
and then terminate the loop. So instead of copying block 3 from
the orignal EDID into block 2 of the new EDID, we leave the
original garbage block 2 in place.

Also this memcpy() business seems entirely poinless in the sense
that we could just read each ext block into the final offset
directly AFAICS.

>  
>  			if (!drm_edid_block_valid(block, i, false, NULL))
>  				continue;
>  
>  			memcpy(base, block, EDID_LENGTH);
> -			base += EDID_LENGTH;
> +			base++;
>  		}
>  
>  		kfree(edid);
>  		edid = new;
>  	}
>  
> -	return (struct edid *)edid;
> +	return edid;
>  
>  out:
>  	kfree(edid);
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel