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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
