| Message ID | 1380632773-15183-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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; }
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(-)