Re: gma500: issue with continue statement not doing anything useful

[Colin Ian King]

On 18/06/2021 12:19, Patrik Jakobsson wrote:
> On Fri, Jun 18, 2021 at 12:26 PM Colin Ian King
> <colin.king@xxxxxxxxxxxxx> 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@xxxxxxxxx>
>> 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;
> }

OK, that makes sense. I'll send a fix for this shortly
> 
> -Patrik
> 
>>
>> Colin