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
