Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <9eff854f-92ed-3f09-997f-f81c78a8b5a3@canonical.com>
In-Reply-To: <9eff854f-92ed-3f09-997f-f81c78a8b5a3@canonical.com>
From:   Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Date:   Fri, 18 Jun 2021 13:19:38 +0200
Message-ID: <CAMeQTsaWZ8Y+sMr4BMS0r_xdwz3hJ+KRT3byyJmq+OeEo=5F1g@mail.gmail.com>
Subject: Re: gma500: issue with continue statement not doing anything useful
To:     Colin Ian King <colin.king@canonical.com>
Cc:     David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jun 18, 2021 at 12:26 PM Colin Ian King
<colin.king@canonical.com> wrote:
>
> Hi,

Hi Colin

>
> Static analysis with Coverity has found a rather old issue in
> drivers/gpu/drm/gma500/oaktrail_crtc.c with the following commit:

Relevant code is in drivers/gpu/drm/gma500/oaktrail_lvds.c

>
> commit 9bd81acdb648509dbbc32d4da0477c9fae0d6a73
> Author: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Date:   Mon Dec 19 21:41:33 2011 +0000
>
>     gma500: Convert Oaktrail to work with new output handling
>
> The analysis is as follows:
>
> 114        /* Find the connector we're trying to set up */
> 115        list_for_each_entry(connector, &mode_config->connector_list,
> head) {
> 116                if (!connector->encoder || connector->encoder->crtc
> != crtc)
>
>    Continue has no effect (NO_EFFECT)useless_continue: Statement
> continue does not have any effect.
>
> 117                        continue;
> 118        }
> 119
> 120        if (!connector) {
> 121                DRM_ERROR("Couldn't find connector when setting mode");
> 122                gma_power_end(dev);
> 123                return;
> 124        }
>
> Currently it appears the loop just iterates to the end of the list
> without doing anything useful. I'm not sure what the original intent
> was, so I'm not sure how this should be fixed.

The code assumes there is only one correct connector so when iterating
over the connectors it checks for the connectors that does _not_ match
our criteria (!connector->encoder || connector->encoder->crtc
> != crtc). When the loop is done we end up with the correct connector set in the connector variable, hence the immediate check of "if (!connector) ...".

So the code is correct but perhaps unintuitive. You could do the
opposite (if that makes more sense to you):

list_for_each_entry(connector, &mode_config->connector_list, head) {
        if (connector->encoder && connector->encoder->crtc == crtc)
                break;
}

-Patrik

>
> Colin