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@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>
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
