On Tue, 01 Oct 2013, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@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.
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
