On Tue, 01 Oct 2013, Chris Wilson chris@chris-wilson.co.uk wrote:

On Tue, Oct 01, 2013 at 06:49:42PM +0300, Ville Syrjälä wrote:

...

On Tue, Oct 01, 2013 at 02:06:13PM +0100, Chris Wilson wrote:

...

CC drivers/gpu/drm/drm_edid_load.o drivers/gpu/drm/drm_edid_load.c: In function ‘drm_load_edid_firmware’: include/linux/err.h:39:17: warning: ‘edid’ may be used uninitialised in this function [-Wuninitialized] drivers/gpu/drm/drm_edid_load.c:141:22: note: ‘edid’ was declared here

In the process, we can make the error handling more resilient.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk

drivers/gpu/drm/drm_edid_load.c | 75 +++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 271b42b..4b57a4c 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -136,59 +136,51 @@ static u8 generic_edid[GENERIC_EDIDS][128] = { static u8 *edid_load(struct drm_connector *connector, const char *name, const char *connector_name) {

• const struct firmware *fw;

• const struct firmware *fw = NULL; struct platform_device *pdev;

• u8 *fwdata = NULL, *edid, *new_edid;
• int fwsize, expected;
• int builtin = 0, err = 0;

• u8 *fwdata, *edid;

Orthogonal issue, but fwdata, generic_edid and generic_edid_names could all be const.

...

• int fwsize, expected, err, builtin; int i, valid_extensions = 0; bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);

  pdev = platform_device_register_simple(connector_name, -1, NULL, 0);

• if (IS_ERR(pdev)) {

• DRM_ERROR("Failed to register EDID firmware platform device "
  

•     "for connector \"%s\"\n", connector_name);
  

• err = -EINVAL;
  

• goto out;
  

• }
• err = request_firmware(&fw, name, &pdev->dev);
• platform_device_unregister(pdev);

• if (!IS_ERR(pdev)) {

• err = request_firmware(&fw, name, &pdev->dev);
  

• platform_device_unregister(pdev);
  

• } else

• err = PTR_ERR(pdev);
  
  

• if (err) {

• if (err == 0) {

• fwdata = (u8 *)fw->data;
  

• fwsize = fw->size;
  

• builtin = 0;
  

• } else { i = 0; while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i])) i++;

• if (i < GENERIC_EDIDS) {
  

• 	err = 0;
  

• 	builtin = 1;
  

• 	fwdata = generic_edid[i];
  

• 	fwsize = sizeof(generic_edid[i]);
  

• if (i >= GENERIC_EDIDS) {
  

• 	DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
  

• 			name, err);
  

• 	edid = ERR_PTR(err);
  

• 	goto out;
  

Due to the 'if (fw)' check in the cleanup code, you could eliminate the out label.

...

}

• }

• if (err) {

• DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
  

•     name, err);
  

• goto out;
  

• }

• if (fwdata == NULL) {

• fwdata = (u8 *) fw->data;
  

• fwsize = fw->size;
  

• fwdata = generic_edid[i];
  

• fwsize = sizeof(generic_edid[i]);
  

• builtin = 1;
  

  }

  expected = (fwdata[0x7e] + 1) * EDID_LENGTH;

Not your bug, but we're missing a check for fwsize > 0x7e.

Can't spot any real bugs, so w/ or w/o the out label idea: Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

Might as well spend the time to fix up the little warts whilst we are here, so expect a v2 shortly.

One thing that bugs me about the current code is that with CONFIG_FW_LOADER_USER_HELPER=y, if the firmware isn't builtin and the kernel can't load the firmware directly from the filesystem, it will take a full minute to timeout if userspace/udev isn't ready yet. This usually happens when the user is trying to use the DRM builtin EDIDs, and a user has reported this happening.

request_firmware() tries to load the firmware in order: 1) kernel builtin - fw_get_builtin_firmware() 2) kernel direct load - fw_get_filesystem_firmware() 3) usermode helper - fw_load_from_user_helper()

Given the above order, I don't think it would be unreasonable to move the DRM builtin EDID loading to the top of the list, especially since 3) is prone to take a long time in early boot. An alternative would be to use request_firmware_nowait(), but that seems like it could get messy.

I don't know if that fits in with what you're doing, or whether you'd like to do that, but it's something to think about while at it.

BR, Jani.

On Wed, Oct 02, 2013 at 10:07:39AM +0100, Chris Wilson wrote:

If the firmware is not builtin and userspace is not yet running, we can stall the boot process for a minute whilst the firmware loader times out. This is contrary to expectations of providing a builtin EDID!

In the process, we can rearrange the code to make the error handling more resilient and prevent gcc warning about unitialised variables along the error paths.

v2: Load builtins first, fix gcc second (Jani) and cosmetics (Ville).

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Jani Nikula jani.nikula@linux.intel.com

---

drivers/gpu/drm/drm_edid_load.c | 117 +++++++++++++++++++++------------------ 1 file changed, 63 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 271b42b..9aed4d9 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -32,7 +32,7 @@ MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob " "from built-in data or /lib/firmware instead. ");

#define GENERIC_EDIDS 5 -static char *generic_edid_name[GENERIC_EDIDS] = { +static const char *generic_edid_name[GENERIC_EDIDS] = { "edid/1024x768.bin", "edid/1280x1024.bin", "edid/1600x1200.bin", @@ -40,7 +40,7 @@ static char *generic_edid_name[GENERIC_EDIDS] = { "edid/1920x1080.bin", };

-static u8 generic_edid[GENERIC_EDIDS][128] = { +static const u8 generic_edid[GENERIC_EDIDS][128] = { { 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, @@ -133,63 +133,77 @@ static u8 generic_edid[GENERIC_EDIDS][128] = { }, };

+static int edid_size(const u8 *edid) +{

• return (edid[0x7e] + 1) * EDID_LENGTH;

+}

+static bool edid_check_size(const u8 *data, int data_size) +{

• if (data[0x7e] > 0x7e)

• return false;
  

That should be 'if (data_size <= 0x7e) return false;' no?

Or maybe just 'data_size < EDID_LENGTH' since we anyway want a multiple of EDID_LENGTH.

• if (edid_size(data) != data_size)

• return false;
  

• return true;

+}

static u8 *edid_load(struct drm_connector *connector, const char *name, const char *connector_name) {

• const struct firmware *fw;
• struct platform_device *pdev;
• u8 *fwdata = NULL, *edid, *new_edid;
• int fwsize, expected;
• int builtin = 0, err = 0;

• const struct firmware *fw = NULL;
• const u8 *fwdata;
• u8 *edid;
• int fwsize, builtin; int i, valid_extensions = 0; bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);

• pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
• if (IS_ERR(pdev)) {

• DRM_ERROR("Failed to register EDID firmware platform device "
  

•     "for connector \"%s\"\n", connector_name);
  

• err = -EINVAL;
  

• goto out;
  

• }
• err = request_firmware(&fw, name, &pdev->dev);
• platform_device_unregister(pdev);
• if (err) {

• i = 0;
  

• while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
  

• 	i++;
  

• if (i < GENERIC_EDIDS) {
  

• 	err = 0;
  

• 	builtin = 1;
  

• builtin = 0;
• for (i = 0; i < GENERIC_EDIDS; i++) {

• if (strcmp(name, generic_edid_name[i]) == 0) {
  fwdata = generic_edid[i];
  fwsize = sizeof(generic_edid[i]);
  

• 	builtin = 1;
  

• 	break;
  

  } }
• if (!builtin) {

• struct platform_device *pdev;
  

• int err;
  
  

• if (err) {

• DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
  

•     name, err);
  

• goto out;
  

• }

• pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
  

• if (IS_ERR(pdev)) {
  

• 	DRM_ERROR("Failed to register EDID firmware platform device "
  

• 		  "for connector \"%s\"\n", connector_name);
  

• 	return ERR_CAST(pdev);
  

• }
  
  

• if (fwdata == NULL) {

• fwdata = (u8 *) fw->data;
  

• err = request_firmware(&fw, name, &pdev->dev);
  

• platform_device_unregister(pdev);
  

• if (err) {
  

• 	DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
  

• 		  name, err);
  

• 	return ERR_PTR(err);
  

• }
  

• fwdata = (const u8 *)fw->data;
  

No need to cast now that fwdata is const.

fwsize = fw->size;

}

• expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
• if (expected != fwsize) {

• if (!edid_check_size(fwdata, fwsize)) { DRM_ERROR("Size of EDID firmware "%s" is invalid "

•     "(expected %d, got %d)\n", name, expected, (int) fwsize);
  

• err = -EINVAL;
  

• goto relfw_out;
  

• 	  "(expected %d, got %d, limit %d)\n", name,
  

• 	  edid_size(fwdata), (int)fwsize,
  

• 	  0x7f*EDID_LENGTH);
  

• edid = ERR_PTR(-EINVAL);
  

• goto out;
  

  }

  edid = kmemdup(fwdata, fwsize, GFP_KERNEL); if (edid == NULL) {

• err = -ENOMEM;
  

• goto relfw_out;
  

• edid = ERR_PTR(-ENOMEM);
  

• goto out;
  

  }

  if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {

@@ -197,8 +211,8 @@ static u8 *edid_load(struct drm_connector *connector, const char *name, DRM_ERROR("Base block of EDID firmware "%s" is invalid ", name); kfree(edid);

• err = -EINVAL;
  

• goto relfw_out;
  

• edid = ERR_PTR(-EINVAL);
  

• goto out;
  

  }

  for (i = 1; i <= edid[0x7e]; i++) {

@@ -210,19 +224,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name, }

if (valid_extensions != edid[0x7e]) {

• u8 *new_edid;
  

• edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; DRM_INFO("Found %d valid extensions instead of %d in EDID data " ""%s" for connector "%s"\n", valid_extensions, edid[0x7e], name, connector_name); edid[0x7e] = valid_extensions;
• new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,

•     GFP_KERNEL);
  

• if (new_edid == NULL) {
  

• 	err = -ENOMEM;
  

• 	kfree(edid);
  

• 	goto relfw_out;
  

• }
  

• edid = new_edid;
  

• 		    GFP_KERNEL);
  

• if (new_edid)
  

• 	edid = new_edid;
  

  }

  DRM_INFO("Got %s EDID base block and %d extension%s from "

@@ -230,13 +243,9 @@ static u8 *edid_load(struct drm_connector *connector, const char *name, "external", valid_extensions, valid_extensions == 1 ? "" : "s", name, connector_name);

-relfw_out:

• release_firmware(fw);

out:

• if (err)

• return ERR_PTR(err);
  

• if (fw)

• release_firmware(fw);
  

  return edid;

}

-- 1.7.9.5

On Wed, 02 Oct 2013, Chris Wilson chris@chris-wilson.co.uk wrote:

If the firmware is not builtin and userspace is not yet running, we can stall the boot process for a minute whilst the firmware loader times out. This is contrary to expectations of providing a builtin EDID!

In the process, we can rearrange the code to make the error handling more resilient and prevent gcc warning about unitialised variables along the error paths.

v2: Load builtins first, fix gcc second (Jani) and cosmetics (Ville). v3: Verify that we do not read beyond the end of the fwdata (Ville)

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Jani Nikula jani.nikula@linux.intel.com

---

drivers/gpu/drm/drm_edid_load.c | 108 +++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 271b42b..9081172 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -32,7 +32,7 @@ MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob " "from built-in data or /lib/firmware instead. ");

#define GENERIC_EDIDS 5 -static char *generic_edid_name[GENERIC_EDIDS] = { +static const char *generic_edid_name[GENERIC_EDIDS] = { "edid/1024x768.bin", "edid/1280x1024.bin", "edid/1600x1200.bin", @@ -40,7 +40,7 @@ static char *generic_edid_name[GENERIC_EDIDS] = { "edid/1920x1080.bin", };

-static u8 generic_edid[GENERIC_EDIDS][128] = { +static const u8 generic_edid[GENERIC_EDIDS][128] = { { 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, @@ -133,63 +133,68 @@ static u8 generic_edid[GENERIC_EDIDS][128] = { }, };

+static int edid_size(const u8 *edid, int data_size) +{

• if (data_size < EDID_LENGTH)

• return 0;
  

• return (edid[0x7e] + 1) * EDID_LENGTH;

+}

static u8 *edid_load(struct drm_connector *connector, const char *name, const char *connector_name) {

• const struct firmware *fw;
• struct platform_device *pdev;
• u8 *fwdata = NULL, *edid, *new_edid;
• int fwsize, expected;
• int builtin = 0, err = 0;

• const struct firmware *fw = NULL;
• const u8 *fwdata;
• u8 *edid;
• int fwsize, builtin; int i, valid_extensions = 0; bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);

• pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
• if (IS_ERR(pdev)) {

• DRM_ERROR("Failed to register EDID firmware platform device "
  

•     "for connector \"%s\"\n", connector_name);
  

• err = -EINVAL;
  

• goto out;
  

• }
• err = request_firmware(&fw, name, &pdev->dev);
• platform_device_unregister(pdev);
• if (err) {

• i = 0;
  

• while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
  

• 	i++;
  

• if (i < GENERIC_EDIDS) {
  

• 	err = 0;
  

• 	builtin = 1;
  

• builtin = 0;
• for (i = 0; i < GENERIC_EDIDS; i++) {

• if (strcmp(name, generic_edid_name[i]) == 0) {
  fwdata = generic_edid[i];
  fwsize = sizeof(generic_edid[i]);
  

• 	builtin = 1;
  

• 	break;
  

  } }
• if (!builtin) {

• struct platform_device *pdev;
  

• int err;
  
  

• if (err) {

• DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
  

•     name, err);
  

• goto out;
  

• }

• pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
  

• if (IS_ERR(pdev)) {
  

• 	DRM_ERROR("Failed to register EDID firmware platform device "
  

• 		  "for connector \"%s\"\n", connector_name);
  

• 	return ERR_CAST(pdev);
  

• }
  

• err = request_firmware(&fw, name, &pdev->dev);
  

• platform_device_unregister(pdev);
  

• if (err) {
  

• 	DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
  

• 		  name, err);
  

• 	return ERR_PTR(err);
  

• }
  
  

• if (fwdata == NULL) {

• fwdata = (u8 *) fw->data;
  

• fwdata = fw->data;
  

  fwsize = fw->size; }

• expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
• if (expected != fwsize) {

• if (edid_size(fwdata, fwsize) != fwsize) { DRM_ERROR("Size of EDID firmware "%s" is invalid "

•     "(expected %d, got %d)\n", name, expected, (int) fwsize);
  

• err = -EINVAL;
  

• goto relfw_out;
  

• 	  "(expected %d, got %d\n", name,
  

Obligatory nitpick, closing brace missing... ;)

Good stuff.

Reviewed-by: Jani Nikula jani.nikula@intel.com

• 	  edid_size(fwdata, fwsize), (int)fwsize);
  

• edid = ERR_PTR(-EINVAL);
  

• goto out;
  

  }

  edid = kmemdup(fwdata, fwsize, GFP_KERNEL); if (edid == NULL) {

• err = -ENOMEM;
  

• goto relfw_out;
  

• edid = ERR_PTR(-ENOMEM);
  

• goto out;
  

  }

  if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {

@@ -197,8 +202,8 @@ static u8 *edid_load(struct drm_connector *connector, const char *name, DRM_ERROR("Base block of EDID firmware "%s" is invalid ", name); kfree(edid);

• err = -EINVAL;
  

• goto relfw_out;
  

• edid = ERR_PTR(-EINVAL);
  

• goto out;
  

  }

  for (i = 1; i <= edid[0x7e]; i++) {

@@ -210,19 +215,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name, }

if (valid_extensions != edid[0x7e]) {

• u8 *new_edid;
  

• edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; DRM_INFO("Found %d valid extensions instead of %d in EDID data " ""%s" for connector "%s"\n", valid_extensions, edid[0x7e], name, connector_name); edid[0x7e] = valid_extensions;
• new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,

•     GFP_KERNEL);
  

• if (new_edid == NULL) {
  

• 	err = -ENOMEM;
  

• 	kfree(edid);
  

• 	goto relfw_out;
  

• }
  

• edid = new_edid;
  

• 		    GFP_KERNEL);
  

• if (new_edid)
  

• 	edid = new_edid;
  

  }

  DRM_INFO("Got %s EDID base block and %d extension%s from "

@@ -230,13 +234,9 @@ static u8 *edid_load(struct drm_connector *connector, const char *name, "external", valid_extensions, valid_extensions == 1 ? "" : "s", name, connector_name);

-relfw_out:

• release_firmware(fw);

out:

• if (err)

• return ERR_PTR(err);
  

• if (fw)

• release_firmware(fw);
  

  return edid;

}

-- 1.7.9.5