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

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