Re: [PATCH 2/7] drm/edid: Test for EDID header in drm_edit_probe_custom()

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

On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> EDID read functions do not validate their return data. So they might
> return the required number of bytes of probing, but with invalid
> data. Therefore test for the presence of an EDID header similar to
> the code in edid_block_check().

I don't think the point of drm_probe_ddc() ever was to validate
anything. It reads one byte to see if there's any response. That's all
there is to it.

All EDID validation happens in the _drm_do_get_edid() when actually
reading the EDID.

I don't think I like duplicating this behaviour in both the probe and
the EDID read. And I'm not sure why we're giving drivers the option to
pass a parameter whether to validate or not. Just why?

BR,
Jani.


>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_edid.c | 50 +++++++++++++++++++++++++++++---------
>  include/drm/drm_edid.h     |  2 +-
>  2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a3e9333f9177a..4e12d4b83a720 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1758,6 +1758,18 @@ static void edid_header_fix(void *edid)
>  	memcpy(edid, edid_header, sizeof(edid_header));
>  }
>  
> +static int edid_header_score(const u8 *header)
> +{
> +	int i, score = 0;
> +
> +	for (i = 0; i < sizeof(edid_header); i++) {
> +		if (header[i] == edid_header[i])
> +			score++;
> +	}
> +
> +	return score;
> +}
> +
>  /**
>   * drm_edid_header_is_valid - sanity check the header of the base EDID block
>   * @_edid: pointer to raw base EDID block
> @@ -1769,14 +1781,8 @@ static void edid_header_fix(void *edid)
>  int drm_edid_header_is_valid(const void *_edid)
>  {
>  	const struct edid *edid = _edid;
> -	int i, score = 0;
>  
> -	for (i = 0; i < sizeof(edid_header); i++) {
> -		if (edid->header[i] == edid_header[i])
> -			score++;
> -	}
> -
> -	return score;
> +	return edid_header_score(edid->header);
>  }
>  EXPORT_SYMBOL(drm_edid_header_is_valid);
>  
> @@ -2612,17 +2618,37 @@ EXPORT_SYMBOL(drm_edid_free);
>   * drm_edid_probe_custom() - probe DDC presence
>   * @read_block: EDID block read function
>   * @context: Private data passed to the block read function
> + * @validate: True to validate the EDID header
>   *
>   * Probes for EDID data. Only reads enough data to detect the presence
> - * of the EDID channel.
> + * of the EDID channel. Some EDID block read functions do not fail,
> + * but return invalid data if no EDID data is available. If @validate
> + * has been specified, drm_edid_probe_custom() validates the retrieved
> + * EDID header before signalling success.
>   *
>   * Return: True on success, false on failure.
>   */
> -bool drm_edid_probe_custom(read_block_fn read_block, void *context)
> +bool drm_edid_probe_custom(read_block_fn read_block, void *context, bool validate)
>  {
> -	unsigned char out;
> +	u8 header[8] = {0, 0, 0, 0, 0, 0, 0, 0};
> +	int ret;
> +	size_t len = 1;
> +
> +	if (validate)
> +		len = sizeof(header); // read full header
> +
> +	ret = read_block(context, header, 0, len);
> +	if (ret)
> +		return false;
> +
> +	if (validate) {
> +		int score = edid_header_score(header);
> +
> +		if (score < clamp(edid_fixup, 0, 8))
> +			return false;
> +	}
>  
> -	return (read_block(context, &out, 0, 1) == 0);
> +	return true;
>  }
>  EXPORT_SYMBOL(drm_edid_probe_custom);
>  
> @@ -2635,7 +2661,7 @@ EXPORT_SYMBOL(drm_edid_probe_custom);
>  bool
>  drm_probe_ddc(struct i2c_adapter *adapter)
>  {
> -	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter);
> +	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter, false);
>  }
>  EXPORT_SYMBOL(drm_probe_ddc);
>  
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 4d1797ade5f1d..299278c7ee1c2 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -412,7 +412,7 @@ static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *pro
>  
>  bool
>  drm_edid_probe_custom(int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
> -		      void *context);
> +		      void *context, bool validate);
>  bool drm_probe_ddc(struct i2c_adapter *adapter);
>  struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,

-- 
Jani Nikula, Intel