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

On Wed, Mar 30, 2022 at 06:16:17PM +0300, Jani Nikula wrote:

On Wed, 30 Mar 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:

...

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.

So I've gone back and forth with this. I think I want to get rid of u8* no matter what, because it always requires casting. I've used void* here and there to allow mixed use, internally in drm_edid.c while transitioning, and in public interfaces due to usage all over the place.

OTOH I don't much like arithmetics on void*. It's a gcc extension.

struct edid * is useful for e.g. ->checksum and arithmetics. In many places I've named it struct edid *block to distinguish. We could have a struct edid_block too, which could have ->tag and ->checksum members, for example, but then it would require casting or a function for "safe" typecasting.

I've also gone back and forth with the helpers for getting a pointer to a block. For usage like this, kind of need both const and non-const versions. And, with the plans I have for future, I'm not sure I want to promote any EDID parsing outside of drm_edid.c, so maybe they should be static.

Undecided. C is a bit clunky here.

...

...

• 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.

Ugh. I end up fixing this in the series, in "drm/edid: split out invalid block filtering to a separate function", but I don't mention it anywhere.

Looks like it's been broken for 5+ years since commit 14544d0937bf ("drm/edid: Only print the bad edid when aborting").

Which really makes you wonder about the usefulness of trying to "fix" the EDID by skipping extension blocks. It was added in commit 0ea75e23356f ("DRM: ignore invalid EDID extensions").

...

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

This is how it was before commit 14544d0937bf.

Hmm. This is actually even a bit worse than I though since it looks like we can leak uninitialized stuff from kmalloc_array(). I originally thought it was a krealloc()+memmove() but that is not the case.

I guess the point is if we decide the EDID is garbage, we want to print the original EDID, once, not something we've already changed. I also kind of like the idea of hiding the broken EDID path magic in a separate function.

I'm wondering we should just stop with this bad block filtering entirely? Just let the block be all zeroes/crap if that is really what we got from the sink. And we could still skip known broken blocks during parsing to avoid getting too confused I guess.

On Wed, Mar 30, 2022 at 07:28:56PM +0300, Jani Nikula wrote:

On Wed, 30 Mar 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:

...

On Wed, Mar 30, 2022 at 06:16:17PM +0300, Jani Nikula wrote:

...

On Wed, 30 Mar 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:

...

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.

So I've gone back and forth with this. I think I want to get rid of u8* no matter what, because it always requires casting. I've used void* here and there to allow mixed use, internally in drm_edid.c while transitioning, and in public interfaces due to usage all over the place.

OTOH I don't much like arithmetics on void*. It's a gcc extension.

struct edid * is useful for e.g. ->checksum and arithmetics. In many places I've named it struct edid *block to distinguish. We could have a struct edid_block too, which could have ->tag and ->checksum members, for example, but then it would require casting or a function for "safe" typecasting.

I've also gone back and forth with the helpers for getting a pointer to a block. For usage like this, kind of need both const and non-const versions. And, with the plans I have for future, I'm not sure I want to promote any EDID parsing outside of drm_edid.c, so maybe they should be static.

Undecided. C is a bit clunky here.

...

...

• 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.

Ugh. I end up fixing this in the series, in "drm/edid: split out invalid block filtering to a separate function", but I don't mention it anywhere.

Looks like it's been broken for 5+ years since commit 14544d0937bf ("drm/edid: Only print the bad edid when aborting").

Which really makes you wonder about the usefulness of trying to "fix" the EDID by skipping extension blocks. It was added in commit 0ea75e23356f ("DRM: ignore invalid EDID extensions").

...

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

This is how it was before commit 14544d0937bf.

Hmm. This is actually even a bit worse than I though since it looks like we can leak uninitialized stuff from kmalloc_array(). I originally thought it was a krealloc()+memmove() but that is not the case.

...

I guess the point is if we decide the EDID is garbage, we want to print the original EDID, once, not something we've already changed. I also kind of like the idea of hiding the broken EDID path magic in a separate function.

I'm wondering we should just stop with this bad block filtering entirely? Just let the block be all zeroes/crap if that is really what we got from the sink. And we could still skip known broken blocks during parsing to avoid getting too confused I guess.

I think by far the most common extension count must be 1. Especially with older displays I think anything beyond 256 bytes is virtually non-existent. Agreed?

More than one extension block is certainly pretty rare.

edid-decode has a few with 3 (base + CTA + DispID), and my edid.tv dump seems to have two with 4 blocks (one has base + CTA + 2 DispID blocks, the other has base + block map + 2 CTA blocks).

With that, going from 1 to 0 extensions, it actually works by coincidence, no leaks, no uninitialized stuff. (Looks like maybe any scenario where it's the last N extensions that are invalid works just fine, and it's the broken extensions in the middle that make this go haywire.)

So maybe it's not so scary after all. I could fix that bit first, and then proceed with the rest of the series.

I'd fix this up front so we don't end having to backport the whole thing if/when some security scan gizmo stumbles on this.

I'm a bit hesitant to make big functional changes now, like stopping the filtering entirely, because it's not just drm_edid.c parsing the returned EDIDs, parsing is all over the place.

Sure. Just threw it out there as a future idea.

And on that note, my idea (also for HF-EEODB, my end goal) is to move towards an opaque struct drm_edid, which is 1) generated and parsed exclusively within drm_edid.c, nowhere else, 2) always valid to be passed to drm_edid.c (i.e. always be able to handle what we've generated, whatever that is). If you want the benefits of HF-EEODB or new DisplayID 2.0 features or whatever, you switch your driver to struct drm_edid. But you can keep rolling with the legacy struct edid crap ad infinitum.

Hopefully we can shame enough people to fix their stuff eventually :P But yeah, trying to fix it all right now is a bit much.

On Tue, Mar 29, 2022 at 09:42:14PM +0300, Jani Nikula wrote:

Add edid_block_check() that only checks the EDID block validity, without any actions. Turns out it's simple and crystal clear.

Rewrite drm_edid_block_valid() around it, keeping all the functionality fairly closely the same, warts and all. Turns out it's incredibly complicated for a function you'd expect to be simple, with all the fixing and printing and special casing. (Maybe we'll want to simplify it in the future.)

Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com

---

drivers/gpu/drm/drm_edid.c | 150 ++++++++++++++++++++++--------------- 1 file changed, 88 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 481643751d10..04eb6949c9c8 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1668,10 +1668,55 @@ bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2) } EXPORT_SYMBOL(drm_edid_are_equal);

+enum edid_block_status {

• EDID_BLOCK_OK = 0,
• EDID_BLOCK_NULL,
• EDID_BLOCK_HEADER_CORRUPT,
• EDID_BLOCK_HEADER_REPAIR,
• EDID_BLOCK_HEADER_FIXED,
• EDID_BLOCK_CHECKSUM,
• EDID_BLOCK_VERSION,

+};

+static enum edid_block_status edid_block_check(const void *_block, bool base)

s/base/is_base_block/ or something?

+{

• const struct edid *block = _block;
• if (!block)

• return EDID_BLOCK_NULL;
  

• if (base) {

• int score = drm_edid_header_is_valid(block);
  

• if (score < clamp(edid_fixup, 6, 8))
  

That should clamp to 0-8?

Might be nicer to just define a .set() op for the modparam and check it there, but that's clearly material for a separate patch.

• 	return EDID_BLOCK_HEADER_CORRUPT;
  

• if (score < 8)
  

• 	return EDID_BLOCK_HEADER_REPAIR;
  

• }
• if (edid_block_compute_checksum(block) != edid_block_get_checksum(block))

• return EDID_BLOCK_CHECKSUM;
  

• if (base) {

• if (block->version != 1)
  

• 	return EDID_BLOCK_VERSION;
  

• }
• return EDID_BLOCK_OK;

+}

+static bool edid_block_status_valid(enum edid_block_status status, int tag) +{

• return status == EDID_BLOCK_OK ||

• status == EDID_BLOCK_HEADER_FIXED ||
  

• (status == EDID_BLOCK_CHECKSUM && tag == CEA_EXT);
  

+}

/**

• drm_edid_block_valid - Sanity check the EDID block (base or extension)
• @raw_edid: pointer to raw EDID block

• • @block: type of block to validate (0 for base, extension otherwise)

• • @block_num: type of block to validate (0 for base, extension otherwise)
  • @print_bad_edid: if true, dump bad EDID blocks to the console
  • @edid_corrupt: if true, the header or checksum is invalid

@@ -1680,88 +1725,69 @@ EXPORT_SYMBOL(drm_edid_are_equal);

• Return: True if the block is valid, false otherwise.

*/ -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, +bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid, bool *edid_corrupt) {

• u8 csum;
• struct edid *edid = (struct edid *)raw_edid;

• struct edid *block = (struct edid *)_block;
• enum edid_block_status status;
• bool base = block_num == 0;
• bool valid;

• if (WARN_ON(!raw_edid))

• if (WARN_ON(!block)) return false;

• if (edid_fixup > 8 || edid_fixup < 0)

• edid_fixup = 6;
  

• if (block == 0) {

• int score = drm_edid_header_is_valid(raw_edid);
  

• status = edid_block_check(block, base);
• if (status == EDID_BLOCK_HEADER_REPAIR) {

• DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
  

• edid_header_fix(block);
  
  

• if (score == 8) {
  

• 	if (edid_corrupt)
  

• 		*edid_corrupt = false;
  

• } else if (score >= edid_fixup) {
  

• 	/* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
  

• 	 * The corrupt flag needs to be set here otherwise, the
  

• 	 * fix-up code here will correct the problem, the
  

• 	 * checksum is correct and the test fails
  

• 	 */
  

• 	if (edid_corrupt)
  

• 		*edid_corrupt = true;
  

• 	DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
  

• 	edid_header_fix(raw_edid);
  

• } else {
  

• 	if (edid_corrupt)
  

• 		*edid_corrupt = true;
  

• 	goto bad;
  

• }
  

• /* Retry with fixed header, update status if that worked. */
  

• status = edid_block_check(block, base);
  

• if (status == EDID_BLOCK_OK)
  

• 	status = EDID_BLOCK_HEADER_FIXED;
  

  }

• csum = edid_block_compute_checksum(raw_edid);
• if (csum != edid_block_get_checksum(raw_edid)) {

• if (edid_corrupt)
  

• if (edid_corrupt) {

• /*
  

•  * Unknown major version isn't corrupt but we can't use it. Only
  

•  * the base block can reset edid_corrupt to false.
  

•  */
  

• if (base && (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION))
  

• 	*edid_corrupt = false;
  

• else if (status != EDID_BLOCK_OK)
  *edid_corrupt = true;
  

• /* allow CEA to slide through, switches mangle this */
  

• if (edid_block_tag(raw_edid) == CEA_EXT) {
  

• 	DRM_DEBUG("EDID checksum is invalid, remainder is %d\n", csum);
  

• 	DRM_DEBUG("Assuming a KVM switch modified the CEA block but left the original checksum\n");
  

• } else {
  

• 	if (print_bad_edid)
  

• 		DRM_NOTE("EDID checksum is invalid, remainder is %d\n", csum);
  

• 	goto bad;
  

• }
  

  }

• /* per-block-type checks */

• switch (edid_block_tag(raw_edid)) {

• case 0: /* base */

• if (edid->version != 1) {
  

• 	DRM_NOTE("EDID has major version %d, instead of 1\n", edid->version);
  

• 	goto bad;
  

• /* Determine whether we can use this block with this status. */
• valid = edid_block_status_valid(status, edid_block_tag(block));
• /* Some fairly random status printouts. */
• if (status == EDID_BLOCK_CHECKSUM) {

• if (valid) {
  

• 	DRM_DEBUG("EDID block checksum is invalid, remainder is %d\n",
  

• 		  edid_block_compute_checksum(block));
  

• 	DRM_DEBUG("Assuming a KVM switch modified the block but left the original checksum\n");
  

• } else if (print_bad_edid) {
  

• 	DRM_NOTE("EDID block checksum is invalid, remainder is %d\n",
  

• 		 edid_block_compute_checksum(block));
  

  }

• if (edid->revision > 4)
  

• 	DRM_DEBUG("EDID minor > 4, assuming backward compatibility\n");
  

This debug message seems to disappear. Intentional?

• break;
  

• default:

• break;
  

• } else if (status == EDID_BLOCK_VERSION) {

• DRM_NOTE("EDID has major version %d, instead of 1\n",
  

• 	 block->version);
  

  }

• return true;

-bad:

• if (print_bad_edid) {

• if (edid_is_zero(raw_edid, EDID_LENGTH)) {
  

• if (!valid && print_bad_edid) {

• if (edid_is_zero(block, EDID_LENGTH)) {
  pr_notice("EDID block is all zeroes\n");
  

  } else { pr_notice("Raw EDID:\n"); print_hex_dump(KERN_NOTICE, " \t", DUMP_PREFIX_NONE, 16, 1,

• 		       raw_edid, EDID_LENGTH, false);
  

• 		       block, EDID_LENGTH, false);
  

  } }

• return false;

• return valid;

} EXPORT_SYMBOL(drm_edid_block_valid);

-- 2.30.2

On Tue, Mar 29, 2022 at 09:42:19PM +0300, Jani Nikula wrote:

The code modifying the EDID block should not need to do tricks to fix the checksum. We have a function for computing the checksum, use it.

Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com

---

drivers/gpu/drm/drm_edid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index d0a76781ed19..d2dfab28b5b7 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1840,8 +1840,8 @@ static struct edid *edid_filter_invalid_blocks(const struct edid *edid, memcpy(base++, block, EDID_LENGTH); }

• new->checksum += new->extensions - valid_extensions; new->extensions = valid_extensions;

• new->checksum = edid_block_compute_checksum(new);

Seems to happen after we've validated the base block so this won't accidentally fix up an already bad checksum.

Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

out: kfree(edid); -- 2.30.2