[PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware

[PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware

[Chris Wilson]

 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;
+	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;
 		}
-	}
 
-	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;
 	if (expected != fwsize) {
 		DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
 		    "(expected %d, got %d)\n", name, expected, (int) fwsize);
-		err = -EINVAL;
+		edid = ERR_PTR(-EINVAL);
 		goto relfw_out;
 	}
 
 	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
 	if (edid == NULL) {
-		err = -ENOMEM;
+		edid = ERR_PTR(-ENOMEM);
 		goto relfw_out;
 	}
 
@@ -197,7 +189,7 @@ 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;
+		edid = ERR_PTR(-EINVAL);
 		goto relfw_out;
 	}
 
@@ -210,19 +202,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 "
@@ -231,12 +222,10 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
 	    name, connector_name);
 
 relfw_out:
-	release_firmware(fw);
+	if (fw)
+		release_firmware(fw);
 
 out:
-	if (err)
-		return ERR_PTR(err);
-
 	return edid;
 }
 
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware

[Ville Syrjälä]

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>

>  	if (expected != fwsize) {
>  		DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
>  		    "(expected %d, got %d)\n", name, expected, (int) fwsize);
> -		err = -EINVAL;
> +		edid = ERR_PTR(-EINVAL);
>  		goto relfw_out;
>  	}
>  
>  	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
>  	if (edid == NULL) {
> -		err = -ENOMEM;
> +		edid = ERR_PTR(-ENOMEM);
>  		goto relfw_out;
>  	}
>  
> @@ -197,7 +189,7 @@ 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;
> +		edid = ERR_PTR(-EINVAL);
>  		goto relfw_out;
>  	}
>  
> @@ -210,19 +202,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 "
> @@ -231,12 +222,10 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
>  	    name, connector_name);
>  
>  relfw_out:
> -	release_firmware(fw);
> +	if (fw)
> +		release_firmware(fw);
>  
>  out:
> -	if (err)
> -		return ERR_PTR(err);
> -
>  	return edid;
>  }
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware

[Jani Nikula]

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.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Try loading builtin EDIDs first

[Chris Wilson]

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;
+
+	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;
 		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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Try loading builtin EDIDs first

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: Try loading builtin EDIDs first

[Chris Wilson]

On Wed, Oct 02, 2013 at 12:22:07PM +0300, Ville Syrjälä wrote:
> > +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.

Hmm, I'm missing the point here then. If edid_size() only returns a
non-zero mulitple of EDID_LENGTH, data_size must also be a non-zero
multiple of EDID_LENGTH for it to pass.

If you want to simply give me your ideal check_size()... :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm: Try loading builtin EDIDs first

[Ville Syrjälä]

On Wed, Oct 02, 2013 at 10:32:35AM +0100, Chris Wilson wrote:
> On Wed, Oct 02, 2013 at 12:22:07PM +0300, Ville Syrjälä wrote:
> > > +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.
> 
> Hmm, I'm missing the point here then. If edid_size() only returns a
> non-zero mulitple of EDID_LENGTH, data_size must also be a non-zero
> multiple of EDID_LENGTH for it to pass.

The point is to check that the edid[0x7e] access in fact lands inside
our data and not somewhere else.

> 
> If you want to simply give me your ideal check_size()... :)
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC

[PATCH] drm: Try loading builtin EDIDs first

[Chris Wilson]

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,
+			  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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Try loading builtin EDIDs first

[Jani Nikula]

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
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel