linux-kernel - Re: [PATCH] drm: Fix EDID firmware load on resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <875yjjotnb.wl-tiwai@suse.de>
Date:   Wed, 27 Jul 2022 10:18:48 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Matthieu CHARETTE <matthieu.charette@...il.com>
Cc:     lkp@...el.com, airlied@...ux.ie, andrealmeid@...lia.com,
        daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
        kbuild-all@...ts.01.org, linux-kernel@...r.kernel.org,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        tzimmermann@...e.de
Subject: Re: [PATCH] drm: Fix EDID firmware load on resume

On Wed, 27 Jul 2022 09:41:52 +0200,
Matthieu CHARETTE wrote:
> 
> Loading an EDID using drm.edid_firmware parameter makes resume to fail
> after firmware cache is being cleaned. This is because edid_load() use a
> temporary device to request the firmware. This cause the EDID firmware
> not to be cached from suspend. And, requesting the EDID firmware return
> an error during resume.
> So the request_firmware() call should use a permanent device for each
> connector. Also, we should cache the EDID even if no monitor is
> connected, in case it's plugged while suspended.
> 
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
> Signed-off-by: Matthieu CHARETTE <matthieu.charette@...il.com>

Can we simply cache the already loaded EDID bytes instead?
Something like below (totally untested).


thanks,

Takashi

-- 8< --
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..b9d2803b518b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -286,6 +286,7 @@ int drm_connector_init(struct drm_device *dev,
 	connector->status = connector_status_unknown;
 	connector->display_info.panel_orientation =
 		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+	connector->firmware_edid = NULL;
 
 	drm_connector_get_cmdline_mode(connector);
 
@@ -485,6 +486,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	ida_simple_remove(&dev->mode_config.connector_ida,
 			  connector->index);
 
+	kfree(connector->firmware_edid);
 	kfree(connector->display_info.bus_formats);
 	drm_mode_object_unregister(dev, &connector->base);
 	kfree(connector->name);
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..a38fe4e00e4a 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -253,6 +253,13 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 			edid = new_edid;
 	}
 
+	connector->firmware_edid = drm_edid_duplicate((struct edid *)edid);
+	if (!connector->firmware_edid) {
+		kfree(edid);
+		edid = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+
 	DRM_INFO("Got %s EDID base block and %d extension%s from "
 	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
 	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
@@ -269,6 +276,12 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
 	struct edid *edid;
 
+	/* already loaded? */
+	if (connector->firmware_edid) {
+		edid = drm_edid_duplicate(connector->firmware_edid);
+		return edid ? edid : ERR_PTR(-ENOMEM);
+	}
+
 	if (edid_firmware[0] == '\0')
 		return ERR_PTR(-ENOENT);
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..b5d0c87327a3 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1528,6 +1528,8 @@ struct drm_connector {
 	enum drm_connector_force force;
 	/** @override_edid: has the EDID been overwritten through debugfs for testing? */
 	bool override_edid;
+	/** @firmware_edid: the cached firmware EDID bytes */
+	struct edid *firmware_edid;
 	/** @epoch_counter: used to detect any other changes in connector, besides status */
 	u64 epoch_counter;